BLE Wire Protocol v2: throughput cleanup across asg, mobile SDK, and app layer#3286
BLE Wire Protocol v2: throughput cleanup across asg, mobile SDK, and app layer#3286nic-olo wants to merge 65 commits into
Conversation
Set SENSOR_EXPOSURE_TIME = 11_111_111 ns (1/90s) with AE disabled in both recorded-video and WebRTC/WHIP live-stream capture paths: - PreviewRequestConfigurator: applies inside the forVideo branch, overriding the default CONTROL_AE_MODE_ON so the HAL honours the manual shutter value. - WhipCameraCapturer: applies after CONTROL_MODE_AUTO in startRepeatingRequest, disabling AE for the same reason. ISO is set to 800 and frame duration to 33_333_333 ns (30 fps). Both values can be tuned as needed. Requires the camera HAL to advertise REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR; if not supported the request keys are silently ignored. Co-authored-by: Nicolo Micheletti <michelettinik@gmail.com>
Auto-enable v2 binary session setup, preserve negotiated wire_caps, default K900 pack helpers to BE with endian-aware fallbacks, fix frame duration FPS, repackStringFrame immutability, settings debounce timer reset, and tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
bugbot run |
| // it when CONTROL_AE_MODE_ON is active. | ||
| builder.set(CaptureRequest.CONTROL_AE_MODE, CaptureRequest.CONTROL_AE_MODE_OFF); | ||
| builder.set(CaptureRequest.SENSOR_EXPOSURE_TIME, 11_111_111L); // 1/90s in nanoseconds | ||
| builder.set(CaptureRequest.SENSOR_FRAME_DURATION, 33_333_333L); // 30fps frame duration |
There was a problem hiding this comment.
WHIP frame duration FPS mismatch
Medium Severity
Live WHIP capture sets CONTROL_AE_TARGET_FPS_RANGE from mCameraFps but hardcodes SENSOR_FRAME_DURATION to 33ms (30fps). When streaming below or above 30fps, frame duration and FPS range disagree, which can cause unstable timing or dropped frames on the HAL.
Reviewed by Cursor Bugbot for commit aad318f. Configure here.
PR Agent HandoffExit reason: Budget exhausted — fix or review cycle cap reached.
CI status
Remaining blocking findings
Nits (informational)
Humans merge when ready. Agents do not auto-merge. Label |
There was a problem hiding this comment.
2 issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/utils/K900ProtocolUtils.java">
<violation number="1" location="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/utils/K900ProtocolUtils.java:142">
P1: This changes binary wire frames to use a big-endian outer K900 length, but the binary protocol parser expects that length little-endian. Binary v2 handshakes/fragments created via packBinaryFragment will fail to parse.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // End code $$ | ||
| result[5 + dataLength] = CMD_END_CODE[0]; // $ | ||
| result[6 + dataLength] = CMD_END_CODE[1]; // $ | ||
| return packDataToK900(data, cmdType, K900LengthCodec.Endian.BE); |
There was a problem hiding this comment.
P1: This changes binary wire frames to use a big-endian outer K900 length, but the binary protocol parser expects that length little-endian. Binary v2 handshakes/fragments created via packBinaryFragment will fail to parse.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/utils/K900ProtocolUtils.java, line 142:
<comment>This changes binary wire frames to use a big-endian outer K900 length, but the binary protocol parser expects that length little-endian. Binary v2 handshakes/fragments created via packBinaryFragment will fail to parse.</comment>
<file context>
@@ -136,34 +139,15 @@ public static byte[] packDataCommand(byte[] data, byte cmdType, K900LengthCodec.
- // End code $$
- result[5 + dataLength] = CMD_END_CODE[0]; // $
- result[6 + dataLength] = CMD_END_CODE[1]; // $
+ return packDataToK900(data, cmdType, K900LengthCodec.Endian.BE);
+ }
</file context>
| return packDataToK900(data, cmdType, K900LengthCodec.Endian.BE); | |
| return packDataToK900( | |
| data, | |
| cmdType, | |
| cmdType == CMD_TYPE_BINARY_MSG ? K900LengthCodec.Endian.LE : K900LengthCodec.Endian.BE); |
Binary 0x40 fragments now wrap with little-endian K900 lengths so payloads above 255 bytes parse correctly. Revert manual 1/90s shutter and fixed frame duration from video recording and WHIP capture; rely on AE and FPS range again. Co-authored-by: Cursor <cursoragent@cursor.com>
I my review everything was good on big endian and it was the android producer on the phone that mistakenly was using little endian. I changed it to use big endian on the toolkit work branch. |
|
I see retro-compatibility breaking troubleshooting documentation here, we should review the user-facing story about that https://github.com/Mentra-Community/Mentra-Bluetooth-SDK-Starter-Kit/pull/19/changes |
Guard v2 handshakes and binary reassembly so legacy paths stay stable, and fix the mobile/SDK tests and types needed for CI. Co-authored-by: Cursor <cursoragent@cursor.com>
…ed repos; modify staging-builds.yml to prevent inclusion of test_bes_ota_prod_live_version.json in staging OTA payload.
PR Agent HandoffExit reason: Budget exhausted — fix or review cycle cap reached.
CI status
Remaining blocking findings
Nits (informational)
Humans merge when ready. Agents do not auto-merge. Label |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 03631fc. Configure here.
| K900LengthCodec.Detected detected = | ||
| K900LengthCodec.detectLength(message); | ||
| if (detected != null) { | ||
| uartToBesEndian = detected.endian; |
There was a problem hiding this comment.
UART TX endian flips heuristically
Medium Severity
Each inbound K900 STRING frame updates uartToBesEndian from K900LengthCodec.detectLength, so outbound length encoding can switch to little-endian based on receive heuristics before wire_caps negotiates LE. A mis-resolved ambiguous frame can make ASG→BES transmits use the wrong endianness for legacy big-endian firmware.
Reviewed by Cursor Bugbot for commit 03631fc. Configure here.
There was a problem hiding this comment.
5 issues found across 15 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/sgcs/MentraLive.kt">
<violation number="1" location="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/sgcs/MentraLive.kt:7146">
P1: The `wireHandshakeQueued` latch in `sendWireHandshake()` is set to `true` before the BLE write is confirmed, and there is no failure path that clears it. If `bluetoothGatt.writeCharacteristic` returns false or throws (see `sendDataInternal`), the handshake packet is silently dropped, but `wireHandshakeQueued` remains true. Because `maybeSendWireHandshake()` short-circuits when that flag is true, the phone will never retry v2 negotiation for the rest of the connection, forcing the link to stay on legacy mode even though the glasses advertised binary capability.
A safer pattern is to only arm the latch once the characteristic write is accepted (e.g. after `writeCharacteristic` returns true), or to reset `wireHandshakeQueued` in the write-failure / `onCharacteristicWrite` error path so the handshake can be reattempted.</violation>
</file>
<file name="asg_client/app/src/test/java/com/mentra/asg_client/io/bluetooth/managers/mentralive/internal/BesWireFormatTest.java">
<violation number="1" location="asg_client/app/src/test/java/com/mentra/asg_client/io/bluetooth/managers/mentralive/internal/BesWireFormatTest.java:171">
P2: The length-field assertions in `formatMessageForTransmission_fallbackKeepsNegotiatedEndian` verify themselves instead of independently validating the encoded length. `readLength` reads bytes at positions 3–4, computes `length`, and then the assertions compare `packed[3]` and `packed[4]` against bytes derived from that same `length` — a circular check that always passes regardless of the actual length value. If the length encoding were ever incorrect (wrong endianness, wrong payload size), these assertions would not detect it.
Consider independently asserting the expected length value instead. For example, the fallback with `"{bad"` produces a payload of `{"C":"{bad"}` (12 bytes), so the BE-encoded length bytes at offsets 3–4 should be `0x00` and `0x0C` (or, more flexibly, assert that `K900LengthCodec.readLength(packed, 3, BE)` equals `12`).</violation>
</file>
<file name="asg_client/app/src/test/java/com/mentra/asg_client/service/core/processors/ChunkReassemblerTest.java">
<violation number="1" location="asg_client/app/src/test/java/com/mentra/asg_client/service/core/processors/ChunkReassemblerTest.java:23">
P2: The test `addBinaryFragment_duplicateFirstFragmentDoesNotResetProgress` cannot detect the bug it describes because the duplicate fragment uses identical data. If the implementation erroneously reset progress on receipt of a duplicate first fragment, the same data would be written again, producing the same final string `'hello'`. Use different data for the duplicate (e.g., `'HEL'`) so that a reset would yield `'HELlo'` instead of `'hello'`, making the assertion actually validate the no-reset guarantee.</violation>
</file>
<file name=".github/workflows/staging-builds.yml">
<violation number="1" location=".github/workflows/staging-builds.yml:908">
P1: The staging payload guard was narrowed from `prod_live_version.json` to `test_bes_ota_prod_live_version.json`, but the canonical production manifest filename used throughout the codebase is still `prod_live_version.json` (found in 7 files across mobile SDK, mobile app, and ASG client). The new filename does not exist anywhere else in the repository, so the guard is no longer effective at preventing accidental inclusion of the production manifest in the staging Pages payload. To preserve the intended channel isolation, continue checking for `prod_live_version.json` (either alongside or instead of the new name).</violation>
</file>
<file name="mobile/modules/bluetooth-sdk/ios/Source/sgcs/MentraLive.swift">
<violation number="1" location="mobile/modules/bluetooth-sdk/ios/Source/sgcs/MentraLive.swift:3709">
P2: The `wireHandshakeQueued` flag permanently suppresses handshake retry after a single attempt. If the queued handshake packet is dropped or the peer response is lost while the BLE link stays up, `maybeSendWireHandshake()` will never retry because `!wireHandshakeQueued` guards against it, and the flag is only cleared on disconnect or successful v2 activation. This can leave the session stuck on legacy protocol for the entire connection. Consider adding a short timeout or resetting the flag on a confirmed write error so the handshake can be retried.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| payload | ||
| ) | ||
| Bridge.log("LIVE: Sending BLE wire v2 handshake") | ||
| wireHandshakeQueued = true |
There was a problem hiding this comment.
P1: The wireHandshakeQueued latch in sendWireHandshake() is set to true before the BLE write is confirmed, and there is no failure path that clears it. If bluetoothGatt.writeCharacteristic returns false or throws (see sendDataInternal), the handshake packet is silently dropped, but wireHandshakeQueued remains true. Because maybeSendWireHandshake() short-circuits when that flag is true, the phone will never retry v2 negotiation for the rest of the connection, forcing the link to stay on legacy mode even though the glasses advertised binary capability.
A safer pattern is to only arm the latch once the characteristic write is accepted (e.g. after writeCharacteristic returns true), or to reset wireHandshakeQueued in the write-failure / onCharacteristicWrite error path so the handshake can be reattempted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/sgcs/MentraLive.kt, line 7146:
<comment>The `wireHandshakeQueued` latch in `sendWireHandshake()` is set to `true` before the BLE write is confirmed, and there is no failure path that clears it. If `bluetoothGatt.writeCharacteristic` returns false or throws (see `sendDataInternal`), the handshake packet is silently dropped, but `wireHandshakeQueued` remains true. Because `maybeSendWireHandshake()` short-circuits when that flag is true, the phone will never retry v2 negotiation for the rest of the connection, forcing the link to stay on legacy mode even though the glasses advertised binary capability.
A safer pattern is to only arm the latch once the characteristic write is accepted (e.g. after `writeCharacteristic` returns true), or to reset `wireHandshakeQueued` in the write-failure / `onCharacteristicWrite` error path so the handshake can be reattempted.</comment>
<file context>
@@ -7139,6 +7143,7 @@ class MentraLive : SGCManager() {
payload
)
Bridge.log("LIVE: Sending BLE wire v2 handshake")
+ wireHandshakeQueued = true
queueData(packed, null)
} catch (e: Exception) {
</file context>
| if [ -e asg-pages-site/test_bes_ota_prod_live_version.json ]; then | ||
| echo "Staging OTA payload must not contain test_bes_ota_prod_live_version.json." >&2 | ||
| exit 1 |
There was a problem hiding this comment.
P1: The staging payload guard was narrowed from prod_live_version.json to test_bes_ota_prod_live_version.json, but the canonical production manifest filename used throughout the codebase is still prod_live_version.json (found in 7 files across mobile SDK, mobile app, and ASG client). The new filename does not exist anywhere else in the repository, so the guard is no longer effective at preventing accidental inclusion of the production manifest in the staging Pages payload. To preserve the intended channel isolation, continue checking for prod_live_version.json (either alongside or instead of the new name).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/staging-builds.yml, line 908:
<comment>The staging payload guard was narrowed from `prod_live_version.json` to `test_bes_ota_prod_live_version.json`, but the canonical production manifest filename used throughout the codebase is still `prod_live_version.json` (found in 7 files across mobile SDK, mobile app, and ASG client). The new filename does not exist anywhere else in the repository, so the guard is no longer effective at preventing accidental inclusion of the production manifest in the staging Pages payload. To preserve the intended channel isolation, continue checking for `prod_live_version.json` (either alongside or instead of the new name).</comment>
<file context>
@@ -905,8 +905,8 @@ jobs:
- if [ -e asg-pages-site/prod_live_version.json ]; then
- echo "Staging OTA payload must not contain prod_live_version.json." >&2
+ if [ -e asg-pages-site/test_bes_ota_prod_live_version.json ]; then
+ echo "Staging OTA payload must not contain test_bes_ota_prod_live_version.json." >&2
exit 1
</file context>
| if [ -e asg-pages-site/test_bes_ota_prod_live_version.json ]; then | |
| echo "Staging OTA payload must not contain test_bes_ota_prod_live_version.json." >&2 | |
| exit 1 | |
| if [ -e asg-pages-site/prod_live_version.json ] || [ -e asg-pages-site/test_bes_ota_prod_live_version.json ]; then | |
| echo "Staging OTA payload must not contain production manifest." >&2 | |
| exit 1 | |
| fi |
| int length = K900LengthCodec.readLength(packed, 3, K900LengthCodec.Endian.BE); | ||
|
|
||
| assertThat(packed[2]).isEqualTo(BesWireFormat.CMD_TYPE_STRING); | ||
| assertThat(packed[3]).isEqualTo((byte) ((length >> 8) & 0xFF)); |
There was a problem hiding this comment.
P2: The length-field assertions in formatMessageForTransmission_fallbackKeepsNegotiatedEndian verify themselves instead of independently validating the encoded length. readLength reads bytes at positions 3–4, computes length, and then the assertions compare packed[3] and packed[4] against bytes derived from that same length — a circular check that always passes regardless of the actual length value. If the length encoding were ever incorrect (wrong endianness, wrong payload size), these assertions would not detect it.
Consider independently asserting the expected length value instead. For example, the fallback with "{bad" produces a payload of {"C":"{bad"} (12 bytes), so the BE-encoded length bytes at offsets 3–4 should be 0x00 and 0x0C (or, more flexibly, assert that K900LengthCodec.readLength(packed, 3, BE) equals 12).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At asg_client/app/src/test/java/com/mentra/asg_client/io/bluetooth/managers/mentralive/internal/BesWireFormatTest.java, line 171:
<comment>The length-field assertions in `formatMessageForTransmission_fallbackKeepsNegotiatedEndian` verify themselves instead of independently validating the encoded length. `readLength` reads bytes at positions 3–4, computes `length`, and then the assertions compare `packed[3]` and `packed[4]` against bytes derived from that same `length` — a circular check that always passes regardless of the actual length value. If the length encoding were ever incorrect (wrong endianness, wrong payload size), these assertions would not detect it.
Consider independently asserting the expected length value instead. For example, the fallback with `"{bad"` produces a payload of `{"C":"{bad"}` (12 bytes), so the BE-encoded length bytes at offsets 3–4 should be `0x00` and `0x0C` (or, more flexibly, assert that `K900LengthCodec.readLength(packed, 3, BE)` equals `12`).</comment>
<file context>
@@ -161,6 +161,17 @@ public void packDataCommand_bigEndianWritesLengthMsbFirst() {
+ int length = K900LengthCodec.readLength(packed, 3, K900LengthCodec.Endian.BE);
+
+ assertThat(packed[2]).isEqualTo(BesWireFormat.CMD_TYPE_STRING);
+ assertThat(packed[3]).isEqualTo((byte) ((length >> 8) & 0xFF));
+ assertThat(packed[4]).isEqualTo((byte) (length & 0xFF));
+ }
</file context>
| byte[] first = "hel".getBytes(StandardCharsets.UTF_8); | ||
| byte[] second = "lo".getBytes(StandardCharsets.UTF_8); | ||
|
|
||
| assertThat(reassembler.addBinaryFragment(firstFlag, 7, 0, 2, first)).isNull(); |
There was a problem hiding this comment.
P2: The test addBinaryFragment_duplicateFirstFragmentDoesNotResetProgress cannot detect the bug it describes because the duplicate fragment uses identical data. If the implementation erroneously reset progress on receipt of a duplicate first fragment, the same data would be written again, producing the same final string 'hello'. Use different data for the duplicate (e.g., 'HEL') so that a reset would yield 'HELlo' instead of 'hello', making the assertion actually validate the no-reset guarantee.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At asg_client/app/src/test/java/com/mentra/asg_client/service/core/processors/ChunkReassemblerTest.java, line 23:
<comment>The test `addBinaryFragment_duplicateFirstFragmentDoesNotResetProgress` cannot detect the bug it describes because the duplicate fragment uses identical data. If the implementation erroneously reset progress on receipt of a duplicate first fragment, the same data would be written again, producing the same final string `'hello'`. Use different data for the duplicate (e.g., `'HEL'`) so that a reset would yield `'HELlo'` instead of `'hello'`, making the assertion actually validate the no-reset guarantee.</comment>
<file context>
@@ -0,0 +1,30 @@
+ byte[] first = "hel".getBytes(StandardCharsets.UTF_8);
+ byte[] second = "lo".getBytes(StandardCharsets.UTF_8);
+
+ assertThat(reassembler.addBinaryFragment(firstFlag, 7, 0, 2, first)).isNull();
+ assertThat(reassembler.addBinaryFragment(firstFlag, 7, 0, 2, first)).isNull();
+
</file context>
| return | ||
| } | ||
| Bridge.log("LIVE: Sending BLE wire v2 handshake") | ||
| wireHandshakeQueued = true |
There was a problem hiding this comment.
P2: The wireHandshakeQueued flag permanently suppresses handshake retry after a single attempt. If the queued handshake packet is dropped or the peer response is lost while the BLE link stays up, maybeSendWireHandshake() will never retry because !wireHandshakeQueued guards against it, and the flag is only cleared on disconnect or successful v2 activation. This can leave the session stuck on legacy protocol for the entire connection. Consider adding a short timeout or resetting the flag on a confirmed write error so the handshake can be retried.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mobile/modules/bluetooth-sdk/ios/Source/sgcs/MentraLive.swift, line 3709:
<comment>The `wireHandshakeQueued` flag permanently suppresses handshake retry after a single attempt. If the queued handshake packet is dropped or the peer response is lost while the BLE link stays up, `maybeSendWireHandshake()` will never retry because `!wireHandshakeQueued` guards against it, and the flag is only cleared on disconnect or successful v2 activation. This can leave the session stuck on legacy protocol for the entire connection. Consider adding a short timeout or resetting the flag on a confirmed write error so the handshake can be retried.</comment>
<file context>
@@ -3703,12 +3706,14 @@ class MentraLive: NSObject, SGCManager {
return
}
Bridge.log("LIVE: Sending BLE wire v2 handshake")
+ wireHandshakeQueued = true
queueSend(packed, id: "-1")
}
</file context>
|
Backcompat review. The negotiation design is right (legacy by default, capability handshake), but it's gated on the wrong signals in both directions. Nothing breaks while BES stays at 17.26.05.22 — but these must be fixed before the LE BES firmware ever ships, and nothing later will force it. Blockers
v2-path bugs (dormant until LE fw — fix in this PR)
Regression independent of glasses fw
Nits: staging-builds.yml should guard both Verified good: v2 frame layout / key maps / enums are byte-identical across all three implementations, 0x40 can't collide with the legacy cmd namespace, OTA/auth + file-transfer framing untouched, and state resets on disconnect are correct. |
|
@nic-olo heads up — this branch's changes (asg + mobile wire-v2) have been pulled into aisraelov/os-1409-ble-v2-merged (PR #3349): it's this branch + One critical finding you'll want before shipping any of the fw side: wire-v2 firmware ↔ today's v1 phones = phone→glasses BLE JSON is completely dead (writes are ATT-acked then vanish in the BES RX path — |
|
Hello sir please see #3349 |
The glasses proactively sent the BLE wire v2 handshake after glasses_ready whenever the BES firmware advertised binary relay - a condition that says nothing about the PHONE's capabilities. That call flipped the entire phone-facing TX path to v2 binary frames (setBinaryProtocolActive), muting the glasses toward any pre-v2 phone app the moment v2 BES firmware ships (blockers 1+2 from the #3286 review). The correct negotiation already exists end-to-end: glasses_ready advertises wire_caps (old phones ignore the extra JSON key), a v2-capable phone initiates the handshake (maybeSendWireHandshake gates on those caps), and handleInboundBinaryFrame replies and activates. Drop the proactive send; the responder path is now the only activation site, so v2<->v2 pairs negotiate identically and v1 phones keep a working legacy link against v2 firmware.


Summary
##command frame length fields to little-endian (fixes UART parse failures with updated asg_client ↔ BES firmware).0x40), capability handshake (phone_ready/glasses_ready), MTU 509, compact JSON helpers, and app-layer dedup/coalescing/debounce for display, settings, camera, streaming, and menu_apps.Scope
45 commits (one per file) covering:
BesWireFormat, binary chunking/reassembly,BleJsonCompact,K900BluetoothManager,AsgClientServicehandshakeBleWireProtocol, LEK900ProtocolUtils, binary transport,MentraLiveAndroid/iOSstream_status, photo/camera dedupPairing note
Requires matching BES firmware with LE UART length encoding (
cmd_int2Chars/cmd_chars2Intinm8_prot.cpp). File packet fields remain big-endian.Test plan
Extracted length=9472in logcat)glasses_ready/phone_readyhandshakeasg_clientunit tests:BesWireFormatTest,BinaryMessageChunkerTest,BleJsonCompactTestK900ProtocolUtilsEndiannessTest,BinaryMessageChunkerTest,BleJsonCompactTestMade with Cursor