SenseCAP Indicator: RP2040 peripherals for the main firmware#6220
Open
caveman99 wants to merge 25 commits into
Open
SenseCAP Indicator: RP2040 peripherals for the main firmware#6220caveman99 wants to merge 25 commits into
caveman99 wants to merge 25 commits into
Conversation
caveman99
force-pushed
the
indicator-comms
branch
from
March 4, 2025 00:56
3c30821 to
65b50ba
Compare
thebentern
approved these changes
Mar 4, 2025
fifieldt
added
triaged
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR integrates a secondary SenseCap Indicator MCU by removing the local COBS implementation and introducing proxy UART/I2C bridges to tunnel sensor and NMEA data over Meshtastic interdevice messages.
- Removed in-tree COBS serialization in favor of external or consolidated implementation
- Added
FakeUART/FakeI2CandSensecapIndicatorto proxy serial and I²C traffic - Updated build flags, main setup, and GPS class to select the fake transports under
SENSECAP_INDICATOR
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| variants/seeed-sensecap-indicator/platformio.ini | Switched build flags to esp32s3_base and added RingBuffer dependency |
| src/serialization/cobs.h & cobs.cpp | Removed local COBS encode/decode files |
| src/modules/Telemetry/Sensor/IndicatorSensor.* | Removed the IndicatorSensor class and its integration |
| src/modules/Telemetry/EnvironmentTelemetry.cpp | Cleaned out indicatorSensor instantiation and calls |
| src/mesh/comms/FakeUART.h & FakeUART.cpp | Introduced FakeUART to simulate serial and forward NMEA over uplink |
| src/mesh/comms/FakeI2C.h & FakeI2C.cpp | Introduced FakeI2C to simulate I²C and forward commands/responses |
| src/mesh/IndicatorSerial.h & IndicatorSerial.cpp | Added SensecapIndicator thread to encode/decode interdevice protobuf messages |
| src/main.cpp | Wired up the indicator thread and replaced native Wire1 with FakeI2C under the feature flag |
| src/gps/GPS.h & GPS.cpp | Switched GPS serial backend to FakeUART when SENSECAP_INDICATOR is defined |
Comments suppressed due to low confidence (3)
src/mesh/comms/FakeUART.h:24
- The
flush(bool)signature doesn’t match theStream::flush()override (which takes no parameters), potentially leading to unexpected behavior; consider renaming or correctly overridingvoid flush().
void flush(bool wait = true);
src/mesh/comms/FakeI2C.cpp:37
- Returning
0xFFwhenquantity != 1may be misinterpreted as 255 bytes available; return0to match the Wire API's convention of 0 bytes read on failure.
if (quantity != 1) return 0xFF;
caveman99
force-pushed
the
indicator-comms
branch
from
July 9, 2026 11:07
45ecd13 to
0fad1a5
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a protobuf-framed SenseCAP Indicator link with tunneled I2C, UART, GPS, and filesystem operations. It initializes the bridge during boot, routes peripherals through it, removes legacy telemetry paths, and updates logging, hardware settings, and pinned revisions. ChangesSenseCAP Indicator support
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant main
participant SensecapIndicator
participant I2CProxy
participant RemoteSDService
participant IndicatorRemoteFS
main->>SensecapIndicator: construct and wait_ready
I2CProxy->>SensecapIndicator: send tunneled I2C request
SensecapIndicator-->>I2CProxy: return correlated I2C response
RemoteSDService->>IndicatorRemoteFS: read or write remote file
IndicatorRemoteFS->>SensecapIndicator: send file request
SensecapIndicator-->>IndicatorRemoteFS: return file response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (28)
Build artifacts expire on 2026-08-14. Updated for |
This was referenced Jul 12, 2026
caveman99
force-pushed
the
indicator-comms
branch
3 times, most recently
from
July 12, 2026 17:35
8d1c7ae to
3bc42a3
Compare
caveman99
changed the title
caveman99
marked this pull request as ready for review
July 12, 2026 17:39
caveman99
force-pushed
the
indicator-comms
branch
from
July 12, 2026 17:48
3bc42a3 to
1737793
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/gps/GPS.cpp (1)
1433-1454: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPreserve GPS-mode checks for the bridged backend.
This skips both
GpsMode_NOT_PRESENTand disabled-mode exits, whilecreateGps()always creates the GPS object for SenseCAP. Since this variant defaults GPS to not present, incoming RP2040 NMEA can still be processed and published despite the user disabling GPS. Keep the configuration checks and only bypass physical probing/setup.Also applies to: 1917-1920
🤖 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/gps/GPS.cpp` around lines 1433 - 1454, Preserve the GPS mode checks in the SenseCAP/bridged backend path: ensure GpsMode_NOT_PRESENT exits before processing incoming NMEA, and GpsMode_ENABLED is required before publishing updates. Restrict the conditional compilation around GPS physical probing and setup only, updating the initialization logic near GPSInitFinished and the corresponding code near createGps() so bridged input cannot bypass configuration.
🧹 Nitpick comments (9)
src/RedirectablePrint.cpp (2)
81-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment exceeds the 1–2 line limit.
This explanatory comment spans three lines. As per coding guidelines, "Keep code comments minimal: one or two lines maximum, only when the reason is not obvious, and do not restate the next line."
✏️ Suggested trim
- // A leading "[Tag] " in the message simulates the thread name tag (used - // by contexts without an OSThread, e.g. the device-ui task). Print it - // before applying the level color so it renders like a real thread name. + // A leading "[Tag] " simulates a thread-name tag (e.g. for device-ui); + // print it uncolored, before the level color codes.🤖 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/RedirectablePrint.cpp` around lines 81 - 83, Shorten the explanatory comment near the level-color printing logic to no more than two lines while preserving only the essential reason for printing the leading “[Tag] ” before applying the color. Remove redundant wording that restates the surrounding code.Source: Coding guidelines
85-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTag-detection window is a magic number.
The hardcoded
24bound for the tag scan window has no named constant or brief rationale. Consider extracting it (e.g.constexpr size_t kMaxTagScanLen = 24;) for readability.🤖 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/RedirectablePrint.cpp` around lines 85 - 89, Extract the hardcoded 24-byte scan bound in the tag-detection logic around printBuf into a named constexpr constant such as kMaxTagScanLen, and use that symbol in the memchr length expression while preserving the existing limit.src/mesh/IndicatorSerial.cpp (2)
256-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten this implementation comment.
It exceeds the two-line limit and largely describes the following operations.
As per coding guidelines, “Keep code comments minimal: one or two lines maximum.”
🤖 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/IndicatorSerial.cpp` around lines 256 - 258, Shorten the comment above the protobuf decoding logic to one or two concise lines, retaining only that decoding occurs and any remaining buffer bytes are preserved for the next packet.Source: Coding guidelines
47-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Throttlefor the timeout checks.Both loops implement elapsed-time checks with raw
millis()subtraction.As per coding guidelines, use
Throttleinstead of rawmillis()arithmetic for “did N ms pass” checks.Also applies to: 173-185
🤖 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/IndicatorSerial.cpp` around lines 47 - 58, Update SensecapIndicator::wait_response and the other referenced response-wait loop to use the existing Throttle mechanism for elapsed-time checks instead of millis() subtraction. Configure Throttle with the requested timeout and use its expiration/check API while preserving the current flag handling, pumping, delay, and timeout return behavior.Source: Coding guidelines
src/mesh/IndicatorSerial.h (1)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep implementation comments to two lines or fewer.
These newly added comments exceed the repository limit; condense each without restating implementation details.
As per coding guidelines, “Keep code comments minimal: one or two lines maximum.”
Also applies to: 38-40, 54-56, 71-74
🤖 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/IndicatorSerial.h` around lines 33 - 35, Condense the newly added comments in IndicatorSerial around the request/response flow and the corresponding sections at the other referenced locations so each is no more than two lines. Keep only the essential intent, removing redundant implementation details and preserving the surrounding code unchanged.Source: Coding guidelines
src/mesh/comms/FakeUART.cpp (1)
58-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
setRxBufferSizereports success without actually resizing anything.Returning
sizeunconditionally (regardless of the fixedBUF_SIZE = 2048buffer) can mislead callers expecting the requested capacity was honored. Consider returning the actualBUF_SIZEinstead.🤖 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/comms/FakeUART.cpp` around lines 58 - 61, Update FakeUART::setRxBufferSize to return the actual fixed receive-buffer capacity, BUF_SIZE, instead of the requested size; leave the existing buffer behavior unchanged.src/mesh/comms/FakeI2C.cpp (1)
44-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent clamp of
requestFromlength instead of an invariant check.
lenis silently truncated toMAX_READ(line 47-48) rather than treated as a caller bug.MAX_READis derived from the negotiated protobuf limit (line 50-51 in the header), so any caller requesting more indicates a driver bug, not a runtime condition to gracefully clamp.🛡️ Suggested change
size_t FakeI2C::requestFrom(uint8_t address, size_t len, bool stopBit) { (void)stopBit; - if (len > MAX_READ) - len = MAX_READ; + assert(len <= MAX_READ);As per coding guidelines, "Use
assert()for invariants that should never fail."🤖 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/comms/FakeI2C.cpp` around lines 44 - 60, Replace the silent len truncation in FakeI2C::requestFrom with an assert that len does not exceed MAX_READ, preserving the existing transaction flow for valid requests. Include the appropriate assert support if needed and remove the clamp assignment.Source: Coding guidelines
src/mesh/comms/FakeI2C.h (1)
8-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClass doc comment exceeds the 1–2 line limit.
This 15-line block comment explains multiple behaviors (buffering semantics, repeated-start combining, bus-0 construction rationale) in one long explanatory block.
✏️ Suggested trim
-/** - * TwoWire implementation that tunnels bus traffic to the RP2040 over the - * interdevice serial link, making the sensors attached to the secondary MCU - * usable by the regular sensor drivers of the main firmware. - * - * beginTransmission()/write() buffer an outgoing write. The buffered write is - * executed as a single serial round trip on endTransmission(true). A write - * followed by endTransmission(false) and requestFrom() is combined into one - * write+read transaction with repeated start, matching driver access - * patterns. - * - * Constructed on bus number 0: TwoWire::begin() is final and cannot be - * intercepted, but it returns without touching hardware when the bus is - * already initialized, which is always true for bus 0 (local touch panel). - */ +// Tunnels TwoWire traffic over the interdevice serial link to the RP2040. +// write()+endTransmission(false)+requestFrom() combine into one repeated-start transaction.As per coding guidelines, "Keep code comments minimal: one or two lines maximum, only when the reason is not obvious, and do not restate the next line."
🤖 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/comms/FakeI2C.h` around lines 8 - 22, Shorten the class-level documentation for TwoWire to one or two lines, retaining only the non-obvious purpose or rationale. Remove the detailed buffering, transaction, and bus-0 behavior explanation from the comment without changing the implementation.Source: Coding guidelines
src/mesh/comms/FakeUART.h (1)
23-23: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
flush(bool wait = true)also hides rather than overridesPrint::flush().Same root cause as the
write()mismatch:Print::flush()takes no arguments, so this signature hides it instead of overriding. Likely intentional to mirrorHardwareSerial::flush(bool), but worth confirming it's never called through aStream&/Print&reference where the argument would be silently dropped.🤖 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/comms/FakeUART.h` at line 23, Update FakeUART::flush so it does not hide Print::flush(): preserve the bool-based HardwareSerial-compatible API while also exposing a no-argument flush path that correctly overrides Print::flush(). Ensure calls through Stream& or Print& invoke the no-argument override, and retain the existing wait behavior for direct bool-based calls.
🤖 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.
Inline comments:
In `@src/mesh/comms/FakeUART.cpp`:
- Around line 73-87: Update FakeUART::write to reserve one byte in
message.data.nmea for the null terminator by limiting the copied size to at most
sizeof(message.data.nmea) - 1. Preserve the existing truncation, logging,
send_uplink, and returned-size behavior while ensuring the buffer remains
null-terminated.
In `@src/mesh/IndicatorSerial.cpp`:
- Around line 61-64: Move the packets_received readiness checks in i2c_transact
and the other indicated request paths below their respective link_lock LockGuard
acquisitions. Ensure each check and subsequent transaction logic uses the
protected value, while preserving the existing fail-fast return behavior when no
packets have been received.
- Around line 188-205: Update SensecapIndicator::send_uplink to serialize all
access to the shared pb_tx_buf using link_lock, while preserving a private
lock-held sender for request methods that already hold the lock. Ensure
FakeUART::write and GPS uplink paths use the public locking sender, and request
methods call the private variant to avoid recursive locking.
- Around line 208-219: Update SensecapIndicator::serial_check and the
corresponding frame-length handling at the referenced path to reserve space for
the four-byte header and reject oversized payloads before filling pb_rx_buf.
Check available capacity before writing each byte, preventing the current
write-before-boundary-check overflow while preserving normal frame reads.
- Around line 61-75: Add a per-transaction correlation ID to
meshtastic_InterdeviceMessage, assign and retain the expected ID in
SensecapIndicator::i2c_transact, and include it in the request. Update
handle_packet() to compare incoming replies against the active transaction ID
and ignore mismatches before setting i2c_result_ready or copying the result.
---
Outside diff comments:
In `@src/gps/GPS.cpp`:
- Around line 1433-1454: Preserve the GPS mode checks in the SenseCAP/bridged
backend path: ensure GpsMode_NOT_PRESENT exits before processing incoming NMEA,
and GpsMode_ENABLED is required before publishing updates. Restrict the
conditional compilation around GPS physical probing and setup only, updating the
initialization logic near GPSInitFinished and the corresponding code near
createGps() so bridged input cannot bypass configuration.
---
Nitpick comments:
In `@src/mesh/comms/FakeI2C.cpp`:
- Around line 44-60: Replace the silent len truncation in FakeI2C::requestFrom
with an assert that len does not exceed MAX_READ, preserving the existing
transaction flow for valid requests. Include the appropriate assert support if
needed and remove the clamp assignment.
In `@src/mesh/comms/FakeI2C.h`:
- Around line 8-22: Shorten the class-level documentation for TwoWire to one or
two lines, retaining only the non-obvious purpose or rationale. Remove the
detailed buffering, transaction, and bus-0 behavior explanation from the comment
without changing the implementation.
In `@src/mesh/comms/FakeUART.cpp`:
- Around line 58-61: Update FakeUART::setRxBufferSize to return the actual fixed
receive-buffer capacity, BUF_SIZE, instead of the requested size; leave the
existing buffer behavior unchanged.
In `@src/mesh/comms/FakeUART.h`:
- Line 23: Update FakeUART::flush so it does not hide Print::flush(): preserve
the bool-based HardwareSerial-compatible API while also exposing a no-argument
flush path that correctly overrides Print::flush(). Ensure calls through Stream&
or Print& invoke the no-argument override, and retain the existing wait behavior
for direct bool-based calls.
In `@src/mesh/IndicatorSerial.cpp`:
- Around line 256-258: Shorten the comment above the protobuf decoding logic to
one or two concise lines, retaining only that decoding occurs and any remaining
buffer bytes are preserved for the next packet.
- Around line 47-58: Update SensecapIndicator::wait_response and the other
referenced response-wait loop to use the existing Throttle mechanism for
elapsed-time checks instead of millis() subtraction. Configure Throttle with the
requested timeout and use its expiration/check API while preserving the current
flag handling, pumping, delay, and timeout return behavior.
In `@src/mesh/IndicatorSerial.h`:
- Around line 33-35: Condense the newly added comments in IndicatorSerial around
the request/response flow and the corresponding sections at the other referenced
locations so each is no more than two lines. Keep only the essential intent,
removing redundant implementation details and preserving the surrounding code
unchanged.
In `@src/RedirectablePrint.cpp`:
- Around line 81-83: Shorten the explanatory comment near the level-color
printing logic to no more than two lines while preserving only the essential
reason for printing the leading “[Tag] ” before applying the color. Remove
redundant wording that restates the surrounding code.
- Around line 85-89: Extract the hardcoded 24-byte scan bound in the
tag-detection logic around printBuf into a named constexpr constant such as
kMaxTagScanLen, and use that symbol in the memchr length expression while
preserving the existing limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2654ff4f-f2ad-4822-9b32-96312efbe48b
⛔ Files ignored due to path filters (2)
src/mesh/generated/meshtastic/interdevice.pb.cppis excluded by!**/generated/**,!src/mesh/generated/**src/mesh/generated/meshtastic/interdevice.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (21)
platformio.iniprotobufssrc/RedirectablePrint.cppsrc/configuration.hsrc/detect/ScanI2CTwoWire.cppsrc/gps/GPS.cppsrc/gps/GPS.hsrc/graphics/tftSetup.cppsrc/main.cppsrc/mesh/IndicatorSerial.cppsrc/mesh/IndicatorSerial.hsrc/mesh/comms/FakeI2C.cppsrc/mesh/comms/FakeI2C.hsrc/mesh/comms/FakeUART.cppsrc/mesh/comms/FakeUART.hsrc/modules/Telemetry/EnvironmentTelemetry.cppsrc/modules/Telemetry/Sensor/IndicatorSensor.cppsrc/modules/Telemetry/Sensor/IndicatorSensor.hsrc/serialization/cobs.cppsrc/serialization/cobs.hvariants/esp32s3/seeed-sensecap-indicator/variant.h
💤 Files with no reviewable changes (5)
- src/serialization/cobs.cpp
- src/modules/Telemetry/Sensor/IndicatorSensor.h
- src/modules/Telemetry/Sensor/IndicatorSensor.cpp
- src/serialization/cobs.h
- src/modules/Telemetry/EnvironmentTelemetry.cpp
caveman99
force-pushed
the
indicator-comms
branch
2 times, most recently
from
July 12, 2026 18:12
7beaaf3 to
5fe2ed5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/gps/GPS.cpp (1)
1433-1454: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve GPS mode and serial guards for SENSECAP builds.
Compiling out this entire block causes
SENSECAP_INDICATORbuilds to ignoreGPS_NOT_PRESENT, other non-enabled GPS modes, and_serial_gps == nullptr;runOnce()proceeds directly into active GPS processing. Keep those common guards outside the preprocessor branch, while only bypassing hardware probing/setup for the tunneled backend.🤖 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/gps/GPS.cpp` around lines 1433 - 1454, Move the GPS mode and _serial_gps guards from the `#if` !defined(SENSECAP_INDICATOR) block into shared runOnce() logic so SENSECAP builds still disable for GPS_NOT_PRESENT, non-enabled modes, and a null serial interface. Keep only hardware probing/setup and GNSS detection conditional, bypassing those operations for the tunneled backend while preserving GPSInitFinished and publishUpdate behavior.
🤖 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.
Inline comments:
In `@src/gps/GPS.cpp`:
- Around line 1917-1920: Update the GPS::createGps() guard around _rx_gpio and
_serial_gps to validate _serial_gps for SENSECAP_INDICATOR as well, preventing
construction when the serial dependency is null. Restore the SENSECAP-specific
protection or use a guaranteed runtime getter instead of relying on the cross-TU
FakeSerial initialization.
---
Outside diff comments:
In `@src/gps/GPS.cpp`:
- Around line 1433-1454: Move the GPS mode and _serial_gps guards from the `#if`
!defined(SENSECAP_INDICATOR) block into shared runOnce() logic so SENSECAP
builds still disable for GPS_NOT_PRESENT, non-enabled modes, and a null serial
interface. Keep only hardware probing/setup and GNSS detection conditional,
bypassing those operations for the tunneled backend while preserving
GPSInitFinished and publishUpdate behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a30ca51-4457-4ea7-a312-bf500853a20a
⛔ Files ignored due to path filters (2)
src/mesh/generated/meshtastic/interdevice.pb.cppis excluded by!**/generated/**,!src/mesh/generated/**src/mesh/generated/meshtastic/interdevice.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (21)
platformio.iniprotobufssrc/RedirectablePrint.cppsrc/configuration.hsrc/detect/ScanI2CTwoWire.cppsrc/gps/GPS.cppsrc/gps/GPS.hsrc/graphics/tftSetup.cppsrc/main.cppsrc/mesh/IndicatorSerial.cppsrc/mesh/IndicatorSerial.hsrc/mesh/comms/FakeI2C.cppsrc/mesh/comms/FakeI2C.hsrc/mesh/comms/FakeUART.cppsrc/mesh/comms/FakeUART.hsrc/modules/Telemetry/EnvironmentTelemetry.cppsrc/modules/Telemetry/Sensor/IndicatorSensor.cppsrc/modules/Telemetry/Sensor/IndicatorSensor.hsrc/serialization/cobs.cppsrc/serialization/cobs.hvariants/esp32s3/seeed-sensecap-indicator/variant.h
💤 Files with no reviewable changes (5)
- src/modules/Telemetry/EnvironmentTelemetry.cpp
- src/serialization/cobs.cpp
- src/serialization/cobs.h
- src/modules/Telemetry/Sensor/IndicatorSensor.h
- src/modules/Telemetry/Sensor/IndicatorSensor.cpp
🚧 Files skipped from review as they are similar to previous changes (14)
- protobufs
- src/configuration.h
- src/mesh/IndicatorSerial.h
- src/graphics/tftSetup.cpp
- src/mesh/comms/FakeI2C.h
- variants/esp32s3/seeed-sensecap-indicator/variant.h
- src/mesh/comms/FakeUART.cpp
- src/mesh/comms/FakeUART.h
- src/RedirectablePrint.cpp
- src/mesh/comms/FakeI2C.cpp
- src/detect/ScanI2CTwoWire.cpp
- src/gps/GPS.h
- src/main.cpp
- src/mesh/IndicatorSerial.cpp
caveman99
force-pushed
the
indicator-comms
branch
from
July 12, 2026 18:33
9d9c50c to
36b1be7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/modules/Telemetry/EnvironmentTelemetry.cpp`:
- Around line 676-684: Replace the sprintf call in the notification construction
block with snprintf, passing sizeof(notification->message) as the buffer size
while preserving the existing format string and arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 22f28c40-3a0a-4870-9d06-b32aa56a27fd
⛔ Files ignored due to path filters (2)
src/mesh/generated/meshtastic/interdevice.pb.cppis excluded by!**/generated/**,!src/mesh/generated/**src/mesh/generated/meshtastic/interdevice.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (20)
platformio.inisrc/RedirectablePrint.cppsrc/configuration.hsrc/detect/ScanI2CTwoWire.cppsrc/gps/GPS.cppsrc/gps/GPS.hsrc/graphics/tftSetup.cppsrc/main.cppsrc/mesh/IndicatorSerial.cppsrc/mesh/IndicatorSerial.hsrc/mesh/comms/FakeI2C.cppsrc/mesh/comms/FakeI2C.hsrc/mesh/comms/FakeUART.cppsrc/mesh/comms/FakeUART.hsrc/modules/Telemetry/EnvironmentTelemetry.cppsrc/modules/Telemetry/Sensor/IndicatorSensor.cppsrc/modules/Telemetry/Sensor/IndicatorSensor.hsrc/serialization/cobs.cppsrc/serialization/cobs.hvariants/esp32s3/seeed-sensecap-indicator/variant.h
💤 Files with no reviewable changes (4)
- src/modules/Telemetry/Sensor/IndicatorSensor.h
- src/serialization/cobs.h
- src/modules/Telemetry/Sensor/IndicatorSensor.cpp
- src/serialization/cobs.cpp
🚧 Files skipped from review as they are similar to previous changes (14)
- src/configuration.h
- platformio.ini
- src/gps/GPS.h
- variants/esp32s3/seeed-sensecap-indicator/variant.h
- src/mesh/comms/FakeUART.h
- src/RedirectablePrint.cpp
- src/mesh/comms/FakeI2C.h
- src/detect/ScanI2CTwoWire.cpp
- src/gps/GPS.cpp
- src/mesh/IndicatorSerial.h
- src/graphics/tftSetup.cpp
- src/mesh/comms/FakeI2C.cpp
- src/mesh/comms/FakeUART.cpp
- src/mesh/IndicatorSerial.cpp
Collaborator
|
I wouldn't call it FakeI2C, FakeUART, but I2CProxy / UARTProxy as these classes implement the known proxy pattern. Then its more clear alone from the name what its purpose is. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/mesh/IndicatorSerial.cpp (1)
44-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Throttleinstead of rawmillis()arithmetic for the probe rate limit.
probe_link()open-codes a "250 ms since last probe" gate withmillis() - last_probe. Prefer the repo'sThrottlehelper.♻️ Suggested change
- if (last_probe != 0 && millis() - last_probe < 250) - return; + if (Throttle::isWithinTimespanMs(last_probe, 250)) + return; ... - last_probe = millis();Adjust
last_probehandling to matchThrottle::isWithinTimespanMssemantics.As per coding guidelines: "Use
Throttlefor time-based rate limiting instead of rawmillis()arithmetic; preferThrottle::isWithinTimespanMs(lastMs, intervalMs)".🤖 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/IndicatorSerial.cpp` around lines 44 - 52, Update the probe rate-limit check in probe_link() to use Throttle::isWithinTimespanMs(last_probe, 250) instead of raw millis() subtraction, preserving the existing early return and last_probe update behavior.Source: Coding guidelines
src/mesh/comms/LinkSpiLock.h (1)
9-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim rationale comments to the project's 1-2 line limit.
These four new comment blocks explain genuinely non-obvious reasoning but each exceeds the repository's comment-length rule.
src/mesh/comms/LinkSpiLock.h#L9-L13: shorten thespiLockHolderextern comment to 1-2 lines.src/mesh/comms/LinkSpiLock.h#L15-L22: shorten theSpiLockBreakDoxygen block to 1-2 lines (or move detail to a design doc/PR description).src/mesh/comms/UARTProxy.h#L9-L17: shorten theUARTProxyclass doc comment to 1-2 lines.src/main.cpp#L643-L647: shorten the comment ahead ofwait_ready(5000)to 1-2 lines.As per coding guidelines, "Keep code comments minimal: one or two lines maximum, only when the reason is not obvious, and do not restate the next line."
🤖 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/comms/LinkSpiLock.h` around lines 9 - 13, Shorten the rationale comments to the project’s one- or two-line limit without changing behavior: update the spiLockHolder comment in src/mesh/comms/LinkSpiLock.h lines 9-13, the SpiLockBreak Doxygen comment in src/mesh/comms/LinkSpiLock.h lines 15-22, the UARTProxy class comment in src/mesh/comms/UARTProxy.h lines 9-17, and the comment before wait_ready(5000) in src/main.cpp lines 643-647; retain only the essential non-obvious rationale and avoid restating the following code.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.
Inline comments:
In `@src/mesh/comms/UARTProxy.cpp`:
- Around line 90-93: Update UARTProxy::write to capture the boolean result from
sensecapIndicator->send_uplink(message) and return size only when transmission
succeeds; return the appropriate failure/zero count when it returns false so
callers know the bytes were not accepted.
- Around line 47-50: Update UARTProxy::flush(bool wait) to honor the wait
argument and match HardwareSerial semantics: flush(false) must discard buffered
NMEA output without clearing unread RX data, while the appropriate wait behavior
should preserve existing stream expectations. Adjust the current buf_clear()
call accordingly.
---
Nitpick comments:
In `@src/mesh/comms/LinkSpiLock.h`:
- Around line 9-13: Shorten the rationale comments to the project’s one- or
two-line limit without changing behavior: update the spiLockHolder comment in
src/mesh/comms/LinkSpiLock.h lines 9-13, the SpiLockBreak Doxygen comment in
src/mesh/comms/LinkSpiLock.h lines 15-22, the UARTProxy class comment in
src/mesh/comms/UARTProxy.h lines 9-17, and the comment before wait_ready(5000)
in src/main.cpp lines 643-647; retain only the essential non-obvious rationale
and avoid restating the following code.
In `@src/mesh/IndicatorSerial.cpp`:
- Around line 44-52: Update the probe rate-limit check in probe_link() to use
Throttle::isWithinTimespanMs(last_probe, 250) instead of raw millis()
subtraction, preserving the existing early return and last_probe update
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bf74c9a-ce51-48a7-881c-9e57ad0dbdbe
⛔ Files ignored due to path filters (2)
src/mesh/generated/meshtastic/interdevice.pb.cppis excluded by!**/generated/**,!src/mesh/generated/**src/mesh/generated/meshtastic/interdevice.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (20)
platformio.iniprotobufssrc/configuration.hsrc/detect/ScanI2CTwoWire.cppsrc/gps/GPS.cppsrc/gps/GPS.hsrc/gps/RTC.cppsrc/graphics/tftSetup.cppsrc/input/kbI2cBase.cppsrc/main.cppsrc/mesh/IndicatorSerial.cppsrc/mesh/IndicatorSerial.hsrc/mesh/comms/I2CProxy.cppsrc/mesh/comms/I2CProxy.hsrc/mesh/comms/LinkSpiLock.hsrc/mesh/comms/UARTProxy.cppsrc/mesh/comms/UARTProxy.hsrc/motion/BMI270Sensor.cppsrc/motion/BMM150Sensor.cppsrc/motion/ICM20948Sensor.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/configuration.h
- src/gps/GPS.cpp
- src/graphics/tftSetup.cpp
The SenseCAP Indicator RP2040 co-processor serves as a generic peripheral bridge over a serial protobuf link (interdevice.proto): - FakeI2C implements TwoWire and tunnels write and read transactions, so the standard sensor drivers and the I2C scan work unmodified on the bridged second bus (WIRE1) - FakeUART forwards GPS NMEA to the regular GPS driver - SD card access with chunked file transfers, paged directory listings and card statistics; device-ui loads map tiles and map styles from the card behind the RP2040 - link at 2M baud with 4KB chunks, message structs kept off task stacks Log messages carrying their own bracket tag render it like a thread name. Replaces the earlier IndicatorSensor/COBS approach.
Correlate responses with request ids, serialize the shared TX buffer, reject oversized frames, fix RX buffer overflow and NMEA truncation, full-length file paths.
Static initialization order across translation units is undefined, so createGps() assigns and null-checks the bridged serial instead. Bound the NMEA length defensively.
The RP2040 sends nothing unsolicited without a GPS module attached, so wait_ready now probes with the new ping message instead of listening passively. runOnce skips its pump while a requester holds link_lock, keeping the main loop from blocking for a full request timeout. FakeI2C serializes transactions between the UI task and the main loop with an owner-tracked lock held from beginTransmission to transaction end.
…ts validity Frame resync scans to the next magic instead of flushing the RX buffer, and the pump handles all buffered frames per pass. The RX drain reads in bulk and the protobuf encoder gets the correct buffer bound. GPS honors the gps_mode setting on the Indicator instead of always running. RTC, I2C keyboard and motion sensor drivers resolve WIRE1 through ScanI2CTwoWire::fetchI2CBus so bridged buses reach the right transport. FakeUART implements flush/availableForWrite/const-write from the Stream contract and fences its cross-core ring buffer. SdCardInfo.stats_valid is passed through to device-ui, and the remote FS backend gains the remove operation used for cleanup of failed tile saves.
Remote FS operations retry once on a transport timeout. Correlation ids drop late responses of the first attempt; a retried append whose first attempt landed is recognized by the offset conflict carrying the resulting file size. Definitive failures are not retried, missing-tile probes stay a single round trip. Regenerated bindings add the I2CResult.Status UNSPECIFIED zero value so an empty result cannot decode as success.
A request the co-processor cannot decode or handle is nacked, so the requester fails fast instead of burning its timeout. All requests stage the shared tx_message under link_lock. FakeI2C and FakeUART are renamed to I2CProxy and UARTProxy after the pattern they implement, with their instances following suit. Drops dead code (unused NO_NEWS_PAUSE, unreachable not-running branches, doubled include guards) and the GPS pin log line that is meaningless on the tunneled port.
The ping/pong handshake now carries InterdeviceVersion. A pong reporting a version other than ours means the RP2040 runs firmware that does not match this build, so the bridge stays shut down for the session and the mismatch is logged with both versions. Requests fail fast instead of being misinterpreted by the other side.
…lures The bridged I2C bus is shared between the main loop and the UI task, and TwoWire has no transaction bracket a lock can span: drivers drain the read buffer with available()/read() long after requestFrom() returned. Each calling task therefore gets its own staging and read buffers instead of a lock that could be left held (or that could not protect the read buffer anyway). The transaction is staged inside the link, under its lock. No request is sent before the co-processor has completed the version handshake, and runOnce keeps probing until it does, so a co-processor that boots slowly or reboots on its watchdog no longer leaves the bridge dead for the session. Requests in flight are counted, not flagged: two threads can be in a request and the first one out must not clear the other's state. File operations are retried on a lost frame and on a co-processor busy with card maintenance, but not on a refusal (nack) or a definitive failure, and they release the SPI lock while they wait so a slow link does not starve the radio.
FileStatus moved to a fresh tag: reusing the tag of the removed success flag made every failure status decode as success on a peer that predates it. A card being mounted (busy) is retried rather than reported as an empty slot, and a co-processor busy with card maintenance is waited out: mounting takes seconds and the free space scan of a large card walks its whole FAT, which is not a reason to report a missing tile. The bridged I2C bus releases the SPI lock as well, so the keyboard scan on the UI task cannot starve the radio either. Slot claims in the I2C proxy are atomic, NMEA is not sent to a peer we refuse to talk to, and the handshake is completed by the unsolicited ping the co-processor sends when it has booted, which also reports a reboot.
caveman99
force-pushed
the
indicator-comms
branch
from
July 13, 2026 10:46
d61ce58 to
a13bff5
Compare
flush() through a Stream pointer discarded the receive buffer: the flag is txOnly, and HardwareSerial::flush() keeps what has been received. write() reported bytes as written even when the link refused to send them. The link probe uses Throttle for its rate limit.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/RedirectablePrint.cpp (1)
81-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the explanatory comment within the two-line limit.
This new comment spans three lines; condense it to one or two lines while preserving only the non-obvious rationale.
As per coding guidelines, C++ comments must be at most one or two lines and must not restate the following code.
🤖 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/RedirectablePrint.cpp` around lines 81 - 83, Condense the explanatory comment above the leading “[Tag] ” handling to no more than two lines, retaining only the non-obvious rationale that it simulates a thread name tag for contexts without an OSThread and renders it like a real thread name. Remove wording that restates the following code.Source: Coding guidelines
src/mesh/IndicatorSerial.cpp (2)
365-371: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
memmovealways shifts a full-buffer-sized region instead of the actual remaining bytes.The length passed is
PB_BUFSIZE - MT_HEADER_SIZE - payload_len, computed from the buffer capacity rather thanpb_rx_size. Every processed frame (pings, NMEA, i2c results, file chunks) shifts up to the full remaining buffer capacity instead of only the valid bytes, wasting cycles on this hot path at 2M baud.⚡ Proposed fix
- pb_istream_t stream = pb_istream_from_buffer(pb_rx_buf + MT_HEADER_SIZE, payload_len); - bool status = pb_decode(&stream, meshtastic_InterdeviceMessage_fields, &message); - memmove(pb_rx_buf, pb_rx_buf + MT_HEADER_SIZE + payload_len, PB_BUFSIZE - MT_HEADER_SIZE - payload_len); - pb_rx_size -= MT_HEADER_SIZE + payload_len; + pb_istream_t stream = pb_istream_from_buffer(pb_rx_buf + MT_HEADER_SIZE, payload_len); + bool status = pb_decode(&stream, meshtastic_InterdeviceMessage_fields, &message); + size_t remaining = pb_rx_size - MT_HEADER_SIZE - payload_len; + memmove(pb_rx_buf, pb_rx_buf + MT_HEADER_SIZE + payload_len, remaining); + pb_rx_size = remaining;🤖 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/IndicatorSerial.cpp` around lines 365 - 371, Update the memmove call in the protobuf decode path to copy only the valid bytes remaining after the processed frame, using pb_rx_size minus MT_HEADER_SIZE and payload_len as the length. Preserve the existing source and destination offsets and subsequent pb_rx_size adjustment.
42-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Throttleinstead of rawmillis()arithmetic.
probe_link()'s 250ms cooldown (Line 44) and the deadline checks inwait_response()(Line 74) andwait_ready()(Line 278) are exactly the "did N ms pass since X" pattern the guideline calls out forThrottle.♻️ Proposed fix for probe_link
- if (last_probe != 0 && millis() - last_probe < 250) - return; + if (last_probe != 0 && Throttle::isWithinTimespanMs(last_probe, 250)) + return;The same substitution applies to the timeout checks at Line 74 and Line 278.
Also applies to: 65-79, 261-283
🤖 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/IndicatorSerial.cpp` around lines 42 - 53, Replace raw millis()-based elapsed-time checks in SensecapIndicator::probe_link, wait_response, and wait_ready with the established Throttle utility. Preserve the current 250 ms probe cooldown and the existing response/readiness timeout durations, using Throttle to track and test each operation’s deadline without changing surrounding message or state behavior.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.
Inline comments:
In `@src/mesh/IndicatorSerial.cpp`:
- Line 389: Update the stale-response and nack log statements in the relevant
IndicatorSerial flow to format the 32-bit request IDs with 0x%08x instead of %u,
including both message.id and expected_id while preserving the existing log
messages.
In `@src/RedirectablePrint.cpp`:
- Around line 175-177: Update the tag-suppression condition in the relevant
RedirectablePrint formatting path to inspect the rendered message produced by
vprintf, not format[0]. Reuse the existing validated “[Tag] ” prefix detection
after formatting so dynamic prefixes suppress the thread tag and templates
beginning with “[” do not incorrectly suppress it.
---
Nitpick comments:
In `@src/mesh/IndicatorSerial.cpp`:
- Around line 365-371: Update the memmove call in the protobuf decode path to
copy only the valid bytes remaining after the processed frame, using pb_rx_size
minus MT_HEADER_SIZE and payload_len as the length. Preserve the existing source
and destination offsets and subsequent pb_rx_size adjustment.
- Around line 42-53: Replace raw millis()-based elapsed-time checks in
SensecapIndicator::probe_link, wait_response, and wait_ready with the
established Throttle utility. Preserve the current 250 ms probe cooldown and the
existing response/readiness timeout durations, using Throttle to track and test
each operation’s deadline without changing surrounding message or state
behavior.
In `@src/RedirectablePrint.cpp`:
- Around line 81-83: Condense the explanatory comment above the leading “[Tag] ”
handling to no more than two lines, retaining only the non-obvious rationale
that it simulates a thread name tag for contexts without an OSThread and renders
it like a real thread name. Remove wording that restates the following code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 39fba7cb-21d0-47b9-b143-e9be5593d04d
⛔ Files ignored due to path filters (2)
src/mesh/generated/meshtastic/interdevice.pb.cppis excluded by!**/generated/**,!src/mesh/generated/**src/mesh/generated/meshtastic/interdevice.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (27)
platformio.iniprotobufssrc/RedirectablePrint.cppsrc/configuration.hsrc/detect/ScanI2CTwoWire.cppsrc/gps/GPS.cppsrc/gps/GPS.hsrc/gps/RTC.cppsrc/graphics/tftSetup.cppsrc/input/kbI2cBase.cppsrc/main.cppsrc/mesh/IndicatorSerial.cppsrc/mesh/IndicatorSerial.hsrc/mesh/comms/I2CProxy.cppsrc/mesh/comms/I2CProxy.hsrc/mesh/comms/LinkSpiLock.hsrc/mesh/comms/UARTProxy.cppsrc/mesh/comms/UARTProxy.hsrc/modules/Telemetry/EnvironmentTelemetry.cppsrc/modules/Telemetry/Sensor/IndicatorSensor.cppsrc/modules/Telemetry/Sensor/IndicatorSensor.hsrc/motion/BMI270Sensor.cppsrc/motion/BMM150Sensor.cppsrc/motion/ICM20948Sensor.cppsrc/serialization/cobs.cppsrc/serialization/cobs.hvariants/esp32s3/seeed-sensecap-indicator/variant.h
💤 Files with no reviewable changes (5)
- src/modules/Telemetry/Sensor/IndicatorSensor.h
- src/serialization/cobs.h
- src/serialization/cobs.cpp
- src/modules/Telemetry/Sensor/IndicatorSensor.cpp
- src/modules/Telemetry/EnvironmentTelemetry.cpp
🚧 Files skipped from review as they are similar to previous changes (20)
- protobufs
- src/mesh/comms/LinkSpiLock.h
- platformio.ini
- src/motion/BMM150Sensor.cpp
- variants/esp32s3/seeed-sensecap-indicator/variant.h
- src/mesh/comms/UARTProxy.h
- src/configuration.h
- src/detect/ScanI2CTwoWire.cpp
- src/motion/BMI270Sensor.cpp
- src/input/kbI2cBase.cpp
- src/mesh/comms/I2CProxy.h
- src/motion/ICM20948Sensor.cpp
- src/mesh/IndicatorSerial.h
- src/graphics/tftSetup.cpp
- src/mesh/comms/UARTProxy.cpp
- src/gps/GPS.h
- src/main.cpp
- src/gps/GPS.cpp
- src/gps/RTC.cpp
- src/mesh/comms/I2CProxy.cpp
The thread tag was suppressed based on the printf template, which disagrees with the rendered message it is compared against: a format starting with a conversion could produce two tags, and one without a trailing bracket-space lost the tag entirely. vprintf now receives the thread name and picks. Also shifts only the bytes actually buffered after a frame, throttles with Throttle and logs request ids as hex.
The busy retry re-armed its own budget on every busy answer, so a co-processor that stayed busy kept the caller in the loop with no way out. Transport retries and the wait for a busy card are now separate budgets that only count down.
A byte run lost mid-response (a UART overflow during a 4KB tile chunk, when the display starves the RX interrupt) misaligns the assembly buffer. The buffer was never reset, so the poison outlived the request and cascaded into the following chunks of the same tile: one glitch dropped a whole multi-chunk tile, while single-chunk tiles resynced in the idle gap and survived. Each request now flushes the buffer first, bounding a glitch to the one chunk it hit. Adds resync/decode/timeout counters, logged rarely, to see the rate.
The heap was 3MB and the image cache reserves 1.5MB of it, so a low-zoom map tile could not find a large enough contiguous block to decode and rendered white. 5MB of the 8MB PSRAM fixes it with room to spare.
…mits Point the protobufs submodule at the merged SD command protos (protobufs #986) so it matches the checked in interdevice sources, and bump the device-ui archive to the current indicator branch tip that carries the SD button and format UI.
8 tasks
Resolve the device-ui pin to the indicator branch merge commit, which carries both develop's device-ui updates and the remote SD support, so neither side is lost. Take develop's protobufs pointer, which already contains the merged SD command protos.
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
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.
The Indicator's RP2040 co-processor serves as a generic peripheral bridge over a serial protobuf link (interdevice.proto). Companion PRs: meshtastic/protobufs#721 (protocol), meshtastic/device-ui#342 (map tile and SD card services), meshtastic/indicator_rp2040#3 (co-processor firmware).
The generated interdevice bindings are included in-tree; the protobufs submodule stays on the develop pin until meshtastic/protobufs#721 merges, after which a regeneration commit follows the usual flow.
Verified on hardware: bridged sensor scan and telemetry (AHT10, SCD41), GPS fix, map tiles and SD statistics all work end to end.
Summary by CodeRabbit