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

[ocornu]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• I have added changelog entries and/or migration guide notes in the sections below, or I will ask a maintainer to add the skip-changelog or manual-changelog label as appropriate.
• My changes are in accordance to the esp-rs developer guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Fix Request Frame bug introduced recently [68b8554], add HIL test to detect it.

Assertions wrongly failed for Request Frame (data_len == 0) with a dlc > 0. This was not caught by HIL tests because there was no way to create a Request Frame for self_reception.

Fixing the bug and adding a new HIL test involved deprecating 3 new*() frame methods and replacing them with 2 "self_reception-abled" ones. Additionally these 2 methods:

• return a Result instead of an Option (TWAI: EspTwaiFrame::new_*() methods should return a Result instead of an Option #5952),
• provide official support for DLC>8 (TWAI: Add support for frame DLC>8 #6048),
• abandon the remote name in favor of request (CAN Frame: is_remote_frame() and new_remote() rust-embedded/embedded-hal#740).

This change should be temporary as a (breaking) type-safe redesign, with DataFrame and RequestFrame types, would also allow returning to the new()/new_self_reception() naming scheme, should we choose to do so.

Testing

Live CAN bus Rx, HIL tests.

---

Changelog

esp-hal

• Added: Two new methods for constructing data and request frames.
• Changed: Three old methods for constructing frames have been deprecated.

[bugadani]

why is this unsafe?

[ocornu]

Because input parameters are not checked/validated.

[bugadani]

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

[ocornu]

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]

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

[ocornu]

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

[bugadani]

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]

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…

[bugadani]

No deprecation attributes, please

[ocornu]

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

[bugadani]

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

[ocornu]

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

[bugadani]

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

[ocornu]

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.

[bugadani]

This bothers me too much. This is supposed to be a bugfix PR, not a design-a-frame-api PR.

[bugadani]

Here's what I think we should try. This makes it pretty unmistakeable what argument means what, and that a >8 DLC is not a CAN2.0A/B compliant feature of the hardware.

transmit_to_self isn't a bad name, but it kind of implies that the frame doesn't leave the MCU. Self-reception isn't a property of the frame, it's a hardware feature and we are kind of misappropriating a frame information bit for it for no good reason.

// Either creating a u4 returns the conversion error, or we use u8 and EspTwaiFrame will need an IncorrectRtrLength and IncorrectDlc errors besides BufferTooBig and ConflictingDataLengthCode
struct u4(u8);

enum FrameKind<'a> {
    Data(&'a [u8]),
    Request(u4), // length
}

// Either this, or a `new_noncompliant(id, data, dlc: u4)` method
enum DataLengthCode {
    DataLength,
    NonCompliant(u4), // custom DLC
}

impl EspTwaiFrame {
    pub fn new(
        id: impl Into<embedded_can::Id>,
        data: FrameKind<'_>,
        dlc: DataLengthCode,
    ) -> Result<Self, FrameError> {...}
}

impl Twai {
    pub fn transmit(&mut self, frame: EspTwaiFrame) { ... }
    pub fn transmit_self_reception(&mut self, frame: EspTwaiFrame) { ... }
}

[ocornu]

Thanks for pitching in with constructive proposals. Much appreciated. 😊

This bothers me too much. This is supposed to be a bugfix PR, not a design-a-frame-api PR.

Don't mean to be snarky, but what am i supposed to make of this then? Do you want to tackle it all here, or do you prefer decoupling the bug-fix from the design changes?

Back to the design, here's what i have in mind:

• I want to be able to process frames in a type-safe manner, which mean i need a DataFrame type and a RequestFrame type.
• I also want to be able to (type-safe) process frames generically, which mean i need a Frame enum with Data(DataFrame) and Request(RaquestFrame) variants.
• I need those types to be as small as possible, and adding minimal runtime cost (ideally: zero). Which means DataFrame and RequestFrame are mostly wrappers around a raw TwaiFrame.
• I want to be able to process all these types generically, in other words to transparently invoke TwaiFrame methods on the 3 others, which means they all must Deref to the wrapped TwaiFrame.
• I want the transmit() methods to accept all of them transparently, which means its input parameter would be a raw TwaiFrame (that they all deref to).
• I want the receive() methods to return a type-safe type i can route accordingly, which means: Frame.

Altogether, I've got the following (hiding trivial Deref implementations):

pub enum Frame {
    Data(DataFrame),
    Request(RequestFrame),
}

pub struct DataFrame {
    raw: TwaiFrame,
}

pub struct RequestFrame {
    raw: TwaiFrame,
}

impl<'d, Dm: DriverMode> Twai<'d, Dm> {
    pub fn transmit(&mut self, frame: &TwaiFrame) -> nb::Result<(), EspTwaiError> {
        self.tx.transmit(frame)
    }

    pub fn receive(&mut self) -> nb::Result<Frame, EspTwaiError> {
        self.rx.receive()
    }
}

This matters to our discussion because we can then have constructors specific to DataFrame and RequestFrame, and no longer need FrameKind. We can also do without DataLengthCode and the u4 struct, which seem to be a bit overkill just to create a frame:

impl DataFrame {
    pub fn new(id: impl Into<Id>, data: &[u8]) -> Result<Self, EspTwaiError> { ... }
    pub fn new_noncompliant(id: impl Into<Id>, data: &[u8], dlc: u8 or usize) -> Result<Self, EspTwaiError> { ...  }
}

And lastly we add the transmit_self_reception(&mut self, frame: TwaiFrame) method to the sync and async drivers.

[ocornu]

If we were to go that route, would you still want an explicit non-compliant method for RequestFrame:

impl RequestFrame {
    pub fn new(id: impl Into<Id>, dlc: usize) -> Result<Self, EspTwaiError> { … }
    pub fn new_noncompliant(id: impl Into<Id>, dlc: usize) -> Result<Self, EspTwaiError> { … }
}

…or do we just process it as a sub-case of new()?

[bugadani]

Where does type-safety come into play when you just deref to the raw frame type anyway? You add an extra discriminator with the enum, while that information is already encoded in the raw frame. Deref isn't something I would recommend using, it is pretty trash for the documentation, and you'll still be able to call data() on a request frame, etc. You just add complexity via indirection, and win nothing in return, especially since the method documentations for the different frame types will likely be different.

I want to be able to process frames in a type-safe manner, which mean i need a DataFrame type and a RequestFrame type.

Frankly, this can be an abstraction on your application side if you really need it, but I don't see the added value for the driver. If you get it wrong, you can end up with a Remote variant that contains a data frame by accident, and then you have typesafe nonsense.

[ocornu]

Where does type-safety come into play when you just deref to the raw frame type anyway?

Because i can write:

fn process_data_frame(data: DataFrame) { … }
fn process_request_frame(request: RequestFrame) { … }
// and then:
fn forward_processed_frame(frame: Frame) { … }

It is irrelevant to these functions what those types deref to, or even, whether they deref at all.

You add an extra discriminator with the enum, while that information is already encoded in the raw frame.

"Frame type" is already one lone bit in a raw TwaiFrame, yes. And taking it off would involve runtime and/or memory cost. How is that redundancy an issue?

Deref isn't something I would recommend using, it is pretty trash for the documentation, and you'll still be able to call data() on a request frame, etc.

Well, as i see it, implementing Deref for a pure wrapper type is not particularly exotic… And it makes a lot of sense in this case. But it's not a hill i'm ready to die on: if you don't care for it, we could have explicit as_raw() dereferencing methods instead.

You can call data() on a request frame as it is. And it returning an empty slice is not problematic per se. It never seemed to bother you so far…

• First, i don't see the value of reimplementing pretty much every wrapper method just to hide data() => &[]. But if you feel strongly about it, why not? They'll all disappear at compile time anyway…
• However, what you are losing if you do so is compatibility with the embedded_can::Frame trait for Frame, DataFrame and RequestFrame. That seems more relevant to me than hiding the data() method for request frames.

Frankly, this can be an abstraction on your application side if you really need it, but I don't see the added value for the driver.

Sure I can.

The added value is that the TWAI hardware can send and receive exactly two types of frames, and the driver already typed them for you: you can then do your client business in what i consider a type-safe way (although you may reasonably argue it's the "softer side" of type-safety).

If you get it wrong, you can end up with a Remote variant that contains a data frame by accident, and then you have typesafe nonsense.

How? As i see it, only driver code may construct DataFrames and RequestFrames, so it's not particularly difficult to avoid that…

[ocornu]

The added value is that the TWAI hardware can send and receive exactly two types of frames, and the driver already typed them for you: you can then do your client business in what i consider a type-safe way (although you may reasonably argue it's the "softer side" of type-safety).

I should have added that it allows de-multiplexing the new and new_noncompliant constructors for each type of frame in an elegant manner, an issue we're supposed to solve here if we want DLC>8 support.

[ocornu]

Deref isn't something I would recommend using, it is pretty trash for the documentation, and you'll still be able to call data() on a request frame, etc.

Well, as i see it, implementing Deref for a pure wrapper type is not particularly exotic… And it makes a lot of sense in this case.

This is how embedded_can CAN trait (that we support) is defined:

/// A CAN interface that is able to transmit and receive frames.
pub trait Can {
    /// Associated frame type.
    type Frame: crate::Frame;
    ...

    fn transmit(&mut self, frame: &Self::Frame) -> nb::Result<Option<Self::Frame>, Self::Error>;
    fn receive(&mut self) -> nb::Result<Self::Frame, Self::Error>;
}

It forces transmit() and receive() to use the same associated type Self::Frame. Therefore, if receive outputs a type-safe Frame enum, it has to be dereferencable to a transmit parameter.

[ocornu]

Look, there's a lot of interacting constraints here: it's difficult to reasonably make one's mind just by imagining things.

I'll send another PR with the code, so we can have a better idea of what we're talking about and the consequences…

[github-actions]

New commits in main have made this PR unmergeable. Please resolve the conflicts.