-
Notifications
You must be signed in to change notification settings - Fork 474
Fix: TWAI transmit hangs on empty bus #6070
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1219,6 +1219,24 @@ where | |
| self.regs().status().read().bus_off_st().bit_is_set() | ||
| } | ||
|
|
||
| /// Initiates recovery from the bus-off state. | ||
| /// | ||
| /// The peripheral recovers once it has observed 128 occurrences of 11 | ||
| /// consecutive recessive (idle) bits on the bus. Use [`Self::is_bus_off`] | ||
| /// to poll whether recovery has completed. | ||
| /// | ||
| /// Does nothing if the peripheral is not in the bus-off state. | ||
| #[instability::unstable] | ||
| pub fn initiate_recovery(&mut self) { | ||
| if self.is_bus_off() { | ||
| // Entering the bus-off state parks the peripheral in reset mode; | ||
| // the 1-to-0 transition of the reset mode bit starts the recovery | ||
| // sequence. | ||
| self.regs().mode().modify(|_, w| w.reset_mode().set_bit()); | ||
| self.regs().mode().modify(|_, w| w.reset_mode().clear_bit()); | ||
| } | ||
| } | ||
|
|
||
| /// Get the number of messages that the peripheral has available in the | ||
| /// receive FIFO. | ||
| /// | ||
|
|
@@ -1289,6 +1307,14 @@ where | |
| /// NOTE: TODO: This may not work if using the self reception/self test | ||
| /// functionality. See notes 1 and 2 in the "Frame Identifier" section | ||
| /// of the reference manual. | ||
| /// | ||
| /// If a previous frame is still pending and the peripheral has entered the | ||
| /// error-passive state because of it (the bus cannot carry the frame, e.g. | ||
| /// no other node acknowledges it), the pending transmission is aborted and | ||
| /// [`EspTwaiError::TransmissionAborted`] is returned. An unacknowledged | ||
| /// transmitter's error counter does not increase past the error-passive | ||
| /// threshold, so without giving up the transmission would be retried | ||
| /// forever. | ||
| pub fn transmit(&mut self, frame: &EspTwaiFrame) -> nb::Result<(), EspTwaiError> { | ||
| let status = self.regs().status().read(); | ||
|
|
||
|
|
@@ -1298,6 +1324,12 @@ where | |
| } | ||
| // Check that the peripheral is not already transmitting a packet. | ||
| if status.tx_buf_st().bit_is_clear() { | ||
| if is_error_passive(self.regs()) { | ||
| // Give up on the pending frame: the bus is apparently unable to | ||
| // carry it. | ||
| self.regs().cmd().write(|w| w.abort_tx().set_bit()); | ||
| return nb::Result::Err(nb::Error::Other(EspTwaiError::TransmissionAborted)); | ||
| } | ||
| return nb::Result::Err(nb::Error::WouldBlock); | ||
| } | ||
|
|
||
|
|
@@ -1365,6 +1397,10 @@ pub enum TwaiInterrupt { | |
| ArbitrationLost, | ||
| /// The controller has entered an error passive state. | ||
| ErrorPassive, | ||
| /// The error or bus status has changed: an error counter crossed the | ||
| /// error warning limit in either direction, or the controller entered or | ||
| /// left the bus-off state. | ||
| ErrorWarning, | ||
| } | ||
|
|
||
| /// Represents errors that can occur in the TWAI driver. | ||
|
|
@@ -1375,6 +1411,10 @@ pub enum TwaiInterrupt { | |
| pub enum EspTwaiError { | ||
| /// TWAI peripheral has entered a bus-off state. | ||
| BusOff, | ||
| /// The transmission was aborted because the peripheral repeatedly failed | ||
| /// to transmit the frame and entered the error-passive state (e.g. no | ||
| /// other node on the bus acknowledged the frame). | ||
| TransmissionAborted, | ||
| /// The received frame contains an invalid DLC. | ||
| NonCompliantDlc(u8), | ||
| /// Invalid data length. | ||
|
|
@@ -1483,6 +1523,13 @@ pub trait PrivateInstance: crate::private::Sealed { | |
| TwaiInterrupt::BusError => w.bus_err_int_ena().bit(enable), | ||
| TwaiInterrupt::ArbitrationLost => w.arb_lost_int_ena().bit(enable), | ||
| TwaiInterrupt::ErrorPassive => w.err_passive_int_ena().bit(enable), | ||
| TwaiInterrupt::ErrorWarning => { | ||
| #[cfg(any(esp32, esp32c3, esp32s2, esp32s3))] | ||
| let w = w.err_warn_int_ena().bit(enable); | ||
| #[cfg(any(esp32c6, esp32h2))] | ||
| let w = w.ext_err_warning_int_ena().bit(enable); | ||
| w | ||
| } | ||
| }; | ||
| } | ||
| w | ||
|
|
@@ -1509,6 +1556,13 @@ fn release_receive_fifo(register_block: &RegisterBlock) { | |
| register_block.cmd().write(|w| w.release_buf().set_bit()); | ||
| } | ||
|
|
||
| /// Check if the peripheral is in the error-passive state, i.e. one of the | ||
| /// error counters has reached 128. | ||
| fn is_error_passive(register_block: &RegisterBlock) -> bool { | ||
| register_block.tx_err_cnt().read().tx_err_cnt().bits() >= 128 | ||
| || register_block.rx_err_cnt().read().rx_err_cnt().bits() >= 128 | ||
| } | ||
|
|
||
| /// Write a frame to the peripheral. | ||
| fn write_frame(register_block: &RegisterBlock, frame: &EspTwaiFrame) { | ||
| // SAFETY: safe because there are 13 data registers and the slice is 13 bytes long max | ||
|
|
@@ -1685,6 +1739,12 @@ mod asynch { | |
| /// stops it, in case it is activly transmitting. Therefor it could be | ||
| /// the case that even though the future is dropped, the frame was sent | ||
| /// anyways. | ||
| /// | ||
| /// If the bus cannot carry the frame (e.g. no other node acknowledges | ||
| /// it), the transmission is aborted once the peripheral enters the | ||
| /// error-passive or bus-off state and the future resolves to | ||
| /// [`EspTwaiError::TransmissionAborted`] or [`EspTwaiError::BusOff`], | ||
| /// respectively. | ||
| pub async fn transmit_async(&mut self, frame: &EspTwaiFrame) -> Result<(), EspTwaiError> { | ||
| self.tx.transmit_async(frame).await | ||
| } | ||
|
|
@@ -1732,7 +1792,21 @@ mod asynch { | |
| } | ||
|
|
||
| // Check that the peripheral is not currently transmitting a packet. | ||
| // This must come before the error-passive check: a frame whose | ||
| // buffer has been released was transmitted successfully, no matter | ||
| // what state the error counters are in. | ||
| if status.tx_buf_st().bit_is_clear() { | ||
| if is_error_passive(regs) { | ||
| // Give up on the pending frame: the bus is apparently | ||
| // unable to carry it (e.g. no other node acknowledges it), | ||
| // and an unacknowledged transmitter's error counter does | ||
| // not increase past the error-passive threshold, so the | ||
| // frame would otherwise be retried forever. The pending | ||
| // frame is not necessarily this future's own; a blocking | ||
| // `transmit` may have left it behind. | ||
| regs.cmd().write(|w| w.abort_tx().set_bit()); | ||
| return Poll::Ready(Err(EspTwaiError::TransmissionAborted)); | ||
| } | ||
| return Poll::Pending; | ||
| } | ||
|
|
||
|
|
@@ -1763,6 +1837,12 @@ mod asynch { | |
| /// stops it, in case it is actively transmitting. Therefor it could be | ||
| /// the case that even though the future is dropped, the frame was sent | ||
| /// anyways. | ||
| /// | ||
| /// If the bus cannot carry the frame (e.g. no other node acknowledges | ||
| /// it), the transmission is aborted once the peripheral enters the | ||
| /// error-passive or bus-off state and the future resolves to | ||
| /// [`EspTwaiError::TransmissionAborted`] or [`EspTwaiError::BusOff`], | ||
| /// respectively. | ||
| pub async fn transmit_async(&mut self, frame: &EspTwaiFrame) -> Result<(), EspTwaiError> { | ||
| TransmitFuture::new(self.twai.reborrow(), frame).await | ||
| } | ||
|
|
@@ -1797,21 +1877,35 @@ mod asynch { | |
| let int_ena_reg = register_block.int_ena(); | ||
| let int_ena = int_ena_reg.read(); | ||
|
|
||
| // The error warning interrupt fires on every change of the error or | ||
| // bus status. Entering bus-off sets both the bus-off and the error | ||
| // warning status; during bus-off recovery the transmit error counter | ||
| // counts down and the error warning status clears while the bus-off | ||
| // status is still set. Gating on both ensures the bus-off state is | ||
| // signalled only once per entry, instead of on every error warning | ||
| // interrupt while the state persists. | ||
| if int_raw.bits() & 0b100 > 0 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a specific
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok - then I probably should align SVD/PACs first instead of unleash even more cfg hell here
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea |
||
| let status = register_block.status().read(); | ||
|
|
||
| #[cfg(any(esp32, esp32c3, esp32s2, esp32s3))] | ||
| let error_warning = status.err_st().bit_is_set(); | ||
| #[cfg(any(esp32c6, esp32h2))] | ||
| let error_warning = status.err().bit_is_set(); | ||
|
|
||
| if status.bus_off_st().bit_is_set() && error_warning { | ||
| // Any pending transmission is halted by entering the bus-off | ||
| // state; abort it to release the transmit buffer. | ||
| register_block.cmd().write(|w| w.abort_tx().set_bit()); | ||
| let _ = async_state.rx_queue.try_send(Err(EspTwaiError::BusOff)); | ||
| async_state.tx_waker.wake(); | ||
| async_state.err_waker.wake(); | ||
| } | ||
| } | ||
|
|
||
| if int_raw.rx_int_st().bit_is_set() { | ||
| let status_reg = register_block.status(); | ||
| let status = status_reg.read(); | ||
|
|
||
| let rx_queue = &async_state.rx_queue; | ||
|
|
||
| if status.bus_off_st().bit_is_set() { | ||
| let _ = rx_queue.try_send(Err(EspTwaiError::BusOff)); | ||
| // Abort transmissions and wake senders if we are in bus-off state. | ||
| if status.tx_buf_st().bit_is_clear() { | ||
| register_block.cmd().write(|w| w.abort_tx().set_bit()); | ||
| async_state.tx_waker.wake(); | ||
| } | ||
| } | ||
|
|
||
| // Consumme all pending frames in the Rx FIFO | ||
| while register_block | ||
| .rx_message_cnt() | ||
|
|
@@ -1846,6 +1940,11 @@ mod asynch { | |
| // future. | ||
| let _ = register_block.err_code_cap().read(); | ||
| async_state.err_waker.wake(); | ||
| // A frame that cannot be transmitted never raises the transmit | ||
| // interrupt, so wake transmitters on errors to let them | ||
| // re-evaluate their pending frame and give up once the peripheral | ||
| // reaches the error-passive or bus-off state. | ||
| async_state.tx_waker.wake(); | ||
| } | ||
|
|
||
| // Clear interrupt request bits | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| [target.'cfg(target_arch = "riscv32")'] | ||
| runner = "espflash flash --monitor" | ||
| rustflags = [ | ||
| "-C", "link-arg=-Tlinkall.x", | ||
| "-C", "force-frame-pointers", | ||
| ] | ||
|
|
||
| [target.'cfg(target_arch = "xtensa")'] | ||
| runner = "espflash flash --monitor" | ||
| rustflags = [ | ||
| # GNU LD | ||
| "-C", "link-arg=-Wl,-Tlinkall.x", | ||
| "-C", "link-arg=-nostartfiles", | ||
|
|
||
| # LLD | ||
| # "-C", "link-arg=-Tlinkall.x", | ||
| # "-C", "linker=rust-lld", | ||
| ] | ||
|
|
||
| [env] | ||
| ESP_LOG = "info" | ||
|
|
||
| [unstable] | ||
| build-std = ["core", "alloc"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| [package] | ||
| name = "embassy-twai" | ||
| version = "0.0.0" | ||
| edition = "2024" | ||
| publish = false | ||
|
|
||
| [dependencies] | ||
| embassy-executor = "0.10.0" | ||
| embassy-time = "0.5.0" | ||
| esp-backtrace = { path = "../../../esp-backtrace", features = [ | ||
| "panic-handler", | ||
| "println", | ||
| ] } | ||
| esp-bootloader-esp-idf = { path = "../../../esp-bootloader-esp-idf" } | ||
| esp-hal = { path = "../../../esp-hal", features = ["log-04", "unstable"] } | ||
| esp-rtos = { path = "../../../esp-rtos", features = ["embassy", "log-04"] } | ||
| esp-println = { path = "../../../esp-println", features = ["log-04"] } | ||
|
|
||
| [features] | ||
| esp32 = [ | ||
| "esp-backtrace/esp32", | ||
| "esp-bootloader-esp-idf/esp32", | ||
| "esp-rtos/esp32", | ||
| "esp-hal/esp32", | ||
| ] | ||
| esp32c3 = [ | ||
| "esp-backtrace/esp32c3", | ||
| "esp-bootloader-esp-idf/esp32c3", | ||
| "esp-rtos/esp32c3", | ||
| "esp-hal/esp32c3", | ||
| ] | ||
| esp32c6 = [ | ||
| "esp-backtrace/esp32c6", | ||
| "esp-bootloader-esp-idf/esp32c6", | ||
| "esp-rtos/esp32c6", | ||
| "esp-hal/esp32c6", | ||
| ] | ||
| esp32h2 = [ | ||
| "esp-backtrace/esp32h2", | ||
| "esp-bootloader-esp-idf/esp32h2", | ||
| "esp-rtos/esp32h2", | ||
| "esp-hal/esp32h2", | ||
| ] | ||
| esp32s2 = [ | ||
| "esp-backtrace/esp32s2", | ||
| "esp-bootloader-esp-idf/esp32s2", | ||
| "esp-rtos/esp32s2", | ||
| "esp-hal/esp32s2", | ||
| ] | ||
| esp32s3 = [ | ||
| "esp-backtrace/esp32s3", | ||
| "esp-bootloader-esp-idf/esp32s3", | ||
| "esp-rtos/esp32s3", | ||
| "esp-hal/esp32s3", | ||
| ] | ||
|
|
||
| [profile.release] | ||
| debug = true | ||
| debug-assertions = true | ||
| lto = "fat" | ||
| codegen-units = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - we should align things