feat(dronecan): on-demand GetNodeInfo, GetSet, ExecuteOpcode, RestartNode via async slot#15
Closed
daijoubu wants to merge 42 commits into
Closed
feat(dronecan): on-demand GetNodeInfo, GetSet, ExecuteOpcode, RestartNode via async slot#15daijoubu wants to merge 42 commits into
daijoubu wants to merge 42 commits into
Conversation
…CMStream tests
Closes coverage gaps identified in audit 2026-06-01:
GAP-D1: GetNodeInfoResponse encode/decode round-trip, empty name,
80-char max-length name
GAP-D2: RTCMStream basic encode/decode, empty payload
GAP-D3: SoftwareVersion optional_field_flags behaviour —
vcs_commit/image_crc are always on the wire; flags are
app-level hint only. Tests document this and verify correct
flag-checking is needed in handle_GetNodeInfoResponse.
HardwareVersion unique_id[16] round-trip and zero case.
Constants: GetNodeInfo and RTCMStream signatures, IDs, max sizes.
Node table logic (GAP-N1..N4), MSP byte-layout (GAP-M1..M2), and
shouldAcceptTransfer dispatch (GAP-S1..S3) require dronecan.c with
mocked INAV dependencies — tracked for dronecan_application_unittest.cc.
13 tests, 0 failures.
12 tests covering GAP-N1..N4 (node table insert, update, last_seen_ms, overflow rejection) and GAP-S1 (shouldAcceptTransfer accept/reject logic). dronecan.c: activeNodeCount and nodeTable are non-static in UNIT_TEST builds so SetUp() can reset state between test cases. RejectsGetNodeInfoResponseBeforePhase3 is intentionally a canary — it documents current behaviour and will fail when Phase 3 wires up the response handler, signalling a required test update.
… handle_NodeStatus Phase 2 removed UNUSED(ins) and now calls canardRequestOrRespond(ins, ...) on new node discovery. Tests were passing nullptr which caused a segfault on the second node in any test that added two nodes. Fix: DroneCANNodeTableTest fixture initialises a real CanardInstance with a 4096-byte pool and local node ID 1. All handle_NodeStatus calls now pass &ins.
…tNodeInfo Phase 1: Add sw/hw version fields to dronecanNodeInfo_t; zero-initialise on node insert so table is valid before the GetNodeInfo response arrives. Phase 2: Send GetNodeInfo request to each newly discovered node from handle_NodeStatus via canardRequestOrRespond. Phase 3: Accept GetNodeInfo responses in shouldAcceptTransfer so the response handler wired in Phase 4 will receive them. Tests: update AcceptsGetNodeInfoResponse (was canary RejectsBeforePhase3).
Verifies onTransferReceived dispatches GetNodeInfo responses to handle_GetNodeInfoResponse and the node table entry is populated with name, sw version, hw version, and unique_id. Fails until Phase 4 implements the handler.
Rename local variable nodeId → nodeID in handle_NodeStatus, handle_GetNodeInfoResponse, and the MSP2_INAV_DRONECAN_NODE_INFO handler to match the struct field naming used throughout dronecanNodeInfo_t, dronecanNodeStatus_t, and dronecanConfig_t.
…ytes Append sw_major, sw_minor, sw_optional_field_flags, sw_vcs_commit, hw_major, hw_minor, and hw_unique_id after the existing 46-byte payload. Update size guard from 46 to 71 bytes. Remove duplicate found = true.
Add sw_major, sw_minor, sw_optional_field_flags, sw_vcs_commit, hw_major, hw_minor, hw_unique_id fields to MSP2_INAV_DRONECAN_NODE_INFO reply payload. Update notes to document 71-byte response size and conditional availability depending on GetNodeInfo response receipt.
The sw/hw version fields added to dronecanNodeInfo_t were only in the working tree and never staged. A clean checkout of this branch would fail to compile all subsequent commits that write or read those fields.
- Use static transfer_id for GetNodeInfo requests; per Canard API the pointer must outlive the call so the library can increment it across successive transfers. Stack-local was a protocol violation. Add TODO for retry-on-failure (currently a single attempt at discovery). - Fix log message typo: "GetNodeInfoRespones" -> "GetNodeInfoResponse" - Use UAVCAN_PROTOCOL_SOFTWAREVERSION_OPTIONAL_FIELD_FLAG_VCS_COMMIT named constant instead of magic literal 1 - Use sbufWriteDataSafe for hw_unique_id, consistent with name field
…ase block Remove trailing spaces/tabs throughout dronecan.c and dronecan.h. Remove spurious blank line inside MSP2_INAV_DRONECAN_NODE_INFO case block in fc_msp.c. No functional change.
The field carries elapsed milliseconds (millis() - last_seen_ms at request time), not an absolute FC timestamp. Update the NODE_INFO field description and the NODES notes string to reflect the actual wire semantics.
Canard requires a separate transfer_id counter per (data_type_id, dst_node_id) descriptor. The previous static local was shared across all destination nodes, violating the protocol and risking mismatched or dropped responses when multiple nodes are discovered simultaneously. Add getNodeInfo_transfer_id to dronecanNodeInfo_t, zero-init it at node insertion, and pass its address to canardRequestOrRespond.
The buffer size check for MSP2_INAV_DRONECAN_NODE_INFO now uses an explicit per-field sum matching the writes that follow, so the check stays in sync when fields are added or removed.
…FAILED Previously busoffTimeUs was reset unconditionally, causing endless 20ms recovery attempts on a permanently-faulted bus with no exit path. After 50 attempts (~1 second at 20ms cadence) without clearing BusOff, transition to STATE_DRONECAN_FAILED and log the failure. The retry counter resets on successful recovery so a transient fault is handled correctly.
… lookups Add a private findNodeByID() helper in dronecan.c and a public dronecanGetNodeByID() accessor in the driver API. Use them to consolidate three independent linear-search loops that all performed the same nodeID lookup: - handle_NodeStatus: replace loop with findNodeByID() - handle_GetNodeInfoResponse: replace loop with findNodeByID() - MSP2_INAV_DRONECAN_NODE_INFO handler: replace loop+found flag with dronecanGetNodeByID(), simplifying to a flat early-exit structure
…SIZE constant Define the constant near the DroneCAN include with a breakdown comment showing each field's contribution. The check site now reads clearly and the single definition is the one place to update when the wire format changes.
…nav.h The size constant belongs alongside the message code definition, not inside the implementation file. Removed from fc_msp.c, added with field breakdown comment next to MSP2_INAV_DRONECAN_NODE_INFO.
Replace eight individual zero-assignments with memset() on the whole dronecanNodeInfo_t slot before setting non-zero fields. Future struct additions are zeroed automatically. Update coverage comment in dronecan_application_unittest.cc — GetNodeInfo response acceptance has been implemented since Phase 3; remove the stale 'Phase 3 will change this' note.
- Extend dronecanNodeInfo_t name field from 32 to 80 bytes to match the full UAVCAN GetNodeInfo name length (spec allows up to 80 bytes) - Use sizeof(node->name) in truncation logic so it tracks the struct - Update MSP2_INAV_DRONECAN_NODE_INFO wire format: 71 -> 119 bytes - Log when node table is full and a new node is silently dropped
…ase artifact Orphaned blob (old non-static TX for-loop + stray closing brace + duplicate non-static process1HzTasks) slipped through the 27-commit getnodeinfo rebase. The correct versions already exist: processCanardTxQueue() for ISR-driven TX and static process1HzTasks() at line 426 with NVIC masking.
…t symbols Two fixes found by running unit tests after the getnodeinfo rebase: 1. shouldAcceptTransfer response block was empty — GetNodeInfo responses were silently dropped by canard before handle_GetNodeInfoResponse could run. Restores the UAVCAN_PROTOCOL_GETNODEINFO_RESPONSE_SIGNATURE case that was present on the pre-rebase branch but clobbered during conflict resolution. 2. UNIT_TEST build: expose handle_NodeStatus, shouldAcceptTransfer, and onTransferReceived as non-static so dronecan_application_unittest.cc can call them directly. Add txErrCount/busOffCount to UNIT_TEST block. Fix stub typo canardSTM32Recieve→canardSTM32Receive and add missing canardSTM32GetAndClearRxDropCount stub. All 13 dronecan_application_unittest + 13 dronecan_getnodeinfo_unittest pass.
- Add transfer ID guard in handle_GetNodeInfoResponse to reject stale or replayed frames (uses per-node getNodeInfo_transfer_id counter) - Use sizeof(node->hw_unique_id) instead of magic number 16 in memcpy - Fix comment label: "Canard Senders" -> "Canard Handlers and Senders" - Remove extra blank line before #endif at end of file - Rename MSP field last_seen_ms -> elapsed_ms to match notes wording - Fix TwoDistinctNodesStoredSeparately test: interleave encode+dispatch so node 10 is not inserted with node 20's payload data
Replace auto-fetch-on-discovery with a single shared async slot that serves any DroneCAN service request (GetNodeInfo, param GetSet) via two new MSP commands: MSP2_INAV_DRONECAN_ASYNC_REQUEST (0x2043) — fires any service request MSP2_INAV_DRONECAN_ASYNC_RESULT (0x2044) — polls result with seq guard Design changes: - Strip node table to runtime fields only (nodeID, health, mode, uptime_sec, vendor_status_code, last_seen_ms); saves ~3.5 KB RAM - Remove automatic GetNodeInfo fetch on node discovery - Add dronecanAsyncRequest() as single entry point for all service calls - Add handle_AsyncServiceResponse() dispatching on service_id - Add 2-second timeout with seq generation counter to detect stale polls - Add UAVCAN_PROTOCOL_PARAM_GETSET_ID to shouldAcceptTransfer response block - Extend MSP2_INAV_DRONECAN_NODES wire format with uptime_sec and vendor_status_code (13 bytes/record, was 7)
…expiry - Add ExecuteOpcode (service 10) and RestartNode (service 5) to async slot framework: request builder, response decoder, shouldAccept filter, MSP ASYNC_REQUEST/RESULT handlers - Fix GetSet write: zero-initialise encode buffer so reserved bits are not contaminated by stack garbage - Fix GetSet write: include parameter name in request; ArduPilot nodes require the name field to accept writes - Prune node table entries silent for >10s so stale nodes (e.g. after a node ID change) are removed automatically
- Change service_id to uint8_t throughout (slot, function sig, fc_msp local) - Replace obvious buffer zero-init comment with explanation of why (UAVCAN reserved bits) - Fix extra indentation on timeout expiry assignment - MSP reads service_id as u16 for protocol compat; casts to u8 on use
… comments - GetNodeInfo name buffer: clamp to sizeof-1, add null terminator (matches param result pattern); grow name[80] -> name[81] to hold the terminator - Add DRONECAN_STATE_NOT_READY 0xFF constant to header; use it in fc_msp.c instead of a bare magic number - Update accepted/busy comment to note 1 also covers unrecognised service_id
…t, logging - Add UAVCAN-spec transfer_id match in handle_AsyncServiceResponse to reject stale frames after bus-off recovery (in-flight id is (transfer_id-1) & 0x1F) - Add comment clarifying buf_ptr=NULL is intentional for zero-length payloads - Add LOG_DEBUG for ExecuteOpcodeResponse decode failure (matches GetNodeInfo and ParamGetSet pattern)
- PARAM_GETSET minimum-size guard: < 2 → < 3 (index+is_write required); remove fallback ternary for is_write - handle_AsyncServiceResponse: update stale comment to name service_id specifically rather than vague "parameter"
Decode NumericValue min_value and max_value from UAVCAN param.GetSet response and thread them to the configurator. The FC now serializes min/max type and value bytes after the param value in the async result MSP payload.
…e field zeroing, named constants, struct alignment - dronecan.c: map NUMERICVALUE_* → DRONECAN_PARAM_TYPE_* explicitly in min/max decode switch; zero inactive fields before each switch to prevent stale values across consecutive GetSet calls - dronecan.h: reorder min/max struct fields (int64_t first) to eliminate 7-byte ARM alignment padding per min/max group - fc_msp.c: replace bare 1/2 with DRONECAN_PARAM_TYPE_INT/FLOAT; add braces to else-if blocks in min/max serializer
…mResult_t Reorder min/max fields to match existing value field convention (type, int64, float). Fix comment that incorrectly claimed the prior ordering eliminated ARM alignment padding — both orderings produce identical struct size.
Replace the removed MSP2_INAV_DRONECAN_NODE_INFO (0x2043) entry with documentation for the new async slot pattern introduced in the dronecan-param-getset feature: - MSP2_INAV_DRONECAN_NODES (0x2042): fix per-node record size 7→13 bytes (added uptime_sec u32 + vendor_status_code u16) - MSP2_INAV_DRONECAN_ASYNC_REQUEST (0x2043): document all four services (GetNodeInfo, RestartNode, ExecuteOpcode, ParamGetSet read/write) including param type encoding and sequence number correlation - MSP2_INAV_DRONECAN_ASYNC_RESULT (0x2044): document state machine, all service-specific result payloads, and slot-reset behaviour
Replace stale MSP2_INAV_DRONECAN_NODE_INFO entry and fix MSP2_INAV_DRONECAN_NODES record size to match the current dronecan-param-getset implementation: - MSP2_INAV_DRONECAN_NODES (8258): expand per-node record from 7 to 13 bytes — add uptime_sec(u32) and vendor_status_code(u16) fields - MSP2_INAV_DRONECAN_ASYNC_REQUEST (8259): replace removed NODE_INFO entry; documents common header + service-specific request fields for all four services (GetNodeInfo, RestartNode, ExecuteOpcode, ParamGetSet) - MSP2_INAV_DRONECAN_ASYNC_RESULT (8260): new entry documenting the 5-byte common header and all service-specific result payloads
Re-run gen_msp_md.py after updating msp_messages.json to keep the generated Markdown in sync with the JSON source of truth.
…rRespond dronecanAsyncRequest() is called from the main loop (via MSP handler). canardRequestOrRespond() mutates the libcanard TX queue, which is also accessed by the ISR-driven processCanardTxQueue(). Without masking, a TX-complete ISR could corrupt the queue mid-enqueue.
The struct dronecanNodeInfo_t was stripped of GetNodeInfo fields in 96f8a4b (shared async slot) to save ~3.5 KB RAM. GetNodeInfo results now go to dronecanAsyncSlot.result.node_info (DRONECAN_ASYNC_READY) instead of the node table. Update the test to: - Prime the async slot (PENDING + service_id + node_id) before dispatch, since handle_AsyncServiceResponse guards on all three - Assert slot state transitions to DRONECAN_ASYNC_READY - Assert result fields via dronecanAsyncSlot.result.node_info - Reset dronecanAsyncSlot in SetUp() for test isolation
- Replace hardcoded 2000 with DRONECAN_ASYNC_TIMEOUT_MS in timeout check and use >= to align with dronecanAsyncRequest guard - Add DRONECAN_NODE_STALE_TIMEOUT_MS constant; replace magic 10000 - Fix double-indented body in timeout check - Fix brace alignment in shouldAcceptTransfer response cases - Clear async slot to IDLE after reading ERROR state in MSP result handler - Add comment to dronecanParamResult_t.name[93] explaining DSDL capacity - Document 0xFF (bus not ready) sentinel in MSP async request JSON/README
…ART_NODE response decode Extends dronecan_application_unittest.cc with 13 new tests covering: - handle_AsyncServiceResponse guard rejections (IDLE state, wrong node_id, wrong transfer_id, wrong service_id) - PARAM_GETSET response decode for all five Value types (INT, FLOAT, BOOL, STRING, EMPTY) including integer and float min/max range fields - EXECUTE_OPCODE response decode (ok=true and ok=false) - RESTART_NODE response decode (ok=true) - dronecanAsyncRequest re-entry guard (slot PENDING → returns false) 26/26 tests pass.
- dronecan.c: clamp name and string-value lengths before memcpy in dronecanAsyncRequest() — safe today because MSP pre-clamps, but the function is part of a public API and must not rely on caller preconditions - dronecan.c: update stale block comment on handle_AsyncServiceResponse - fc_msp.c: replace manual 32-bit split/join of int64 wire values with memcpy through uint64_t, eliminating signed left-shift and implementation-defined right-shift on negative values - fc_msp.c: remove trailing whitespace on USE_DRONECAN #endif - unittest: narrow GAP-S7 re-entry guard test to RESTART_NODE so the null-payload check cannot mask a missing re-entry guard
- fc_msp.c: add missing sbufAdvance after sbufReadData in STRING write arm of PARAM_GETSET decoder — without it, req_name_len read the first byte of the string value, silently targeting the wrong param on the node - fc_msp.c: add sbufAdvance after name read for consistency with INT path - fc_msp.c: replace signed right-shift with memcpy/uint64_t for min_int and max_int range encoding, consistent with the value_int fix in pass 2 - dronecan.c, fc_msp.c: remove trailing whitespace on blank lines in new code
- dronecan.c: expand handle_AsyncServiceResponse comment to explain the single-slot design rationale - dronecan.c: normalise shouldAcceptTransfer to consistent 4-space indentation (was mixed tabs/spaces from prior edits) - unittest: add shouldAcceptTransfer tests for PARAM_GETSET, EXECUTE_OPCODE, and RESTART_NODE response acceptance (29 tests total)
Owner
Author
|
Closing — reopening against inavflight/inav for visibility. |
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
dronecanAsyncSlot) that serialises GetNodeInfo, param GetSet, ExecuteOpcode, and RestartNode requests through a single IDLE→PENDING→READY/ERROR state machine, avoiding per-service response queuesMSP2_INAV_DRONECAN_ASYNC_REQUEST/MSP2_INAV_DRONECAN_ASYNC_RESULT) so the configurator can read/write node parameters and trigger node restarts without custom DroneCAN toolingelapsed_ms(time since last NodeStatus)shouldAcceptTransferfilter, all four guard-rejection paths, all fiveparam.Valuetypes (INT/FLOAT/BOOL/STRING/EMPTY) with min/max range fields, ExecuteOpcode ok/fail, RestartNode, and the re-entry guardStacked on
fix/h7-dronecan-driver(PR iNavFlight#11607) — do not merge until that lands.Configurator branch:
feature/dronecan-configurator-tabcovers the UI for both GetNodeInfo and param GetSet.Test plan