Add chunked PhoneAPI transport payloads#10928
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR bumps the protobufs submodule reference and adds chunked-payload transport in ChangesChunked Payload Support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PhoneAPI
participant ChunkSlots
Client->>PhoneAPI: send chunked ToRadio payload
PhoneAPI->>ChunkSlots: allocate or locate reassembly slot
PhoneAPI->>ChunkSlots: append chunk bytes
alt payload complete
PhoneAPI->>PhoneAPI: forward reassembled bytes
else invalid or overflow
PhoneAPI->>ChunkSlots: clear slot
end
Client->>PhoneAPI: getFromRadio()
PhoneAPI->>ChunkSlots: check pending outbound chunk
alt chunk pending
PhoneAPI-->>Client: return chunk bytes
else encode new FromRadio
PhoneAPI->>PhoneAPI: maybeEncodeChunkedFromRadio(encoded bytes)
PhoneAPI-->>Client: return chunked or original bytes
end
Related PRs None Suggested labels None Suggested reviewers None 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
@kernel-oops, Welcome to Meshtastic!Thanks for opening your first pull request. We really appreciate it. We discuss work as a team in discord, please join us in the #firmware channel. Welcome to the team 😄 |
…-phoneapi # Conflicts: # protobufs
|
Local validation after rebasing onto current
The protobuf PR is now ready for review as well: meshtastic/protobufs#980. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mesh/PhoneAPI.cpp (1)
179-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Throttlefor the stale-slot timeout instead of rawmillis()arithmetic.This stale-transfer expiry is a "did N ms pass since X" check and should go through the
Throttlehelper.♻️ Proposed change
-static ChunkedToRadioSlot *findChunkedToRadioSlot_LH(PhoneAPI *api) -{ - const uint32_t now = millis(); - for (auto &slot : g_chunkedToRadioSlots) { - if (slot.who != nullptr && now - slot.updatedMillis > CHUNKED_TORADIO_TIMEOUT_MS) { - clearChunkedToRadioSlot_LH(slot); - } - } +static ChunkedToRadioSlot *findChunkedToRadioSlot_LH(PhoneAPI *api) +{ + for (auto &slot : g_chunkedToRadioSlots) { + if (slot.who != nullptr && !Throttle::isWithinTimespanMs(slot.updatedMillis, CHUNKED_TORADIO_TIMEOUT_MS)) { + clearChunkedToRadioSlot_LH(slot); + } + }As per coding guidelines: "Use
Throttlefor time-based rate limiting instead of rawmillis()arithmetic; preferThrottle::isWithinTimespanMs(lastMs, intervalMs)... for 'did N ms pass since X' checks."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/PhoneAPI.cpp` around lines 179 - 186, The stale-slot expiry check in findChunkedToRadioSlot_LH is using raw millis() subtraction to decide whether CHUNKED_TORADIO_TIMEOUT_MS has elapsed. Replace that logic with the Throttle helper, using the existing slot timestamp field and Throttle::isWithinTimespanMs(...) for the “has N ms passed since X” check before calling clearChunkedToRadioSlot_LH. Keep the behavior the same, but route the timeout decision through Throttle to match the time-based rate-limiting guideline.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/mesh/PhoneAPI.cpp`:
- Around line 179-186: The stale-slot expiry check in findChunkedToRadioSlot_LH
is using raw millis() subtraction to decide whether CHUNKED_TORADIO_TIMEOUT_MS
has elapsed. Replace that logic with the Throttle helper, using the existing
slot timestamp field and Throttle::isWithinTimespanMs(...) for the “has N ms
passed since X” check before calling clearChunkedToRadioSlot_LH. Keep the
behavior the same, but route the timeout decision through Throttle to match the
time-based rate-limiting guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ccf108a-0720-4d93-a686-99de0a8e10ee
⛔ Files ignored due to path filters (1)
src/mesh/generated/meshtastic/mesh.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (2)
protobufssrc/mesh/PhoneAPI.cpp
2 participants
Summary
This adds a compact chunked PhoneAPI transport wrapper so clients with a small GATT payload limit can exchange full Meshtastic client API protobufs without relying on ATT long reads or long writes.
The immediate motivation is Garmin Connect IQ BLE support. Garmin limits application BLE writes to 20 bytes and does not implement ATT long reads, while normal
ToRadioandFromRadioprotobufs can be much larger than that. Without explicit application-layer chunking, a Garmin client either cannot send larger requests or can misinterpret repeatedfromradioreads as continuations of the same protobuf.Depends on the corresponding protobufs schema PR: meshtastic/protobufs#980
What changed
ChunkedPayloadas a compact ordered wrapper:ToRadio.chunked_payload = 8FromRadio.chunked_payload = 20payload_sizepayload_chunkPhoneAPIreassembly for incomingToRadio.chunked_payloadmessages.FromRadio.chunked_payloademission for clients that have successfully completed a chunkedToRadiotransfer.mesh.pb.hfor the new schema.Transport behavior
The chunking format intentionally assumes ordered, reliable delivery on a single client connection. That matches Garmin's serialized BLE write-with-response and read flow and avoids spending bytes on payload ids and chunk indices in every 20-byte packet.
For Garmin-sized packets:
ToRadiofirst chunk carries 13 payload bytes.ToRadiocontinuation chunks carry 16 payload bytes.FromRadiofirst chunk carries 12 payload bytes.FromRadiocontinuation chunks carry 15 payload bytes.The smaller
FromRadiofirst/continuation sizes account for the largerFromRadio.chunked_payloadfield tag while keeping each serialized wrapper at or below 20 bytes.Safety and compatibility considerations
FromRadiochunking is opt-in only. Legacy clients continue receiving existing unchunkedFromRadiomessages.ToRadiotransfer is reassembled and accepted into the normalPhoneAPIpath.PhoneAPI*and protected by a small lock-guarded slot table.PhoneAPI::close().PhoneAPIclears stale chunk capability state before reuse.Security/code-review notes
I reviewed the memory-safety paths with emphasis on untrusted BLE/client input:
payload_chunkcannot exceed the generated nanopbPB_BYTES_ARRAY_T(228)storage; nanopb rejects oversized bytes fields beforePhoneAPIsees them.payload_sizeis rejected if it exceedsMAX_TO_FROM_RADIO_SIZEor is smaller than the chunk being copied.bytesUsed + chunk.sizewould exceedtotalSize.totalSize <= MAX_TO_FROM_RADIO_SIZEhas been enforced.fromRadioSize <= sizeof(slot->bytes).FromRadio, preserving chunk continuity.Residual risks/tradeoffs:
Testing
heltec-v4with PlatformIO:$HOME/.platformio/penv/bin/pio run -e heltec-v4git diff --checkgit -C protobufs diff --checkheltec-v4test node over/dev/ttyACM0with PlatformIO upload; flashing completed with hash verification.Notes:
.venv/bin/piowrapper currently has a stale shebang pointing at/tmp/meshtastic-firmware/.venv/bin/python3, so the build/flash used$HOME/.platformio/penv/bin/pio.Summary by CodeRabbit