Await acks for write SDO, filter endpoint IDs#22
Conversation
solomondg
force-pushed
the
feature/await-sdo-acks
branch
from
2996d1f to
3c70a7f
Compare
| // Value to write | ||
| memcpy(&data[4], &value, sizeof(T)); | ||
|
|
||
| requested_msg_id_ = 0x005; // Await TxSdo acknowledgment |
There was a problem hiding this comment.
let's only write this when timeout_ms != 0. Then we don't have to reset it below. Feels cleaner not to touch the state at all if we don't have to.
| * @return true if the ODrive acknowledged the write (or timeout_ms == 0), | ||
| * false if no matching TxSdo was received before the timeout. | ||
| * | ||
| * The ODrive replies with a TxSdo message in response to any RxSdo, |
There was a problem hiding this comment.
TxSdo ack was introduced in fw 0.6.11. From changelog:
CANSimple: confirmation messages for write SDO messages
So we should mention here that this requires fw 0.6.11, and that for older firmware, fire-and-forget can be used.
| // Flush pending RX messages before writing. On polling-based platforms (e.g. | ||
| // Arduino Uno R4) the single TX mailbox may report busy if the CAN | ||
| // controller hasn't had a chance to process events. This mirrors the | ||
| // can_intf.events() call in the FlexCAN (Teensy) adapter. |
There was a problem hiding this comment.
The can_intf.events() call says // TODO: is this really needed?. So we should remove that TODO and reference here.
But I have to say I find it a bit unclean in both cases, and a bit weird that the RX path is so entangled with the TX path. Was this experimentally confirmed (plz link)? Can you point to the relevant code in the platform driver stack that shows this behavior?
| data, | ||
| }; | ||
| return can_intf.write(msg) >= 0; | ||
| return can_intf.write(msg) > 0; |
There was a problem hiding this comment.
It seems this change is unnecessary?
https://github.com/arduino/ArduinoCore-API/blob/master/api/HardwareCAN.h#L90C6-L90C109
@return 1 if the message was enqueued, an _implementation defined_ error code < 0 if there was an error
So we should leave it as-is, unless this is confirmed to fix an issue with a non-compliant platform (lest we break another non-compliant platform).
2 participants
The ODrive sends a TxSdo in response to any RxSdo, writes included. setEndpoint() was firing the write and returning true unconditionally, with no confirmation it landed.
This makes setEndpoint() wait for that ACK. Mirrors getEndpoint(): it gains an optional timeout_ms = 10 param and returns true only when an acknowledgment arrives whose echoed Endpoint_ID matches the write (timeout_ms = 0 keeps the old behavior).
getEndpoint() now also discards any TxSdo that echoes a different endpoint than requested.