Controller Area Network (CAN) Take 3

[timokroeger]

Usage Example

stm32-fwupdate

Prototype Implementations

• pcan-basic on top of an existing software API
• stm32fxx-hal (diff) as part of a HAL crate
• candev implementing the traits for linux SocketCAN

TODOs for seperate PRs

• Postpone merge FilteredReceiver and associated traits ref (open a new PR without them)
• Investigate support for UAVCAN requirements
  • RX and TX timestamps
  • Transmission deadline
• Common interface to set bit timings ref

Previous Discussion

• [WIP] CAN Interface take 2 [WIP] CAN Interface take 2 #77
• CAN/CAN-FD API? CAN/CAN-FD API? #21
• added a HAL for CAN interfaces added a HAL for CAN interfaces #53

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thejpster (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[josko7452]

Hey! I am just dropping by I am taking up Rust and I was thinking that I would do CAN layer for embedded-hal since I have quite some encounter with CAN on embedded, but I see I come a bit late.

But now looking at it I was thinking wouldn't it make sense to have some generic timing structure akin to what is in Linux kernel:
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/can/netlink.h#L31
namely structs can_bittiming and can_bittiming_const

The idea is that struct as can_bittiming is generic description of timing settings of a CAN hardware and can_bittiming_const defines the capability of given CAN HW.
While I would not necessarily add implementation to calculate struct can_bittiming in embedded-hal I think it makes sense to have trait that allows you to set generic timing and get the timing capabilities from CAN hardware.

If this is not done then setting timing will be a a bit bypass of HAL layer because the values are specific for each CAN IP core..

I saw that timing was discussed before, but is a bit different way of doing it.. its not implementing anything directly, only defining the interface common to both CAN driver and an application siting on top of embedded-hal this way the drivers can provide concise way to show timing capabilities while application layer can use those to set the timing.

[timokroeger]

I added traits for receive filter configuration. There is quite some variation between CAN controllers:

• Not all filters have a mask
• A shared mask for multiple filters (MPC2515)
• Some controllers only support standard 11bit identifiers (STM32 bxCAN is configurable)
• Some do not include the RTR bit in the filter (Microchip MCP2515, Peak PCAN-Basic)
• Some have the RTR bit but no mask for it (Bosch C_CAN, Infineon MultiCAN)

I tried to cover all those configurations with an interface that exposes capabilities similar to the approach that @josko7452 described for timing. filter_groups() returns an iterator that describes the controllers filter capabilities.

@josko7452 Regarding the bit timings I’m not quite sure if they are in scope of embedded_hal. I’m borrowing this ASCII graphic from @pftbest:

    (1)                 (2)               (3)               (4)            (5)
+--------+         +----------+  +-------------------+  +--------+  +---------------+
|  Some  |   CAN   |   CAN    |  |       HAL         |  |  HAL   |  | Device Dirver |
| Device |<=======>| Hardware |<-|  implementation   |<-| Traits |<-|      for      |
|        |   BUS   | (in MCU) |  | (hardware driver) |  |        |  | "Some Device" |
+--------+         +----------+  +-------------------+  +--------+  +---------------+
                                           ^                                ^
                                           |                                |
                                           |                        +---------------+
                                           |                        |      User     |
                                           +------------------------|    Program    |
                                                                    |               |
                                                                    +---------------+
                                                                           (6)

Usually the system integrator configures the bit timings. I would consider bit timing configuration to be part of the init procedure in (6).
In contrast the HAL only provides traits for the device drivers (4), (5). So even if a generic device driver can query the timing capabilities how would it use that information? The only use case I can come up with: Baud-rate adjustments during runtime (similar to #79). Given that the CAN bus is usually used for reliable applications software based baud-rate adjustment looks rather fragile to me.

[josko7452]

@josko7452 Regarding the bit timings I’m not quite sure if they are in scope of embedded_hal. I’m borrowing this ASCII graphic from @pftbest:

Yes I have seen that ASCII graphics. My point is that I don't know whose scope that would be otherwise.
The problem is that as you probably know CAN timing parameters are given by CAN standards (like this one) and therefore can be calculated generically disregarding the specific implementation of a given CAN IP core.

To me if you are an init procedure in (6) you would only like to pass something generic to the driver which all drivers understand rather than having to deal at (6) which register sets what..

That's the point of the driver to abstract the register accesses or am I wrong? And this is exactly what struct can_bittiming describes. Instead of dealing with registers these are values as specified in the BOSH document so they are common to all CAN hardware.

struct can_bittiming {
	    __u32 bitrate;		/* Bit-rate in bits/second */
	    __u32 sample_point;	/* Sample point in one-tenth of a percent */
	    __u32 tq;		/* Time quanta (TQ) in nanoseconds */
	    __u32 prop_seg;		/* Propagation segment in TQs */
	    __u32 phase_seg1;	/* Phase buffer segment 1 in TQs */
	    __u32 phase_seg2;	/* Phase buffer segment 2 in TQs */
	    __u32 sjw;		/* Synchronisation jump width in TQs */
	    __u32 brp;		/* Bit-rate prescaler */
    };

The driver can take this and translate it to IP core specific registers that set what this describes.
This is usually very thin layer of translation just subtracting 1 or adding 1 or summing some of these.

I am not saying there is need to include this calculation in embedded-hal this can be done elsewhere (even application layer, or pre-calculated constants), but what is needed is a concise interface to set the timing.

Also by these examples I am not saying we should copy what is in the Linux kernel.
But on the other hand there were several takes on CAN HW abstraction there so why to reinvent the wheel.

Just for completness this is how these structs are used:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev.c#L118 (this is the generic calculation of timing heuristic)
And this is how it's used in a driver:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/ti_hecc.c#L175
and
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/ti_hecc.c#L265

As you can see it doesn't add a lot of burden to the driver. What it does is that instead of having
implementation specific method to set bit timings we would end up with a generic one.

And then when you would port your application from one CAN HW to another you wouldn't need to replace your init function in (6) for the CAN HW, because it would still apply (be it constants or a function that calculates struct can_bittiming on the fly).

If there is another place where such a generic interface can be defined for all CAN drivers then I welcome the suggestions.

[timokroeger]

Rebased to resolve the conflict in Cargo.toml.
Any comments on this?
Looking at this again with some distance I would consider dropping/postponing the second commit with filter traits to get a basic interface going.

[marcelbuesing]

I think this looks great. I'd probably though also go for only the "Non-Filter" part so only the first commit. Just for the sake of getting a standard interface quicker.

I implemented this here in a fork of the socketcan crate.

My observations were:

• I had to unwrap() when creating the CanFrame because the identifier may be too large to fit into the frame u32 > 29bit for EFF and u32 > 11bit for SFF. So I assume it's probably a good idea to make both new_extended and new_standard return a Result.
• Unable to implement with_rtr without modification, I assume this is not really critical.
• The Ok return type documentation for Transmitter::transmit was a little unclear to me. I assume the returned frame would be one with lower priority already in the buffer right?
• Maybe add a is_err_frame fn, not sure about this though.
• id in new_standard could be u16

[timokroeger]

Thank you for the comments and suggestions!

• I had to unwrap() when creating the CanFrame because the identifier may be too large to fit into the frame u32 > 29bit for EFF and u32 > 11bit for SFF. So I assume it's probably a good idea to make both new_extended and new_standard return a Result.

Makes sense looking at your socketcan implementation. I did not put a Result because I could not come up with a recovery scenario. When an application wants to create a frame with an invalid identifier something must be going wrong already. Anyway, I’m not feeling strongly about this.

• Unable to implement with_rtr without modification, I assume this is not really critical.

socketcan should be able to support this. The rust wrapper should be extended with a method to set that bit.

• The Ok return type documentation for Transmitter::transmit was a little unclear to me. I assume the returned frame would be one with lower priority already in the buffer right?

You understood that correctly: The returned frame is the one with the lowest priority of all pending messages. As a non-native speaker I’m glad to take suggestions for better a description.

[reneherrero]

Wondering what's up with this PR? Looking forward to having this available...

[reneherrero]

Hey @timokroeger,

You might be asked to rename several trait methods with the try_ prefix for fallible methods to prevent name collisions.

I noticed this while trying to submit a PR in linux-embedded-hal... complicates things a little.

Thoughts?

[reneherrero]

@thejpster I'm guessing a) you got lots on your plate or b) you're out on vacation and very intoxicated (hope it's b)... looking forward to getting your input.

Seems to me like adding CAN support would be a pretty big deal, and Timo's implementation is very solid.

[timokroeger]

I'm on holidays. Give me another week to write a summary about the state of this pull request und outstanding changes/ideas.
The try_ prefix is certainly something we should consider.
AFAIK thejpster has hibernated his embedded-hal activities. It's just the automatic reviewer list that is out of date.

[reneherrero]

Gotcha. Enjoy your off-time and stay safe.

[timokroeger]

Added candev to the issue description and included a list of open items.

New requirements

Protocol libraries like UAVCAN are the intended consumers of the CAN traits.
Luckily UAVCAN has a list of requirements for the CAN interface.
We are are missing two of them:

• Transmission deadline
• RX and TX timestamps

I’m not sure if it possible to add TX deadlines in nice generic way (SocketCAN for example cannot implement it).

RX timestamps, in contrast, can IMO easily be added with a fn timestamp() -> Option<Timestamp> method on the Frame trait.
For TX a common approach is to make use of loopback frames.

Combine Receiver and Transmitter traits

To make the loopback approach work we need to merge the Receiver and Transmitter traits.
This comes with additional benefits of simpler drivers as @reneherrero noted.

impl<Can, F, E> Driver<Can>
where
    F: Frame,
    E: core::fmt::Debug,
    Can: Receiver<Frame = F, Error = E> + Transmitter<Frame = F, Error = E>,

vs.

impl<Can> Driver<Can>
where
    Can: embedded_hal::can::Can,
    Can::Error: core::fmt::Debug,

I will prototype those ideas with PCAN and stm32f1xx-hal to see how well they work.
What do you think?

[marcelbuesing]

I think adding fn timestamp() -> Option<Timestamp> makes sense to the Frame trait. I just updated the socketcan-fork. Seems to work fine. I think it might be good to have an easy way for extracting the id from the enum. E.g. an Into<u32> for Id.

[timokroeger]

Changes

• Removed the filter traits (I will open a seperate pull request for them)
• Added try_ prefixes
• Added default blocking implementation

@marcelbuesing
I also prototyped some timestamping. I think the best solution is to go with embedded-time in the long run. Gonna open another PR soon to show the progress here.

I’m hesitant to add the Into<u32> / From<Id> impls. In systems which use both standard and extended identifiers they convert into the same number even though the are distinct. Having the user explicitly check the type prevents bugs in the case of mixed IDs.

[timokroeger]

Please have a good look at the blocking traits! I’m not overly confident, because I’m not fully familiar with the pattern yet.
Is it a bad thing for both traits to have the exact same function names?

Example blocking trait implementations and usages:

• PCAN-Basic
• stm32f1xx-hal

[marcelbuesing]

Please have a good look at the blocking traits! I’m not overly confident, because I’m not fully familiar with the pattern yet.
Is it a bad thing for both traits to have the exact same function names?

Example blocking trait implementations and usages:

* [PCAN-Basic](https://github.com/timokroeger/pcan-basic-rs/commit/0f25fa7d9c7447590daa7802af103fee58b19a67)

* [stm32f1xx-hal](https://github.com/timokroeger/stm32f1xx-hal/commit/7249d2e6034323bd83849e3625d2236337eb53d8)

Well it seem mostly analogous to the serial implementation, although serial seems to add a b to the default blocking implementation fns. I assume this might become relevant once there are non blocking default implementations.

Regarding the timestamp, not sure what the policy is regarding dependencies. Maybe this could be put behind a feature flag making the dependency to embedded-time optional.

I’m hesitant to add the Into / From impls. In systems which use both standard and extended identifiers they convert into the same number even though the are distinct. Having the user explicitly check the type prevents bugs in the case of mixed IDs.

Yes I assume you are right, it might be a little inconvenient but it probably makes more sense this way.

Btw. not really relevant but since I mostly worked with a Socketcan so far, I'd consider naming the fns try_read and try_write.

[marcelbuesing]

I'm attempting to use the fork for an ISOTP implementation but I'm stuck with Error not enforcing being std::fmt::Debug. Any ideas ;)?

use thiserror::Error as ThisError;

#[derive(ThisError, Debug)]
pub enum Error<TCan> where TCan: Can, <TCan as Can>::Error: std::fmt::Debug {
     // ...
    #[error("CAN error")]
    Can(#[from] TCan::Error),
}

#[derive(ThisError, Debug)]
   |          ^^^^^^^^^
   |
   = note: conflicting implementation in crate `core`:
           - impl<T> std::convert::From<T> for T;
   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

[timokroeger]

@marcelbuesing
Looks like the error described in dtolnay/thiserror#85.
Probably removing #[from] fixes it.

[Sympatron]

What do you think about starting with a seperate crate for can traits for now? That way it could already be used by other crates.

[timokroeger]

Great idea! The traits now live at: https://github.com/timokroeger/embedded-can and there is a v0.1 release on crates.io

[reneherrero]

Just want to point out that @timokroeger 's crate has 39,887 downloads. I think that's a pretty clear indicator that CAN support should be included here.

[eldruin]

Here an update about the changes that have happened embedded-hal:

• This should be in a new top-level can module with blocking/nb
• We decided against including default implementations (Remove the various Default traits #289)
• We removed all try_ prefixes.
• You can propose error variants for this over at Add error traits for communication interfaces #296

It is great to see these traits have so many users and 2 implementations. I would welcome these traits on embedded-hal.
Given the field experience:

1. What are the open design questions?
2. Do you foresee any breaking changes?

[rene-at-dell]

Appreciate you looking into this @eldruin. Timo's implementation has matured over time. 0.3.0 is quite a bit simpler than what's here: I like it, much easier to consume.

@timokroeger Why did you switch the 'Id' trait to an actual implementation? The one piece I'm really not crazy about are the constructors. If the value is out of range, shouldn't that be an error? Take 4? :)

[timokroeger]

Thank you bringing the PR to my attention again.
The comment section on this PR has become rather big. I created a new take 4 PR with the changes suggested by @eldruin.

@reneherrero
I found that all implementations of an Id trait would look almost the same and with a concrete implementation we do not need another associate trait for the Can interface.

I still have to catch up #296 on the error handling. If we have concrete error types I agree that it might make sense for the Id constructors to return a Result

[adamgreig]

Closed by take 4 in #314.