Skip to content

Fix Request bug introduced recently [68b8554c], add HIL test. #6081

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.

Sign up for GitHub

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

Open
ocornu wants to merge 3 commits into
base: main
Choose a base branch
from
+101 −47
Open

Fix Request bug introduced recently [68b8554c], add HIL test. #6081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93f2acd
Fix Request bug introduced recently [68b8554c], add HIL test to detec…
ocornu Aug 9, 2026
f6631b0
Fix formatting in HIL twai tests.
ocornu Aug 9, 2026
2bb3093
Update esp-hal/src/twai/mod.rs
ocornu Aug 10, 2026
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
121 changes: 82 additions & 39 deletions esp-hal/src/twai/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,22 +98,22 @@
//! can_rx_pin,
//! can_tx_pin,
//! TWAI_BAUDRATE,
//! TwaiMode::SelfTest
//! TwaiMode::SelfTest,
//! );
//!
//! // Partially filter the incoming messages to reduce overhead of receiving
//! // undesired messages
//! can_config.set_filter(const { SingleStandardFilter::new(b"xxxxxxxxxx0",
//! b"x", [b"xxxxxxxx", b"xxxxxxxx"]) });
//! can_config.set_filter(
//! const { SingleStandardFilter::new(b"xxxxxxxxxx0", b"x", [b"xxxxxxxx", b"xxxxxxxx"]) },
//! );
//!
//! // Start the peripheral. This locks the configuration settings of the
//! // peripheral and puts it into operation mode, allowing packets to be sent
//! // and received.
//! let mut can = can_config.start();
//!
//! # // TODO: `new_*` should return Result not Option
//! let frame = EspTwaiFrame::new_self_reception(StandardId::ZERO,
//! &[1, 2, 3]).unwrap(); // Wait for a frame to be received.
//! let frame = EspTwaiFrame::new_data(StandardId::ZERO, &[1, 2, 3], 0, true).unwrap();
//! // Wait for a frame to be received.
//! let frame = block!(can.receive())?;
//!
//! # loop {}
Expand Down Expand Up @@ -409,11 +409,11 @@ impl From<embedded_can::Id> for Id {
#[instability::unstable]
impl embedded_can::Frame for EspTwaiFrame {
fn new(id: impl Into<embedded_can::Id>, data: &[u8]) -> Option<Self> {
Self::new(id.into(), data)
Self::new_data(id.into(), data, 0, false).ok()
}

fn new_remote(id: impl Into<embedded_can::Id>, dlc: usize) -> Option<Self> {
Self::new_remote(id.into(), dlc)
Self::new_request(id.into(), dlc, false).ok()
}

fn is_extended(&self) -> bool {
Expand Down Expand Up @@ -569,77 +569,120 @@ impl EspTwaiFrame {
Self { bytes }
}

/// Make a new [`EspTwaiFrame`] from parameters.
fn new_from_parameters(
id: impl Into<Id>,
/// Make a new [`EspTwaiFrame`] from parameters, without validation.
unsafe fn new_unchecked(

@bugadani bugadani Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why is this unsafe?

@ocornu ocornu Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because input parameters are not checked/validated.

@bugadani bugadani Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's not Rust's interpretation of memory safety, though.

@ocornu ocornu Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Dude, do you even read the code?

This line will panic if data is too large:

        bytes[data_start..data_end].copy_from_slice(data);

@bugadani bugadani Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

panicking is not a safety concern. The only requirement is that safe code must not cause undefined behaviour.

@ocornu ocornu Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, my bad.

I was mimicking what we're doing in StandardId and ExtendedId:

impl StandardId {
    /// Creates a new `StandardId` without checking if it is inside the valid
    /// range.
    ///
    /// # Safety
    /// Using this method can create an invalid ID and is thus marked as unsafe.
    #[inline]
    pub const unsafe fn new_unchecked(raw: u16) -> Self {
        StandardId(raw)
    }
}

remote_request: bool,
self_reception: bool,
dlc: usize,
id: impl Into<Id>,
data: &[u8],
) -> Result<Self, EspTwaiError> {
let data_len = data.len();

// Assert that:
// - Max data length is 8
// - remote request frames have no data payload
if data_len > 8 || (remote_request & (data_len > 0)) {
return Err(EspTwaiError::InvalidDataLength(data_len as u8));
}
// Assert that:
// - Max DLC is 15
// - Data length smaller than 8 must have equal DLC
// - Data length equal to 8 hmust ave DLC >= 8
if dlc > 15 || ((data_len < 8) & (dlc != data_len)) || ((data_len == 8) & (dlc < 8)) {
return Err(EspTwaiError::NonCompliantDlc(dlc as u8));
}

dlc: usize,
self_reception: bool,
) -> Self {
let mut bytes = [0u8; 13];

// Id
let (extended_format, data_start): (u8, usize) = match id.into() {
let (extended_format, data_start): (bool, usize) = match id.into() {
Id::Standard(id) => {
let raw = id.as_raw();
bytes[1] = (raw >> 3) as u8;
bytes[2] = (raw << 5) as u8;
(0, 3)
(false, 3)
}
Id::Extended(id) => {
let raw = id.as_raw();
bytes[1] = (raw >> 21) as u8;
bytes[2] = (raw >> 13) as u8;
bytes[3] = (raw >> 5) as u8;
bytes[4] = (raw << 3) as u8;
(1, 5)
(true, 5)
}
};
// Frame Info
let ff = extended_format << 7;
let ff = (extended_format as u8) << 7;
let rtr = (remote_request as u8) << 6;
let sr = (self_reception as u8) << 4;
let dlc = (dlc as u8) & 0b1111;
bytes[0] = ff | rtr | sr | dlc;
// Data
let data_end = data_start + data_len;
let data_end = data_start + data.len();
bytes[data_start..data_end].copy_from_slice(data);

Ok(Self { bytes })
Self { bytes }
}

/// Create a new Data Frame.
///
/// # Arguments
/// * `id` - Identifier.
/// * `data` - Data payload (up to 8 bytes).
/// * `dlc` - Custom Data Length Code (`0` defaults to `data.len()`).
/// * `self_reception` - If `true`, this frame will be received after a call to `transmit()`.
pub fn new_data(
id: impl Into<Id>,
data: &[u8],
dlc: usize,
self_reception: bool,

@bugadani bugadani Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have two issues with self_reception:

  • It's a positional boolean, which are usually rather difficult to understand
  • It's a test feature mostly. Add a separate method that sets the bit, or remove it from the frame entirely and make it an input of the driver.

@ocornu ocornu Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree: as written in the description above, this is temporary. To do this cleanly i'll need the new type-safe implementation, which is a bigger (breaking) commit.

  • It's a test feature mostly. Add a separate method that sets the bit, or remove it from the frame entirely and make it an input of the driver.

I've asked for feedback on this in the past and got none. So i went the closest to the original implementation as i could.

A separated method to set the bit is a no-go: the content of a TWAI frame is immutable on purpose.

I proposed adding a "fake_transmit()" to the driver instead…

) -> Result<Self, EspTwaiError> {
let data_len = data.len();
// Default DLC
let dlc = match dlc {
0 => data_len,
_ => dlc,
};

// Assert that max data length is 8
if data_len > 8 {
return Err(EspTwaiError::InvalidDataLength(data_len as u8));
}
// Assert that:
// - Max DLC is 15
// - Data length smaller than 8 must have equal DLC
// - Data length equal to 8 must have DLC >= 8
if dlc > 15 || ((data_len < 8) && (dlc != data_len)) || ((data_len == 8) && (dlc < 8)) {
return Err(EspTwaiError::NonCompliantDlc(dlc as u8));
}

// SAFETY: Safe because we validated the parameters above.
unsafe { Ok(Self::new_unchecked(false, id, data, dlc, self_reception)) }
}

/// Create a new Request Frame.
///
/// # Arguments
/// * `id` - Identifier.
/// * `dlc` - Data Length Code.
/// * `self_reception` - If `true`, this frame will be received after a call to `transmit()`.
pub fn new_request(
id: impl Into<Id>,
dlc: usize,
self_reception: bool,
) -> Result<Self, EspTwaiError> {
// Assert that max DLC is 15
if dlc > 15 {
return Err(EspTwaiError::NonCompliantDlc(dlc as u8));
}

// SAFETY: Safe because we validated the parameters above.
unsafe { Ok(Self::new_unchecked(true, id, &[], dlc, self_reception)) }
}

/// Create a new `EspTwaiFrame` with the specified ID and data payload.
#[deprecated(note = "Please use `new_data` instead")]
pub fn new(id: impl Into<Id>, data: &[u8]) -> Option<Self> {
Self::new_from_parameters(id.into(), false, false, data.len(), data).ok()
Self::new_data(id, data, 0, false).ok()
}

/// Create a new `EspTwaiFrame` for a transmission request with the
/// specified ID and data length (DLC).
#[deprecated(note = "Please use `new_request` instead")]
pub fn new_remote(id: impl Into<Id>, dlc: usize) -> Option<Self> {
Self::new_from_parameters(id.into(), true, false, dlc, &[]).ok()
Self::new_request(id, dlc, false).ok()
}

/// Create a new `EspTwaiFrame` ready for self-reception with the specified
/// ID and data payload.
#[deprecated(note = "Please use `new_data` instead")]

@bugadani bugadani Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No deprecation attributes, please

@ocornu ocornu Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You prefer straight-on breaking change, even when it can be prevented?

@bugadani bugadani Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, I prefer keeping these methods over the one weird constructor that rules them all. (well, except new_self_reception)

@ocornu ocornu Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Except you need another (4th) constructor for new_self_reception_request() (the case the HIL test if testing) then…

@bugadani bugadani Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, and this makes me wonder if the entire concept is wrong as it is. But it is also out of scope here.

@ocornu ocornu Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As far as i'm concerned the wrong concept here was to introduce a constructor method for self_reception data frame alone (none for request frames) in the first place. That is broken, but that's the hand you've dealt me.

In my opinion, self_reception is a Tx modality, it's not a property of a frame (even though an Rx frame may carry with it the (undocumented) fact that it is). The proper design would have been to add another send method next to transmit() (transmit_to_self()?). This way you can build a frame without worrying about that (saving up to 2 constructors), and only at sending-time do you decide to send it on the bus or to yourself.

But this would be an even more dramatic code change: the current (again: temporary) solution is still fully compatible with the current API (if you allow deprecated). That one would break it (removing new_self_reception()) and modify both the blocking and non-blocking drivers.

pub fn new_self_reception(id: impl Into<Id>, data: &[u8]) -> Option<Self> {
Self::new_from_parameters(id.into(), false, true, data.len(), data).ok()
Self::new_data(id, data, 0, true).ok()
}
}

Expand Down
27 changes: 19 additions & 8 deletions hil-test/src/bin/misc_drivers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,13 +574,25 @@ mod twai {
}

#[test]
fn test_send_receive(mut ctx: Context<Blocking>) {
let frame = EspTwaiFrame::new_self_reception(StandardId::ZERO, &[1, 2, 3]).unwrap();
fn test_send_receive_data(mut ctx: Context<Blocking>) {
let frame = EspTwaiFrame::new_data(StandardId::ZERO, &[1, 2, 3], 0, true).unwrap();
block!(ctx.twai.transmit(&frame)).unwrap();

let frame = block!(ctx.twai.receive()).unwrap();

assert_eq!(frame.data(), &[1, 2, 3])
assert_eq!(frame.data(), &[1, 2, 3]);
assert_eq!(frame.data_length_code(), 3);
}

#[test]
fn test_send_receive_request(mut ctx: Context<Blocking>) {
let frame = EspTwaiFrame::new_request(StandardId::ZERO, 4, true).unwrap();
block!(ctx.twai.transmit(&frame)).unwrap();

let frame = block!(ctx.twai.receive()).unwrap();

assert_eq!(frame.data(), &[]);
assert_eq!(frame.data_length_code(), 4);
}
}

Expand Down Expand Up @@ -644,17 +656,15 @@ mod twai {

#[test]
async fn test_async_transmit_and_receive(mut ctx: Context<Async>) {
let frame =
EspTwaiFrame::new_self_reception(StandardId::new(0).unwrap(), b"12345678").unwrap();
let frame = EspTwaiFrame::new_data(StandardId::ZERO, b"12345678", 0, true).unwrap();
transmit_frames(&mut ctx, &frame, 31).await;
receive_frames(&mut ctx, 31).await;
}

#[test]
// regression test for https://github.com/esp-rs/esp-hal/issues/4235
async fn test_buffer_overrun_on_empty_queue(mut ctx: Context<Async>) {
let frame =
EspTwaiFrame::new_self_reception(StandardId::new(0).unwrap(), b"12345678").unwrap();
let frame = EspTwaiFrame::new_data(StandardId::ZERO, b"12345678", 0, true).unwrap();

interrupt::disable(Cpu::ProCpu, TWAI0);

Expand Down Expand Up @@ -693,7 +703,8 @@ mod twai {

let mut twai = config.into_async().start();

let frame = EspTwaiFrame::new(StandardId::new(5).unwrap(), b"12345678").unwrap();
let frame =
EspTwaiFrame::new_data(StandardId::new(5).unwrap(), b"12345678", 0, false).unwrap();

twai.transmit_async(&frame).await.unwrap();
}
Expand Down
Loading