search

ArduPilot / ardupilot

ArduPlane, ArduCopter, ArduRover, ArduSub source
http://ardupilot.org/
GNU General Public License v3.0
10.47k stars 17.12k forks source link

AP_DroneCAN: No check for undefined all zero UID #26203

Closed reilly-callaway closed 3 weeks ago

reilly-callaway commented 6 months ago

Bug report

Issue details

No check exists for an all zero UID. The DSDL spec suggests that this should be considered an undefined UID.

From the DSDL for HardwareVersion.uavcan:

Unique ID is a 128 bit long sequence that is globally unique for each node.
All zeros is not a valid UID.
If filled with zeros, assume that the value is undefined.

From a code read through, and a CAND log message containing a UID reporting as zeros, there appears to be no check for this undefined UID. Seems reasonable that a check for this undefined UID could be included in AP_DroneCAN_DNA_Server::handleNodeInfo.

Version Master (5d789f46ea06a92bf199e8e0d6241149e0b5ccb2)

Platform [ x ] All [ ] AntennaTracker [ ] Copter [ ] Plane [ ] Rover [ ] Submarine

Airframe type Any

Hardware type Any

Logs N/A

IamPete1 commented 6 months ago

The spec also says "Version information shall not be changed while the node is running"

IamPete1 commented 6 months ago

To elaborate slightly more, we could refuse UID 0. But that would result in any existing devices in the wild that use 0 not being accepted anymore. The UID is used in the DNA database for allocation of nodes. 0 is a valid value for that use case although perhaps not as unique as we would like but its unlikely there will be two nodes with unique ID 0 both trying to get the same node ID.

peterbarker commented 1 month ago

So I think this is a "Can't Fix".

@reilly-callaway are you OK with that resolution, or do you have a workaround for @IamPete1 's "we'd break vast numbers of setups" problem?

reilly-callaway commented 3 weeks ago

Seems reasonable to avoid breaking these setups.

This came up in rare instances of a node sending junk, zeroed out data in a node info packet (which turns out to produce a valid checksum), causing duplicate node id warnings from ArduPilot. Using the ignore duplicates nodes setting also worked.