AP_Bootloader: reject allocation of broadcast node ID #28327
Conversation
It is technically legal to receive an "allocation" of the broadcast node ID. Fortunately, this was already ignored by `canardSetLocalNodeID`, though it would trigger an assertion failure if those were enabled. Fix by rejecting that ID. There is effectively no change in behavior but the code now correctly ignores that ID and retries the allocation as it did before.
| @@ -429,7 +429,7 @@ static void handle_allocation_response(CanardInstance* ins, CanardRxTransfer* tr | |||
| // The allocator has confirmed part of unique ID, switching to the next stage and updating the timeout. | |||
| node_id_allocation_unique_id_offset = msg.unique_id.len; | |||
| send_next_node_id_allocation_request_at_ms -= UAVCAN_PROTOCOL_DYNAMIC_NODE_ID_ALLOCATION_MIN_REQUEST_PERIOD_MS; | |||
| } else { | |||
| } else if (msg.node_id != CANARD_BROADCAST_NODE_ID) { // new ID valid? (if not we will time out and start over) | |||
There was a problem hiding this comment.
i don't think this wins anything, just costs flash, the set of bcast is OK, and we stay in DNA
There was a problem hiding this comment.
Currently having msg.node_id equal to CANARD_BROADCAST_NODE_ID would cause libcanard to execute CANARD_ASSERT(false);: https://github.com/dronecan/libcanard/blob/6f74bc67656882a4ee51966c7c0022d04fa1a3fb/canard.c#L122
Sure by default the assert doesn't do anything, but it's not really correct to do and the same change was accepted for #28045. We are already careful to not pass that value:
ardupilot/Tools/AP_Bootloader/can.cpp
Lines 827 to 829 in 41753b4
There was a problem hiding this comment.
I checked and it only consumes flash on one periph.
It is technically legal to receive an "allocation" of the broadcast node ID. Fortunately, this was already ignored by
canardSetLocalNodeID, though it would trigger an assertion failure if those were enabled.Fix by rejecting that ID. There is effectively no change in behavior but the code now correctly ignores that ID and retries the allocation as it did before.
Similar to #28045 . Tested on CubeOrange (with artificially limited DNA database size) and a pile of MatekL431 boards with updated bootloaders. Not sure the policy on bootloader rebuilds.