Mentra Live BLE photos: 3x transfer speed + WiFi-parity quality ladder (OS-1409)#3349
Merged
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>
Contributor
🤖 PR Agent Review — cycle 17✅ No blocking findings · 0 blocking · 0 nits No model reviews ran this cycle. Updated automatically by the PR Agent Orchestrator each review cycle. Nits do not block merge. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| + " (current=" | ||
| + currentFileTransfer.currentPacketIndex | ||
| + " (highestAcked=" | ||
| + currentFileTransfer.highestAckedIndex |
There was a problem hiding this comment.
Push mode NACK retries duplicate
High Severity
When the BES rejects a file packet with state=0, the handler sets currentPacketIndex back to the failed index and calls sendNextFilePacket(), which fills the push window from that index. Packets above that index may already be in flight, so this path re-sends them unlike the ack-timeout handler, which only calls sendFilePacketAt for one index.
Reviewed by Cursor Bugbot for commit 86ac10c. Configure here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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/src/services/SocketComms.ts">
<violation number="1" location="mobile/src/services/SocketComms.ts:198">
P2: Battery state can be skipped after websocket reconnects because deduping is global to the SocketComms singleton, not scoped to a connection session. Consider resetting `lastBatteryWsSignature` when the websocket reconnects (or on disconnect) so each new backend session gets the current battery snapshot.</violation>
</file>
<file name="mobile/src/stores/display.ts">
<violation number="1" location="mobile/src/stores/display.ts:57">
P2: Back-to-back display events for different views can lose one view’s latest state, so switching views may show stale content. The coalescer keeps only a single pending event across both buckets; flushing or storing pending state per view would preserve correctness while still reducing write frequency.</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:137">
P3: Test `createTransmissionWrapperJson_dropsVersionAndBodyWhenV2` (line) and `createTransmissionWrapperJson_keepsLowRoiExpandedOnV2` pass inputs without V/B fields (`{"type":"glasses_ready","timestamp":1}` and `{"type":"ping"}`), so the claimed "drops version and body" behavior is never actually tested. The assertions only verify V/B aren't injected. To properly test V/B stripping, pass a JSON that *has* V and B fields and verify they are removed from the output.</violation>
<violation number="2" location="asg_client/app/src/test/java/com/mentra/asg_client/io/bluetooth/managers/mentralive/internal/BesWireFormatTest.java:168">
P3: Tautological assertions in test `formatMessageForTransmission_fallbackKeepsNegotiatedEndian`: the `length` variable is derived from `readLength(packed, 3, BE)`, then used to verify `packed[3]` and `packed[4]` — this is circular. The assertions would pass with any consistent endian encoding (even swapped or wrong) because they derive the expected value from the same bytes under test. Check the length against the known input payload length instead.</violation>
</file>
<file name="asg_client/app/src/main/java/com/mentra/asg_client/io/bluetooth/managers/mentralive/internal/K900LengthCodec.java">
<violation number="1" location="asg_client/app/src/main/java/com/mentra/asg_client/io/bluetooth/managers/mentralive/internal/K900LengthCodec.java:71">
P3: Short K900 frames currently return the original array from `repackStringFrame`, so downstream mutation of the returned buffer can mutate the input unexpectedly. This contradicts the method contract that it returns a new array; cloning short non-null frames keeps behavior consistent.</violation>
</file>
<file name="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/utils/BleJsonCompact.java">
<violation number="1" location="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/utils/BleJsonCompact.java:394">
P2: Hash-only `resolvedConfig` updates can lose recovery metadata on Android: when cache lookup misses, the code drops `rch` anyway, leaving no `resolvedConfig` and no hash to correlate later. According to linked Linear issue OS-1409, Android/iOS behavior should stay in parity; matching iOS here avoids silent context loss after session/cache desync.</violation>
</file>
<file name="asg_client/app/src/main/java/com/mentra/asg_client/service/core/processors/ChunkedMessageProtocolStrategy.java">
<violation number="1" location="asg_client/app/src/main/java/com/mentra/asg_client/service/core/processors/ChunkedMessageProtocolStrategy.java:69">
P3: This introduces a public binary-payload parsing path that is never executed, so future fixes can drift between the real flow and this dead helper. Either wire this method into the inbound binary handling flow or remove it to keep one authoritative parse path.</violation>
</file>
<file name="mobile/src/utils/debouncedPatch.ts">
<violation number="1" location="mobile/src/utils/debouncedPatch.ts:24">
P2: A synchronous error in `flush` drops the queued patch permanently because `pending` is cleared before the callback runs. Keeping `pending` until after a successful flush preserves data for retry/error handling.</violation>
</file>
<file name="mobile/src/services/miniapps/MiniappCatalog.ts">
<violation number="1" location="mobile/src/services/miniapps/MiniappCatalog.ts:419">
P2: Recent menu edits can be reverted when a delayed sync fires, because the timer callback writes a previously computed `next` snapshot without checking for newer `menu_apps` changes. Merging only running-state updates into the latest stored menu list would avoid clobbering user ordering/selection changes.</violation>
</file>
<file name="mobile/modules/bluetooth-sdk/src/BluetoothSdk.types.ts">
<violation number="1" location="mobile/modules/bluetooth-sdk/src/BluetoothSdk.types.ts:545">
P2: `appId` is declared in `PhotoRequestParams` but never reaches native or any JS consumer — it's silently dropped at `photoRequestParamsForNative()` and the Android/iOS native models don't have the field either. If this field is meant for JS-side routing, add a consumer; if it's intended for the wire or native pipeline, plumb it through `photoRequestParamsForNative()`, `PhotoRequest.fromMap()`, and `PhotoRequest.fromDictionary()`. Otherwise, remove it to avoid misleading API surface.</violation>
</file>
<file name="mobile/src/services/streaming/PhoneStreamCoordinator.ts">
<violation number="1" location="mobile/src/services/streaming/PhoneStreamCoordinator.ts:453">
P1: `slimData` replaces the full event in the fanout payload, dropping kind-specific fields that miniapps may rely on: `errorDetails` (error events), `attempt`/`maxAttempts`/`reason` (reconnect events), and `streaming`/`reconnecting` (snapshot events) are all silently stripped. The slimmed payload is correct for the dedup signature but should not replace the forwarded data — miniapps lose visibility into reconnection progress, error detail, and snapshot state. Keep using `slimData` only for the signature check and forward the full event (or extend slimStreamStatusEvent to preserve kind-specific fields) so downstream consumers continue receiving the complete status data.</violation>
</file>
<file name="asg_client/app/src/main/java/com/mentra/asg_client/io/bluetooth/managers/mentralive/internal/BesWireFormat.java">
<violation number="1" location="asg_client/app/src/main/java/com/mentra/asg_client/io/bluetooth/managers/mentralive/internal/BesWireFormat.java:187">
P3: Camera-command detection is duplicated in two places, and this new copy now gates binary wrapper behavior. Centralizing on `BleJsonCompact.isCameraCommandJson` would reduce drift risk where one path updates camera detection and the other silently diverges.</violation>
<violation number="2" location="asg_client/app/src/main/java/com/mentra/asg_client/io/bluetooth/managers/mentralive/internal/BesWireFormat.java:602">
P1: The no-arg payload extractor now assumes little-endian frames, which breaks legacy big-endian `##` messages for any caller that still uses this helper. The compatibility-safe path is already present as `extractPayloadAuto()`, so the default should stay legacy-safe or the callers need to be updated together.</violation>
</file>
<file name="mobile/src/services/outboundJson.ts">
<violation number="1" location="mobile/src/services/outboundJson.ts:9">
P1: `Date`, `RegExp`, `Map`, and `Set` instances are silently stripped from the payload. The `typeof value === "object"` check catches them, `!Array.isArray(value)` passes, and `Object.keys(value).length === 0` is true for `Date`/`RegExp` — so they get dropped as if they were empty `{}`. If any caller passes a `Date` (e.g., a future `timestamp` field using `new Date()` instead of `Date.now()`), it disappears without warning.</violation>
</file>
<file name="asg_client/app/src/main/java/com/mentra/asg_client/service/core/processors/ChunkReassembler.java">
<violation number="1" location="asg_client/app/src/main/java/com/mentra/asg_client/service/core/processors/ChunkReassembler.java:20">
P2: Large wire-v2 JSON payloads can be dropped mid-transfer because binary reassembly is capped at 4 KB even though the protocol supports much larger fragmented messages. Consider sizing this limit to the protocol maximum (or making it dynamic) so valid multi-fragment payloads don’t fail with overflow.</violation>
<violation number="2" location="asg_client/app/src/main/java/com/mentra/asg_client/service/core/processors/ChunkReassembler.java:133">
P2: An out-of-order or erroneous binary fragment (e.g., `fragIdx != receivedCount` or buffer overflow) silently destroys the entire session — all previously accumulated fragments for that `msgId` are lost and the caller gets `null`, the same return value as "still waiting for more fragments." The phone-side implementation tolerates reordering gracefully via a fragment map, but this glasses-side path uses a sequential buffer and evicts the session on any ordering error. While BLE transport typically delivers in order, a retransmission storm, dual-path delivery, or protocol bug could trigger this silent data loss with no recovery signal to the sender. Consider keeping the session alive on transient failures and only removing it on timeout, or distinguishing the error from "in progress" so upstream logic can react.</violation>
</file>
<file name="asg_client/app/src/main/java/com/mentra/asg_client/io/bluetooth/managers/mentralive/internal/MessageChunker.java">
<violation number="1" location="asg_client/app/src/main/java/com/mentra/asg_client/io/bluetooth/managers/mentralive/internal/MessageChunker.java:33">
P2: Binary mode can skip chunking for payloads that become larger after wire wrapping, which can produce oversized single-frame binary messages and intermittent send/drop behavior. It would be safer to base the binary chunking decision on `buildOutboundPayloadBytes(message).length` (or equivalent encoded payload length), not the raw input string length.</violation>
</file>
<file name="asg_client/app/src/main/java/com/mentra/asg_client/io/bluetooth/utils/BleJsonCompact.java">
<violation number="1" location="asg_client/app/src/main/java/com/mentra/asg_client/io/bluetooth/utils/BleJsonCompact.java:321">
P2: `start_stream` can silently flip boolean intent: compacting currently drops all `false` values, so `sound:false` is lost and the handler falls back to `sound=true`. Consider not globally omitting `false` booleans during compaction (or only omitting a strict per-field allowlist).</violation>
</file>
<file name="mobile/src/services/photo/PhonePhotoCoordinator.test.ts">
<violation number="1" location="mobile/src/services/photo/PhonePhotoCoordinator.test.ts:87">
P2: The test rewrite drops all error-path coverage for takePhoto — the old suite had 6 error scenarios (cloud failure, BLE send failure, poll errors, handlePhotoError races). PhonePhotoCoordinator.ts implements the same error handling (PHOTO_REQUEST_FAILED at line 76, BLE_SEND_FAILED at lines 120 and 125, CAPTURE_TIMEOUT at line 100, and handlePhotoError at lines 221-226). Restoring tests for the critical paths (cloud presign rejection, BLE send sync/async failure, handlePhotoError race) would prevent regressions in the error-handling logic that the rest of the stack depends on.</violation>
<violation number="2" location="mobile/src/services/photo/PhonePhotoCoordinator.test.ts:123">
P2: warmUpCamera test suite skips the disconnected-glasses case — warmUpCamera has the same GLASSES_NOT_CONNECTED guard as takePhoto (PhonePhotoCoordinator.ts:174) but only the happy path is tested. Add a test that verifies the PhotoError with code "GLASSES_NOT_CONNECTED" when glassesSnapshot.connected = false, to match the coverage level the rest of the suite provides for takePhoto.</violation>
</file>
<file name="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/sgcs/MentraLiveL2capChannel.kt">
<violation number="1" location="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/sgcs/MentraLiveL2capChannel.kt:189">
P2: L2CAP and GATT now maintain separate implementations of the same K900 frame parser, which increases the chance that one path drifts when framing rules change. Consider routing both paths through one shared parser/helper so packet validation behavior stays identical.</violation>
</file>
<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:1470">
P2: `sendDataToGlassesBinary()` calls `Thread.sleep(50)` between fragment enqueues directly on whatever thread `sendJsonToDevice` runs on. This is called from message-handler callbacks that may also be responsible for processing incoming BLE data. Each fragment blocks the thread for 50ms — for a multi-fragment message this can add hundreds of milliseconds to 1+ second of blocking during which no other outbound messages can be submitted and no receiver processing can proceed on that thread. The old chunked path did not have this blocking delay. Consider replacing the blocking sleep with a delayed enqueue mechanism (e.g., a Handler with postDelayed or a dedicated pacing loop on an independent thread) so the caller thread can return promptly.</violation>
<violation number="2" location="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/sgcs/MentraLive.kt:3552">
P2: According to linked Linear issue OS-1409, the CoC path should be gated by firmware capability/version before fallback to GATT. This branch opens L2CAP unconditionally, so older firmware still incurs connection attempts/timeouts; consider gating `openL2capFileChannel()` on an advertised capability/version flag.</violation>
<violation number="3" location="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/sgcs/MentraLive.kt:7292">
P3: Incoming wire metrics can be misleading for fragmented binary messages because `wire` logs only the final fragment size. Consider logging cumulative fragment wire bytes for the reassembled message instead of `data.size` from the last callback.</violation>
</file>
<file name="mobile/modules/bluetooth-sdk/ios/Source/utils/BleJsonCompact.swift">
<violation number="1" location="mobile/modules/bluetooth-sdk/ios/Source/utils/BleJsonCompact.swift:236">
P2: `start_stream` can lose an explicit `sound: false`, so a muted stream request may be interpreted as default audio-on. The compactor currently removes all false booleans globally; keeping explicit boolean fields (or limiting omission to specific keys) avoids this behavior change.</violation>
</file>
<file name=".github/workflows/staging-builds.yml">
<violation number="1" location=".github/workflows/staging-builds.yml:909">
P2: The guard that prevents the production OTA manifest from being deployed to the staging Cloudflare Pages site has been narrowed from checking for `prod_live_version.json` to checking only for `test_bes_ota_prod_live_version.json`. Unless the production manifest's filename has been formally changed, the old `prod_live_version.json` — or any other production OTA artifact not matching the new specific name — could now reach the staging site undetected. Consider either keeping the original guard alongside the new one, or verifying that a separate mechanism now prevents production manifest deployment to staging.</violation>
</file>
<file name="mobile/modules/bluetooth-sdk/ios/Source/utils/MessageChunker.swift">
<violation number="1" location="mobile/modules/bluetooth-sdk/ios/Source/utils/MessageChunker.swift:262">
P3: `allBinaryFragmentsFit` is defined in the iOS `MessageChunker` but never called. This adds maintenance surface (tests, binary size, API docs) without any use. Remove it unless an imminent caller is planned.</violation>
</file>
<file name="mobile/src/services/MantleManager.ts">
<violation number="1" location="mobile/src/services/MantleManager.ts:1243">
P2: The stream status event is slimmed twice — once in MantleManager before passing to `sendStreamStatus`, and once inside `sendStreamStatus` itself. Since `slimStreamStatusEvent` is idempotent, the behavior is correct, but it makes the code harder to reason about: a future change to one path (adding/removing fields in one slim but not the other) could silently diverge. Consider either (a) removing the `slimStreamStatusEvent` call from `SocketComms.sendStreamStatus` and having callers always pass raw events, or (b) removing the pre-slim in MantleManager and letting `sendStreamStatus` handle it consistently.</violation>
</file>
<file name="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/utils/BlePhotoUploadService.java">
<violation number="1" location="mobile/modules/bluetooth-sdk/android/src/main/java/com/mentra/bluetoothsdk/utils/BlePhotoUploadService.java:43">
P2: JPEG quality 100 produces substantially larger files than q95 (typically 2-5x) with no visible quality gain, while Android's encoder still applies lossy chroma subsampling (4:2:0) at any quality level. The stated goal is to not compound AVIF loss, but q95 already achieves that — the source has quantization artifacts that quality cannot recover, and subsampling applies at both q95 and q100. The PR description targets q95; q100 adds upload time/bandwidth cost without a visible benefit.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // 100: the source already went through a lossy AVIF pass on the glasses, so | ||
| // this re-encode must not compound the loss. Phone CPU and upload bandwidth | ||
| // are cheap relative to what was paid to get the bytes over BLE. | ||
| private static final int JPEG_QUALITY = 100; |
Contributor
There was a problem hiding this comment.
P2: JPEG quality 100 produces substantially larger files than q95 (typically 2-5x) with no visible quality gain, while Android's encoder still applies lossy chroma subsampling (4:2:0) at any quality level. The stated goal is to not compound AVIF loss, but q95 already achieves that — the source has quantization artifacts that quality cannot recover, and subsampling applies at both q95 and q100. The PR description targets q95; q100 adds upload time/bandwidth cost without a visible benefit.
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/BlePhotoUploadService.java, line 43:
<comment>JPEG quality 100 produces substantially larger files than q95 (typically 2-5x) with no visible quality gain, while Android's encoder still applies lossy chroma subsampling (4:2:0) at any quality level. The stated goal is to not compound AVIF loss, but q95 already achieves that — the source has quantization artifacts that quality cannot recover, and subsampling applies at both q95 and q100. The PR description targets q95; q100 adds upload time/bandwidth cost without a visible benefit.</comment>
<file context>
@@ -37,9 +37,10 @@
+ // 100: the source already went through a lossy AVIF pass on the glasses, so
+ // this re-encode must not compound the loss. Phone CPU and upload bandwidth
+ // are cheap relative to what was paid to get the bytes over BLE.
+ private static final int JPEG_QUALITY = 100;
public interface UploadCallback {
</file context>
Member
Author
There was a problem hiding this comment.
Intentional. This is a re-encode of an image that was already lossy-compressed (AVIF) on the glasses — any q<100 here compounds generational loss on exactly the text detail the OS-1409 ladder was tuned to preserve. The size hit is paid on the phone→cloud WiFi/LTE leg, not BLE, so transfer time is unaffected by the constraint that matters. Chroma subsampling: Android's JPEG encoder switches to 4:4:4 at q100 on the devices we target; even where it doesn't, q100 is still strictly less loss than q95.
Contributor
There was a problem hiding this comment.
The point about this being a lossy AVIF re-encode is fair; given that pipeline, the q95 recommendation was too broad here. On this PR, q100 is intentional for preserving text detail, and the upload cost is on the phone→cloud leg rather than BLE.
The BES UART RX buffer is capped just under 4KB by its HAL's single-descriptor DMA limit (8KB variant was DOA in fw 17.26.7.6), so the in-flight push window must fit ~8 full packs. Firmware now acks every 4th pack, so window 8 still streams continuously. Gate moves to 17.26.7.7 - the only firmware with working batching.
- Retry-after-phone-failure rewound currentPacketIndex but not highestAckedIndex; completion keys off the ack high-water mark, so a retry declared itself complete without resending anything. Reset the mark to -1 in the retry path. - compareDottedVersions threw on hotfix-suffixed firmware strings (17.26.7.5-fix1), silently disabling the baud/batched-ack gates. Components now compare by their leading numeric prefix. - SerialPortBridge.reopen() nulls the stream fields while send/recv threads are mid-call; snapshot the streams (and mark them volatile) so a mid-reopen send fails with a handled IOException instead of NPE. - Testing notes: 17.26.7.6 -> 17.26.7.7, window 8, batch 4.
Member
Author
|
Review-bot triage. cubic's findings split two ways: On this PR's commits — fixed in bfaa3e4:
On this PR's commits — intentional, replied inline: phone re-encode at q100 (no generational loss on text; size paid on the WiFi leg), unconditional L2CAP attempt (fails fast to GATT; proper gating belongs in the wire-v2 caps exchange), separate L2CAP/GATT frame parsers (stream vs framed transport; consolidation queued). Inherited from the base branch ( Also for the record: firmware 17.26.7.6 is DOA (its 8KB UART RX buffer exceeds the BES HAL's 4095-byte single-descriptor DMA limit → UART RX dead → 'Mentra Live is not ready yet'). Fixed as 17.26.7.7 (fengyue120/mentra-live-bes#6); this PR's batched-ack gate + window are retuned to match (gate 17.26.7.7, window 8, fw acks every 4th pack). |
activateBinaryWireV2Session(logMessage:) was called without its argument label at two sites introduced in the dev merge resolution. This was the real cause of the three iOS CI failures (the gh CLI truncates step logs, hiding the BUILD FAILED section).
| // sr_syvr answers. | ||
| private static final int[] BOOT_BAUD_CANDIDATES = { | ||
| SerialPortBridge.DEFAULT_BAUDRATE, TARGET_UART_BAUD, 1152000 | ||
| }; |
There was a problem hiding this comment.
Boot baud recovery array contains duplicate entry
Low Severity
BOOT_BAUD_CANDIDATES is {DEFAULT_BAUDRATE, TARGET_UART_BAUD, 1152000} but TARGET_UART_BAUD is already 1152000, making the array effectively {460800, 1152000, 1152000}. The boot recovery tick cycles through candidates, so it wastes one full BOOT_RECOVERY_STEP_MS (3 seconds) retrying the same baud rate before attempting the next distinct candidate.
Reviewed by Cursor Bugbot for commit 8779603. Configure here.
| private static long sessionConnectEpochMs; | ||
| private static boolean resolvedConfigSent; | ||
| private static String currentSessionResolvedConfigHash; | ||
| private static final Map<String, JSONObject> resolvedConfigByHash = new HashMap<>(); |
There was a problem hiding this comment.
BleJsonCompact uses unsynchronized HashMap across threads
Medium Severity
resolvedConfigByHash is a plain HashMap with static mutable state (sessionConnectEpochMs, resolvedConfigSent, currentSessionResolvedConfigHash) accessed without synchronization. encode() is called from the send path and decodeIfSupported() from the serial receive thread — concurrent put/get on a HashMap can corrupt internal state, causing silent data loss or ConcurrentModificationException.
Reviewed by Cursor Bugbot for commit 8779603. Configure here.
Firmware 17.26.7.8 grew the UART RX ring to 12KB (the 4095B HAL cap only ever applied to the oversized initial DMA arm, not the ring), accepts 800B UART packs and splits them into standard 400B BLE frames itself, and acks every 8th UART pack. On that firmware the sender now uses 800B packs (halved framing + ack overhead on the UART leg, which is the bottleneck) with a 12-pack push window, and writes the REAL file size into headers - the ceil-to-400 inflate trick would make split firmware expect a phantom trailing BLE frame whenever the last pack is <= 400B. Pack size is snapshotted per transfer session. Older firmware keeps the existing gates (7.7: window 8 / batch 4; earlier: window 3, per-pack acks).
The RenderScript bitmap-backed Allocation crashes natively on the K900 (ABitmap_getPixels SEGV in librs_jni, 3/3 reproducible) and a native crash kills the whole asg service mid-photo - the Java fallback never runs. Same plus-shaped unsharp kernel (identity + 0.6x Laplacian) as a fixed-point int[] convolution; ~100ms at 1280px, negligible inside the photo pipeline. Found in first on-device run of the quality ladder.
Contributor
PR Agent HandoffExit reason: Budget exhausted — fix or review cycle cap reached.
CI status
Remaining blocking findingsNone Nits (informational)None Humans merge when ready. Agents do not auto-merge. Label |
Contributor
PR Agent HandoffExit reason: Budget exhausted — fix or review cycle cap reached.
CI status
Remaining blocking findingsNone Nits (informational)None Humans merge when ready. Agents do not auto-merge. Label |
createScaledBitmap returns the SAME bitmap object when the requested size equals the source size - and the medium BLE tier (1280 long edge) exactly matches the camera's capture resolution, so the unconditional original.recycle() killed the bitmap before the sharpen + AVIF encode (GetPixelsException on device). Recycle only when a copy was made.
Contributor
PR Agent HandoffExit reason: Budget exhausted — fix or review cycle cap reached.
CI status
Remaining blocking findingsNone Nits (informational)None Humans merge when ready. Agents do not auto-merge. Label |
Contributor
PR Agent HandoffExit reason: Budget exhausted — fix or review cycle cap reached.
CI status
Remaining blocking findingsNone Nits (informational)None Humans merge when ready. Agents do not auto-merge. Label |
liblhsserial opens the tty with O_NONBLOCK (verified by disassembling the JNI open: flags 0x902), so the push-mode design assumption that 'the UART write blocks and self-paces at line rate' was wrong: writes land in the kernel's ~4KB tty TX buffer instantly and the window-12 x 832B burst overruns it - FileOutputStream.write throws EAGAIN at packet ~4 after an unknown number of bytes already left the process, aborting the transfer (and corrupting the stream if retried). The old 8x432B window just fit under 4KB, which is why this never fired before. Write through Os.write for exact-byte accounting and drain EAGAIN with 2ms waits (bounded at 1s cumulative), restoring the intended block-at-line-rate pacing for send/sendFile/sendOta.
Contributor
PR Agent HandoffExit reason: Budget exhausted — fix or review cycle cap reached.
CI status
Remaining blocking findingsNone Nits (informational)None Humans merge when ready. Agents do not auto-merge. Label |
fw 17.26.7.9 deterministically loses pack 0 of an 800B transfer in its UART frame reassembly (packs 1+ parse intact; BES trace shows 'unexpected pack index: expect=0, got=1/2/3' with next_request stuck at 0). Needs DUMP8-instrumented firmware to localize - park the 800B path and keep the validated wins: batched acks, window 12, EAGAIN-drained serial writes, all at 400B packs.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 8 total unresolved issues (including 6 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fae57c4. Configure here.
| synchronized (baudSwitchLock) { | ||
| if (baudProbePending || comManager.mbOtaUpdating) { | ||
| return; // A switch/OTA is mid-flight; stay out of its way. | ||
| } |
There was a problem hiding this comment.
Boot recovery stops after OTA
Medium Severity
When bootBaudRecoveryTick sees mbOtaUpdating or baudProbePending, it returns without scheduling the next tick. If lastSrSyvrTime is still zero, baud hunting never runs again after OTA finishes, leaving a mismatched UART rate silent until reboot.
Reviewed by Cursor Bugbot for commit fae57c4. Configure here.
| Log.e(BAUD_TAG, "Failed to send cs_baud - staying at 460800"); | ||
| synchronized (baudSwitchLock) { | ||
| baudSwitchWaitingSrBaud = false; | ||
| } |
There was a problem hiding this comment.
cs_baud failure blocks retry
Medium Severity
onSrSyvrForBaudSwitch sets baudSwitchAttempted before sending cs_baud, but a failed sendMessage only clears baudSwitchWaitingSrBaud. The attempted flag stays true, so later sr_syvr responses skip negotiation and the link stays at 460800 for the whole boot despite a recoverable send error.
Reviewed by Cursor Bugbot for commit fae57c4. Configure here.
Contributor
PR Agent HandoffExit reason: Budget exhausted — fix or review cycle cap reached.
CI status
Remaining blocking findingsNone Nits (informational)None Humans merge when ready. Agents do not auto-merge. Label |
Same root as the 800B pack loss: under multi-pack bursts fw 17.26.7.9 deterministically loses an early pack (pack 1 at 400B window 12, pack 0 at 800B) and the reject/rewind interplay collapses throughput to ~11KB/s with frequent aborts. Window 3 + per-pack acks is the hardware-validated configuration: measured 53.5-54.4 KB/s end-to-end on fw 17.26.7.9 with the EAGAIN-drained serial writer (up from the 45-50 KB/s baseline). Batching/big-packs re-enable once the fw RX path survives bursts.
Contributor
PR Agent HandoffExit reason: Budget exhausted — fix or review cycle cap reached.
CI status
Remaining blocking findingsNone Nits (informational)None Humans merge when ready. Agents do not auto-merge. Label |
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.
Contributor
PR Agent HandoffExit reason: Budget exhausted — fix or review cycle cap reached.
CI status
Remaining blocking findingsNone Nits (informational)None Humans merge when ready. Agents do not auto-merge. Label |
Contributor
PR Agent HandoffExit reason: Budget exhausted — fix or review cycle cap reached.
CI status
Remaining blocking findingsNone Nits (informational)None Humans merge when ready. Agents do not auto-merge. Label |
PhilippeFerreiraDeSousa
pushed a commit
that referenced
this pull request
Jul 7, 2026
Brings in BLE Wire Protocol v2 (#3349). Resolutions per cluster: K900/endianness: take dev's K900LengthCodec-based implementation wholesale in K900ProtocolUtils.java and MentraLive.swift — this branch's earlier consolidation (fdbb147) dissolves into it. Re-apply the two branch-only Swift hunks (cached-peripheral prefix match, Cloud V2 report artifact path). PhotoRequestTest.kt takes dev's org.junit + AssertJ form. Island successors of files dev changed: port the short 4-hex BLE requestId (+appId) mapping into island PhonePhotoCoordinator and its bun suite (including dev's two short-id tests); land slimStreamStatus.ts in island services (PhoneStreamCoordinator's fanout slimming auto-merged onto the island copy); photo_response short-id resolution goes to island DeviceEventRouter. Files dev changed that stay deleted: MiniappCatalog.ts (dev's menu_apps write batching lives in the Cloud V1 applet poll pipeline; the island-era BuiltInMiniappCatalog sync is event-driven with its own coalescing), SocketComms sendStreamStatus/sendBatteryStatus slimming (V1 relays deleted), RestComms updateGlassesState debounce (device-state sync deleted, WP10), host outboundJson.ts + debouncedPatch.ts (all host consumers deleted; the debounce util moves into island). V1-bridge survivors: SocketComms.handle_display_event passes the event object (not a JSON string) to the display store; MantleManager drops the duplicate timezone key in writeUserSettings. Stores: island display store adopts dev's 150ms display-event coalesce (+flushDisplayCoalesceForTests, re-exported through the @/stores/display shim); island settings store adopts device-model-filtered getBluetoothSettings via bluetoothSettingKeys.ts moved into island (host shim kept for the bun settings test, importing the island file directly so bun avoids the react-native barrel). Dev's 300ms merged-patch debounce for BLE setting pushes lands in island GlassesSettingsSync + MicStateCoordinator (dev had it on MantleManager's hook seams). Reorder the bluetooth-sdk ./types export react-native-before-types to satisfy dev's new module-resolution invariant test. Gates: mobile tsc clean, mobile jest 54 suites / 422 tests, island 220 tests, boundary script clean (empty allowlist), bluetooth-sdk + asg Android compile checks green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
Implements OS-1409 across phone, asg_client, and the BES firmware contract. Firmware counterpart PR: fengyue120/mentra-live-bes#6 (v17.26.7.6 — must ship together with, or before, this branch's asg for the new transport features to engage; everything degrades gracefully against older fw). This branch is based on
feat/ble-wire-protocol-v2(#3286) withdevmerged in — it contains Nicolo's wire-v2 work plus the throughput/quality stack on top.What's in it
Transport (hardware-measured):
cs_baud/sr_baud), probe + auto-revert on both sides, boot-time baud recovery if asg restarts mid-session. 1.5M/2M/3M tested on hardware: negotiate fine, carry nothing — 1.152M is the current stack's ceilingFILE_PACK_SIZE_MAX221→400); batched-ack mode advertised viaFILE_FLAG_PUSH_BATCH_ACKrequestConnectionPriority(HIGH)+setPreferredPhy(2M)on connect (parity with Nex/G2)Quality (the actual point):
Measured (Pixel 6a ↔ Mentra Live)
Test evidence / pending
Known issues surfaced (documented on OS-1409)
Note
High Risk
Changes sit on the critical glasses↔BES UART and phone BLE protocol path (baud switch, framing, file transfer, v2 negotiation); regressions can break connectivity, OTA gating, or transfers on mixed firmware/phone versions.
Overview
asg_client overhauls the K900/BES path for faster BLE photos and wire-v2 phones: negotiated K900 STRING endianness, BLE binary v2 frames with compact JSON (
BleJsonCompact), phone-initiated handshake only (no proactive v2 flip onglasses_ready), andwire_capson handshake/version chunks.UART/file path: runtime 460800 → 1.152M baud (
cs_baud/sr_baud) with probe/revert and boot baud recovery;SerialPortBridge.reopenplus EAGAIN-safe full writes for push streaming. File sends use an ack high-water push window (window 3 in this branch—batched acks and 800B UART packs are wired but disabled pending firmware burst-loss fixes). Default BLE file pack size rises 221 → 400 when MTU allows.Media: BLE photo tiers get higher resolution and AVIF quality, a post-downscale unsharp pass, and ble_photo_ready → file gap 200ms → 20ms.
Minor: staging workflow blocks renamed prod manifest;
.gitignore/ Jest test-path tweaks.Reviewed by Cursor Bugbot for commit d84912d. Bugbot is set up for automated code reviews on this repo. Configure here.