DEV-706 Dock menu for changing S3R USB speed#275
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support structures for controlling S3R USB speed (and related EEPROM-backed settings) via dock/driver capability checks.
Changes:
- Introduces
EepromSensorSettingsDetailsto parse/update an EEPROM “sensor settings” page (BT radio state + USB speed bit). - Wires the new settings container into
ShimmerDevice. - Adds
isHWAndFWSupportedUsbControl()capability gating for Shimmer3R LogAndStream FW >= 1.0.55.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
ShimmerDriver/src/main/java/com/shimmerresearch/driverUtilities/EepromSensorSettingsDetails.java |
New EEPROM settings model with parsing and bitfield update logic for radio + USB speed. |
ShimmerDriver/src/main/java/com/shimmerresearch/driver/ShimmerDevice.java |
Adds EEPROM settings member and a new HW/FW support check for USB control; removes prior BT radio state API from this class. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void parseEepromSensorSettingsDetails(byte[] byteArray) { | ||
| this.mEepromPageArray = byteArray; | ||
|
|
||
| radioHwVer = mEepromPageArray[EXP_BOARD_ARRAY_BYTE_INDEX.RADIO_HW_VER.ordinal()] & 0xFF; | ||
| baudRate = mEepromPageArray[EXP_BOARD_ARRAY_BYTE_INDEX.BAUD_RATE.ordinal()] & 0xFF; | ||
|
|
There was a problem hiding this comment.
parseEepromSensorSettingsDetails assigns mEepromPageArray directly and then indexes into it without validating byteArray for null and expected length (EXP_BOARD_MEMORY_PAGE_SIZE). This can throw NPE/AIOOBE if callers pass null or a short buffer; consider guarding and/or copying into a properly-sized array (and returning an error/boolean if invalid).
| private void updateEepromPageArray() { | ||
| mEepromPageArray[EXP_BOARD_ARRAY_BYTE_INDEX.RADIO_HW_VER.ordinal()] = (byte) radioHwVer; | ||
| mEepromPageArray[EXP_BOARD_ARRAY_BYTE_INDEX.BAUD_RATE.ordinal()] = (byte) baudRate; | ||
|
|
||
| mEepromPageArray[EXP_BOARD_ARRAY_BYTE_INDEX.SENSOR_OPTIONS1.ordinal()] &= 0xFC; // Clear the existing radio state bits (bits 0 and 1) | ||
| mEepromPageArray[EXP_BOARD_ARRAY_BYTE_INDEX.SENSOR_OPTIONS1.ordinal()] |= mRadioState.getBitValue(); // Set the new radio state bits | ||
|
|
||
| mEepromPageArray[EXP_BOARD_ARRAY_BYTE_INDEX.SENSOR_OPTIONS1.ordinal()] &= 0xFB; // Clear the existing USB speed bit (bit 2) | ||
| mEepromPageArray[EXP_BOARD_ARRAY_BYTE_INDEX.SENSOR_OPTIONS1.ordinal()] |= (mUsbHighSpeed ? USB_SPEED.HIGH_SPEED.getBitValue() : USB_SPEED.FULL_SPEED.getBitValue()); // Set the new USB speed bit |
There was a problem hiding this comment.
updateEepromPageArray() assumes mEepromPageArray is already allocated to at least 3 bytes, but it is initialized to an empty array by default. Calling setBtRadioStateFromString/setUsbSpeedStateFromString before parseEepromSensorSettingsDetails will cause an ArrayIndexOutOfBoundsException; consider allocating a default EXP_BOARD_MEMORY_PAGE_SIZE array (and initializing reserved bytes) or no-op/throw until parsed.
| public void setBtRadioStateFromString(String option) { | ||
| if(option.equals(BTRADIO_STATE.BT_CLASSIC_BLE_ENABLED.toString())) { | ||
| mRadioState = BTRADIO_STATE.BT_CLASSIC_BLE_ENABLED; | ||
| }else if (option.equals(BTRADIO_STATE.BT_CLASSIC_ENABLED.toString())) { | ||
| mRadioState = BTRADIO_STATE.BT_CLASSIC_ENABLED; | ||
| }else if (option.equals(BTRADIO_STATE.BLE_ENABLED.toString())) { | ||
| mRadioState = BTRADIO_STATE.BLE_ENABLED; | ||
| } |
There was a problem hiding this comment.
setBtRadioStateFromString uses option.equals(...), which will throw if option is null. Elsewhere in the codebase comparisons are typically written as CONSTANT.equals(value) to be null-safe; consider flipping the equals call here and in setUsbSpeedStateFromString.
| public void setBtRadioStateFromString(String option) { | ||
| if(option.equals(BTRADIO_STATE.BT_CLASSIC_BLE_ENABLED.toString())) { | ||
| mRadioState = BTRADIO_STATE.BT_CLASSIC_BLE_ENABLED; | ||
| }else if (option.equals(BTRADIO_STATE.BT_CLASSIC_ENABLED.toString())) { | ||
| mRadioState = BTRADIO_STATE.BT_CLASSIC_ENABLED; | ||
| }else if (option.equals(BTRADIO_STATE.BLE_ENABLED.toString())) { | ||
| mRadioState = BTRADIO_STATE.BLE_ENABLED; | ||
| } | ||
|
|
||
| updateEepromPageArray(); | ||
| } |
There was a problem hiding this comment.
setBtRadioStateFromString does not handle the NONE_ENABLED option (it can be produced by BTRADIO_STATE.NONE_ENABLED.toString()), so selecting that value will silently keep the previous state while still updating the EEPROM array. Add an explicit branch (or a default) so all enum values are supported or an invalid option is surfaced.
| protected String mComPort = ""; | ||
| public transient CommsProtocolRadio mCommsProtocolRadio = null; | ||
| public BT_STATE mBluetoothRadioState = BT_STATE.DISCONNECTED; | ||
| public DOCK_STATE mDockState = DOCK_STATE.UNDOCKED; | ||
| public BTRADIO_STATE mRadioState = BTRADIO_STATE.UNKNOWN; | ||
| private boolean mUpdateOnlyWhenStateChanges=false; | ||
| public static int EXP_BOARD_MEMORY_LOCATION_FOR_BTRADIO_STATE = 2018; | ||
| public enum BTRADIO_STATE{ | ||
|
|
||
| BT_CLASSIC_BLE_ENABLED("BT Classic and BLE Enabled"), | ||
| BT_CLASSIC_ENABLED("BT Classic Enabled"), | ||
| BLE_ENABLED("BLE Enabled"), | ||
| NONE_ENABLED("None Enabled"), | ||
| UNKNOWN("Unknown"); | ||
| // RECORDING("Recording"); | ||
|
|
||
| private final String text; | ||
|
|
||
| /** | ||
| * @param text | ||
| */ | ||
| private BTRADIO_STATE(final String text) { | ||
| this.text = text; | ||
| } | ||
|
|
||
| /* (non-Javadoc) | ||
| * @see java.lang.Enum#toString() | ||
| */ | ||
| @Override | ||
| public String toString() { | ||
| return text; | ||
| } | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
ShimmerDevice previously exposed BTRADIO_STATE and mRadioState as public API; removing them is a breaking change for downstream consumers even if the repo itself no longer references them. If external compatibility matters, consider keeping the old enum/field (deprecated) and delegating to mEepromSensorSettingsDetails, or providing an equivalent accessor method.
No description provided.