Conversation
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:
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 (7)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds remote filesystem-backed SD-card and tile access for SENSECAP_INDICATOR, integrates remote map storage and asynchronous SD statistics into the TFT view, and adds configurable secondary I2C bus selection for keyboard scanning. ChangesRemote SD storage
Secondary keyboard I2C bus
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MapPanel
participant RemoteSDService
participant LVGL
participant IRemoteFS
MapPanel->>RemoteSDService: Request map tile
RemoteSDService->>LVGL: Set remote image source
LVGL->>RemoteSDService: Open and read file
RemoteSDService->>IRemoteFS: Fetch aligned chunk
IRemoteFS-->>RemoteSDService: Return chunk data
RemoteSDService-->>LVGL: Provide requested bytes
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![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: 2
🧹 Nitpick comments (1)
source/graphics/map/RemoteSDService.cpp (1)
21-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFunction-local
static lv_fs_drv_t drvis shared/re-registered if constructed twice.Since
drvis a function-local static, a secondRemoteSDServiceinstantiation would re-runlv_fs_drv_register(&drv)on the same struct, inserting a duplicate pointer into LVGL's driver list under the same letter. Currently only one instance is created (TFTView_320x240::loadMap()), so this is latent rather than active, but guarding registration (e.g. a one-time static flag, or movingdrv/registration to a static init function) would make the class safe against future misuse.🤖 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 `@source/graphics/map/RemoteSDService.cpp` around lines 21 - 35, Update the RemoteSDService constructor’s LVGL driver registration so repeated RemoteSDService instances do not call lv_fs_drv_register(&drv) more than once. Guard the existing function-local static drv initialization and registration with one-time state, while preserving the current driver configuration and shared driver instance.
🤖 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 `@source/graphics/common/SdCard.cpp`:
- Around line 331-339: Update RemoteSdCard::init and the updateSDCard flow so
the potentially multi-second IRemoteFS::sdInfo call does not run synchronously
on LVGL/UI paths such as ui_event_SDCardButton. Move the SD-status refresh to a
worker/background context, or establish a loading/timeout state before polling
asynchronously, while preserving the existing info.present result once the
refresh completes.
In `@source/graphics/map/RemoteSDService.cpp`:
- Around line 39-50: Replace the unbounded strcat in RemoteSDService::load with
a bounds-checked path construction that limits name to the remaining capacity of
buf and always preserves null termination. Keep the DRIVE_LETTER prefix and
existing lv_image_set_src/error behavior unchanged, and handle an overlong name
by returning false before using a truncated or overflowing path.
---
Nitpick comments:
In `@source/graphics/map/RemoteSDService.cpp`:
- Around line 21-35: Update the RemoteSDService constructor’s LVGL driver
registration so repeated RemoteSDService instances do not call
lv_fs_drv_register(&drv) more than once. Guard the existing function-local
static drv initialization and registration with one-time state, while preserving
the current driver configuration and shared driver instance.
🪄 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: cb4cee29-a315-4dbd-92be-ee67645f58ae
📒 Files selected for processing (7)
include/graphics/common/SdCard.hinclude/graphics/map/RemoteSDService.hinclude/input/I2CKeyboardScanner.hsource/graphics/TFT/TFTView_320x240.cppsource/graphics/common/SdCard.cppsource/graphics/map/RemoteSDService.cppsource/input/I2CKeyboardScanner.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@source/graphics/map/RemoteSDService.cpp`:
- Around line 101-119: Update the read logic around the chunk-fetch loop in
RemoteSDService to track backend or readChunk failures separately from a normal
loop exit. Return LV_FS_RES_UNKNOWN only when an explicit chunk/backend error
occurs, and return LV_FS_RES_OK for EOF or other successful zero-byte reads
while preserving the existing byte-count behavior.
- Around line 44-47: Update the load-failure check in the tile-loading flow
around lv_image_set_src and lv_image_get_src so it verifies that the returned
image source matches buf, rather than only checking for non-null. Preserve the
existing debug log and false return when loading fails, preventing a stale
previous source from being treated as success.
🪄 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: d9348240-f71e-4ada-b0c5-8a7cab042ebe
📒 Files selected for processing (7)
include/graphics/common/SdCard.hinclude/graphics/map/RemoteSDService.hinclude/input/I2CKeyboardScanner.hsource/graphics/TFT/TFTView_320x240.cppsource/graphics/common/SdCard.cppsource/graphics/map/RemoteSDService.cppsource/input/I2CKeyboardScanner.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- source/input/I2CKeyboardScanner.cpp
- include/graphics/common/SdCard.h
- source/graphics/common/SdCard.cpp
caveman99
changed the title
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 `@source/graphics/TFT/TFTView_320x240.cpp`:
- Around line 48-49: Reorder the conditional compilation branches so
SENSECAP_INDICATOR is evaluated before HAS_SD_MMC, ensuring SenseCap builds use
RemoteSDService/RemoteSdCard when both macros are defined. Apply the same
precedence change in updateSDCard(), or explicitly enforce mutual exclusivity
between the macros.
🪄 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: 659140d4-f33e-473a-a845-4be803a03c46
📒 Files selected for processing (7)
include/graphics/common/SdCard.hinclude/graphics/map/RemoteSDService.hinclude/input/I2CKeyboardScanner.hsource/graphics/TFT/TFTView_320x240.cppsource/graphics/common/SdCard.cppsource/graphics/map/RemoteSDService.cppsource/input/I2CKeyboardScanner.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- include/input/I2CKeyboardScanner.h
- source/input/I2CKeyboardScanner.cpp
- source/graphics/common/SdCard.cpp
- include/graphics/map/RemoteSDService.h
- source/graphics/map/RemoteSDService.cpp
- include/graphics/common/SdCard.h
RemoteSDService implements an LVGL filesystem driver that fetches map tiles chunk-wise through an IRemoteFS backend registered by the firmware. RemoteSdCard provides map style detection, url providers and card statistics over the same backend. The I2CKeyboardScanner secondary bus is injectable so the bus 1 rescan can use a bridged TwoWire implementation instead of the local Wire1.
…validity fs_read verifies the fetched chunk covers the read position (a shrunk file would loop or over-read) and picks up size changes. The LVGL fs cache is disabled for the remote driver, its own chunk cache already serves header reads without pulling whole tiles twice over the UART. SEEK_END follows the LVGL size+pos convention. Failed multi-chunk saves delete the partial file via the new IRemoteFS remove operation. Map style listing only accepts directories. The .url provider reads until the first line is complete. SD stats display shows a placeholder while the co-processor scan is running (RemoteSdInfo.statsValid). SENSECAP branches take precedence over generic SD defines in SdCard.h/.cpp.
… chain Polling the co-processor for the background computed used/free values no longer recreates the card object: that reset its updated flag, so the next loadMap() rescanned the styles with a stale prefix and could offer the zoom level directories of a flat /map card as map styles. The generic SD implementations are now excluded on SENSECAP_INDICATOR in the definition chain as well, matching their declarations.
The poll treated every failure as a scan that had not finished, so a removed card or a down link made it retry forever, blocking the UI thread on a link round trip each time. refreshStats now reports whether the statistics are pending or unavailable, the poll is bounded, and a card that disappears re-runs the detection instead of painting statistics over the error.
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
1 participant
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.
Companion to meshtastic/firmware#6220, which makes the SenseCAP Indicator RP2040 peripherals available to the main firmware over a serial protobuf link.
All wiring is gated on SENSECAP_INDICATOR; other targets are unaffected. Verified on hardware: map tiles, style dropdown and SD statistics work with tiles on the SD card behind the RP2040.
Summary by CodeRabbit