Implement async support for TWAI peripheral

[ptpaterson]

The primary goal of this PR is to provide an async implementation. I had one implementation working and even submitted a PR to add an async trait for embedded-can, but the conclusion there was that an async implementation should probably allow you to have a separate owned references for Tx and Rx.

So this PR does a few things:

• splits the implementation of TWAI transmit and receive into separate structs.
  • I modeled this a lot after the existing UART implementation, including the reliance on PhantomData and static methods in the Instance trait.
  • The interrupt handler needed to perform read from and reset the FIFO, so a large chunk was refactored into static methods for the Instance traits. I tried to keep that sane based on what I saw with UART.
  • There are specific things that only a device in configuration mode or operational mode should be able to do, so I also moved some static methods into an OperationInstance trait. It's all internal, so it's not technically necessary, but I think it makes it more clear that certain things should only be called from the Twai implementation (or interrupt handler).
• adds inherent implementations for receive and transmit.
  • The embedded-hal Can trait now reuses the inherent implementations. It is not necessary to have the trait in scope to use the methods now.
• adds async transmit and receive methods
  • async state handling is inspired by what was done in embassy
• I followed precedent for implementing the Instance traits for esp32c3, esp32s3, and esp32c6
• added examples showing how to send the Tx and Rx halves to different tasks, modeled after the embassy_serial example

Testing

I am only able to test this on an ESP32-C3 device. My understanding is that the ESP32-S3 should work the same, so I added the example for S3, but I can't test it.

I went back and tested the regular twai examples and that works too. Would have caused a warning for unused import of Can trait, so I did remove those.

I didn't add the async example to C6 because there is not an existing twai example, and can't test it.

Must

• The code compiles without errors or warnings.
• All examples work.
• cargo fmt was run.
• Your changes were added to the CHANGELOG.md in the proper section.
• You updated existing examples or added examples (if applicable).
• Added examples are checked in CI

Nice to have

• You add a description of your work to this PR.
• You added proper docs for your newly added features and code.

[jessebraham]

Thanks for the PR, apologies for not having gotten to this yet. I unfortunately have quite a few other things to take care of right now, but either myself or another maintainer should hopefully be able to get to this soon :)

[jessebraham]

@bjoernQ @MabezDev @JurajSadel I don't think I'm going to have time to get to this before the end of the year realistically, given my upcoming travel and vacation time. Any chance one of you would be able to review/test this at some point please?

[bjoernQ]

I tried the example on ESP32-S3. The old example still works as it did before

In general, I am not sure if I understand what the example is supposed to do and how to use it - needs a bit more explanation

But I wasn't able to get it to do anything

I modified things a bit and ended with one async sender and one async receiver - then things started to apparently work.

The receiver stops receiving after approximately 17 frames and doesn't receive anything after that

[ptpaterson]

I tried the example on ESP32-S3. The old example still works as it did before

In general, I am not sure if I understand what the example is supposed to do and how to use it - needs a bit more explanation

But I wasn't able to get it to do anything

I modified things a bit and ended with one async sender and one async receiver - then things started to apparently work.

Can you elaborate on what changes you made? The example re-transmits any received messages. So it should accomplish the same task as the twai example.

In the receiver task, every incoming frame is added to the channel.

In the transmitter task, every frame collected in the channel is transmitted.

This is a rather contrived used of multiple tasks, but then so is a loop where block and receive then block to transmit that received frame.

The receiver stops receiving after approximately 17 frames and doesn't receive anything after that

That sounds like a buffer got full. The internal read channel is size 32, so maybe you are not flushing the channel that the receiver is putting messages into? As the example is written, the transmitter task should pull the messages out of the channel and make room for more messages. But maybe your setup is drastically different than mine? I was only testing the example with one other node simply pinging once every couple of seconds, so I'm not worried about race conditions.

[ptpaterson]

The receiver awaits adding the message to the channel. channel.send(frame).await; It probably makes sense to make this non-blocking. Otherwise, when the channel gets full, additional frames will start filling up the internal read buffer.

[bjoernQ]

Can you elaborate on what changes you made? The example re-transmits any received messages. So it should accomplish the same task as the twai example.

I tried the example with the non-async example "on the other end" (on S3 with IS_FIRST_SENDE = true) and nothing happened. I just changed it to have one sender and one receiver (without the channel) running on one device each ... in the end I figured out what the example should do but wasn't able to get it working as is (but didn't try too hard t.b.h.)

[ptpaterson]

hi @bjoernQ. I had some time to work on this.

I tried the example with the non-async example "on the other end" (on S3 with IS_FIRST_SENDE = true) and nothing happened. I just changed it to have one sender and one receiver (without the channel) running on one device each ... in the end I figured out what the example should do but wasn't able to get it working as is (but didn't try too hard t.b.h.)

I wasn't having trouble myself with the examples before, but I also wasn't using the examples on both nodes. The C3 and S3 examples are different, including the fact that the C3 twai example never initiates sending any frames. I started by copying the S3 initial frame stuff to the C3 example, but I noticed I was encountering buss-off issues.

I've harmonized the twai examples for C3 and S3. Instead of a one-shot message, you can set IS_SENDER to true and it will keep pinging. I also added some error handling and a counter which helps show that things are changing. Now, if you have two "sender" nodes communicating you can see the difference between the frames received and the frames transmitted. You can also detach and reattach the "other" node and it should start transmitting and receiving just fine.

I tweaked the embassy_twai examples a bit, but they are essentially the same: receive a frame -> push frame to a channel -> transmit the frames in the channel. This should work out-of-the-box running the twai example on one node (with IS_SENDER = true) and one node running the embassy_twai example. It does on my C3 anyway.

[ptpaterson]

I haven't dug into the mut in the phantom types yet, but will start on that.

[ptpaterson]

I removed the mut from the Phantom data and things are still compiling fine.

[ptpaterson]

I see other discussion now for decoupling embassy (#1035) and automatically binding interrupt handlers (#1063).

I can say that I was also cautious about depending directly on embassy stuff and taking control of the interrupt handlers, but I was following precedent in for other async peripherals.

If there are significant changes that come of those RFCs, then it makes less sense to try and merge this now, but instead build it on top of whatever new async/embassy package is built. So... keeping an eye on that.

[bjoernQ]

Thanks for your continued effort!

This looks quite good to me. Also, the examples worked fine for me without modification (I had issues using one of the S3 as async-receive while it worked with sync but might be just me ... btw are you able to test with both S3 and C3?)

I don't think those RFCs should cause much issues for this - don't think we should delay this because of the RFCs

However, I would really love to hear what @jessebraham thinks. From my side this seems fine

[MabezDev]

I see other discussion now for decoupling embassy (#1035) and automatically binding interrupt handlers (#1063).

Just my 2c, until @jessebraham wakes up :D, I think the interrupt changes are mostly likely a few months away at best, we don't have a full design for it yet. So in my opinion, we should merge this now. We'll take care of refactoring the interrupt handling when we get around to the switch.

[jessebraham]

Yeah I think @MabezDev nailed it, no changes will be happening any time soon that I can see. So no point in blocking this in the meantime.

[ptpaterson]

Thanks @bjoernQ @MabezDev @jessebraham for the feedback!

I do not have any other esp boards in my possession currently. I picked up some QTPy ESP32-C3 boards for development from Microcenter because I live 5 minutes away (pretty awesome haha :D). The C3 is great for my primary project. The on-board CAN controller is :chef-kiss:, but the S3 is overkill for me.

I'd still love to get this over the finish line, but better to feel good about it being well tested. If I ordered something, is there a recommendation or canonical dev board I could look to order for testing the S3 chip?

[bjoernQ]

If you want to get a S3 dev-kit, probably an official https://docs.espressif.com/projects/esp-idf/en/latest/esp32s3/hw-reference/esp32s3/user-guide-devkitc-1.html would be a good choice. But probably any generic board will do

[jessebraham]

What's left to do here in order to merge? I'm not really sure what's holding it up right now (sorry if it's been stated in a comment), is it just testing? Would be nice to enumerate what needs to be done in a task list so we can keep track.

For context, we need to make some pretty invasive changes to the twai module (see #1123) so I would like to get this merged first to avoid causing further pain.

[ptpaterson]

What's left to do here in order to merge? I'm not really sure what's holding it up right now (sorry if it's been stated in a comment), is it just testing? Would be nice to enumerate what needs to be done in a task list so we can keep track.

I'm trying to keep things up to date. I just rebased on top of v0.15. Still working for me.

I still don't have an S3 board to test with. I mostly want to get one to test further development. If we can get this merged as is that would be great. But I defer to y'all @bjoernQ @jessebraham @MabezDev for what level of confidence is needed to merge, or if there are any other issues that need addressed. I acknowledge this is a pretty large diff 😅

BTW, once again I appreciate all the feedback and support on this!

[bjoernQ]

IIRC there was some weird issue with S3 (while there is nothing obvious wrong with the code). If we want to merge this ASAP we could cfg-away S3 support until this is solved

Maybe it's also just a "me" issue so would be great if anyone else could test on S3

[MabezDev]

I dug out an old comment where I managed to test the original TWAI PR without transceivers: #192 (comment) - I won't be able to test the s3 until Friday at the earliest, but maybe this info might enable someone else to try before then :).

[MabezDev]

@ptpaterson I'm so sorry for the delay, but I finally got around to testing this PR with an esp32s3. AFAIK the implementation works well, but the example doesn't actually do anything unless there is other traffic on the twai bus already, which I think is unexpected compared to the other examples.

I amended the example to contain a "seed" transmission for the case where the bus is quiet on one of the esp32s3's.

let channel = &*make_static!(Channel::new());

channel.send(Frame::new(embedded_can::StandardId::ZERO, &[0xAA]).unwrap()).await;

spawner.spawn(receiver(rx, channel)).ok();
spawner.spawn(transmitter(tx, channel)).ok();

This works, but I would expect the two devices to keep re-pinging the frame back and forth (at least if I'm understanding the example correctly) but it doesn't.

The terminal on the right is the seeder s3, the one on the right just listens and resends. Unfortunately, the seeder never receives the frame which should have been sent back.

As I understand it you have been testing by hooking into an existing CAN bus, correct? Do you have two esp32c3's in which you can test this setup as well?

EDIT: I'm not using proper transceivers so its possible there is a connection issue on my end, so it would be really nice if you, or someone else with transceivers could test.

[MabezDev]

Can we add the open drain work around here with a comment to remove when using transceivers, like we do in the TWAI example?

[ptpaterson]

I haven't gotten the open drain bit to work, but now with the S3 I'll try again. Makes sense to include. We'll need the seed transmission like you said to make it work.

[MabezDev]

@ptpaterson Sorry for taking so long to properly review. I appreciate this has been open for a while, but do you plan to finish this up?

[jessebraham]

I don't think this will make it into the 0.16.0 release, but I'd love to see this wrapped up for the subsequent release! I think this is a very nice contribution, so please let us know how we can help move things forward here :)

[ptpaterson]

As I understand it you have been testing by hooking into an existing CAN bus, correct? Do you have two esp32c3's in which you can test this setup as well?

EDIT: I'm not using proper transceivers so its possible there is a connection issue on my end, so it would be really nice if you, or someone else with transceivers could test.

I have indeed been using two boards to test. The twai examples (C3 and S3) by default send a new message on an interval. I designed the embassy_twai example to work when connected to another board running the twai example.

esp-hal/esp32s3-hal/examples/embassy_twai.rs

Lines 10 to 11 in a2d71b5

	//! This example should work with another ESP board running the `twai` example
	//! with `IS_SENDER` set to `true`.

But I see the issue with that for folks who only have one board.

I will work on updating the embassy_twai example to behave like the twai one, only with async functions. That should make it so you can mix and match boards on the network with either example, or use the open-drain setup with a single board.

This works, but I would expect the two devices to keep re-pinging the frame back and forth (at least if I'm understanding the example correctly) but it doesn't.

There is a fundamental issue with making only one initial message and then ping-ponging between nodes: Once there is an error you have to catch it and then make sure to send another seed message. That's not anything that cannot be solved, but it will make the example more complex. That's why I updated the twai example to send a new message on an interval when configured with IS_SENDER = true. All it has to do is reconnect and it will keep doing its job; nodes with IS_SENDER = false just read and print the messages.

Instead of trying to set up a ping-pong example, it think it will be more concise to replicate the IS_SENDER logic in the embassy_twai example. It will be pretty nice with an async task!

IIRC there was some weird issue with S3 (while there is nothing obvious wrong with the code). If we want to merge this ASAP we could cfg-away S3 support until this is solved

I also got my S3 devkit board! So I can test the examples properly. If that happens to be an utter failure then I can drop the async config for S3.

@MabezDev @jessebraham Thanks again for keeping up on this! I'm going to try to get that work done this weekend, along with rebasing onto the latest changes.

[ptpaterson]

I have the twai example running on an S3 (left console) connected with transceivers to a C3 (right console) also running the twai example.

Screen.Recording.2024-03-09.at.3.02.26.PM.mov

But the embassy_twai example is not working on s3.

Screen.Recording.2024-03-09.at.3.04.19.PM.mov

Using two C3 boards, one running the embassy_twai works fine.

So now I suspect that something about the async implementation is not working. I cannot tell why at the moment. It's compiling an loading onto the board.

I went through the tech ref. It looks like the TWAI interrupts are indeed supposed to work the same. I don't know if others can spot what is different between the two.

• S3 - https://www.espressif.com/sites/default/files/documentation/esp32-s3_technical_reference_manual_en.pdf#subsubsection.31.5.3
• C3 - https://www.espressif.com/sites/default/files/documentation/esp32-c3_technical_reference_manual_en.pdf#subsubsection.31.4.3

I'm not sure what is going on at this time, so I am inclined to remove the async implementation for S3 until we can work that out.

BTW you might be ale to tell that the I'm still running examples pre-consolodation. I knew that the C3 boards were working in the older branch so I wanted to start there.

I'll drop S3 from the async implementation and get the latest updates merged.

I'm bummed this didn't work, since the two boards seems the same in all other respects for TWAI. Sorry for the trouble, and again I appreciate the help.

[bjoernQ]

I think I know what is going on:

There is no initial enabling of interrupts in the code.

On C3 the initial value is
00000000000000000000000011110011
TWAI_RX_INT_ENA
TWAI_TX_INT_ENA
TWAI_ERR_PASSIVE_INT_ENA
TWAI_ARB_LOST_INT_ENA
TWAI_BUS_ERR_INT_ENA

On S3 the initial value is:
00000000000000000000000010001100
TWAI_ERR_WARN_INT_ENA
TWAI_OVERRUN_INT_ENA
TWAI_BUS_ERR_INT_ENA

So, doing something like

            let register_block = TWAI0::register_block();
            register_block.int_ena().modify(|_, w| {
                w.rx_int_ena()
                    .set_bit()
                    .tx_int_ena()
                    .set_bit()
                    .bus_err_int_ena()
                    .set_bit()
                    .arb_lost_int_ena()
                    .set_bit()
                    .err_passive_int_ena()
                    .set_bit()
            });

as the first thing in transmit_async and receive_async to make sure relevant interrupts (I just enabled some random interrupts for testing, needs checking) are enabled, makes it work on ESP32-S3 for me.

Probably it's a good idea to never rely on the reset value (doesn't match the TRMs at all for both) and only enable the needed interrupts there.

Additionally, in embassy\mod.rs there is code to enable the peripheral interrupt and we don't do it for the TWAI peripheral here. Seems it is enabled by default but again - relying on that is asking for trouble.

I guess in the future we won't do the enabling of peripheral interrupts in embassy\mod.rs but do it in the respective drivers - but not yet.

[bjoernQ]

@ptpaterson Thanks for all the effort, I think this is a really useful contribution. If you don't mind, I will take over this PR to get it over the finish line

[jessebraham]

With #1320 merged I guess we can go ahead and close this now :)

Thank you again @ptpaterson for your work on this, it's very much appreciated! And thanks @bjoernQ for getting this over the finish line!