Generic peripheral bridge for the interdevice link#3
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors serial protocol callbacks to use I2C commands, integrates a buzzer module for beep control, and removes obsolete sensor code and dependencies.
- Replace sensor callback with
mt_set_i2c_callbackand handle I2C command/response messages - Add a buzzer module (
buzzer.h/cpp) and support beep commands inserial_proto.cpp - Remove old sensor drivers and clean up
platformio.iniand Trunk config
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/serial_proto.h | Replaced sensor callback declaration with I2C callback |
| src/serial_proto.cpp | Added I2C command handling, beep support; removed sensor logic |
| src/main.cpp | Implemented onI2CReceived, wired buzzer, and pruned sensors |
| src/buzzer.h | Introduced buzzer interface and GPIO constant |
| src/buzzer.cpp | Buzzer implementation with init, on, and off functions |
| platformio.ini | Dropped sensor-related libraries from lib_deps |
| .trunk/trunk.yaml | Bumped Trunk CLI and plugin versions |
| .trunk/configs/.shellcheckrc | Provided shellcheck configuration |
Comments suppressed due to low confidence (3)
src/main.cpp:25
- [nitpick] The variable name
current_addris ambiguous. Consider renaming it tocurrentI2CAddressorcurrent_i2c_addressfor clarity.
uint8_t current_addr = 0; // Current I2C address for operations
src/serial_proto.h:22
- Add a brief comment explaining when and how the I2C callback is invoked and what the
meshtastic_I2CCommandparameter represents.
void mt_set_i2c_callback(void (*callback)(meshtastic_I2CCommand command));
src/main.cpp:34
- Consider adding unit or integration tests for
onI2CReceivedto verify handling of eachOperationcase and correct uplink of the I2C response.
void onI2CReceived(meshtastic_I2CCommand command) {
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe firmware replaces sensor interdevice messaging with SD-card, file-transfer, I2C, beep, and NMEA operations. It adds buzzer and SD runtime support, updates GPS and serial wiring, refreshes protobuf bindings, and revises PlatformIO, Trunk, VS Code, and CI configuration. ChangesRP2040 firmware update
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Serial1
participant serial_proto
participant SDFilesystem
participant I2CBus
Serial1->>serial_proto: Decode interdevice message
serial_proto->>SDFilesystem: Process SD or file operation
serial_proto->>I2CBus: Process I2C operation
SDFilesystem-->>serial_proto: Return SD response
I2CBus-->>serial_proto: Return I2C response
serial_proto-->>Serial1: Encode response
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
The RP2040 serves the ESP32 main firmware as a dumb bridge: I2C transactions and bus scans execute on the local bus, GPS NMEA is forwarded, and the SD card is exposed through chunked file transfers, directory listings and card statistics. The link runs at 2M baud with 4KB chunks; message structs are static since they exceed the core stack. SD init retries on demand, the slot has no card detect line.
The FAT scan behind usedBytes takes seconds on FAT16/32 cards and stalled the UI on the other end of the link. sd_info now answers from a cache filled from loop().
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.trunk/configs/.shellcheckrc (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid disabling
SC2154globally.This suppresses a useful undefined-variable check for every script. Prefer local
# shellcheck disable=SC2154annotations only where sourced variables are intentional.🤖 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 @.trunk/configs/.shellcheckrc around lines 1 - 3, Remove SC2154 from the global disable list in .shellcheckrc, leaving the other ShellCheck settings unchanged. Add local # shellcheck disable=SC2154 annotations only in scripts and specific locations where sourced variables are intentionally referenced.src/serial_proto.cpp (2)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a shared header over ad-hoc
externdeclarations.
sd_ensure/sd_used_freeare declaredexternhere rather than in a small shared header (e.g.sd.h) included by bothmain.cppandserial_proto.cpp. Works today, but signature drift between the two files wouldn't be caught until link time.🤖 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/serial_proto.cpp` around lines 6 - 7, Replace the ad-hoc extern declarations for sd_ensure and sd_used_free in serial_proto.cpp with a shared header declaration, such as sd.h, and include that header from both serial_proto.cpp and main.cpp. Keep the existing function signatures consistent in the shared header and remove duplicate declarations from the source files.
234-256: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPaged directory listing rescans from the start of the directory every request.
Each page re-walks all entries from the beginning of the directory up to
index, discarding everything beforerequest.offset(lines 241-253). For directories with many entries (e.g., SD-hosted map tiles per the PR description), a full paginated traversal becomes roughly O(n²) in FS operations. Consider caching an openDir/cursor plus the last-served index between calls, mirroring theread_cachepattern already used for file GETs.🤖 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/serial_proto.cpp` around lines 234 - 256, The directory listing logic around the `SD.open(request.directory)` traversal rescans from the first entry for every page, causing repeated filesystem work. Add a directory cursor cache analogous to the existing `read_cache`, retaining the open directory and last-served index; reuse and advance it when requests continue from the cached index, and reopen/reset the cursor for a different directory or non-sequential offset while preserving pagination counts and output 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/main.cpp`:
- Around line 62-73: Update sd_update_stats so a failed SDFS.info(info) call
preserves or re-arms sd_stats_pending, allowing the loop to retry until
statistics are successfully refreshed; only clear the pending flag after a
successful update and keep sd_stats_valid unchanged on failure.
In `@src/meshtastic/interdevice.pb.h`:
- Around line 44-73: Update the meshtastic_FileTransfer filepath field
definition from 255 to 256 bytes so it can preserve 255-character filenames plus
the null terminator, matching meshtastic_DirectoryListing.filenames. Regenerate
the protobuf-generated header rather than editing only the generated output.
In `@src/serial_proto.cpp`:
- Around line 82-122: Configure a finite Wire timeout during I2C initialization
in main.cpp near Wire.begin(), then update the I2C transaction handling in the
interdevice message case and the scan loop to detect timeout results from
Wire.endTransmission() and Wire.requestFrom(). Map timed-out operations to
meshtastic_I2CResult_Status_ERROR and ensure the loop continues without blocking
indefinitely.
---
Nitpick comments:
In @.trunk/configs/.shellcheckrc:
- Around line 1-3: Remove SC2154 from the global disable list in .shellcheckrc,
leaving the other ShellCheck settings unchanged. Add local # shellcheck
disable=SC2154 annotations only in scripts and specific locations where sourced
variables are intentionally referenced.
In `@src/serial_proto.cpp`:
- Around line 6-7: Replace the ad-hoc extern declarations for sd_ensure and
sd_used_free in serial_proto.cpp with a shared header declaration, such as sd.h,
and include that header from both serial_proto.cpp and main.cpp. Keep the
existing function signatures consistent in the shared header and remove
duplicate declarations from the source files.
- Around line 234-256: The directory listing logic around the
`SD.open(request.directory)` traversal rescans from the first entry for every
page, causing repeated filesystem work. Add a directory cursor cache analogous
to the existing `read_cache`, retaining the open directory and last-served
index; reuse and advance it when requests continue from the cached index, and
reopen/reset the cursor for a different directory or non-sequential offset while
preserving pagination counts and output 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bf73600-0715-4d23-844a-c8c2af1a7bab
📒 Files selected for processing (13)
.trunk/configs/.shellcheckrc.trunk/trunk.yaml.vscode/extensions.json.vscode/settings.jsonplatformio.iniprotobufssrc/buzzer.cppsrc/buzzer.hsrc/main.cppsrc/meshtastic/interdevice.pb.cppsrc/meshtastic/interdevice.pb.hsrc/serial_proto.cppsrc/serial_proto.h
Echo the request id in responses, reject oversized frames with a buffer reset, fix a one byte RX buffer overflow.
Core1 now owns SD mounting, the stats scan and card-removal recovery; core0 claims the card per request with a try-lock and reports SD busy instead of stalling the link while a mount attempt or FAT scan runs. Failed mounts no longer block the loop with no card inserted, a pulled card is detected and remounted, and get_sd_info answers from state cached at mount time. Directory listings compute total_count once per directory instead of per page. Downlink NMEA drains through a ring buffer instead of blocking on the 9600 baud GPS UART. Adds the pong response for the new ping liveness probe.
Frame resync scans to the next magic instead of flushing the RX buffer, handles all buffered frames per pass and logs Serial1 RX overflows. The protobuf encoder gets the correct buffer bound. GET on a directory path is rejected. sd_info reports stats_valid so a scan in progress is distinguishable from a full card. An I2C scan where every address responds is reported as empty (stuck bus). Hardware watchdog on the core0 request loop. Buzzer timing is millis-wrap safe.
|
I regenerated the protos, built und flashed the RP2040 target but my Indicator makes a long beep sound on startup. No output on serial. |
|
The repos are in desynch right now as i am working through a few quirks and i don't push every comma i change :-) |
An undecodable or unhandled request gets a nack so the requester fails fast instead of burning its timeout. The core reports every failed non-empty write as a generic error, so a zero-length probe now tells an absent device apart from a data NACK or a bus fault, both for writes and for failed reads. Drops the unused NO_NEWS_PAUSE and a doubled include guard.
…path File and directory responses now carry a FileStatus, so the requester can tell a transient BUSY (card maintenance) from a definitive failure and a PUT offset conflict from a write error. DELETE is idempotent. sd_mark_dead only fires on genuine card failures: a seek past the end, a full card or a bad path no longer unmount the card (a full card used to thrash between remounts), and cached handles are closed while the mutex is still held so they cannot be flushed onto a freshly mounted volume. The free space scan runs only after core0 has left the card alone for a few seconds, so map tiles are not starved by it, and its result is sanity checked (SDFS.info reports success even when the FAT walk failed). sd_claim reports an empty slot as NOCARD instead of BUSY. The directory walk feeds the watchdog, and the version mismatch is logged once per boot.
…can wait sd_claim answered NO_CARD for the whole two second mount attempt, and the requester treats that as definitive: the first tile requests after boot made the UI conclude there is no card. A mount in progress now answers BUSY, which the requester retries. The free space scan waited for core0 to leave the card alone, which a steady stream of tile reads never does; it now takes the card anyway after 30 seconds rather than never producing statistics.
…ries A pulled card fails exactly like a missing file, and map tiles are probed constantly, so the open failure path (not just seek/read on a cached handle) now asks the card whether it is still there. Card level failures during write and delete are classified the same way. Directory listings iterate with the Dir API: openNextFile() re-opens every entry by full path, which made a walk O(n^2) and could block core0 for minutes on a directory full of tiles, watchdog included. The remount path closes cached handles under the mutex (core0 could re-open one after the card was marked dead), sd_get_info copies the identity cache under the mutex, a POST with a nonzero offset no longer truncates the file before rejecting the request, and the version mismatch is logged whenever the version we see changes rather than once per boot.
The main firmware scans the bridged bus once during its setup, so a co-processor that only comes up afterwards would leave its sensors undiscovered; it now sends an unsolicited ping when it has booted, which also lets the main firmware notice a reboot. The free space scan holds the card for seconds on a large card. It now runs once per mount; later writes keep used/free current by accounting for the bytes they add or free, so serving map tiles is never interrupted by a rescan. A card that is being mounted reports busy rather than absent, the identity snapshot is taken without waiting for the card, and the I2C scan feeds the watchdog.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main.cpp (1)
144-148: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCross-core read of
sd_used_bytes/sd_free_bytes/sd_stats_validis unsynchronized.sd_get_info()(core0, no mutex — by design) reads these while core1 writes them undersd_mutexduring the per-mount stats scan. They are non-volatile64-bit values, so on the RP2040 a concurrent write can tear or be cached. Impact is limited to a transiently wrong UI readout, but consider snapshotting behind the samegenerationguard already used for the identity cache, or marking themvolatile.🤖 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/main.cpp` around lines 144 - 148, Synchronize the stats snapshot in sd_get_info() with the core1 updates to sd_stats_valid, sd_used_bytes, and sd_free_bytes. Reuse the existing generation-guard snapshot pattern from the identity cache so the 64-bit values are read consistently without introducing a separate synchronization mechanism.
🤖 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 @.github/workflows/release.yml:
- Line 57: Replace the floating develop.tar.gz PlatformIO installation in
.github/workflows/release.yml at lines 57-57 and .github/workflows/build.yml at
lines 42-42 with the same reviewed PlatformIO Core commit SHA, ensuring both
workflows install the identical pinned revision.
---
Nitpick comments:
In `@src/main.cpp`:
- Around line 144-148: Synchronize the stats snapshot in sd_get_info() with the
core1 updates to sd_stats_valid, sd_used_bytes, and sd_free_bytes. Reuse the
existing generation-guard snapshot pattern from the identity cache so the 64-bit
values are read consistently without introducing a separate synchronization
mechanism.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cced1ab9-e9d0-407e-afcc-b14ce3143994
📒 Files selected for processing (11)
.github/workflows/build.yml.github/workflows/release.ymlplatformio.iniprotobufssrc/buzzer.cppsrc/main.cppsrc/meshtastic/interdevice.pb.cppsrc/meshtastic/interdevice.pb.hsrc/serial_proto.cppsrc/serial_proto.hsuppress_lib_warnings.py
🚧 Files skipped from review as they are similar to previous changes (5)
- protobufs
- src/buzzer.cpp
- src/meshtastic/interdevice.pb.cpp
- src/serial_proto.h
- src/meshtastic/interdevice.pb.h
| restore-keys: pio-${{ runner.os }}- | ||
|
|
||
| - name: Install PlatformIO | ||
| run: pip install --upgrade https://github.com/pioarduino/platformio-core/archive/develop.tar.gz |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-remote https://github.com/pioarduino/platformio-core.git refs/heads/developRepository: meshtastic/indicator_rp2040
Length of output: 2006
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- .github/workflows/release.yml ---\n'
nl -ba .github/workflows/release.yml | sed -n '45,70p'
printf '\n--- .github/workflows/build.yml ---\n'
nl -ba .github/workflows/build.yml | sed -n '30,55p'
printf '\n--- platformio-core refs (if network available) ---\n'
git ls-remote https://github.com/pioarduino/platformio-core.git refs/heads/develop refs/tags/* 2>/dev/null | sed -n '1,20p'Repository: meshtastic/indicator_rp2040
Length of output: 1977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- .github/workflows/release.yml ---\n'
nl -ba .github/workflows/release.yml | sed -n '50,65p'
printf '\n--- .github/workflows/build.yml ---\n'
nl -ba .github/workflows/build.yml | sed -n '35,50p'
printf '\n--- platformio-core refs ---\n'
git ls-remote https://github.com/pioarduino/platformio-core.git \
refs/heads/develop \
refs/tags/* 2>/dev/null | sed -n '1,30p'Repository: meshtastic/indicator_rp2040
Length of output: 1977
🌐 Web query:
site:github.com meshtastic/indicator_rp2040 .github/workflows/release.yml .github/workflows/build.yml "pioarduino/platformio-core" "develop.tar.gz"
💡 Result:
The items referenced in your query are associated with the development, build automation, and infrastructure of the Meshtastic project and the PlatformIO ecosystem. meshtastic/indicator_rp2040 is a specific repository within the Meshtastic GitHub organization, typically used for firmware or indicator software related to RP2040-based hardware [1][2]..github/workflows/release.yml and.github/workflows/build.yml are standard GitHub Actions workflow configuration files used in repositories (such as the main Meshtastic firmware repository) to automate testing, compilation (build), and release processes [3][4][5]. These files define the CI/CD pipeline, including steps for building firmware for various architectures like RP2040, ESP32, and nRF52 [3][4]. pioarduino/platformio-core is a community-maintained fork of the standard PlatformIO Core repository [6]. PlatformIO Core itself is the command-line interface and underlying build system used to manage dependencies, compile code, and flash firmware for embedded projects like Meshtastic [7][8]. The pioarduino fork is sometimes utilized in specific build environments or to support custom platform requirements [9]. develop.tar.gz is a common naming convention for a source code archive generated from the develop branch of a software repository. In the context of Meshtastic, firmware development is primarily tracked in the develop branch, and build systems or automated scripts may generate or reference such archives during development, testing, or release packaging [10][11][12][13].
Citations:
- 1: https://github.com/meshtastic/indicator_rp2040
- 2: https://github.com/meshtastic/indicator_rp2040/actions
- 3: https://github.com/meshtastic/firmware/blob/b2bda3b0/.github/workflows/main_matrix.yml
- 4: https://github.com/meshtastic/firmware/blob/b2bda3b0/.github/workflows/build_one_target.yml
- 5: meshtastic/firmware@69495dc
- 6: https://github.com/pioarduino/platformio-core
- 7: https://github.com/platformio/platformio-core?tab=readme-ov-file
- 8: https://github.com/platformio/platformio-docs/blob/develop/core/index.rst
- 9: https://github.com/pioarduino/platform-espressif32/blob/879780c5/builder/penv_setup.py
- 10: https://github.com/meshtastic/firmware/tree/refs/heads/develop
- 11: https://www.github.com/meshtastic/Meshtastic-device
- 12: https://github.com/meshtastic/firmware/releases/tag/v2.7.24.472b14c
- 13: https://github.com/meshtastic/firmware/releases/tag/v2.7.23.b246bcd
Pin PlatformIO Core to a commit SHA.
.github/workflows/release.yml and .github/workflows/build.yml both install from develop.tar.gz, which can drift over time and make CI/release artifacts diverge. Use the same reviewed commit SHA in both workflows.
📍 Affects 2 files
.github/workflows/release.yml#L57-L57(this comment).github/workflows/build.yml#L42-L42
🤖 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 @.github/workflows/release.yml at line 57, Replace the floating
develop.tar.gz PlatformIO installation in .github/workflows/release.yml at lines
57-57 and .github/workflows/build.yml at lines 42-42 with the same reviewed
PlatformIO Core commit SHA, ensuring both workflows install the identical pinned
revision.
|
still doesn't work for me. Have tried indicator V1 and V2. Compagnion firmware (Release V2) doesn't detect SD card and doesn't output serial. 
|
|
I gave it a second try, and the two MCUs could communicate and exchange map tiles. Though, no serial output. |
Companion to meshtastic/firmware#6220 and meshtastic/device-ui#342.
The RP2040 serves the ESP32 main firmware as a dumb peripheral bridge over the serial protobuf link:
Verified on hardware together with the firmware and device-ui PRs: sensor telemetry, GPS fix and map tiles from the SD card all work end to end.
Summary by CodeRabbit