Request a current GPS position for the map - #11403
Conversation
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
|
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:
📝 WalkthroughWalkthroughThe PR adds on-demand GPS position requests. Admin messages trigger GPS sampling, GPS throttles requests, and valid requested positions are forwarded to the phone. Tests cover serialization and unavailable-GPS handling. ChangesGPS position request flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AdminModule
participant GPS
participant PositionModule
participant Phone
AdminModule->>GPS: requestPositionUpdate()
GPS->>GPS: Validate, throttle, and schedule GPS sampling
GPS->>PositionModule: sendOurPositionToPhone()
PositionModule->>Phone: Forward valid requested position
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
e8132b0 to
f513b73
Compare
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/test_admin_radio/test_main.cpp (1)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the new test function to camelCase.
test_request_position_update_admin_message_round_tripis a C++ function and does not follow the required naming convention. Rename the declaration and update theRUN_TESTcall at Line 1903.As per coding guidelines,
**/*.{cpp,h,cc,hh}requires camelCase functions and members.Proposed rename
-static void test_request_position_update_admin_message_round_trip() +static void testRequestPositionUpdateAdminMessageRoundTrip() ... - RUN_TEST(test_request_position_update_admin_message_round_trip); + RUN_TEST(testRequestPositionUpdateAdminMessageRoundTrip);Also applies to: 1903-1903
🤖 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 `@test/test_admin_radio/test_main.cpp` at line 57, Rename test_request_position_update_admin_message_round_trip to the required camelCase form, and update the corresponding RUN_TEST invocation so the declaration and test registration use the same name.Source: Coding guidelines
src/modules/AdminModule.cpp (1)
690-703: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd runtime coverage for the position-update request path.
Cover unavailable GPS, first acceptance, and duplicate requests within 10 seconds.
GPS::requestPositionUpdate()returnstruefor cooldown duplicates, soBAD_REQUESTapplies only to rejected initialization or configuration cases.🤖 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/modules/AdminModule.cpp` around lines 690 - 703, Add runtime tests for the request_position_update handler covering unavailable GPS, the first accepted request, and duplicate requests within 10 seconds. Assert that accepted cooldown duplicates remain successful because GPS::requestPositionUpdate() returns true, while BAD_REQUEST is produced only when GPS initialization or configuration rejects the request.
🤖 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/gps/GPS.cpp`:
- Line 1610: Move the forwardPositionToPhone = false assignment from before
publishUpdate() to after the tooLong timeout handling completes, keeping the
flag set throughout the terminal timeout publication and clearing it only
afterward.
---
Nitpick comments:
In `@src/modules/AdminModule.cpp`:
- Around line 690-703: Add runtime tests for the request_position_update handler
covering unavailable GPS, the first accepted request, and duplicate requests
within 10 seconds. Assert that accepted cooldown duplicates remain successful
because GPS::requestPositionUpdate() returns true, while BAD_REQUEST is produced
only when GPS initialization or configuration rejects the request.
In `@test/test_admin_radio/test_main.cpp`:
- Line 57: Rename test_request_position_update_admin_message_round_trip to the
required camelCase form, and update the corresponding RUN_TEST invocation so the
declaration and test registration use the same name.
🪄 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: d6f62f97-de19-4dfd-9c0b-8f81a32b6c6c
⛔ Files ignored due to path filters (1)
src/mesh/generated/meshtastic/admin.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (5)
protobufssrc/gps/GPS.cppsrc/gps/GPS.hsrc/modules/AdminModule.cpptest/test_admin_radio/test_main.cpp
|
Addressed the timeout ordering and test naming feedback, and added coverage for the unavailable-GPS rejection path. The accepted and coalesced paths require a live GPS worker and remain covered by the GPS request state logic. |
|
@coderabbitai review |
|
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 `@test/test_admin_radio/test_main.cpp`:
- Around line 1573-1575: Update the test flow after sendAdmin(request) to
inspect testAdmin->reply() and assert that its routing error is
meshtastic_Routing_Error_BAD_REQUEST before calling drainReply(). Replace the
insufficient non-null-only assertion while preserving the existing reply
draining.
🪄 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: efc475ae-fbc4-4cfd-8adf-cc9abfd3c145
📒 Files selected for processing (2)
src/gps/GPS.cpptest/test_admin_radio/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/gps/GPS.cpp
|
@coderabbitai review |
|
|
Removed the generated binding change. This feature PR now follows the repository workflow and depends on protobuf PR #1032 plus its dedicated automated generated-binding update. |
|
Updated the dependency to protobuf PR #1032 after rebasing it onto |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Adds authenticated AdminMessage handling for an on-demand GPS sample and forwards the resulting local position to the connected client without broadcasting it over the mesh. Requests are coalesced for 10 seconds.
Depends on protobuf PR #1032.
Testing
trunk fmt