fix(NodeDB): repair the USERPREFS_FIXED_GPS build path - #11434
Draft
h3lix1 wants to merge 1 commit into
Draft
Conversation
The USERPREFS_FIXED_GPS block in NodeDB::NodeDB() carried two defects. No in-tree config sets the flag (all four USERPREFS_FIXED_GPS* keys are commented out in userPrefs.jsonc), so stock builds were unaffected and CI never saw it. Any vendor build that uncomments USERPREFS_FIXED_GPS + _LAT + _LON - bin/platformio-custom.py turns uncommented keys into -D flags - hit it. 1. `nodePositions[info->num]` referenced an `info` that does not exist in this scope. The ctor declares no such local, NodeDB has no `info` member, and there is no global or macro by that name: a hard "'info' was not declared in this scope" compile error. Replaced with `getNodeNum()`, which is what `info->num` resolved to when the line still compiled, and which matches how nodePositions is keyed everywhere else (std::map<NodeNum, ...>, NodeDB.h:270). 2. `nodeDB->setLocalPosition(fixedGPS)` dereferenced the global `nodeDB` pointer, which stays null until `new NodeDB` returns (main.cpp:856). On MESHTASTIC_EXCLUDE_POSITIONDB variants (STM32WL) defect 1 is preprocessed out and this became a null-this call on first boot. Replaced with the direct member call, matching the fixed-position restore a few lines above. Provenance: 94bb21e ("2.8: NodeDB shrink, decoupling, and restructuring", meshtastic#10413) mechanically rewrote `info->position = ...` into `nodePositions[info->num] = ...`; `info` was still declared in the ctor at that point, so it still built. 79a7dcc ("Pr1 nodedb warmstore", meshtastic#10705) then moved `meshtastic_NodeInfoLite *info = getOrCreateMeshNode(getNodeNum())` out of the ctor into nodeDBSelfCare(), leaving the name dangling. Both are refactor regressions, not intent. Defect 2 is older still - it arrived with the feature itself in d4d8944 (meshtastic#5341). nodeDBSelfCare() runs at NodeDB.cpp:470, well before this block, so our own lite row already exists by the time we write nodePositions[getNodeNum()]. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Contributor
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
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
1 participant
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.
The
USERPREFS_FIXED_GPSblock in theNodeDBconstructor has two defects:nodePositions[info->num]referencesinfo, which is not declared in that scope — no local, no member, no global. Any vendor build settingUSERPREFS_FIXED_GPS+_LAT+_LONfails with'info' was not declared in this scope.nodeDB->setLocalPosition(...)via the globalnodeDB, which is null untilnew NodeDB()returns. Another line in the same constructor already calls the member directly and correctly.Stock builds are unaffected — no in-tree config sets the flag — but the documented fixed-GPS user-prefs feature is broken as written.
Provenance
Two separate regressions, not one:
info->position→nodePositions[info->num]rewrite landed in 94bb21e, butinfowas still declared then, so it compiled. The declaration was moved intonodeDBSelfCare()later by 79a7dcc (Pr1 nodedb warmstore #10705) — that is the commit that broke the build.nodeDBcall predates both; it arrived with the feature in d4d8944 (Adds fixed GPS, BUTTON_PIN and BLE code to userPrefs.h #5341).Compile-testing this
The block is behind a flag no in-tree config sets, so a normal build does not exercise it:
PLATFORMIO_BUILD_FLAGS='-DUSERPREFS_FIXED_GPS=1 -DUSERPREFS_FIXED_GPS_LAT=48.85873920 -DUSERPREFS_FIXED_GPS_LON=2.294508368 -DUSERPREFS_FIXED_GPS_ALT=0' pio run -e native-macosAdd
-DMESHTASTIC_EXCLUDE_POSITIONDB=1to exercise the STM32WL path instead.Related issue, not fixed here
This block only fires on
reboot_count == 1, when a factory-fresh device still hasregion == UNSET— so keygen is suppressed and the fixed position is written under the MAC-derived node number. When the user later sets a region,createNewIdentity()→removeNodeByNum()→eraseNodeSatellites()deletes it, whileconfig.position.fixed_positionstays true: fixed-position mode enabled with no coordinates. Not fixable inside this block.Validation
heltec-v4(ESP32-S3) against a clean baseline build of the same environment.heltec-v4andseeed-xiao-s3.bin/run-tests.shrefuses off-Linux) andbin/test-native-docker.shneeds Docker, which was unavailable on the dev host. Relying on CI for the native suite.Found during an adversarial review of deriving
NodeNumfrom the node public key. Filed as a draft for maintainer judgement.🤖 Generated with Claude Code
Verified with the flag enabled
The block this fixes is behind
USERPREFS_FIXED_GPS, which no in-tree config sets — so an ordinary build never compiles it. Built both trees with the flag to confirm the defect and the fix:Command used (
heltec-v4, ESP32-S3):PLATFORMIO_BUILD_FLAGS='-DUSERPREFS_FIXED_GPS=1 -DUSERPREFS_FIXED_GPS_LAT=48.85873920 -DUSERPREFS_FIXED_GPS_LON=2.294508368 -DUSERPREFS_FIXED_GPS_ALT=0' pio run -e heltec-v4The second defect (the null global
nodeDBonMESHTASTIC_EXCLUDE_POSITIONDBvariants) is not reachable on this target and remains verified by inspection.