feat(dronecan): GPS node-ID/battery-ID filtering and node health guard#11698
Draft
daijoubu wants to merge 132 commits into
Draft
feat(dronecan): GPS node-ID/battery-ID filtering and node health guard#11698daijoubu wants to merge 132 commits into
daijoubu wants to merge 132 commits into
Conversation
SJW=8 was overly conservative (80% of bit time at 1Mbps with 10 quanta). SJW=3 is the standard value also used by the F7 driver. Tested with 6037 arm/disarm cycles at 500kbps: TEC=0, REC=0, zero errors.
…DCAN PLL2Q was 3 (266 MHz, invalid for FDCAN ≤ 80 MHz). Fix to 10 (80 MHz). Extend PLL2 guard from USE_SDCARD_SDIO to USE_SDCARD_SDIO || USE_DRONECAN so H7 boards with CAN but no SD card get PLL2 configured. Adopt upstream PLL2M/N formula (VCI=1.6 MHz, VCO=800 MHz) and error check on HAL_RCCEx_PeriphCLKConfig.
…pport Remove redundant PeriphClkInitStruct clock config from canardSTM32CAN1_Init. system_stm32h7xx.c already configures FDCAN to use PLL2Q (80 MHz) when USE_DRONECAN is defined; duplicating it in the driver overwrites with PLL1. Also add CAN1 pin definitions and USE_DRONECAN to KAKUTEH7WING target (PD0/PD1, CAN1_STANDBY PD3 disabled by default).
Use HAL_RCCEx_GetPeriphCLKFreq(RCC_PERIPHCLK_FDCAN) instead of HAL_RCC_GetPCLK1Freq() for bit timing calculation. FDCAN is clocked from PLL2Q (80 MHz) configured in system_stm32h7xx.c; using PCLK1 (100 MHz) produced a ~25% baud rate error causing immediate bus-off. Restore SJW to 3 for better synchronisation tolerance.
Remove high-frequency LOG_DEBUG messages from GNSS Fix/Fix2/Auxiliary handlers, onTransferReceived, dronecanInit, and gps_dronecan HDOP path that fired at 25 Hz and flooded the log. Fix PLL2 VCO input to target 1.6 MHz (PLL2M = HSE/1600000, PLL2N = 500) rather than 2.0 MHz, keeping the operating point clearly within VCIRANGE_0 (1-2 MHz) as the original SDCARD-only code did with PLL2M=5. VCO output remains 800 MHz; FDCAN (80 MHz via PLL2Q=10) and SDMMC (200 MHz via PLL2R=4) outputs are unchanged.
Drop high-frequency and verbose-but-low-value LOG_DEBUG(CAN messages: - dronecan.c: Battery Info (x2), GetNodeInfo, NodeStatus, TX success, RX loop, commented-out debug blocks - canard_stm32h7xx_driver.c: timing computation intermediates (Baudrate, Max Quanta, Prescaler BS, Prescaler, Timings summary) - canard_stm32f7xx_driver.c: same timing intermediates, TX success, In CAN Init, commented-out clock and RX blocks Retain error-path messages (decode failed, TX/RX error, init failures) and the single-line Prescaler/SJW/BS summary logged at init.
… operation Tested at 1 Mbps on KAKUTEH7WING hardware and confirmed bus operational.
…ivers Remove CubeMX boilerplate markers, commented-out dead code, and development-time question comments from both drivers.
Fix: DroneCAN GNSS messages were being applied to gpsSolDRV regardless of the configured GPS provider. Guard added in gps_dronecan.c where it belongs, keeping CAN transport layer unaware of GPS config.
…g difference F7 bxCAN HAL writes SJW directly to BTR register where hardware adds 1, so stored value 3 gives 4 tq. This wider SJW is needed for reliable bus operation on F7 targets and is different from H7 where SJW=1 is actual tq.
Prevents state machine from continuing in INIT state when the CAN peripheral fails to initialize.
Prevents out-of-bounds access when STATE_DRONECAN_FAILED is active.
Prevents stale pre-bus-off frames from storming the bus on recovery.
With AutoRetransmission=ENABLE, frames that fail on a degraded bus occupy FIFO slots indefinitely. All 32 slots fill, HAL_FDCAN_AddMessage returns HAL_ERROR, and all outgoing traffic stalls permanently with no indication until full bus-off. DroneCAN reliability is handled at the application layer via periodic republishing.
Matches the H7 driver pattern. Previously the return value was silently discarded; if timing computation failed, uninitialized stack bytes were passed to HAL_CAN_Init.
The H7 FDCAN 128x11 recessive-bit recovery sequence takes up to 11.264ms at 125kbps. The 1ms delay was restarting the counter before it could complete, preventing the node from ever exiting bus-off. 20ms gives safe margin above worst-case and allows time to detect immediate re-entry.
Guard against non-DroneCAN GPS provider at the transport boundary (handle_GNSS* functions) rather than in each leaf function in gps_dronecan.c. Also adds the guard to handle_GNSSRCTMStream which had none. Removes stale UNUSED(pgnssAux) and placeholder comment from dronecanGPSReceiveGNSSAuxiliary.
canardSTM32GetProtocolStatus() was called on every dronecanUpdate() invocation (~500Hz) to detect bus-off. Moved into the existing 1Hz task block — bus-off detection latency of up to 1s is acceptable. Adds LOG_DEBUG to report BusOff and ErrorPassive flags each second for bench diagnostics.
AutoBusOff=ENABLE handles the 128x11 recovery sequence automatically, but ESR.BOFF is a sticky read-only flag that is NOT cleared when hardware recovery completes. GetProtocolStatus() reads this flag, so the state machine was permanently stuck in STATE_DRONECAN_BUS_OFF after any bus-off event on F7 targets. Stop/Start re-enters init mode which clears ESR.BOFF, allowing recovery detection to work correctly.
Comment incorrectly stated '25MHz' as a supported HSE value — 25MHz fails the assert. CMake always provides HSE_VALUE per-target via -DHSE_VALUE=<n> so the stm32h7xx_hal_conf.h fallback of 25MHz is never used. Current targets use 8MHz (default) or 16MHz (KAKUTEH7WING).
…adence GetProtocolStatus() was called every dronecanUpdate() cycle (~500Hz) in BUS_OFF state. Moved inside the 20ms recovery timer block so it runs at the same cadence as RecoverFromBusOff() — still detects recovery within 20ms but reduces MMIO reads from ~500/sec to ~50/sec.
HAL_CAN_Stop/Start called from the scheduler context with CAN interrupts active caused a full FC lockup. Reverted to empty stub pending investigation of a safe mechanism to clear the sticky ESR.BOFF flag on F7.
Unconditional 1Hz LOG_DEBUG was flooding the bootlog with healthy status messages. Now only logs when an error condition is actually present.
HAL_CAN_AddTxMessage returns non-OK when all mailboxes are busy — a normal transient condition at startup. The log was noise. Matches the H7 driver which already handles this path silently.
DroneCAN float16 optional fields encode NaN when unpopulated. Without a guard, NaN * 100 converts to 0 on Cortex-M (ARM VCVT saturation), permanently blocking the HDOP fallback path. Also passes values through gpsConstrainHDOP() to prevent uint16_t overflow for extreme DOP values.
…ompatible gpsSolDRV has hdop but no vdop field. VDOP and EPV are not interchangeable (different units, conversion requires receiver UERE). lastVDOP was a dead store with no valid consumer.
…ames size Guards the CLI state name array against future enum additions — if a new state is added without updating the array, the build fails immediately.
…necan.c - Add isNodeUnhealthy() helper in dronecan.c, used by GNSSRCTMStream - Strip node-ID filter from dronecan.c GNSS handlers; pass source_node_id - Add activeGpsNodeId tracking, health guard, node-ID filter, and dronecanGpsIsHealthy() to gps_dronecan.c (GPS-domain decisions) - Populate isGPSHealthy() with provider dispatch: DroneCAN uses node health; UART/MSP/CRSF uses sensors(SENSOR_GPS) pattern - Add 'first over the fence' comment for multi-GNSS PnP scenarios
Add logNodeHealth() helper that emits LOG_INFO/WARNING/ERROR for each health level. Call it in handle_NodeStatus() on health state transitions for known nodes, and on first seen for new nodes that appear already degraded.
…thout reboot Driver-based providers (MSP, FAKE, DroneCAN) leave gpsState.gpsPort NULL because gpsInit() skips serial port opening for them. If gps_provider is changed via CLI to a serial-based provider without rebooting, the serial handler would dereference NULL on the next tick and hard-fault. Add a guard in gpsUpdate() that returns false early when a serial-based provider is configured but no port is open. Also guard gpsEnablePassthrough() which had the same latent NULL dereference.
… version USE_GPS_PROTO_DRONECAN was defined unconditionally in common.h, causing gps_dronecan.c to be compiled on all targets including those without CAN hardware. The new dronecanGetNodeByID() and dronecanConfig() calls in gps_dronecan.c are only defined under USE_DRONECAN, producing linker failures on non-CAN F4/AT32 targets. Move the define into gps.h under #ifdef USE_DRONECAN so it is only active on targets with CAN hardware. Change the file-level guard in gps_dronecan.c from USE_GPS_PROTO_DRONECAN to USE_DRONECAN so the guard resolves from the target header (via platform.h) before any includes fire. Also bump PG_DRONECAN_CONFIG from version 0 to 1 since dronecanConfig_t has gained three new fields (dronecanUseDNAServer, batteryId, gpsNodeId).
activeGpsNodeId exclusivity: once a GPS node is selected by the "first over the fence" rule, subsequent Fix/Fix2/Auxiliary messages from other nodes are ignored. Previously multiple nodes could race and corrupt gpsSolDRV. Also adds dronecanGpsNodeEvicted() so process1HzTasks can clear activeGpsNodeId when a stale node is removed from the table. Remove handle_GNSSRCTMStream receive stub: the FC transmits RTCM corrections to GPS nodes, it does not receive them. The stub was copied from AP_Periph example code and does nothing useful. isNodeUnhealthy() helper is removed with it as it was only used there. Whitespace: normalize handle_NodeStatus to 4-space indent (was mixed tabs/spaces).
…g newline Handlers Fix2 and Auxiliary had a 3-line copy of the rationale comment already present in the Fix handler. Replace with a one-line back-reference.
Replaces the commented-out time stubs with a parseGnssTime() helper that converts the DroneCAN gnss_timestamp (µs since epoch) to INAV's dateTime_t. Supports all three time standards: - UTC: direct Unix epoch conversion - GPS: add GPS epoch offset (315964800s), subtract leap seconds - TAI: subtract leap seconds directly GPS and TAI require num_leap_seconds != 0; validTime is set false otherwise. NONE standard also sets validTime false.
- Add parseGnssTime() converting Fix2 gnss_timestamp to dateTime_t. UTC passthrough; GPS uses DSDL formula UTC=GPS-leap+9; TAI uses UTC=TAI-leap-10. The +315964800 GPS epoch offset was wrong per DSDL — GPS timestamp epoch is defined as µs since GPS time at UTC 1970-01-01, not since 1980-01-06. - Fix EPH/EPV extraction from Fix2 covariance field. DSDL does not specify covariance layout. AP_Periph (dominant DroneCAN peripheral firmware) packs [0]=[1]=hacc², [2]=vacc², [3-5]=sacc². Previous code incorrectly combined hacc²+vacc² for EPH and used sacc² for EPV. Now uses sqrt([0]) for EPH and sqrt([2]) for EPV. - Drop deprecated GNSS Fix handler; replace with one-shot LOG_WARNING. Fix (msg ID 1060) is superseded by Fix2 (1063). Nodes sending only Fix receive a single warning prompting an upgrade. - Split DroneCAN GPS declarations into gps_dronecan.h. Removes unconditional <dronecan_msgs.h> include from gps.h, which was pulling DroneCAN headers into all GPS translation units regardless of USE_DRONECAN. - Add dronecanGpsNodeEvicted() to reset activeGpsNodeId when the active GPS node is removed from the node table by the 1 Hz stale-node sweep.
After rebasing onto feature/dronecan-param-getset, dronecan.c had six build errors introduced by the merge of gps-health-guard additions onto the param-getset base: - Remove 'static' from CanardInstance canard — dronecan_dna_server.h declares it extern, so the translation unit must have external linkage - Remove duplicate EXECUTE_OPCODE/RESTART_NODE cases in handle_AsyncServiceResponse (stale copy from pre-rebase history) - Pass transfer->source_node_id to dronecanGPSReceiveGNSSAuxiliary and dronecanGPSReceiveGNSSFix2 — gps-health-guard added a sourceNodeId parameter to both functions; the call sites in dronecan.c were not updated during the rebase - Remove handle_GNSSFix (calls dronecanGPSReceiveGNSSFix which no longer exists; dispatch table already emits a deprecation warning and routes GNSS_FIX_ID to the warning path) - Remove handle_GNSSRCTMStream stub (never dispatched; removed by the "enforce single active GPS node and remove RTCMStream stub" commit) - Restore logNodeHealth calls in handle_NodeStatus — added by the "log node health transitions" commit but lost when taking HEAD for handle_NodeStatus during conflict resolution; calls on health-change for existing nodes and on first-seen for new nodes
docs/DroneCAN.md and docs/DroneCAN-Driver.md picked up DNA server content during the rebase onto param-getset - leftover from this branch's pre-split ancestry (it briefly shared history with the DNA server work before that was extracted into its own branch). This branch has no DNA server code, so the docs shouldn't describe it either. Reverted both files to param-getset's current documentation baseline.
dronecanGpsOnNodeEvicted() (renamed from dronecanGpsNodeEvicted - "On" makes clear it's a generic eviction notification the GPS module filters for itself, not an assertion that the evicted node was a GPS node) was declared and defined but never called. The stale-node eviction loop in process1HzTasks() dropped nodes from the table without notifying it, so once the locked-on GPS node actually went away, activeGpsNodeId was never reset - GPS data from any replacement node was silently dropped for the rest of the boot, permanently, defeating the point of the health guard.
dronecan_application_unittest.cc stubs dronecanGPSReceiveGNSSFix2/ Auxiliary as no-ops instead of linking the real gps_dronecan.c, so none of the single-active-node lock, eviction, or health-based rejection logic was ever actually exercised - the stub signatures had even drifted out of sync with the real ones (missing sourceNodeId). Add gps_dronecan_unittest.cc, compiling the real gps_dronecan.c and dronecan.c together. Covers: first-over-fence locking, rejecting a second node while one is locked in, releasing the lock on eviction, confirming eviction of an unrelated node is a no-op (self-filtering in dronecanGpsOnNodeEvicted), unhealthy-node rejection, and the dronecan_gps_node_id static filter in both directions. The eviction-release test also has an end-to-end variant that drives dronecanUpdate()'s real 1Hz stale-node purge rather than calling dronecanGpsOnNodeEvicted() directly - verified to fail if the process1HzTasks() wiring is reverted, so it's actually pinning the fix rather than just exercising the callee in isolation. gps_dronecan.c: expose activeGpsNodeId as non-static under UNIT_TEST, same pattern already used for activeNodeCount/nodeTable in dronecan.c, so tests can reset it between cases.
…yInfo The setting was fully wired end-to-end (config struct, PG, settings.yaml, docs, configurator UI) but never actually enforced - every BatteryInfo message was forwarded regardless of source, silently defeating the filter for anyone using it in a multi-battery setup. Filter is on the message's own battery_id field (the BMS-reported slot), not the CAN source node ID - a BMS node can report multiple battery_id slots over the same node ID, which is what the setting is meant to isolate. Battery monitor node-health/device-association work is formally out of scope for this branch per review-dronecan-gps-node-health's own scope doc (tracked separately under review-dronecan-battery-monitor), but this one filter mirrors the already-reviewed dronecan_gps_node_id pattern closely enough, and the UI/settings plumbing was already fully built, that finishing it here was worth the small scope extension rather than ripping out working UI. Manager should be made aware this slice landed here so review-dronecan-battery-monitor's tracking doesn't go stale. Added dronecan_application_unittest.cc coverage: a recording stub for dronecanBatterySensorReceiveInfo (call count + last battery_id) proves the filter actually gates delivery, not just that the DSDL round-trips. Verified differentially - fails if the guard is removed, passes with it restored, no effect on the other 29 pre-existing tests in that binary. Also fixes a link break in dronecan_application_unittest introduced by the earlier GPS eviction-wiring fix (e571ff992): that commit added a call to dronecanGpsOnNodeEvicted() in process1HzTasks(), which this test target compiles but never stubbed.
- gps_dronecan.c: reword the "first-over-fence" comment that pointed at dronecanGPSReceiveGNSSFix, a function this branch already removed - gps_dronecan.c: reindent parseGnssTime() from a stray 2-space base to the file's prevailing 4-space indent - gps_dronecan.h: drop the orphaned USE_GPS_PROTO_DRONECAN define - no consumer anywhere in the tree since an earlier commit switched to USE_DRONECAN directly - dronecan.c: collapse four double-blank-line artifacts left over from removing handle_GNSSFix/handle_GNSSRCTMStream and their dispatch cases No behavior change; verified via SITL, KAKUTEH7WING, and the gps_dronecan_unittest/dronecan_application_unittest suites (40/40 passing).
…ogic - dronecan.c: wrap UAVCAN_EQUIPMENT_GNSS_FIX_ID's case body in braces - a static declaration directly after a bare case label is non-portable under -Wpedantic even though current toolchains accept it - gps_dronecan.c: extract dronecanGpsAcceptSource() - the health check, gpsNodeId filter, and first-over-fence lock were identical in both dronecanGPSReceiveGNSSFix2() and dronecanGPSReceiveGNSSAuxiliary(), duplicated logic that would silently drift on the next edit to one but not the other No behavior change; 40/40 unit tests pass (gps_dronecan_unittest, dronecan_application_unittest), SITL and KAKUTEH7WING build clean.
…d reconfig dronecanGpsAcceptSource() checked the configured gpsNodeId filter but then fell through into the activeGpsNodeId lock unconditionally. If a node had already locked in under accept-any (gpsNodeId=0) and the user then reconfigured gpsNodeId to a different node without rebooting, the newly configured node was rejected forever - the lock never released since the old node was still present and broadcasting. Worse, dronecanGpsIsHealthy() checks the configured gpsNodeId directly against the node table, so it kept reporting GPS healthy the whole time even though gpsSolDRV was frozen on stale data. isGPSHealthy() feeds ARMING_DISABLED_HARDWARE_FAILURE, so this was a false-healthy report on an arming safety gate, not just a display glitch. Fix: when gpsNodeId is configured, skip the first-over-fence lock entirely - a static filter already uniquely selects the source - and keep activeGpsNodeId synced to whatever it accepts, so if the filter is later cleared back to accept-any, first-over-fence resumes from the last actually-accepted node instead of a stale value. Added GPS-9 reproducing the exact scenario found in review; verified differentially against the pre-fix logic.
makeFix2() always defaults gnss_timestamp.usec to UAVCAN_TIMESTAMP_UNKNOWN, so none of the existing GPS health-guard tests ever exercised the UTC/GPS/TAI epoch conversion branches, the leap-second offset arithmetic, or the NUM_LEAP_SECONDS_UNKNOWN rejection guards in parseGnssTime(). Added 5 tests calling dronecanGPSReceiveGNSSFix2() directly with real timestamps, checking gpsSolDRV.time.* against gmtime() on an independently-computed expected unix time rather than hand-transcribed calendar dates. Verified differentially: flipping the sign on either the GPS (-leap+9) or TAI (-leap-10) offset, or removing the NUM_LEAP_SECONDS_UNKNOWN guard, breaks exactly the test targeting that path and no others.
dronecanState and next_1hz_service_at (function-local static inside dronecanUpdate()) had no UNIT_TEST reset hook, so StaleNodeEvictionThroughOneHzTaskReleasesGpsLock only passed by accident of being the last test declared in the file - gtest runs TEST_Fs in declaration order by default. Under --gtest_shuffle or --gtest_repeat it failed reliably, since both statics retained whatever state a prior test/iteration left them in. Expose both as non-static under UNIT_TEST (moving next_1hz_service_at to file scope, matching the existing pattern already used for activeNodeCount/nodeTable/activeGpsNodeId), reset both in SetUp(). Verified with 250 shuffled/repeated iterations across 8 random seeds, 0 failures. dronecan_application_unittest (32/32) and both firmware targets (SITL, KAKUTEH7WING) unaffected - both statics stay static in non-UNIT_TEST builds.
dronecan.c: CanardInstance canard was made non-static (external linkage) to satisfy a DNA-server header that declared it extern - that header and all DNA-server source no longer exist anywhere in the tree. Nothing else references it externally; reverted to static. dronecan_application_unittest.cc: removed the dronecanGPSReceiveGNSSFix stub (the v1 Fix handler this same branch removed - the function isn't called anywhere in production code, so the stub was dead weight), and added the missing sourceNodeId parameter to the dronecanGPSReceiveGNSSFix2/Auxiliary stub signatures to match what dronecan.c actually calls with. These stubs are currently unreachable (gpsConfig_System.provider stays != GPS_DRONECAN by default) but the signature mismatch was a latent UB trap for whoever changes that later - C doesn't cross-check function signatures across translation units, so it silently linked and ran despite the extra argument. 40/40 unit tests still pass, SITL and KAKUTEH7WING build clean.
…ant tests gps.c: isGPSHealthy()'s non-DroneCAN fallback had been changed from return true to return sensors(SENSOR_GPS) - out of scope for a DroneCAN-specific health guard. Confirmed inert (isGPSHealthy() has exactly one caller in the whole tree, diagnostics.c, which already gates on sensors(SENSOR_GPS) before calling it), so reverting is zero-risk scope cleanup. dronecan_messages_unittest.cc: removed BatteryInfo_BatteryId_FilteringValues and BatteryInfo_BatteryId_MultipleBatteries - both only round-trip battery_id through DSDL encode/decode, which BatteryInfo_BoundaryValues already covers. The actual filter logic is correctly tested separately via GAP-B1-B3 in dronecan_application_unittest.cc. dronecan.c: added UNUSED(nodeID) to logNodeHealth() - the parameter is only referenced inside LOG_* macros, which expand to nothing when USE_LOG is undefined (unit test builds), triggering an unused-parameter warning there. 69/69 tests pass across the three suites, SITL and KAKUTEH7WING build clean with no warnings.
The function the "health guard" is named for had zero direct test coverage - all 14 existing tests exercised dronecanGpsAcceptSource(), dronecanGpsOnNodeEvicted(), or parseGnssTime(), but none ever called dronecanGpsIsHealthy() itself. Added 6 tests (GPS-15 through GPS-20) covering every filter-on/off x node present/absent/unhealthy/evicted combination: no filter with no active node, healthy active node, and active-node-evicted; static filter with a healthy configured node, an ERROR-health configured node, and a configured node that's never sent NodeStatus. The two ERROR-health and never-seen filter tests plant a healthy decoy node on activeGpsNodeId first (via a real Fix2 message) before setting the actual test scenario - without the decoy, both tests would pass even if the implementation ignored the gpsNodeId filter entirely and fell back to activeGpsNodeId, since that defaults to 0 and happens to produce the same "unhealthy" answer by coincidence. Verified via mutation testing: forcing dronecanGpsIsHealthy() to always use activeGpsNodeId now correctly fails all 3 filter-mode tests (was 1/3 before the decoy). Also fixes a stray whitespace-only blank line in dronecanGPSReceiveGNSSFix2() left over from an earlier cleanup pass.
…een nodes dronecanGpsAcceptSource() and dronecanGpsIsHealthy() disagree on how to treat a node with no NodeStatus yet (node == NULL): AcceptSource lets its data through, IsHealthy reports unhealthy. This is intentional - AcceptSource fails open since it gates real-time data flow and NodeStatus may lag Fix2, while IsHealthy fails closed since it feeds arming/OSD/telemetry status - but it wasn't documented anywhere, so a future reader could easily mistake it for a bug (as this review pass briefly did before tracing both call paths).
The guard in gpsUpdate()/gpsEnablePassthrough() against a NULL gpsState.gpsPort when gps_provider is changed via CLI without a reboot is already fixed upstream in release/9.1 and will land on maintenance-10.x through that path shortly. Keeping it here would just create duplicate/conflicting content once it merges. This branch's own DroneCAN-specific changes to gps.c (the gps_dronecan.h include and the isGPSHealthy() DroneCAN branch) are untouched. 40/40 unit tests pass, SITL and KAKUTEH7WING build clean (both recompile gps.c, confirming the removal doesn't affect either build).
If the locked-in GPS node degrades to ERROR/CRITICAL health but keeps broadcasting NodeStatus, it's never evicted by the stale-node purge, so a second, healthy GPS node's data stays rejected too - no handoff to a redundant sensor. dronecanGpsIsHealthy() correctly reports unhealthy in this state, so this isn't a false-healthy hazard, just a missing failover. Deliberately not implementing that here: INAV has no general redundant-sensor story, we don't know anything about the characteristics of whatever second device might be sending GPS data, and picking one node over another when both are transmitting needs its own design (reliability detection, configuration) rather than being improvised as part of this health guard.
The top-of-file "Coverage" comment only listed GPS-1 through GPS-7, undercounting actual coverage 7-of-20 after several rounds of test additions (eviction-wiring, reconfig, parseGnssTime, isHealthy). Test numbering was also out of file order (GPS-9 appeared before GPS-10..14, GPS-8 appeared after them). Updated the index to list all 20 cases and renumbered comments to run sequentially in file order. Comment-only change; verified 20/20 tests still pass with unchanged names.
The 5th review pass flagged commit 1dd3095d4 (LOG_DEBUG -> LOG_WARNING for dronecanAsyncRequest and the GetNodeInfo/ParamGetSet/ExecuteOpcode/ RestartNode response decode-failure logs) as unrelated to this branch's GPS-health-guard scope - it's async-service logging for functionality that belongs to feature/dronecan-param-getset, not GPS filtering/health checking/battery filtering. Confirmed feature/dronecan-param-getset's current tip still has these as LOG_DEBUG, so the fix doesn't exist there yet. Reverting here; applying the equivalent change directly on param-getset separately, where these functions actually live. 40/40 unit tests pass, SITL and KAKUTEH7WING build clean.
The battery-ID filter tests this branch added weren't listed in the file's top-of-file coverage index. Comment-only change; 32/32 tests still pass.
The rebase onto param-getset's new tip (which now carries the LOG_DEBUG->LOG_WARNING fix moved there earlier) replayed this branch's own now-stale revert commit on top, which cleanly reapplied and incorrectly flipped the same 5 lines back to LOG_DEBUG - undoing the fix this branch's diff should just be inheriting from its base. Re-applied LOG_WARNING on all 5 lines so the diff against param-getset is clean on this file (no residual difference on these lines at all, as intended - this branch doesn't own that code). 20/20 + 32/32 unit tests pass, SITL and KAKUTEH7WING build clean.
2 tasks
|
Test firmware build ready — commit Download firmware for PR #11698 240 targets built. Find your board's
|
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.
Summary
dronecan_gps_node_idanddronecan_battery_idsettings so a specific DroneCAN GPS/battery node can be pinned, rejecting messages from any other node (0 = accept any, matching existing DroneCAN filter conventions)gps_dronecan.c: enforces a single active GPS node, evicts stale nodes from the stale-node cleanup path, and logs node health transitions (OK/WARNING/ERROR/CRITICAL) viahandle_NodeStatusFix(v1) message — nodes sending it are logged once with a "deprecated, node must send Fix2" warning and otherwise ignored. OnlyFix2is parsed:gnss_timestamp(UTC/GPS/TAI, with leap-second handling), covariance, and related fields, split out into their own header contentRTCMStreammessage stub fromshouldAcceptTransferUSE_GPS_PROTO_DRONECANtoUSE_DRONECANand bumps thedronecanConfig_tPG version for the new fieldsgps_dronecan_unittest.ccplus expandeddronecan_application_unittest.cccoverageStacked on
feature/dronecan-param-getset(#11683) andfeature/dronecan-dna-server(#11688) — do not merge until both land.Configurator branch:
fix/dronecan-gps-health-guard(UI for the new node-ID/battery-ID filter fields) — iNavFlight/inav-configurator#2673Test plan