diff --git a/components/twai/include/twai.hpp b/components/twai/include/twai.hpp index e76bd7896a..d5e3961014 100644 --- a/components/twai/include/twai.hpp +++ b/components/twai/include/twai.hpp @@ -15,6 +15,7 @@ #include "freertos/FreeRTOS.h" #include "freertos/queue.h" +#include "freertos/semphr.h" #include "base_component.hpp" #include "format.hpp" @@ -72,8 +73,10 @@ class Twai : public BaseComponent { /// \brief Convert this Message into a driver \c twai_frame_t for transmission. /// \note The returned frame's \c buffer points into this Message's \c data - /// array, so the Message must outlive the returned frame (which it - /// does for the duration of a synchronous transmit call). + /// array, and the driver reads both the frame and the buffer from the + /// TX ISR after \c twai_node_transmit() queues them -- so both must + /// stay valid until the on_tx_done callback (transmit() guarantees + /// this by using member storage and waiting for completion). /// \return A \c twai_frame_t describing this message. twai_frame_t to_twai_frame() const { // classic CAN carries at most MAX_DATA_LEN (8) data bytes; clamp defensively @@ -237,6 +240,7 @@ class Twai : public BaseComponent { // register the ISR event callbacks twai_event_callbacks_t cbs = {}; cbs.on_rx_done = &Twai::on_rx_done_cb; + cbs.on_tx_done = &Twai::on_tx_done_cb; if (config_.on_state_change) { cbs.on_state_change = &Twai::on_state_change_cb; } @@ -290,6 +294,23 @@ class Twai : public BaseComponent { }); task_->start(); + // Transmit-completion semaphore (given from the on_tx_done ISR callback). + // Created here, after every fallible setup step above has succeeded and + // before the node is enabled: no earlier failure path (which delete node_ + // inline) can leak it, and no TX -- hence no on_tx_done -- can fire before + // it exists, since transmitting requires an enabled node. On failure, + // teardown() (below / in the destructor) releases task, queue and node. + if (!tx_done_sem_) { + tx_done_sem_ = xSemaphoreCreateBinary(); + if (!tx_done_sem_) { + logger_.error("Failed to create TX-done semaphore"); + ec = std::make_error_code(std::errc::not_enough_memory); + lock.unlock(); + teardown(); + return false; + } + } + // enable the node if requested if (config_.auto_start) { err = twai_node_enable(node_); @@ -356,18 +377,37 @@ class Twai : public BaseComponent { return true; } - /// \brief Transmit a CAN message. + /// \brief Transmit a CAN message and wait for it to complete. + /// \details \c twai_node_transmit() only QUEUES the transmission: the driver + /// keeps the passed \c twai_frame_t pointer (and its data buffer) + /// and formats the frame later, in the TX ISR. The descriptor must + /// therefore stay valid until the on_tx_done callback fires -- a + /// stack-local frame is read after the caller's stack is gone, + /// transmitting garbage (or tripping the driver's DLC assert). This + /// method copies the message into member storage that outlives the + /// call, serializes transmitters, and blocks until the driver + /// reports transmission complete. /// \param message The message to transmit. - /// \param ec The error code, set if the transmission could not be queued. - /// \param timeout_ms Max time (ms) to wait if the TX queue is full (-1 = forever). - /// \return True if the message was queued for transmission, false otherwise. + /// \param ec The error code, set if the transmission could not be queued or + /// did not complete within the timeout. + /// \param timeout_ms Max time (ms) to wait to queue the frame (-1 = forever + /// for the queueing step). The subsequent wait for transmit + /// completion is always bounded (by this value when >= 0, else by + /// DEFAULT_TX_TIMEOUT_MS) so an unacknowledged frame cannot hang the + /// caller. + /// \return True if the message was transmitted, false otherwise. bool transmit(const Message &message, std::error_code &ec, int timeout_ms = DEFAULT_TX_TIMEOUT_MS) { ec.clear(); - // Snapshot the handle under the lock, then call the (potentially blocking, - // up to timeout_ms) driver transmit with the lock RELEASED so a congested - // TX queue cannot stall is_enabled()/get_status()/stop()/the destructor. - // The driver's transmit path is internally thread-safe. + // Serialize the whole transmit -- node access, the driver call, and the + // completion wait -- under tx_mutex_. The driver keeps a pointer to + // tx_frame_ / tx_message_.data until on_tx_done, so only one frame may be + // in flight, and teardown() takes tx_mutex_ before deleting the node / + // semaphore, so neither can be freed while a transmit is using them. Note + // tx_mutex_ is distinct from mutex_ (which guards node_/enabled_ and is + // held only briefly below), so a congested TX queue does not stall + // is_enabled() / get_status() / stop(). + std::lock_guard tx_lock(tx_mutex_); twai_node_handle_t node = nullptr; { std::lock_guard lock(mutex_); @@ -389,8 +429,12 @@ class Twai : public BaseComponent { } node = node_; } - twai_frame_t frame = message.to_twai_frame(); - esp_err_t err = twai_node_transmit(node, &frame, timeout_ms); + // drain a stale completion (e.g. from a prior transmit whose frame we + // aborted below after a timeout) so the wait sees only our own + xSemaphoreTake(tx_done_sem_, 0); + tx_message_ = message; + tx_frame_ = tx_message_.to_twai_frame(); + esp_err_t err = twai_node_transmit(node, &tx_frame_, timeout_ms); if (err != ESP_OK) { logger_.error("Failed to transmit frame: {}", esp_err_to_name(err)); if (err == ESP_ERR_TIMEOUT) { @@ -404,6 +448,39 @@ class Twai : public BaseComponent { } return false; } + // Wait for the on_tx_done callback; on a healthy bus a classic frame + // completes in well under a millisecond, but an unacknowledged frame is + // retransmitted indefinitely, so ALWAYS bound the completion wait -- even + // when timeout_ms < 0 (which means "wait forever to queue", above): an + // unbounded completion wait would hang the caller on a bus with no ACK. + const TickType_t wait_ticks = + timeout_ms < 0 ? pdMS_TO_TICKS(DEFAULT_TX_TIMEOUT_MS) : pdMS_TO_TICKS(timeout_ms); + if (xSemaphoreTake(tx_done_sem_, wait_ticks) != pdTRUE) { + // The frame was queued but did not complete (e.g. nothing ACKed it, so + // the controller keeps retransmitting). The driver still references + // tx_frame_ / tx_message_.data, and we are about to release tx_mutex_ and + // let a later transmit overwrite them -- so first stop the driver from + // referencing that storage by aborting the pending transmission: a + // disable/enable cycle flushes the TX queue. Best-effort, under mutex_. + { + std::lock_guard lock(mutex_); + if (node_ && enabled_) { + twai_node_disable(node_); + // Keep enabled_ consistent with the hardware: if the re-enable fails + // the node is left disabled, so reflect that (later transmit()s will + // then reject early instead of calling into a disabled node). + if (twai_node_enable(node_) != ESP_OK) { + logger_.error("Failed to re-enable TWAI node after aborting a stuck transmit"); + enabled_ = false; + } + } + } + // absorb a completion that may have raced in just before the abort + xSemaphoreTake(tx_done_sem_, 0); + logger_.error("Timed out waiting for transmit completion (no ACK on the bus?)"); + ec = std::make_error_code(std::errc::timed_out); + return false; + } return true; } @@ -502,25 +579,42 @@ class Twai : public BaseComponent { task_->stop(); task_.reset(); } - // delete the node - if (node_) { - // twai_node_delete() requires the node to be disabled first. If a prior - // stop() failed (or was never called) and the node is still enabled, - // deleting it would fail and leak the driver resource -- so make a - // best-effort disable here before deleting. - if (enabled_) { + // Disable the node first (best-effort, under mutex_). twai_node_delete() + // requires a disabled node anyway, and disabling also aborts / unblocks any + // in-flight twai_node_transmit() so a concurrent transmit() blocked on a + // full queue (timeout_ms < 0) can return and release tx_mutex_ -- otherwise + // acquiring tx_mutex_ below could deadlock against it. Done in its own + // scope so mutex_ is released before we take tx_mutex_ then mutex_ (keeping + // the tx_mutex_ -> mutex_ order that transmit() uses). + { + std::lock_guard lock(mutex_); + if (node_ && enabled_) { esp_err_t err = twai_node_disable(node_); if (err != ESP_OK) logger_.warn("Could not disable TWAI node before delete: {}", esp_err_to_name(err)); enabled_ = false; } - twai_node_delete(node_); - node_ = nullptr; } - // delete the queue - if (queue_) { - vQueueDelete(queue_); - queue_ = nullptr; + // Delete the node, queue and TX-done semaphore under tx_mutex_ so a + // transmit() that started before teardown -- which holds tx_mutex_ across + // the (now always bounded) driver call and completion wait, and references + // node_ and tx_done_sem_ -- has finished before we free them. transmit() + // takes tx_mutex_ then mutex_, so acquire them in the same order here. + { + std::lock_guard tx_lock(tx_mutex_); + std::lock_guard lock(mutex_); + if (node_) { + twai_node_delete(node_); + node_ = nullptr; + } + if (queue_) { + vQueueDelete(queue_); + queue_ = nullptr; + } + if (tx_done_sem_) { + vSemaphoreDelete(tx_done_sem_); + tx_done_sem_ = nullptr; + } } enabled_ = false; } @@ -545,6 +639,22 @@ class Twai : public BaseComponent { return higher_priority_task_woken == pdTRUE; } + static bool on_tx_done_cb(twai_node_handle_t handle, const twai_tx_done_event_data_t *edata, + void *user_ctx) { + (void)handle; + (void)edata; + auto *self = static_cast(user_ctx); + // Guard against a lifecycle race: the semaphore may not exist yet (a TX + // completing during a partial init) or may already be freed (teardown), so + // never dereference a null handle from ISR context. + if (!self->tx_done_sem_) { + return false; + } + BaseType_t higher_priority_task_woken = pdFALSE; + xSemaphoreGiveFromISR(self->tx_done_sem_, &higher_priority_task_woken); + return higher_priority_task_woken == pdTRUE; + } + static bool on_state_change_cb(twai_node_handle_t handle, const twai_state_change_event_data_t *edata, void *user_ctx) { (void)handle; @@ -613,6 +723,16 @@ class Twai : public BaseComponent { QueueHandle_t queue_{nullptr}; std::unique_ptr task_; bool enabled_{false}; + + // Transmit path: the driver holds a pointer to tx_frame_ (whose buffer + // points into tx_message_.data) from twai_node_transmit() until the + // on_tx_done ISR callback, so both live here rather than on the stack; + // tx_mutex_ keeps a single frame in flight and tx_done_sem_ signals + // completion from the ISR. + std::mutex tx_mutex_; + SemaphoreHandle_t tx_done_sem_{nullptr}; + Message tx_message_{}; + twai_frame_t tx_frame_{}; }; } // namespace espp