Repository navigation
Frequency enforcement - #2388
HoangHongHieu merged 2 commits into
Conversation
Sorry, something went wrong.
|
Please merge the other PR first as this one shares a lot of common code |
Sorry, something went wrong.
|
Having medium and high enforcement makes sense to me so we can test this. Do we plan on adding a low enforcement? If not, why not have a boolean variable like shouldSetVolumeToMax that's easier to understand? |
Sorry, something went wrong.
Following is the app behavior when the feature is enabled or disabled:
- Enable:
The volume is set to maximum at the beginning of all frequency tests.
During the test, any changes to the volume or peripherals will cause the test to fail.
The test will fail if the required peripherals are missing.
- Disable:
A warning will be raised if the required peripherals are missing or volume is changed.
Thanks for reviewing, I didn't intend to add a low enforcement, so I updated the commit to use the boolean variable. The vibe code looked a bit messy and hard to read, so I refactored some of the functions in the frequency tests. |
Sorry, something went wrong.
| import java.util.TimerTask; | ||
| import java.util.Timer; | ||
|
|
||
| public class TestSupervisor extends AudioDeviceCallback { |
There was a problem hiding this comment.
Please add comments on why we need a test supervisor and why it's intended to be used. If we don't plan on having this for multiple tests, why not just subscribe to device changed callbacks within the test and check for volume changes in the test?
I personally don't like classes like this as it's not obvious what the class does from the naming. One thing we can do is maybe add a simple class that just checks whether the volume has changed. For checking if the devices have changed, we already have onAudioDevicesAdded and onAudioDevicesRemoved
Sorry, something went wrong.
There was a problem hiding this comment.
I placed it in the same package as the other test activities since it is used in both FrequencyTest and DualFrequencyTest. I have renamed the class to specify that it is intended only for frequency tests. If this clutters the com.mobileer.oboetester package, I am happy to move all frequency utility classes into a subpackage.
Regarding the class name, I agree that the previous one was not ideal. The class has been renamed, though all functionality remains the same. It is simple enough to ensure it maintains a single responsibility; while the activity could listen for device events directly, doing so would conflict with its other responsibilities.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks, LGTM!
Sorry, something went wrong.
Frequency tests require a high volume to generate a clear frequency response from the speaker or microphone; however, the volume level is often left unset. Additionally, peripheral adjustments or volume changes during testing can lead to unexpected results. This ticket introduces an enforcement mode to automatically set the volume and monitor any changes during the test. Following is the app behavior when the feature is disabled or enabled:
The default enforcement mode is enabled, though users are free to change. This approach increases the application's flexibility.