fix(twai): keep the TX frame alive until on_tx_done and wait for completion

[finger563]

Problem

twai_node_transmit() only queues the transmission: the esp_driver_twai driver holds the passed twai_frame_t pointer (and its data buffer) and formats the frame later, in the TX ISR. transmit() passed a stack-local frame whose buffer pointed into the caller's Message, so both were dead by the time the ISR read them.

Single, spaced-out transmits usually survived on stale-but-intact stack memory, but two back-to-back transmits (e.g. a PDO followed by a SYNC) overwrite the first frame while it is still queued — the ISR then reads garbage, tripping the driver's byte_len <= TWAIFD_FRAME_MAX_LEN assert in twaifd_len2dlc():

assert failed: twaifd_len2dlc .../twai_types.h:152 (byte_len <= TWAIFD_FRAME_MAX_LEN)
#3 twaifd_len2dlc (byte_len=48828)
#4 twai_hal_format_frame

Fix

• Copy the message into member storage (tx_message_ / tx_frame_) that outlives the call.
• Register an on_tx_done ISR callback that gives a binary semaphore.
• Serialize transmitters with a dedicated mutex (one frame in flight — its storage must not be overwritten until completion).
• Block until the driver reports transmission complete (bounded by timeout_ms; an unacknowledged classic-CAN frame is retransmitted indefinitely).

As a bonus, transmit() returning true now means the frame was actually sent on the bus, not merely queued.

Testing

Found and fixed while bringing up a CANopen master (SDO + PDO + SYNC traffic) on an ESP32-P4 talking to a Basicmicro MCP266. The crash reproduced deterministically on the first back-to-back transmit pair and is gone after this change; position/velocity control traffic runs indefinitely.

🤖 Generated with Claude Code

[github-actions]

✅Static analysis result - no issues found! ✅

[Copilot]

🟡 Changes recommended

There are concurrency/timeout edge cases (semaphore lifetime vs concurrent teardown, and unbounded completion wait when timeout_ms < 0) that can lead to undefined behavior or hangs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a TWAI transmit lifetime bug by ensuring the twai_frame_t descriptor and its data buffer remain valid until the driver’s on_tx_done ISR callback fires, turning transmit() into a “sent on bus (or timed out)” operation rather than a mere enqueue.

Changes:

• Store the outgoing message/frame in Twai member storage to avoid use-after-scope when the TX ISR formats queued frames.
• Add an on_tx_done ISR callback + binary semaphore to wait for completion, and serialize transmitters with a dedicated mutex.
• Update API documentation/comments to reflect the driver’s deferred frame formatting behavior.

File summaries

File	Description
components/twai/include/twai.hpp	Makes TX frame storage persistent, adds TX completion signaling, and changes transmit() to wait for actual completion.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[finger563]

Addressed the review comments:

• Unbounded completion wait when timeout_ms < 0: the completion wait is now always bounded — it uses DEFAULT_TX_TIMEOUT_MS when timeout_ms < 0 (which still means "wait forever to queue"), so an unacknowledged frame can't hang the caller. Docstring updated to state the queue vs completion timeout split.
• Semaphore lifetime vs concurrent teardown: teardown() now takes tx_mutex_ before deleting tx_done_sem_. The node is already deleted at that point (so no new transmit starts and no ISR fires), and since the completion wait is now bounded, acquiring tx_mutex_ waits out any in-flight transmit() (at most the completion timeout) before freeing the semaphore.
• Semaphore leak on init failure paths: moved semaphore creation to just before the node is enabled, after every fallible setup step (which delete node_ inline). No earlier failure path can leak it, and no TX — hence no on_tx_done — can fire before it exists, since transmitting requires an enabled node. Its own creation-failure path routes through teardown().

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

[finger563]

Second round addressed:

• Timeout leaves the driver referencing our storage (the key one): on a completion timeout, transmit() now aborts the pending transmission — a twai_node_disable()/twai_node_enable() cycle flushes the TX queue — before returning, so the driver no longer references tx_frame_/tx_message_.data when a later transmit reuses them. Also, the whole transmit (node access → driver call → completion wait) is now serialized under tx_mutex_, so a single frame is genuinely in flight at a time.
• Node deletion not synchronized with transmit(): teardown() now (1) disables the node under mutex_ first — which also unblocks a transmit() stuck in twai_node_transmit() on a full queue with timeout_ms < 0, avoiding a deadlock — then (2) takes tx_mutex_ then mutex_ (same order as transmit()) before deleting the node, queue and semaphore. So a transmit can no longer call twai_node_transmit() on a handle teardown just deleted.
• ISR robustness: on_tx_done_cb now null-checks tx_done_sem_ before xSemaphoreGiveFromISR, so a completion racing with teardown / partial init can't deref a freed/absent handle.

[finger563]

Self-review — one fix applied, and two tradeoffs I want to flag for reviewers:

• Fixed: on the completion-timeout abort, if twai_node_enable() fails after the disable, the node is left disabled but enabled_ was still true. Now enabled_ is set false (and logged) so later transmit()s reject early instead of calling into a disabled node.

• Tradeoff (by design): the timeout abort bounces the whole node. A completion timeout means nothing ACKed the frame within the window (dead bus / lone node / prolonged arbitration loss), and the driver still holds a pointer to tx_frame_. With the single in-flight buffer, the only way to make that pointer safe to reuse is to stop the driver referencing it, so transmit() does a disable()/enable() which also drops any queued RX. I judged this acceptable because a TX timeout already indicates a real fault; the alternative (a per-transmit frame pool or an EBUSY-until-on_tx_done gate) is heavier. Happy to switch to a pool if you'd prefer to avoid the RX disruption.

• Best-effort ISR guard: the on_tx_done_cb null-check closes the partial-init / freed-handle cases; the ordering (teardown disables the node before taking tx_mutex_ to free the semaphore) is what actually prevents a new completion ISR from firing after the semaphore is freed. A already-latched interrupt racing vSemaphoreDelete is a narrow window I did not fully close (would need interrupt masking); this matches the component's existing best-effort teardown semantics.

Compiles clean; the MIB CANopen app (heavy SDO traffic through this transmit()) runs against it. The new timeout-abort path is exercised only on a dead bus, which I have not bench-tested on hardware.