DroneCAN: Add node table, CLI status, and MSP2 node query commands#11527
DroneCAN: Add node table, CLI status, and MSP2 node query commands#11527daijoubu wants to merge 3 commits intoiNavFlight:maintenance-10.xfrom
Conversation
Review Summary by QodoAdd DroneCAN node table, MSP node query commands, and CLI status output
WalkthroughsDescriptionThis description is generated by an AI tool. It may have inaccuracies • Add DroneCAN node table infrastructure with dronecanNodeInfo_t struct and accessor functions • Implement two new MSP2 commands (0x2042, 0x2043) for querying detected DroneCAN nodes • Add DroneCAN status output to CLI status command showing nodeID, bitrate, bus state, and node count • Update DroneCAN documentation with node table, MSP commands, and CLI diagnostics Diagramflowchart LR
NodeStatus["NodeStatus Messages"] -->|upsert| NodeTable["Node Table<br/>nodeTable[32]"]
NodeTable -->|dronecanGetNode| Accessor["Accessor Functions"]
Accessor -->|dronecanGetNodeCount| CLI["CLI Status Command"]
Accessor -->|dronecanGetState| CLI
Accessor -->|dronecanGetBitrateKbps| CLI
Accessor -->|dronecanGetNode| MSP["MSP2 Commands<br/>0x2042/0x2043"]
CLI -->|display| User["User Output"]
MSP -->|reply| Configurator["Configurator/GCS"]
File Changes1. src/main/drivers/dronecan/dronecan.h
|
Code Review by Qodo
1. MSP2_INAV_DRONECAN_NODES buffer overflow
|
daijoubu
force-pushed
the
feature/msp-dronecan-support
branch
from
051da24 to
a5e4298
Compare
…DES and MSP2_INAV_DRONECAN_NODE_INFO in preparation to add dronecan support to Inav Configurator.
…CLI status output
daijoubu
force-pushed
the
feature/msp-dronecan-support
branch
from
a5e4298 to
3ce50c5
Compare
|
Test firmware build ready — commit Download firmware for PR #11527 234 targets built. Find your board's
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| case MSP2_INAV_DRONECAN_NODES: | ||
| { | ||
| uint8_t count = dronecanGetNodeCount(); | ||
| sbufWriteU8(dst, count); | ||
| for (uint8_t i = 0; i < count; i++) { | ||
| const dronecanNodeInfo_t *node = dronecanGetNode(i); | ||
| sbufWriteU8(dst, node->nodeID); | ||
| sbufWriteU8(dst, node->health); | ||
| sbufWriteU8(dst, node->mode); | ||
| sbufWriteU32(dst, node->uptime_sec); | ||
| sbufWriteU16(dst, node->vendor_status_code); | ||
| sbufWriteU32(dst, node->last_seen_ms); | ||
| sbufWriteU8(dst, node->name_len); | ||
| sbufWriteData(dst, node->name, 16); | ||
| } |
There was a problem hiding this comment.
1. msp2_inav_dronecan_nodes buffer overflow 📘 Rule violation ⛨ Security
The new MSP2_INAV_DRONECAN_NODES reply writes nodeCount records to dst without checking remaining buffer space, so a full 32-node table (1 + 32*30 = 961 bytes) can exceed the fixed MSP out buffer and corrupt memory. This violates the requirement to validate protocol payload sizes before parsing/serialization.
Agent Prompt
## Issue description
`MSP2_INAV_DRONECAN_NODES` serializes `nodeCount` node records without verifying that the MSP reply buffer (`dst`) has enough remaining capacity, which can overflow the fixed `MSP_PORT_OUTBUF_SIZE` out buffer.
## Issue Context
- `sbufWriteU8/U16/U32` and `sbufWriteData` do not perform bounds checking.
- Default MSP out buffer is `MSP_PORT_OUTBUF_SIZE` (512 unless `USE_FLASHFS`).
- Worst-case payload is `1 + nodeCount * 30` bytes (per-node record is 30 bytes in current implementation).
## Fix Focus Areas
- src/main/fc/fc_msp.c[1774-1788]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| case MSP2_INAV_DRONECAN_NODE_INFO: | ||
| { | ||
| if (sbufBytesRemaining(src) < 1) { | ||
| return MSP_RESULT_ERROR; | ||
| } | ||
| uint8_t nodeId = sbufReadU8(src); | ||
| uint8_t count = dronecanGetNodeCount(); | ||
| for (uint8_t i = 0; i < count; i++) { | ||
| const dronecanNodeInfo_t *node = dronecanGetNode(i); | ||
| if (node->nodeID == nodeId) { | ||
| sbufWriteU8(dst, node->nodeID); | ||
| sbufWriteU8(dst, node->health); | ||
| sbufWriteU8(dst, node->mode); | ||
| sbufWriteU32(dst, node->uptime_sec); | ||
| sbufWriteU16(dst, node->vendor_status_code); | ||
| sbufWriteU32(dst, node->last_seen_ms); | ||
| sbufWriteU8(dst, node->name_len); | ||
| sbufWriteData(dst, node->name, 32); | ||
| return MSP_RESULT_ACK; | ||
| } | ||
| } | ||
| return MSP_RESULT_ERROR; | ||
| } |
There was a problem hiding this comment.
2. Wrong msp error result 🐞 Bug ≡ Correctness
MSP2_INAV_DRONECAN_NODE_INFO returns MSP_RESULT_ERROR/MSP_RESULT_ACK from a bool-returning function and never updates *ret, so “node not found” (and malformed request) can be treated as handled but still replied as ACK. This breaks the documented protocol behavior and can cause clients to interpret an empty/partial payload as success.
Agent Prompt
### Issue description
`mspFCProcessInOutCommand()` returns `bool` (handled/unhandled) and uses the `mspResult_e *ret` out-param to communicate ACK vs ERROR. The new `MSP2_INAV_DRONECAN_NODE_INFO` case incorrectly `return`s `MSP_RESULT_*` values directly and never assigns `*ret`, causing error cases (e.g. unknown node ID) to be replied as ACK.
### Issue Context
`mspFcProcessCommand()` initializes `ret = MSP_RESULT_ACK` and only changes it if `mspFCProcessInOutCommand()` writes to `*ret`. Returning `MSP_RESULT_ERROR` (-1) from a `bool` function is still truthy and prevents the fallback path, so the reply result remains ACK.
### Fix Focus Areas
- src/main/fc/fc_msp.c[4273-4310]
- src/main/fc/fc_msp.c[4826-4853]
### Implementation notes
- In the `MSP2_INAV_DRONECAN_NODE_INFO` case:
- Replace `return MSP_RESULT_ERROR;` with `*ret = MSP_RESULT_ERROR; break;`
- Replace `return MSP_RESULT_ACK;` with `*ret = MSP_RESULT_ACK; break;`
- Keep the function’s final `return true;` behavior for handled commands.
- Ensure the “not found” path sets `*ret = MSP_RESULT_ERROR` (per docs) and does not accidentally return ACK.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Summary
statusCLI command — shows nodeID, bitrate, bus state, and detected node countdronecanNodeInfo_tstruct,nodeTable[32], updatedhandle_NodeStatus()to upsert per-node data, and new accessors (dronecanGetState(),dronecanGetNodeCount(),dronecanGetNode(),dronecanGetBitrateKbps())MSP2_INAV_DRONECAN_NODES(0x2042) — returns count and status of all detected DroneCAN nodesMSP2_INAV_DRONECAN_NODE_INFO(0x2043) — returns full detail for a single node by IDdocs/development/msp/DroneCAN.mdandDroneCAN-Driver.mdupdated to cover node table, CLI output, and MSP commandsThese MSP commands are the prerequisite for DroneCAN support in the Configurator UI (feature-dronecan-configurator-tab).
Test plan
statusCLI shows correct nodeID, bitrate, and node countMSP2_INAV_DRONECAN_NODESreturns the battery monitor node (health=OK, mode=OPERATIONAL)MSP2_INAV_DRONECAN_NODE_INFOreturns correct data for node IDMSP2_INAV_DRONECAN_NODE_INFOreturns empty response for unknown node IDUSE_DRONECANbuild cleanly.