feat(BaseUI): show 'GPS Time Only' when GNSS has time but no position fix - #11361
feat(BaseUI): show 'GPS Time Only' when GNSS has time but no position fix#11361jp-bennett wants to merge 1 commit into
Conversation
… fix The position frame's drawGpsCoordinates() only distinguished 'No GPS present' / 'No GPS Lock' / coordinates, so a GNSS that had decoded valid time but no fix displayed identically to a cold chip. - GPSStatus: add per-acquisition hasTime flag (5th ctor param, accessor, matches() term, updateStatus() copy) - GPS::runOnce(): publish immediately on the gotTime rising edge so the flag reaches observers on the time-only path, which previously never published; done directly rather than via the end-of-loop block so fixHoldEnds is preserved and hold/power behavior is unchanged. Safe without a location: PositionModule ignores invalid positions. gotTime is already cleared on each GPS_ACTIVE entry, so the state is not sticky across acquisitions. - UIRenderer::drawGpsCoordinates(): the 'No GPS Lock' line becomes 'GPS Time Only' when time is valid. The drawGps() header renderer is intentionally untouched (its branches need separate de-clobbering work). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CcrasD4QsatunDreANDgCx
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
📝 WalkthroughWalkthrough
ChangesGPS time status
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant GPS
participant GPSStatus
participant UIRenderer
GPS->>GPSStatus: Construct status with gotTime
GPS->>GPS: Detect first valid GPS time
GPS->>GPSStatus: Publish status update
UIRenderer->>GPSStatus: Read getHasTime()
GPSStatus-->>UIRenderer: Return time availability
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/gps/GPS.cpp (1)
1539-1541: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the new rationale within the C++ comment-length guideline.
The explanation at Lines [1539]-[1541] uses three physical comment lines. The repository guideline limits comments to one or two lines. Keep the non-obvious reason, but condense the block.
Proposed two-line comment
- // Publish immediately (rather than via the block below, which would clear fixHoldEnds) so the - // time-only state reaches the UI without waiting for a location. Safe without a valid location: - // PositionModule::handleNewPosition ignores invalid positions. + // Publish immediately so the UI sees time-only status without waiting for a location. + // Preserve fixHoldEnds; the position module must ignore invalid positions.🤖 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/gps/GPS.cpp` around lines 1539 - 1541, Condense the explanatory comment immediately above the immediate publish call to no more than two physical C++ comment lines, preserving both the reason for bypassing the block that clears fixHoldEnds and the safety of publishing without a valid location.Source: Coding guidelines
🤖 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/GPSStatus.h`:
- Line 20: Resolve the missing copy operations declared in GPSStatus by either
defining its copy constructor and copy assignment operator or removing those
declarations when compiler-generated copying is sufficient. Keep the class
copyable and ensure all existing GPSStatus fields, including hasTime, are copied
correctly.
---
Nitpick comments:
In `@src/gps/GPS.cpp`:
- Around line 1539-1541: Condense the explanatory comment immediately above the
immediate publish call to no more than two physical C++ comment lines,
preserving both the reason for bypassing the block that clears fixHoldEnds and
the safety of publishing without a valid location.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 98a2be77-6b86-4115-b506-db4536457c22
📒 Files selected for processing (3)
src/GPSStatus.hsrc/gps/GPS.cppsrc/graphics/draw/UIRenderer.cpp
|
|
||
| bool hasLock = false; // default to false, until we complete our first read | ||
| bool isConnected = false; // Do we have a GPS we are talking to | ||
| bool hasTime = false; // GPS has decoded a valid time this acquisition, even without a position fix |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expected: every custom GPSStatus copy implementation propagates hasTime.
rg -n -C 12 \
'GPSStatus\s*\(const\s+GPSStatus\s*&|GPSStatus::GPSStatus\s*\(|GPSStatus::operator\s*=|hasTime' \
src -g '*.cpp' -g '*.h' -g '*.hpp'Repository: meshtastic/firmware
Length of output: 6173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- matching GPSStatus custom copy symbols across tracked files ---\n'
rg -n -C 16 \
'GPSStatus\s*\(\s*const\s+GPSStatus\s*&|::GPSStatus\s*\([^{\w]*const\s+.*\)|::operator\s*=\s*\(|operator\s*=\s*\(\s*const\s+GPSStatus|hasTime' \
. -g '*.cpp' -g '*.h' -g '*.hpp' | sed -n '1,220p'
printf '\n--- all GPSStatus operator symbols ---\n'
rg -n 'GPSStatus::operator\s*=|GPSStatus\s*::\s*::operator\s*=' . -g '*.cpp' -g '*.h' -g '*.hpp' || trueRepository: meshtastic/firmware
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- matching GPSStatus custom copy symbols across tracked files ---'
rg -n -C 16 \
'GPSStatus[[:space:]]*\([[:space:]]*const[[:space:]]+GPSStatus[[:space:]]*&|::GPSStatus[[:space:]]*\(.*const[[:space:]]+.*\)|::operator[[:space:]]*=[[:space:]]*\(|operator[[:space:]]*=[[:space:]]*([^(]*|[^(0-9a-zA-Z_])*\([[:space:]]*const[[:space:]]+GPSStatus|hasTime' \
. -g '*.cpp' -g '*.h' -g '*.hpp' | sed -n '1,220p'
printf '%s\n' ''
printf '%s\n' '--- all GPSStatus operator symbols ---'
rg -n 'GPSStatus::operator[[:space:]]*=|GPSStatus[[:space:]]*::[[:space:]]*operator[[:space:]]*=' . -g '*.cpp' -g '*.h' -g '*.hpp' || trueRepository: meshtastic/firmware
Length of output: 7221
Link the declared GPSStatus copy operations.
src/GPSStatus.h declares the copy constructor and copy assignment operator but the definitions are missing, so this no longer builds. Add the definitions or remove the declarations if the default copy operators are sufficient.
🤖 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/GPSStatus.h` at line 20, Resolve the missing copy operations declared in
GPSStatus by either defining its copy constructor and copy assignment operator
or removing those declarations when compiler-generated copying is sufficient.
Keep the class copyable and ensure all existing GPSStatus fields, including
hasTime, are copied correctly.
The position frame's drawGpsCoordinates() only distinguished 'No GPS
present' / 'No GPS Lock' / coordinates, so a GNSS that had decoded
valid time but no fix displayed identically to a cold chip.
accessor, matches() term, updateStatus() copy)
the flag reaches observers on the time-only path, which previously
never published; done directly rather than via the end-of-loop
block so fixHoldEnds is preserved and hold/power behavior is
unchanged. Safe without a location: PositionModule ignores invalid
positions. gotTime is already cleared on each GPS_ACTIVE entry, so
the state is not sticky across acquisitions.
'GPS Time Only' when time is valid.
The drawGps() header renderer is intentionally untouched (its
branches need separate de-clobbering work).
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01CcrasD4QsatunDreANDgCx
Summary by CodeRabbit
New Features
Bug Fixes