Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions src/ODriveCAN.h
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,12 @@ class ODriveCAN {
return T{};
}

// Ignore a TxSdo that echoes a different endpoint than we requested.
uint16_t resp_endpoint = (uint16_t)buffer_[1] | ((uint16_t)buffer_[2] << 8);
if (resp_endpoint != endpoint_id) {
return T{};
}

T ret{};
memcpy(&ret, &buffer_[4], sizeof(T));
return ret;
Expand All @@ -334,24 +340,43 @@ class ODriveCAN {
* @tparam T Type of the value from flat_endpoints.json
* @param endpoint_id Unique ID of endpoint from flat_endpoints.json
* @param value value to write to the endpoint
* @param timeout_ms Time to wait for the ODrive's TxSdo acknowledgment.
* Pass 0 to send without waiting (fire-and-forget).
*
* This function returns immediately and does not check if the ODrive
* received the CAN message.
* @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,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* including writes, so this blocks until that acknowledgment is received
* or the timeout is reached.
*/
template<typename T>
bool setEndpoint(uint16_t endpoint_id, T value) {
bool setEndpoint(uint16_t endpoint_id, T value, uint16_t timeout_ms = 10) {
uint8_t data[8] = {};
data[0] = 1; // Opcode write

// Endpoint
data[1] = endpoint_id & 0xFF;
data[2] = (endpoint_id >> 8) & 0xFF;
// Little-endian endpoint
data[1] = (uint8_t)(endpoint_id);
data[2] = (uint8_t)(endpoint_id >> 8);

// Value to write
memcpy(&data[4], &value, sizeof(T));

requested_msg_id_ = 0x005; // Await TxSdo acknowledgment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

can_intf_.sendMsg((node_id_ << ODriveCAN::kNodeIdShift) | 0x004, 8, data);
return true;

if (timeout_ms == 0) {
requested_msg_id_ = REQUEST_PENDING; // fire-and-forget, don't wait
return true;
}

if (!awaitMsg(timeout_ms)) {
return false;
}

// Confirm the acknowledgment echoes the endpoint we wrote to.
uint16_t acked_endpoint = (uint16_t)buffer_[1] | ((uint16_t)buffer_[2] << 8);
return acked_endpoint == endpoint_id;
}

private:
Expand Down
14 changes: 12 additions & 2 deletions src/ODriveHardwareCAN.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
// Must be defined by the application
void onCanMessage(const CanMsg& msg);

// Forward declaration; defined below and used by sendMsg().
static void pumpEvents(HardwareCAN& intf, int max_events = 100);

/**
* @brief Sends a CAN message over the specified platform-specific interface.
*
Expand All @@ -26,12 +29,19 @@ static bool sendMsg(HardwareCAN& can_intf, uint32_t id, uint8_t length, const ui
// Note: Arduino_CAN does not support the RTR bit. The ODrive interprets
// zero-length packets the same as RTR=1, but it creates the possibility of
// collisions.

// 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

pumpEvents(can_intf);

CanMsg msg{
(id & 0x80000000) ? CanExtendedId(id) : CanStandardId(id),
length,
data,
};
return can_intf.write(msg) >= 0;
return can_intf.write(msg) > 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

/**
Expand All @@ -56,7 +66,7 @@ static void onReceive(const CanMsg& msg, ODriveCAN& odrive) {
* @param max_events: The maximum number of events to process. This prevents
* an infinite loop if messages come at a high rate.
*/
static void pumpEvents(HardwareCAN& intf, int max_events = 100) {
static void pumpEvents(HardwareCAN& intf, int max_events) {
// max_events prevents an infinite loop if messages come at a high rate
while (intf.available() && max_events--) {
onCanMessage(intf.read());
Expand Down
Loading