[codex] Add map telemetry and LXMF fixes (without PNG tiles)#23
Open
brothercorvo wants to merge 17 commits into
Open
[codex] Add map telemetry and LXMF fixes (without PNG tiles)#23brothercorvo wants to merge 17 commits into
brothercorvo wants to merge 17 commits into
Conversation
…aring - LVGL PNG decoder (lodepng) + SD card filesystem driver for loading OSM tiles - MapScreen with 2x2 tile grid, GPS marker, peer location markers, pan/zoom - 5th nav button (GPS icon) on conversation list for map access - TelemetryCodec: Sideband/Columba-compatible LXMF telemetry encode/decode - TelemetryManager: per-peer sharing sessions with duration/expiry, SPIFFS persistence - ChatScreen location share button with duration picker (15min/1hr/4hr/indefinite) - UIManager integration: telemetry send/receive via LXMF fields, map marker updates Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TileDownloader fetches OSM raster tiles over HTTPS when not found on SD card, saves them for permanent offline cache. MapScreen calls ensure_tile() before each LVGL load — tiles download once, then load from SD on subsequent views. Default tile server: tile.openstreetmap.org (configurable via set_tile_url). Proper User-Agent set per OSM usage policy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Route mbedtls allocations to PSRAM via mbedtls_platform_set_calloc_free() to fix TLS handshake failure (-32512 SSL alloc) with only ~36KB internal heap - Use WiFiClientSecure with setInsecure() for HTTPS tile downloads from OSM - Move tile downloads to background FreeRTOS task to avoid blocking UI thread - Add incremental tile loading (one PNG decode per update cycle) to prevent LVGL mutex timeout from decoding 4 tiles synchronously - Enable LVGL image cache (LV_IMG_CACHE_DEF_SIZE=8) so decoded PNGs stay in PSRAM and don't re-decode on every redraw - Add touch drag panning for map navigation via LV_EVENT_PRESSING Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three bugs prevented Pyxis telemetry from appearing on Columba's map:
1. Bytes({0x02}) called Bytes(size_t capacity=2) instead of creating a
1-byte buffer containing 0x02. Both field keys were empty, so the
second fields_set() overwrote the first — FIELD_TELEMETRY was missing
from the wire payload. Fixed with Bytes(&key, 1).
2. FIELD_COLUMBA_META was encoded as msgpack but Columba expects JSON
(json.loads after .decode('utf-8')). Changed to manual JSON string.
3. expires was sent as Unix seconds but Columba compares against
System.currentTimeMillis() (milliseconds). Locations were immediately
deleted as expired. Now sends expires_ms = end_time * 1000.
Also: set OPPORTUNISTIC delivery method on telemetry/cease messages,
add pyxis_log() diagnostics for telemetry pipeline, and manage tile
download task lifecycle (start on show, stop on hide).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three issues caused wildly incorrect device time: 1. mktime() was called before setting TZ=UTC, corrupting the time struct. The second mktime() then operated on mutated values. Fix: set TZ=UTC0 before the single mktime() call. 2. GPS cold start takes minutes but boot only waited 15s. If GPS missed the window, time was never set. Fix: retry in main loop once GPS reports a valid fix. 3. GPS timezone used a raw longitude offset with no DST rules, so EDT was shown as EST (off by 1 hour). Fix: use proper POSIX TZ strings with DST rules for US timezones. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…arkers
- Fix Bytes({key_int}) bug in LXMessage::unpack_from_bytes() that caused
incoming field keys to be empty, making fields_get() always miss
- Telemetry-only messages (fields present, no body) now skip chat bubble,
notification sound, and message store entirely
- Implement position_peer_markers() with proper lat/lon to screen coords
- Add display name resolution for map markers via recall_app_data()
- Store peer locations for repositioning on pan/zoom
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Clear peer markers when locations list is empty (e.g. after cease signal) instead of returning early, which left stale markers on the map. Add debug logging around COLUMBA_META field handling to trace cease signal flow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Greptile Summary
Confidence Score: 4/5Safe to merge with minor fixes; no critical correctness or security regressions introduced beyond the already-noted prior-review items. All new findings are P2 — the unsynchronized booleans are benign on ESP32 in practice, the location-load cap bypass requires file corruption, and the Mexico DST error is a cosmetic timezone inaccuracy. No P0/P1 defects were found beyond the carry-over issues from the previous review thread. lib/tdeck_ui/Telemetry/TelemetryManager.cpp (missing load cap), lib/tdeck_ui/UI/LXMF/MapScreen.cpp (unsynchronized task flags), src/main.cpp (DST boundary). Important Files Changed
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
lib/tdeck_ui/UI/LXMF/MapScreen.cpp:40-42
`_download_complete` and `_task_should_stop` are plain `bool` members written by the download FreeRTOS task and read/cleared by the main loop without any synchronization primitive. Without `volatile` or `std::atomic<bool>`, the compiler is free to cache the value in a register across iterations, so a write in one task may never be observed by the other. On ARM Cortex-M4 (ESP32) this won't tear in practice, but it is formally undefined behavior and a real risk if LTO or aggressive optimization is enabled.
```suggestion
_download_complete.store(false, std::memory_order_relaxed);
_download_task = nullptr;
_task_should_stop.store(false, std::memory_order_relaxed);
```
### Issue 2 of 3
lib/tdeck_ui/Telemetry/TelemetryManager.cpp:312-316
The sessions load loop enforces `MAX_SESSIONS` with a break, but the received-locations load loop has no equivalent cap guard. A corrupted or manually crafted `/telemetry_locations.dat` with `count = 255` would push up to 255 entries into `_received`, silently bypassing the `MAX_RECEIVED = 32` invariant that `on_location_received()` maintains at runtime. Add the same check that the sessions loader uses.
```suggestion
for (uint8_t i = 0; i < count; i++) {
if ((int)_received.size() >= MAX_RECEIVED) {
WARNING("Telemetry: Reached location load limit, skipping remaining entries");
break;
}
ReceivedLocation r;
uint8_t hash_len = 0;
f.read(&hash_len, 1);
if (hash_len > 32) break;
```
### Issue 3 of 3
src/main.cpp:374-377
The `uses_north_america_dst` bounding box (`latitude 24–70, longitude -127.5 to -52.5`) includes the bulk of Mexico, which abolished DST nationally in 2023 (most states stopped observing DST with the 2023 Energy Reform). Mexican users inside this box will have the M3.2.0/M11.1.0 DST rules applied incorrectly, giving them a +1h error during what would have been summer DST periods. A tighter southern boundary or a Mexico-exclusion check would prevent this.
```suggestion
static bool uses_north_america_dst(double latitude, double longitude) {
// Mexico abolished DST for most states in 2023; exclude the Mexican interior
// (roughly south of 29.5°N outside the US border zone) to avoid incorrect DST.
if (latitude < 29.5 && longitude < -92.0) {
return false;
}
return latitude >= 24.0 && latitude <= 70.0 &&
longitude >= -127.5 && longitude < -52.5;
}
```
Reviews (4): Last reviewed commit: "Fix remaining PR 23 review findings" | Re-trigger Greptile |
|
@torlando-tech branch is ready for your review |
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.
Supersedes #20.
This PR carries the same functional changes as #20, but removes the tracked
tiles/**/*.pngpayload from the review.What changed
Why
The PNG tile cache should not be reviewed or merged as part of the feature branch. Removing it makes the PR materially smaller and keeps the review focused on source changes.
Impact
Users get the same functional work proposed in #20, without bundling tens of thousands of cached tile artifacts into the diff.
Validation