Controller Area Network (CAN)

[timokroeger]

This implementation is based on the experienced I collected when I picked up #101.
In comparison to the previous PR this is a more high-level approach.
My objective was to implement the minimal possible interfaces to support most use cases of the CAN peripheral.

Design Decisions

Separate assign_pins()

A common pattern is to pass pins in the constructor of a peripheral API. This API takes a different two step approach where the constructor only enables the clock and takes ownership of the CAN pac peripheral.
A second call to assign_pins() is required to route the peripheral to the pins.
It is needed for two special cases:

1. There are two can peripherals CAN1 and CAN2 for some devices (stm32f105/107). The CAN1 clock must be enabled to use the CAN2 receiver. If CAN1 is only used as clock source it should not take ownership of any pins.
2. Silent / Loop-back modes: For self test purposes the peripheral can operate without pins (not implemented yet)

Possible edge case: The user properly configures the CAN pins without assigning them. The CAN peripheral works but does not own the pins.

Combined RX FIFOs

Both FIFOs are tightly coupled because they share acceptance filters: If a filter matches the incoming message it is put into just one of the FIFOs. In the common case where all messages shall be received FIFO1 remains completely unused because all messages are routed to FIFO0.
In my eyes splitting them introduces a leaky abstraction because they cannot really operate independently.

There is one interrupt for each FIFO. The downside of the combined approach is that both interrupts must be handled correctly as shown in the can-rtfm example.

Combined Transmit Mailboxes

Required to prevent the inherent priority inversion problem. The problem is described well in the UAVCAN Specification v1.0-alpha Section 4.2.4.3.
This implementation is based on UAVCAN code. In addition lower priority frames can be swapped out to higher priority frames.

Unsafe bit-timing configuration

http://www.bittiming.can-wiki.info/ exists, works well and is convenient. It may be possible to do the same calculations in a const-fn. Not sure if it is worth the effort.

Testing

I tested the examples on a “18 in 1 Universal CAN Filter” Aliexpress board.
That’s the cheapest was to get a MCU with two CAN interfaces. For more details see this blog post.

TODO

• Filter support
• Support loop-back and silent modes
• Improve error handling: Overrun and error counters
• Improve wakeup and bus-off management

[adamgreig]

Possible edge case: The user properly configures the CAN pins without assigning them. The CAN peripheral works but does not own the pins.

I see this as a feature, a la stm32-rs/stm32h7xx-hal#65, although I expect runtime remapping of CAN pins to be very unusual.

Combined RX FIFOs

This seems fine and I agree with your justification, plus it's nice that correct usage is easily enforced by take_rx(filters) requiring filters be passed in and there's no other way to get a filter. I think the docstrings on take_rx and split_filters could both be extended to cover how and why this works and give a short example, though.

pub unsafe fn set_bit_timing(&mut self, btr: u32)

I don't understand why this method is marked unsafe though; the worst case is your CAN peripheral doesn't receive packets correctly, but there's no value you can write to the BTR register that causes any memory unsafety or undefined behaviour, as far as I can tell. Of course the user needs to provide a correct value for their CAN system, but they also need to provide the pins that are actually wired to a CAN transceiver, yet assign_pins isn't (and shouldn't be!) unsafe.

Otherwise this all looks good to me, I'm excited to start seeing more CAN development in embedded Rust!

[tib888]

@timokroeger
Nice work, I like this higher level Filters abstraction, but it hides some hardware features, which maybe useful for someone.
For example:

• for each filter you could tell in which receiver it should target, so for the most important messages you may reserve one to avoid overflows.
• the received messages may contain the number of the matching filter - so the sw can process the message faster
• more filters can be used by halving the banks

For this reason I would propose to keep the possibility to choose between low level filter access and your high level one.
This could be done by adding an other split_filters_low_level function (of course if one call both splitting functions, the second call should return an error).
Maybe these could be separated into sub-modules to better separate the two use cases...

Or creating a higher level interface would consume the lower level resources?

What do you think?

[timokroeger]

Thank you for the comments!

@adamgreig Removed unsafe from set_bit_timings().

@tib888

Those are good points. I like your split_filters_low_level() suggestion. That way we could continue with the high-level approach and add the low-level filters without breakage later. Possible implementation:

// can.rs

mod traits {
    trait Filters {}
}

impl traits::Filters for Filters {}
impl traits::Filters for LowLevelFilters {}

fn take_rx(&mut self, _filters: impl Filters<Instance>) -> Option<Rx<Instance>> {
    // ...
}

Additionally, on stm32f105/107 the number of filters do not need to be split evenly between CAN1 and CAN2. This is also something we can make configurable with the lower-level filters. Ideally with const-generics as zero-cost abstractions (or with typenum for now).

My plan is to finish this PR soon (without low-level filters) and to start working on embedded-hal traits for CAN. Shall include the filter traits as provision now or shall we skip it until the low-level filter code is ready?

[tib888]

@timokroeger
I think it would be better to see this implementation complete with the simultaneous low lever version before going further and proposing a device independent abstraction for embedded-hal.

It is possible that even the low/high lever filters, RX, TX mailboxes may share behaviors (traits), only the configuration process is different...

[arrowcircle]

Is it possible to make a test suite for hardware, that can be run on 2 boards to check if everything works as expected?

[kevswims]

@timokroeger

A second call to assign_pins() is required to route the peripheral to the pins.
It is needed for two special cases:

1. There are two can peripherals CAN1 and CAN2 for some devices (stm32f105/107). The CAN1 clock must be enabled to use the CAN2 receiver. If CAN1 is only used as clock source it should not take ownership of any pins.

2. Silent / Loop-back modes: For self test purposes the peripheral can operate without pins (not implemented yet)

I have been playing with the CAN peripheral on an SMT32F413 and based on my experiments there I am not sure if #2 is actually correct.

I was not able to get the loop-back mode to work without a a transceiver or at least a pull-up connected to the CAN TX pin. For some strange reason (and not matching the reference manual) it seems that the CAN peripheral will not exit config mode until it sees recessive states on the bus and being in loopback mode does not actually provide these recessive states for it to enter normal mode in my experiments.

If you have a way to work around this I would be very interested though.

That being said, I am hoping to have some time to test this work out on that f4.

[timokroeger]

@arrowcircle
I tried to cover most features with example code. For me testing with an additional CAN-to-USB adapter was the quickest solution. To test with two boards following works:
Board 1: Use the can-echo with some kind of logging for received messages (e.g. semihosting)
Board 2: Use the can-rtfm example
The can-rtfm example sends some messages after startup and then both board just keep ping-ponging messages forever.

@kevswims
Thank you for sharing your experience here. At least for my boards (stm32f105) the can-loopback example works as expected without additional workaround. Have you checked the errata sheet for stm32f413?

[kevswims]

Thank you for sharing your experience here. At least for my boards (stm32f105) the can-loopback example works as expected without additional workaround. Have you checked the errata sheet for stm32f413?

@timokroeger
I don't see anything on the errata sheet for this issue though it does have an awesome errata for the CAN controller (see below). Maybe it's because I was also assigning pins to it and that interferes with the loopback connection. I'll have to play around with it more.

[timokroeger]

Most of the previously discussed "low-level" filter features can now be used with the split_filters_advanced() function. This PR is ready for review now.

@tib888
I have one uncertainty on one of the "low level" functions.

the received messages may contain the number of the matching filter - so the sw can process the message faster

This is how I understood the purpose of this feature:

• Each filter has an filter_idx associated with it. This number is assigned by some logic as described in the reference manual.
• The application reserves memory for received messages: message_buffer[MAX_FILTER_IDX]
• A new low level receive function returns filter_idx alongside the message.
• The application copies the received message to message_buffer[filter_idx] without looking at it’s contents (id or data).
• The application processes message_buffer[filter_idx] independently of the receive action.

As reference the approach for the current interface:

• Receive a message
• Check the message id and perform actions based on that:
  • Process or throw away immediately or
  • Put into a buffer for later processing

Comparing those I do not see a substantial win in processing speed. Am I missing something here?

[tib888]

Comparing those I do not see a substantial win in processing speed. Am I missing something here?

I my understanding: to determine what to do with an incoming message, normally you should match the incoming id with many possible cases, which cost CPU time.
However if you get the filter index, you could build an array of action functions and immediately call the appropriate one (like an interrupt vector - every filter would have its own interrupt handler).
(This way you do not need to copy the message in the you mentioned array, rather pass it to the handler function directly, which can do whatever he wants with it.)

Of course I do not have such time critical application, but in theory the processing can be speed up above described way.

What do you think?

[timokroeger]

Thanks! That way the filter index feature makes more sense. After looking around a bit I found the zephyr driver which uses filtering like in your description. Overall it looks quite a bit more complex (also in terms of memory usage).

[tib888]

I can imagine the implementation by having a callback vector (which is not too much memory):
static HANDLERS : [Option<&dyn Fn(Frame)>; NUM_FILTER_BANKS] = [ None; NUM_FILTER_BANKS];

And if one adds a filter he can also add a handler function, which should be stored on the appropriate index:
pub fn add_advanced(&mut self, filter: &Filter, handler: Option<&'static dyn Fn(Frame)>) -> Result<(), ()>

And the receiver should call the handler like this:

if let Some(handler) = HANDLERS[filter_index]  {
    handler(frame);
}

Then inserting a filter handler would be:
self.add_advanced(filter, Some(&|frame| { ... }));

[arrowcircle]

any updates on this?

[timokroeger]

Rebased onto master. This PR works well as is.
If somebody else can independently confirm the examples are working on his hardware I would say this PR is ready to land.

Independent of this PR there is active discussion on the embedded-hal traits for CAN and I have a prototype for embedded-hal in a separate branch.

Just as I’m typing this I see CI: Looks like I gotta fix something for the stm32f100 and stm32f101 targets still

[arrowcircle]

@timokroeger Do we have any examples to check on the hardware? With one board (loopback mode or something like this) and two boards (sender and receiver)?

[timokroeger]

@timokroeger Do we have any examples to check on the hardware? With one board (loopback mode or something like this) and two boards (sender and receiver)?

@arrowcircle
Yes the can-loopback example should be working even without CAN transceiver.
A test with two board is described here: #215 (comment)

[arrowcircle]

@timokroeger I tested examples with bluepill board.

I hade to make few changes because of only one can interface on f103 mcu.

Here is working code for RTIC example:

//! Interrupt driven CAN transmitter with rtic.
//!
//! CAN frames are allocated from a static memory pool and stored in a priority
//! queue (min heap) for transmisison. To start transmission the CAN TX
//! interrupt has to be triggered manually once. With each successful
//! transmission the interrupt is reentered and more data is fetched from the
//! queue.
//! Received frames are simply echoed back. In contrast to the naive `can-echo`
//! example all messages are also correctly prioritized by the transmit queue.

#![no_main]
#![no_std]

use heapless::{
    binary_heap::{BinaryHeap, Min},
    consts::*,
    pool,
    pool::{
        singleton::{Box, Pool},
        Init,
    },
};
use panic_halt as _;
use rtic::app;
use stm32f1xx_hal::{
    can::{Can, Filter, Frame, Id, Rx, Tx},
    pac::{Interrupt, CAN1},
    prelude::*,
};

pool!(
    #[allow(non_upper_case_globals)]
    CanFramePool: Frame
);

fn alloc_frame(id: Id, data: &[u8]) -> Box<CanFramePool, Init> {
    let frame_box = CanFramePool::alloc().unwrap();
    frame_box.init(Frame::new(id, data))
}

#[app(device = stm32f1xx_hal::pac, peripherals = true)]
const APP: () = {
    struct Resources {
        can_tx: Tx<CAN1>,
        can_tx_queue: BinaryHeap<Box<CanFramePool>, U8, Min>,
        tx_count: usize,
        can_rx: Rx<CAN1>,
    }

    #[init]
    fn init(cx: init::Context) -> init::LateResources {
        static mut CAN_POOL_MEMORY: [u8; 256] = [0; 256];

        unsafe { cx.core.SCB.vtor.write(0x0800_0000) };

        let mut flash = cx.device.FLASH.constrain();
        let mut rcc = cx.device.RCC.constrain();

        let _clocks = rcc.cfgr
            .use_hse(8.mhz())
            .sysclk(64.mhz())
            .hclk(64.mhz())
            .pclk1(16.mhz())
            .pclk2(64.mhz())
            .freeze(&mut flash.acr);

        #[cfg(not(feature = "connectivity"))]
        let mut can1 = Can::new(cx.device.CAN1, &mut rcc.apb1, cx.device.USB);
    
        #[cfg(feature = "connectivity")]
        let mut can1 = Can::new(cx.device.CAN1, &mut rcc.apb1);

        // Select pins for CAN1.
        let mut gpioa = cx.device.GPIOA.split(&mut rcc.apb2);

        let can_rx_pin = gpioa.pa11.into_floating_input(&mut gpioa.crh);
        let can_tx_pin = gpioa.pa12.into_alternate_push_pull(&mut gpioa.crh);
        let mut afio = cx.device.AFIO.constrain(&mut rcc.apb2);
        can1.assign_pins((can_tx_pin, can_rx_pin), &mut afio.mapr);

        can1.configure(|config| {
            // APB1 (PCLK1): 16MHz, Bit rate: 1000kBit/s, Sample Point 87.5%
            // Value was calculated with http://www.bittiming.can-wiki.info/
            config.set_bit_timing(0x001c_0000);
        });

        // Filters are required to use the receiver part of CAN2.
        // Because the filter banks are part of CAN1 we first need to enable CAN1
        // and split the filters between the peripherals to use them for CAN2.
        // let mut can1 = Can::new(cx.device.CAN1, &mut rcc.apb1);
        let mut filters = can1.split_filters().unwrap();

        // To share load between FIFOs use one filter for standard messages and another
        // for extended messages. Accept all IDs by setting the mask to 0. Explicitly
        // allow to receive remote frames.
        filters
            .add(&Filter::new_standard(0).with_mask(0).allow_remote())
            .unwrap();
        filters
            .add(&Filter::new_extended(0).with_mask(0).allow_remote())
            .unwrap();

        let mut can_rx = can1.take_rx(filters).unwrap();
        // let mut can_rx = can1.take_rx(filters).unwrap();
        can_rx.enable_interrupts();

        let mut can_tx = can1.take_tx().unwrap();
        can_tx.enable_interrupt();

        let can_tx_queue = BinaryHeap::new();
        CanFramePool::grow(CAN_POOL_MEMORY);

        // Sync to the bus and start normal operation.
        can1.enable().ok();

        init::LateResources {
            can_tx,
            can_tx_queue,
            tx_count: 0,
            can_rx,
        }
    }

    #[idle(resources = [can_tx_queue, tx_count])]
    fn idle(mut cx: idle::Context) -> ! {
        let mut tx_queue = cx.resources.can_tx_queue;

        // Enqueue some messages. Higher ID means lower priority.
        tx_queue.lock(|tx_queue| {
            tx_queue
                .push(alloc_frame(Id::new_standard(9), &[0, 1, 2, 4]))
                .unwrap();
            tx_queue
                .push(alloc_frame(Id::new_standard(9), &[0, 1, 2, 4]))
                .unwrap();
            tx_queue
                .push(alloc_frame(Id::new_standard(8), &[0, 1, 2, 4]))
                .unwrap();

            // Extended frames have lower priority than standard frames.
            tx_queue
                .push(alloc_frame(Id::new_extended(8), &[0, 1, 2, 4]))
                .unwrap();
            tx_queue
                .push(alloc_frame(Id::new_extended(7), &[0, 1, 2, 4]))
                .unwrap();

            tx_queue
                .push(alloc_frame(Id::new_standard(7), &[0, 1, 2, 4]))
                .unwrap();
        });

        // Manually trigger the tx interrupt to start the transmission.
        rtic::pend(Interrupt::USB_HP_CAN_TX);

        // Add some higher priority messages when 3 messages have been sent.
        loop {
            let tx_count = cx.resources.tx_count.lock(|tx_count| *tx_count);

            if tx_count >= 3 {
                tx_queue.lock(|tx_queue| {
                    tx_queue
                        .push(alloc_frame(Id::new_standard(3), &[0, 1, 2, 4]))
                        .unwrap();
                    tx_queue
                        .push(alloc_frame(Id::new_standard(2), &[0, 1, 2, 4]))
                        .unwrap();
                    tx_queue
                        .push(alloc_frame(Id::new_standard(1), &[0, 1, 2, 4]))
                        .unwrap();
                });
                break;
            }
        }

        // Expected bus traffic:
        //
        // 1. ID:      007 DATA: 00 01 02 04 <- proper reordering happens
        // 2. ID:      008 DATA: 00 01 02 04
        // 3. ID:      009 DATA: 00 01 02 04
        // 4. ID:      001 DATA: 00 01 02 04 <- higher priority messages incoming
        // 5. ID:      002 DATA: 00 01 02 04
        // 6. ID:      003 DATA: 00 01 02 04
        // 7. ID:      009 DATA: 00 01 02 04
        // 8. ID: 00000007 DATA: 00 01 02 04 <- extended frames have the lowest priority
        // 9. ID: 00000008 DATA: 00 01 02 04    and reach the bus last
        //
        // The output can look different if there are other nodes on bus the sending messages.

        loop {
            cortex_m::asm::wfi();
        }
    }

    // This ISR is triggered by each finished frame transmission.
    #[task(binds = USB_HP_CAN_TX, resources = [can_tx, can_tx_queue, tx_count])]
    fn can_tx(cx: can_tx::Context) {
        let tx = cx.resources.can_tx;
        let tx_queue = cx.resources.can_tx_queue;

        tx.clear_interrupt_flags();

        // There is now a free mailbox. Send the next frame if there is still someting
        // in the queue.
        while let Some(frame) = tx_queue.peek() {
            match tx.transmit(&frame) {
                Ok(None) => {
                    tx_queue.pop();
                    *cx.resources.tx_count += 1;
                }
                Ok(pending_frame) => {
                    // A lower priority frame was replaced with our high priority frame.
                    // Put the low priority frame back in the transmit queue.
                    tx_queue.pop();
                    if let Some(frame) = pending_frame {
                        tx_queue
                            .push(CanFramePool::alloc().unwrap().init(frame))
                            .unwrap();
                    }
                }
                Err(nb::Error::WouldBlock) => break,
                _ => unreachable!(),
            }
        }
    }

    #[task(binds = CAN_RX1, resources = [can_rx, can_tx_queue])]
    fn can_rx1(cx: can_rx1::Context) {
        // Echo back received packages with correct priority ordering.
        loop {
            match cx.resources.can_rx.receive() {
                Ok(frame) => {
                    cx.resources
                        .can_tx_queue
                        .push(CanFramePool::alloc().unwrap().init(frame))
                        .ok();
                }
                Err(nb::Error::WouldBlock) => break,
                Err(nb::Error::Other(_)) => {} // Ignore overrun errors.
            }
        }

        // Start transmission of the newly queue frames.
        rtic::pend(Interrupt::USB_HP_CAN_TX);
    }
};

[timokroeger]

Thank you very much for testing @arrowcircle
I adapted the example to your code so it also runs on stm32-f103xx devices.
The only reason I used CAN2 was because that was the only peripheral I had a transceiver available for.

[TheZoq2]

I have not been keeping track of the discussion in here, sorry about that. Is there anything left that you want to do before getting this merged

[timokroeger]

All done, ready to merge!
Really glad to see this rather big PR being merged soon.

[TheZoq2]

Awesome, yea it's indeed a big PR, thanks for putting in all the effort! I'll try to have a closer look at it before the weekend. In the meantime, it looks like there are a few new warnings found by the CI build

[timokroeger]

Rebased to trigger CI

[arrowcircle]

Just found strange thing. After changing pins for CAN to PB8(RX) and PB9(TX) example code stopped working.
Is it possible something broken with latest changes?

These are only changes in the code:

// Select pins for CAN1.
let mut gpiob = cx.device.GPIOB.split(&mut rcc.apb2);
let can_rx_pin = gpiob.pb8.into_floating_input(&mut gpiob.crh);
let can_tx_pin = gpiob.pb9.into_alternate_push_pull(&mut gpiob.crh);

[timokroeger]

Great catch! What a big difference a single character can make inside a unsafe block, should be fixed.

[timokroeger]

Added an Id type and made the constructors fallible. All review comments should be addressed now.

[arrowcircle]

Any updates on this?

[TheZoq2]

I completely forgot to have a look at this. Sorry for the delay, hopefully I'll get to it this week

[jonas-schievink]

I don't think this computation is correct. Standard frames only have priority over extended frames when the most significant 11 bits of the IDs match.

[timokroeger]

Thank you that is a great catch!

[jonas-schievink]

The PartialOrd and PartialEq implementations of a type must agree with each other. With this implementation that is not the case, since two data frames with the same ID but different data would compare equal via PartialOrd, but not via PartialEq.

[timokroeger]

Replaced by #293