Initial TWAI Driver Implementation

[alexbohm]

For some visibility, I wanted to put up this pull request with an initial implementation of a driver for the TWAI peripheral. Here's some of my thoughts on what I have currently:

For the acceptance filters, I'm still thinking through ways of designing this code as there are a couple nuances that aren't initially apparent right now. The acceptance filters have two modes, single filter mode and dual filter mode. Within each of these modes the filter will match both standard ids and extended ids if they have matching bit layouts. This appears to be a limitation of the hardware as the hardware does not seem able to differentiate against id formats in the acceptance filter.

TLDR: I want to avoid situations where a filter is created to only accept a standard id of 0x000, and suddenly later in code extended id packets are received. Perhaps clever naming and documentation such as PartialAcceptanceFilter would be better.

For some background on the TWAI peripheral, most of the setup and communication is done with the TWAI_DATA_x_REG registers. In reset mode, they are used to set the acceptance filter. In operation mode, writing to them prepares data for transmission while reading from them reads received packet data. Because of this shared register layout, the driver does a lot of bitwise operations to encode or decode the packets as the bit layouts are significantly different between modes and frame formats.

This will require a patched pac with read access on the TWAI_DATA_x_REG registers. See: espressif/svd#24 This is patched in main for the esp32c3 pac but not pushed to crates.io yet.

As I only have a ESP32C3 at the moment, I've only tested this driver on that platform.

[bjoernQ]

Awesome progress! Thanks for working on this

I personally don't have experience with TAWI/CAN so I hope that someone else can participate. (@jessebraham @MabezDev ?)

Maybe one nit-pick alrready would be that identifiers like ESPTWAIFrame would better be changes to UpperCamelCase. Also for the final PR the example would need some more documentation but again, this is WIP and most porbably it's all already on you todo-list 😄

[jessebraham]

Sorry for the lack of activity here. Unfortunately I have never worked with CAN/TWAI so I'm not really sure how to approach this 😅

I will at least get an updated PAC published for the C3 this week. Beyond that, I either need to spend some time reading or try to hunt down a colleague who is familiar with this peripheral.

I will update again as needed, sorry again for the delays!

[jessebraham]

I've published esp32c3 version 0.5.1, which includes the TWAI patches. I've also found a colleague who has TWAI experience, so he will review this at some point.

[alexbohm]

Thank you! I appreciate the help. I'm still working through the acceptance filter designs to make it a bit easier to understand without reading the reference manual. Any feedback is appreciated.

[alexbohm]

Looks like the embedded-hal team has released the split out version of embedded-can:
https://crates.io/crates/embedded-can/0.4.1. Going to look into it and figure out what it needs for impls, looks like a copy of the code, just a different crate.

rust-embedded/embedded-hal#411

[dimpolo]

What is the status of this? I skimmed through the code and besides the missing read access on the data registers this seems workable?
Anything I can do to help?

[jessebraham]

Our colleague @DNedic also took a look at this PR and said it looked fine to him. I would also be happy to help out however I can in getting this PR ready to merge.

[alexbohm]

Thanks for having @DNedic take a look at this! I had paused some work on this as I got distracted by embassy and all the interesting work going on between these two repos.

Tonight I'm going to go through and add some #[cfg()] so that this it limited to the esp32c3 and play around with adding embedded-can.

As for how people can help, I think it might be nice to get some feedback on the filter and bitselector designs. Does it complicate a bitmask too much? Are there better libraries that would be acceptable?

Adding support for other models is probably best done in another PR, but I believe expanding to the other models should be pretty straight forward, as it appears they use the same peripheral. This would involve patching the SVD files and testing on those platforms. If someone is interested helping add support for other platforms that would be great.

[dimpolo]

As for how people can help, I think it might be nice to get some feedback on the filter and bitselector designs. Does it complicate a bitmask too much? Are there better libraries that would be acceptable?

Here's my thoughts on this
Your design it very type driven. This makes it easy to know what's going on without reading much documentation.
However I have to admit my initial though was that it was overengineered and simpler APIs should be possible.

... then I read the reference manual. That has got to be the most convoluted register layout I have ever seen...

My ideas:

I tried to come up with an API myself but I'm not overly happy about it either.

My thought process was that there's two scenarios we'd like to cover.

• statically known filters
  • your API is nice here
  • alternatively you could provide filters as bytestrings b"1100xxxxxxxx"
• dynamically set filters
  • much easier to work with raw code and mask data here

This is what I came up with:

pub struct SingleStandardFilter {
    data: [u8; 8],
}

impl SingleStandardFilter {
    pub const fn from_id_bits(bits: BitSelector<false, 11>) -> Self;
    pub const fn from_id_pattern(filter: &'static [u8; 11]) -> Self;
    pub const fn from_id_raw(code: embedded_can::StandardId, mask: embedded_can::StandardId) -> Self;
    pub const fn with_rtr(self, rtr: bool) -> Self;
    pub const fn with_first_byte_bits(self, bits: BitSelector<false, 8>) -> Self;
    pub const fn with_first_byte_pattern(self, filter: &'static [u8; 8]) -> Self;
    pub const fn with_first_byte_raw(self, code: u8, mask: u8) -> Self;
    pub const fn with_second_byte_bits(self, bits: BitSelector<false, 8>) -> Self;
    pub const fn with_second_byte_pattern(self, filter: &'static [u8; 8]) -> Self;
    pub const fn with_second_byte_raw(self, code: u8, mask: u8) -> Self;
}

// Example Usage:
const FILTER: SingleStandardFilter = SingleStandardFilter::from_id_pattern(b"1100xxxxxxxx").with_rtr(false).with_first_byte_pattern(b"xxxxxx01");

In the end it's even more overengineered, so yeah.. not really a recommendation, just some ideas 😃

What other folks do:

• ESP IDF uses raw values link
• On STMs you can specify masks and codes individually link
• In MCP2515 you can specify masks and codes individually link

So not very helpful for us...

PS

It seems the <const EXACT: bool> is always false so maybe remove it?

[alexbohm]

Thanks for those ideas, I really like the bytestrings approach! Yeah, I agree, coming from a background with the STM32 C HAL implementation of bxcan, there was a lot of head scratching about the register layout for the filter on the ESP32.

To me the bytestrings are more readable than the arrays that I used in bitselector so I'm going to play around with some bytestring implementations tonight and probably drop the bitselector if it goes well.

[dimpolo]

Here's the code I wrote when testing the idea :)

const fn from_id_pattern(filter: &'static [u8; 11]) -> Self {
    let mut acceptance_code = 0u16;
    let mut acceptance_mask = 0u16;

    let mut idx = 0;
    while idx < 11 {
        let shift = 15 - idx;
        match filter[idx] {
            b'1' => {
                acceptance_code |= 1 << shift;
                acceptance_mask |= 1 << shift;
            }
            b'0' => {
                acceptance_mask |= 1 << shift;
            }
            b'x' => {}
            _ => panic!("filter bits must be eiter '1', '0' or 'x'"),
        }
        idx += 1;
    }
    let [code_h, code_l] = acceptance_code.to_be_bytes();
    let [mask_h, mask_l] = (!acceptance_mask).to_be_bytes();

    SingleStandardFilter {
        data: [code_h, code_l, 0, 0, mask_h, mask_l, u8::MAX, u8::MAX],
    }
}

[alexbohm]

With the good ideas from @dimpolo, I switched to using bytestrings in the implementation for the SingleStandardFilter.

I ended up combining the parameters into the single constructor as I want to force the user to make the decision for every available parameter. I'd rather just have the person familiar with their specific application set an explicit value instead of trying to pick appropriate default values (ex: what is an appropriate default rtr bit filter value?).

I personally like the bytestrings and if other people like them I'll expand them to the other filter options.

[dimpolo]

Do you have ideas how to cater to this scenario?

• dynamically set filters
  • much easier to work with raw code and mask data here

[alexbohm]

I think this is in a good state at the moment and I'm ready to merge.
With testing against a Raspberry Pi and a MCP2515, the driver appears to work correctly.

Do you have ideas how to cater to this scenario?

• dynamically set filters

  • much easier to work with raw code and mask data here

I don't have any better ideas currently than just setting with raw code and mask so I added an implementation.

[alexbohm]

Updated driver and example to use PeripheralRef for #249.

[alexbohm]

As previously mentioned, I've been testing this driver with a Raspberry Pi, MCP2515 HAT, and can-utils. I thought I'd write down a couple basic commands in case people are unfamiliar.

Setting up the Pi is pretty easy.
Adding this line to /boot/config.txt then rebooting enables the can0 network interface:

dtoverlay=mcp2515-can0,oscillator=16000000,interrupt=25

I can then bring up the network interface with sudo ip link set up can0 type can bitrate 1000000 sjw 3. I had to set the sync jump width (sjw) to 3 on the Pi side otherwise the default sync jump width would cause errors.

Viewing packets can be done with candump.

• candump can0

Sending packets can be done using either cansend for individual packets, or cangen for fuzzing.

• cansend can0 002#ffaabb00
• cangen can0 -I i -D r -g 10

[MabezDev]

Hello @alexbohm, thanks for the PR (and your patience on reviewing!)

I didn't have access to an MCP2515 HAT so I tested with two esp32c3's and modified the examples, one which just transmits and another that prints what it received. I'll explain my process, mostly for future me :D, but also to allow others to test.

With this method, it's possible to test this without a can transceiver by putting the tx pin on each chip into open-drain, and putting a pull-up on the bus wire. See the diagram below (note that you might get away with the internal pull ups at a low enough baud rate):

The code for the transmit example is the same as the supplied one in the PR, but with the following changes:

1. Remove the call to set_filter - there is nothing to receive here so no need for it.

can_config.set_filter(FILTER);

2. Rewrite the tx example loop as follows:

loop {
        let id = StandardId::new(0).unwrap();
        println!("Sending can frame with id {:?}", id);
        let frame = twai::EspTwaiFrame::new(id, &[0xAA]).unwrap();
        let _result = block!(can.transmit(&frame)).unwrap_or_else(|e| { println!("Transmit failed with {:?}", e); None });
        delay.delay_ms(1000u32);
    }

3. Finally ensure that the TX pins on both examples are in open drain output.

With the changes I observed the following output:

[MabezDev]

Overall this is looks fantastic, just a couple of little bits to take care of and this should be ready to go :)

[MabezDev]

LGTM, thank you!