feat: async loading of remote map tiles in background task - #367
feat: async loading of remote map tiles in background task#367mverch67 wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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 (2)
📝 WalkthroughWalkthroughThe PR adds asynchronous map tile loading. Tile services expose raw loading and async lifecycle APIs. Background workers process requests, while ChangesAsynchronous map tile loading
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MapPanel
participant TileService
participant AsyncTileService
participant AsyncTileLoader
participant MapTile
MapPanel->>TileService: loadAsync(hash, generation, filename)
TileService->>AsyncTileService: dispatch async request
AsyncTileService->>AsyncTileLoader: enqueue request
AsyncTileLoader->>TileService: loadRaw(filename)
TileService-->>AsyncTileLoader: decoded image or nullptr
AsyncTileLoader-->>AsyncTileService: drain result
AsyncTileService-->>MapPanel: result callback
MapPanel->>MapTile: applyImage(image)
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 10
🧹 Nitpick comments (2)
source/graphics/map/TileService.cpp (1)
59-65: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePass
consumerby const reference.
tickruns on every UI tick. TakingAsyncResultConsumerby value copies thestd::functionon each call and again on each forwarded call. Aconst &avoids the copies.ITileService::tickat Line 35 ofinclude/graphics/map/TileService.hneeds the same signature change.♻️ Proposed refactor
-void TileService::tick(AsyncResultConsumer consumer) +void TileService::tick(const AsyncResultConsumer &consumer)🤖 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/TileService.cpp` around lines 59 - 65, Update ITileService::tick and TileService::tick to accept AsyncResultConsumer as a const reference, and preserve that const-reference signature when forwarding consumer to service->tick and backup->tick.include/graphics/map/BlockingQueue.h (1)
8-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the comment with the implementation, or add a capacity limit.
The comment describes a bounded queue, but
push()never rejects or blocks, soqueue_can grow without limit. The queue also maps tostd::mutex/std::condition_variable, not directly to FreeRTOS primitives. Choose one of the following:
- Correct the comment to say "unbounded".
- Add a
maxSizelimit and drop or reject items inpush().For tile requests, a limit protects the heap on ESP32 if the UI enqueues faster than the single worker drains.
♻️ Proposed capacity limit
- void push(T item) + // returns false when the queue is full and the item is dropped + bool push(T item, size_t maxSize = 0) { { std::lock_guard<std::mutex> lock(mutex_); + if (maxSize && queue_.size() >= maxSize) + return false; queue_.push_back(std::move(item)); } cv_.notify_one(); + return true; }🤖 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 `@include/graphics/map/BlockingQueue.h` around lines 8 - 21, Update the BlockingQueue class contract and implementation consistently: either revise the class comment to describe an unbounded std::mutex/std::condition_variable-backed queue, or add a maxSize policy that makes push reject or drop items when full. Prefer enforcing a capacity limit for tile requests, and ensure push’s behavior and documentation clearly reflect the chosen policy.
🤖 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 `@include/graphics/map/TileLoader.h`:
- Around line 56-61: Change the running_ member used by TileLoader::start(),
TileLoader::stop(), and workerLoop() from plain bool to an atomic boolean, and
update any required includes or access syntax so cross-thread reads and writes
are synchronized. Leave service_ unchanged because it is initialized before the
worker starts.
In `@source/graphics/map/CURLService.cpp`:
- Around line 81-96: Replace the duplicated set-source and descriptor cleanup
logic in CURLService::load and URLService::load with a shared applyDecodedImage
helper, placing it beside the existing shared decode helpers. Update
source/graphics/map/CURLService.cpp lines 81-96 and
source/graphics/map/URLService.cpp lines 30-43 to call this helper, and
centralize the consistent data/data_size-aware destroy condition there.
In `@source/graphics/map/TileLoader.cpp`:
- Around line 18-26: Update AsyncTileLoader::start() in both platform branches
to call requestQueue_.reset() after confirming the loader is not already running
and before launching the worker task. This must clear the stopped state
established by stop(), while preserving the existing service assignment,
running_ state, and task creation flow.
- Around line 63-71: Update AsyncTileLoader::enqueue to validate filename before
calling strncpy, handling a null pointer safely while preserving the existing
request initialization and queue behavior for valid filenames. Account for the
unchanged pointer forwarding from TileService::loadAsync and the nullable
ITileService::loadAsync contract.
- Around line 28-35: Update AsyncTileLoader::stop() so it waits for explicit
worker-task termination before returning, rather than only clearing running_,
waking requestQueue_, and nulling taskHandle_. Add the exited_ atomic state,
reset it in start(), set it when workerLoop() has fully exited, and have stop()
wait for that acknowledgement before allowing the loader, service, or queues to
be destroyed.
- Around line 95-108: The AsyncTileLoader::workerLoop loadRaw allocation must be
serialized with UI-side lv_free calls in flushQueues. Enable the project’s
FreeRTOS/LVGL integration and acquire the required LVGL lock around
service_->loadRaw, or otherwise use a shared allocation mutex covering both
worker and UI allocation paths; preserve the existing request/result flow.
In `@source/graphics/map/URLService.cpp`:
- Around line 149-166: Update loadRaw so both the decoded-image success path and
the decode-failure path call http.end() before returning, matching the existing
cleanup in earlier failure branches. Ensure cleanup occurs after any needed
saveCB processing and before returning img_dsc, without altering decoding or
ownership behavior.
In `@source/graphics/TFT/TFTView_320x240.cpp`:
- Around line 2579-2597: Update the ESP32 AsyncTileLoader::stop() implementation
used by AsyncTileService so it waits for the FreeRTOS worker task to terminate,
not merely clears running_ and stops the queue. Ensure task completion is
awaited before AsyncTileService::~AsyncTileService() deletes wrapped_,
preventing loadRaw() from accessing freed service state.
- Around line 2579-2580: Serialize the shared RemoteSDService used by the
synchronous primary path and the AsyncTileService backup callback. Update the
relevant RemoteSDService transaction methods, including save() and load(), to
use one shared lock covering the complete cache and remote-filesystem operation,
and ensure the callback in the backup setup uses that same synchronization.
- Around line 2581-2585: Update the SDCardService header guard at the top of
TFTView_320x240.cpp to include SDCardService.h when either HAS_SD_MMC or
SDCARD_SHARE_SPI is defined, matching the loadMap() construction branch while
preserving existing behavior.
---
Nitpick comments:
In `@include/graphics/map/BlockingQueue.h`:
- Around line 8-21: Update the BlockingQueue class contract and implementation
consistently: either revise the class comment to describe an unbounded
std::mutex/std::condition_variable-backed queue, or add a maxSize policy that
makes push reject or drop items when full. Prefer enforcing a capacity limit for
tile requests, and ensure push’s behavior and documentation clearly reflect the
chosen policy.
In `@source/graphics/map/TileService.cpp`:
- Around line 59-65: Update ITileService::tick and TileService::tick to accept
AsyncResultConsumer as a const reference, and preserve that const-reference
signature when forwarding consumer to service->tick and backup->tick.
🪄 Autofix
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: cfc2561b-f51a-4b7c-96ed-dbc4062748aa
📒 Files selected for processing (17)
include/graphics/map/AsyncTileService.hinclude/graphics/map/BlockingQueue.hinclude/graphics/map/CURLService.hinclude/graphics/map/MapPanel.hinclude/graphics/map/MapTile.hinclude/graphics/map/OSMTiles.hinclude/graphics/map/TileLoader.hinclude/graphics/map/TileService.hinclude/graphics/map/URLService.hsource/graphics/TFT/TFTView_320x240.cppsource/graphics/map/AsyncTileService.cppsource/graphics/map/CURLService.cppsource/graphics/map/MapPanel.cppsource/graphics/map/MapTile.cppsource/graphics/map/TileLoader.cppsource/graphics/map/TileService.cppsource/graphics/map/URLService.cpp
| void AsyncTileLoader::workerLoop() | ||
| { | ||
| while (running_) { | ||
| Request req; | ||
| if (!requestQueue_.pop(req, 200)) // blocks up to 200 ms, then re-checks running_ | ||
| continue; | ||
| if (!service_) | ||
| continue; | ||
|
|
||
| lv_image_dsc_t *img_dsc = service_->loadRaw(req.filename); | ||
| Result result{req.hash, req.generation, img_dsc}; | ||
| resultQueue_.push(result); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the LVGL memory and OS configuration used by this project.
set -euo pipefail
fd -t f 'lv_conf.h|lv_conf_template.h' . --exec rg -n 'LV_USE_STDLIB_MALLOC|LV_STDLIB|LV_USE_OS|LV_MEM_SIZE|LV_MEM_POOL' {}
rg -n 'LV_USE_OS|LV_USE_STDLIB_MALLOC|LV_STDLIB_BUILTIN' --glob '*.h' --glob '*.ini' --glob '*.py'Repository: meshtastic/device-ui
Length of output: 1463
🌐 Web query:
Is the LVGL 9 builtin lv_malloc allocator thread-safe, and what does LV_USE_OS configure?
💡 Result:
The built-in memory allocator in LVGL 9 (lv_malloc) is conditionally thread-safe depending on the configuration of LV_USE_OS [1]. 1. Thread Safety of lv_malloc: The built-in memory allocator uses a mutex to protect its operations when an operating system is configured [1]. Specifically, in the source file src/stdlib/builtin/lv_mem_core_builtin.c, the memory allocation functions (such as lv_malloc_core, lv_realloc_core, and _free_core) are wrapped with calls to lv_mutex_lock and lv_mutex_unlock if the LV_USE_OS macro is enabled [1]. Consequently, if LV_USE_OS is set to a value other than LV_OS_NONE, the internal memory management operations become thread-safe [1]. 2. Purpose of LV_USE_OS: The LV_USE_OS configuration macro is used to select and enable operating system support within LVGL [2]. By setting LV_USE_OS to a value other than LV_OS_NONE (e.g., LV_OS_PTHREAD, LV_OS_FREERTOS, LV_OS_CMSIS_RTOS2, etc.), you enable the following [2][3]: - Thread Synchronization: It enables the use of the lv_lock and lv_unlock helper functions, which use a system-level mutex to protect LVGL's global state [3][4]. Since LVGL itself is not thread-safe, these functions allow you to safely call LVGL APIs from multiple threads [5][3][6]. - Internal Integration: When enabled, lv_timer_handler will automatically call these lock/unlock functions internally, eliminating the need to manually bracket the timer handler with mutexes [3][6]. - System Abstractions: It enables OS-specific abstractions for functions such as sleep (lv_sleep_ms) [3][7]. - Advanced Features: Some features, such as multi-threaded rendering (requiring multiple draw units), explicitly require LV_USE_OS to be set to a supported OS [8][2].
Citations:
- 1: https://github.com/lvgl/lvgl/blob/master/src/stdlib/builtin/lv_mem_core_builtin.c
- 2: https://github.com/lvgl/lvgl/blob/master/Kconfig
- 3: https://lvgl.io/docs/open/9.5/integration/overview.html
- 4: https://lvgl.io/docs/open/9.4/API/osal/lv_os_h.html
- 5: https://lvgl.io/docs/open/9.4/details/integration/overview/threading
- 6: https://lvgl.io/docs/open/9.5/integration/overview
- 7: https://lvgl.io/docs/open/integration/overview
- 8: https://lvgl.github.io/open-docs/HTML/9.5/API/lv_conf_h.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- LVGL configuration files ---'
fd -t f -i 'lv_conf.h|lv_conf_template.h' . -x sh -c 'echo "### $1"; rg -n -C 3 "LV_USE_STDLIB_MALLOC|LV_STDLIB_BUILTIN|LV_USE_OS|LV_OS_NONE|LV_MEM_POOL|LV_MEM_SIZE" "$1"' sh {}
echo '--- Tile loader and service call sites ---'
fd -t f 'TileLoader.cpp|TileLoader.h|URLService*|CURLService*' . -x sh -c 'echo "### $1"; rg -n -C 4 "workerLoop|loadRaw|flushQueues|lv_malloc|lv_realloc|lv_free|png|Request|Result" "$1"' sh {}
echo '--- LVGL allocator implementation or vendored sources ---'
rg -n -C 5 "lv_mutex_lock|lv_malloc_core|LV_USE_OS|LV_STDLIB_BUILTIN" --glob '*.{c,cc,cpp,h}' . | head -n 240Repository: meshtastic/device-ui
Length of output: 20069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
files = list(Path(".").rglob("lv_conf.h")) + list(Path(".").rglob("lv_conf_template.h"))
for p in files:
text = p.read_text(errors="replace")
def val(name):
m = re.search(rf"^\s*`#define`\s+{name}\s+(.+?)\s*(?:/\*.*)?$", text, re.M)
return m.group(1).strip() if m else "<unset>"
print(p)
for name in ("LV_USE_STDLIB_MALLOC", "LV_USE_OS", "LV_MEM_POOL_ALLOC"):
print(f" {name} = {val(name)}")
# Read-only static check of the reviewed source: report the worker's loadRaw
# call and all nearby allocator calls in TileLoader.cpp.
for p in Path(".").rglob("TileLoader.cpp"):
text = p.read_text(errors="replace")
print(f"{p}:")
for i, line in enumerate(text.splitlines(), 1):
if re.search(r"loadRaw|lv_(malloc|realloc|free)", line):
print(f" {i}: {line.strip()}")
PYRepository: meshtastic/device-ui
Length of output: 450
Protect concurrent LVGL allocations
include/lv_conf.h selects LV_STDLIB_BUILTIN with LV_USE_OS == LV_OS_NONE. The worker calls loadRaw() while the UI task calls lv_free() in flushQueues(). The built-in allocator has no LVGL OS mutex in this configuration, so concurrent operations can corrupt the heap.
Enable the project’s FreeRTOS integration and use the required LVGL lock, or serialize worker and UI allocations.
🤖 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/TileLoader.cpp` around lines 95 - 108, The
AsyncTileLoader::workerLoop loadRaw allocation must be serialized with UI-side
lv_free calls in flushQueues. Enable the project’s FreeRTOS/LVGL integration and
acquire the required LVGL lock around service_->loadRaw, or otherwise use a
shared allocation mutex covering both worker and UI allocation paths; preserve
the existing request/result flow.
1 participant
reduces UI blocking while waiting for remote transfer
Summary by CodeRabbit
New Features
Bug Fixes