feat: CURLService (portduino map HTTP downloads) - #353
Conversation
|
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:
📝 WalkthroughWalkthroughChangesA Portduino-only Map tile loading
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MapPanel
participant CURLService
participant libcurl
participant SDCardService
participant PNGDecoder
MapPanel->>CURLService: load tile
CURLService->>libcurl: request tile URL
libcurl-->>CURLService: response bytes and status
CURLService->>SDCardService: save PNG payload
SDCardService-->>CURLService: save result
CURLService->>PNGDecoder: decode PNG
PNGDecoder-->>CURLService: decoded image
CURLService-->>MapPanel: provide LVGL image
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/graphics/map/CURLService.cpp (1)
1-202: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFormatting: run
trunk fmt.Static analysis reports a clang-format failure for this file.
🤖 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/CURLService.cpp` around lines 1 - 202, Run trunk fmt on CURLService.cpp and apply the formatter’s changes throughout the file, preserving the existing behavior and generated formatting.Source: Linters/SAST tools
🧹 Nitpick comments (1)
source/graphics/map/CURLService.cpp (1)
66-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
curl_global_init/curl_global_cleanuptied to instance lifetime.libcurl documents
curl_global_initas intended to be called once per program and as not thread-safe; tying it to this object's ctor/dtor is fragile ifCURLServiceis ever recreated/destroyed while other libcurl users exist in-process (libcurl does refcount these calls internally, so today's single-instance-for-app-lifetime usage is safe, but it's a latent footgun).Consider hoisting
curl_global_init()/curl_global_cleanup()to a process-level init (e.g., guarded bystd::call_once) rather than per-CURLService-instance.🤖 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/CURLService.cpp` around lines 66 - 82, Move libcurl global initialization and cleanup out of the CURLService constructor and destructor, respectively, and manage initialization once at process scope using a std::call_once-guarded mechanism. Keep per-instance curl_easy_init and curl_easy_cleanup in CURLService unchanged, while ensuring global initialization occurs before any easy-handle creation.
🤖 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/CURLService.cpp`:
- Around line 84-153: The CURLService::load method performs blocking
curl_easy_perform calls from the tile-loader/redraw callback. Refactor the
tile-fetch flow so load returns promptly without synchronous HTTP work,
delegating requests to an asynchronous worker or using a cache/local tile result
first; preserve existing offline backoff and error handling in the background
fetch path and ensure completed tiles trigger the necessary redraw/update.
In `@source/graphics/TFT/TFTView_320x240.cpp`:
- Around line 2543-2545: Wire the Portduino CURLService backup in the map
initialization branch to the existing tileService save callback, matching the
HAS_SD_MMC/HAS_SDCARD branches. Update the CURLService construction associated
with map->setBackupService so its save callback delegates to
tileService->save(...), allowing fetched tiles to be cached.
---
Outside diff comments:
In `@source/graphics/map/CURLService.cpp`:
- Around line 1-202: Run trunk fmt on CURLService.cpp and apply the formatter’s
changes throughout the file, preserving the existing behavior and generated
formatting.
---
Nitpick comments:
In `@source/graphics/map/CURLService.cpp`:
- Around line 66-82: Move libcurl global initialization and cleanup out of the
CURLService constructor and destructor, respectively, and manage initialization
once at process scope using a std::call_once-guarded mechanism. Keep
per-instance curl_easy_init and curl_easy_cleanup in CURLService unchanged,
while ensuring global initialization occurs before any easy-handle creation.
🪄 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: 8faaaca2-e1b8-4c6c-a0bf-fb2296a2f886
📒 Files selected for processing (3)
include/graphics/map/CURLService.hsource/graphics/TFT/TFTView_320x240.cppsource/graphics/map/CURLService.cpp
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 `@CMakeLists.txt`:
- Around line 45-51: Gate CURL dependency discovery and linking on the
ARCH_PORTDUINO build condition: move find_package(CURL REQUIRED) and
CURL::libcurl from the unconditional CMake flow into the Portduino-specific
configuration, while preserving the existing DeviceUI dependencies for other
builds.
- Around line 53-57: Update the Windows-specific block around
target_link_libraries(DeviceUI) to apply CURL_STATICLIB and the ws2_32, crypt32,
wldap32, and normaliz dependencies only when libcurl is confirmed static. Use
the project’s existing static-libcurl option or reliable CURL::libcurl
target/property detection, while leaving shared-libcurl builds without these
settings.
🪄 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: 74a61f71-485c-44df-b494-4e48ed49b285
📒 Files selected for processing (1)
CMakeLists.txt
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/map/SDCardService.cpp`:
- Around line 93-96: Update the tile replacement logic around SD.exists and
SD.remove so a failed removal immediately aborts the write and reports failure;
do not proceed to FILE_WRITE when the existing tile cannot be deleted. Preserve
the existing behavior for tiles that do not already exist and ensure success is
returned only after a valid replacement write completes.
🪄 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: f9aa242d-9bf1-41d9-81ab-b3b88a4f968e
📒 Files selected for processing (3)
.github/workflows/ci.ymlsource/graphics/TFT/TFTView_320x240.cppsource/graphics/map/SDCardService.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- source/graphics/TFT/TFTView_320x240.cpp
1 participant
load maps via https (when internet connection is available) so no pre-installed map tiles in .portduino/default/maps required 🎉
Summary by CodeRabbit