added a HAL for CAN interfaces

[dunmatt]

Note: this is not yet ready to go, I'm just creating a PR for the sake of discussion in #21

[therealprof]

I'm not quite happy about the can-utils dependency. This should really only contain the bare minimum traits to transmit data over a can bus and leave the implementation details (like setting the speed) up to the concrete HAL implementations.

[dunmatt]

So, to clarify, is your objection to having a new dependency at all, or is it to having one on a crate that also contains implementation logic? I could easily move the data types into a new crate (eg can_types) and have both embedded_hal and can_utils depend on it. Would that be suitable?

Additionally, I think we're interpreting "minimum" differently. It seems like (and please correct me if I'm wrong here!) to you the minimum is only transmit and receive, whereas I'm more focused on "what functions are required to build a decent generic can driver". Can you clarify the value of focusing so narrowly?

[therealprof]

@dunmatt Both I would say. Let me try to clarify: The point of the traits is to abstract the hardware insofar as you can easily set up peripherals by calling implementation specific setup and then have drivers which can use a common set of standard traits to send out the data. So the question really is: Does the hardware (after setup, which is implementation specific) need to know what kind of speeds to transfer at or what kind of data you want to transfer or is it simply some opaque data that you send over an established channel. Only the details actually required to send a chunk of data (like amount of bytes to follow, sender/destination addresses which are not part of a frame) should be part of the traits we're modelling here.

[dunmatt]

Ah, I think I see a disconnect! There are two types of set up that are relevant here. There's the peripherals set up, which is (as you say) implementation specific, and there's the bus set up, which is part of the CAN spec and thus 0% implementation specific. Thus, to maximize code reuse the bus set up should be part of the generic driver that consumes the HAL.

My specific use case is this, I would like to have a driver that is able to automatically determine the bus parameters. So the choices seem to be A) expose the bus parameters in this HAL B) write my own HAL and expose the bus parameters there C) have tons of redundant code or D) give up. Do you see where I'm coming from?

[therealprof]

I don't see anything wrong about providing your own more specific (and reusable) traits providing a higher level abstraction over CAN bus specifics. The traits in this crate represent the lowest possible abstraction required to universally interface with hardware peripherals. In a way you could consider what you want to achieve a universal CAN bus driver and the question you should ask yourself is: What facilities would such a driver need from CAN traits so that any HAL implementing them could be used by your driver and thus should be defined here.

[dunmatt]

I did ask myself exactly that, and this PR was my answer ;-)

But that's cool, if the intent for this HAL is to be that spartan I'm totally ok starting my own. Thanks for at least having a look!

[therealprof]

@dunmatt I'm not sure I follow, you actually wrote down the key aspect of the concept yourself:

Thus, to maximize code reuse the bus set up should be part of the generic driver that consumes the HAL.

This sentence sums up nicely the layering:

1. The HAL traits defining the lowest level interface with hardware (in other words: this crate)
2. The hardware specific HAL implementations, implementing 1)
3. The driver, consuming 1)

The next layer would be:
4) The application initialising the the driver (3) on a platform providing the HAL implementation (2) and using it to do whatever the purpose of the application is

I think it would be great to have CAN hardware traits in here and might even be able to provide an HAL implementation for the STM32F042 (although I don't have the setup or knowledge to properly test it) to get things started.

[dunmatt]

... I guess I'm not understanding what you're asking me to change, it sounded like you were asking me to remove the bus parameters from the HAL, moving the onus of configuring the bus into the hardware bringup code (outside the HAL) rather than as a function of the driver.

[therealprof]

... I guess I'm not understanding what you're asking me to change, it sounded like you were asking me to remove the bus parameters from the HAL, moving the onus of configuring the bus into the hardware bringup code (outside the HAL) rather than as a function of the driver.

You may have noticed that anything hardware specific is not part of any of the traits; there's no speeds, no hardware specifics, no encoding specifics, etc. All of that is part of the HAL implementation, i.e. number 2) and the user of the HAL (i.e. the application) needs to know about it, not even the driver. The purpose of a driver would be to provide the higher layers of "communication" by using the HAL (1) on any hardware implementing it (2). Such a higher could be a LED driver using the GPIO interface to light up a led (or even using a I2C interface to talk to a chip driving the LEDs, really doesn't matter) or a magnetometer driver using the I2C interface to talk to a separate chip and provide data in a usable form without having to read 20 pages documentation or (as in your case) a CAN bus driver allowing an application to send data over a CAN bus and taking care of the framing, error handling and data munching in general.

Of course if there's anything bus specific such a driver would need on the physical layer to operate, like changing states, transmitting data, etc., then that should be in the trait. If it's not absolutely necessary then it should be not. Everything you design into the trait needs to be implemented by every single HAL implementation implementing this particular trait, so by packing in everything and a kitchensink you're enforcing code duplication in 2), not preventing it!

I'm no CAN expert, you obviously are. What I'm asking for is: Reduce your trait to an absolute minimum needed to talk to a CAN and then get working on a driver that implements the rest of the interface based on the minimal hardware abstraction. My gut feeling (which could be wrong!) tells me that framing should be gone, so:
fn receive(&mut self, buf: &mut Frame) -> nb::Result<bool, Self::Error>;
should be
fn receive(&mut self, buf: &mut &[u8]) -> nb::Result<bool, Self::Error>;
instead.

• If you have different types of busses you should put them into different traits instead of putting them all in one trait and having methods allowing to infer capabilities.
• If you have special operation modes they should be modelled as extension traits. If there're no mandatory hardware counters, don't provide functions which are supposed to keep state, since HAL implementations are not supposed to maintain state in RAM.
• If there's no hardware arbiter capable of changing your operation mode so it will be exactly as initialised then don't provide an method to query it because the application will know it is exactly what it was initialised to.
  ...

Rust works a bit differently than other languages which is exactly what makes it so great but sometimes requires a bit different thinking to take full advantages of the capabilities.

[dunmatt]

Ah, so the framing is absolutely mandatory, it is the one and only way to make sense of the incoming data. Sometimes in CAN you don't even get a data payload, the entire message is carried by the framing.

That said, I have reduced the footprint to functions without which there is no way to recover the exposed information or functionality. It is minimal in that sense. It is not minimal in the sense that you could in theory operate a CAN bus just fine without being able to tell the interface to enter or exit sleep mode.

[therealprof]

Ah, so the framing is absolutely mandatory, it is the one and only way to make sense of the incoming data.

Does the hardware need to know about the framing? I.e. do you need to set a register per frame type and the hardware will do the correct framing? If so I'd expect to have a frame type parameter and raw data in the send function and vice versa in the receive function instead of a new frame type. If the hardware just takes the raw data and doesn't care what kind of frame it contains, it really should go to a higher level generic driver.

Note: For a HAL trait to become proven, at least 2 HAL implementations (preferably for two different vendors) must exist. So it might be worthwhile to pick at least two MCUs now and figure out how one would implement a HAL on those and such what a good signature of the trait methods might be.

[kjetilkjeka]

Bare minimum can interface

To make a minimal interface for CAN you actually will need to include a CanFrame. This is because.

1. There are different frames for sending and requesting data. They both must be supported by a minimal can hal. (they could be supported as different functions but we would probably rather want a Frame struct).
2. Both the data and ID contains relevant information. This could be implemented as two different parameters but putting them inside a struct allows guaranteeing that only Frames with an allowable length exists (we want this)

Priority on the can bus is given from the ID. Realtime systems need to reason about framing to give guarantees. A single call to transmit/receive functions can therefore not do any weird framing stuff.

We will also need guidelines of how the CAN drivers should be programmed (Must avoid the can buffer priority inversion problems etc). This will probably require something like:

fn receive(&mut self) -> Result<Option<CanFrame>, SomeError>

/// transmits a can frame
/// 
/// replace (and returns) the lowest priority message if buffer is full
fn transmit(&mut self, frame: CanFrame) -> Result<Option<CanFrame>, SomeError>


/// Transmits a can frame (without replacing with lower pri frames priority)
fn transmit_unchecked(&mut self, frame: CanFrame) -> Result<(), SomeError>

Another (bare minimum) thing is to check for read/write errors or bus off errors (these HW counters exists for every device following the CAN specification)

pub enum CanBusState {
    Active,
    Passive,
    BusOff,
}

fn bus_state(&self) -> CanBusState;
fn transmit_error_count(&self) -> u32;
fn receive_error_count(&self) -> u32;

Would be nice to include (but can be done at a later point)

Everything in the last section is absolutely the bare minimum for a can interface. Since some things are 100% set for the can specification and is required in every compatible hardware it could also make sense to include things as.

• Bitrate enum.
• Types that represent filters.

Probably not general enough to include

everything related to mailboxes, FIFO buffers, etc

---

Note: For a HAL trait to become proven, at least 2 HAL implementations (preferably for two different vendors) must exist. So it might be worthwhile to pick at least two MCUs now and figure out how one would implement a HAL on those and such what a good signature of the trait methods might be.

I might implement this for NXP s32k144

[therealprof]

I just checked the implementation on the STM32 side and looked into the CAN Wikipedia article. Sending on a STM32 chip essentially means setting:

• Data or remote frame
• Identifier (standard and extended)
• The raw data (including the size)

The actual framing is done in hardware so even if you did the framing in software it wouldn't help much.

@kjetilkjeka How would data and remote frames differ from e.g. read/write operations on I2C? In the end it looks like you could have have different traits for standard and extended ID operation and separate methods for sending data and remote frames each of them taking a matching id and the data.

[pftbest]

Maybe I am wrong, but this is how I see it:

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

I believe that this crate is only concerned about (1), (4) and (5); and not about (2), (3) and (6).
The traits are meant to help writing generic (5) easier, so they should only provide functionality that (5) may need, but not necessarily what (6) may need.

If you look at I2C trait, for example, you won't see functions for setting up interface speed, because (5) don't need to change the speed at run-time, only (6) may want to do that.

[kjetilkjeka]

I just checked the implementation on the STM32 side and looked into the CAN Wikipedia article. Sending on a STM32 chip essentially means setting:

• Data or remote frame
• Identifier (standard and extended)
• The raw data (including the size)

Also keep in mind

• A data frame can contain up to 8 bytes of data (or no data).
• A remote frame cannot contain any data

Both of these will need to be enforced programatically.

---

How would data and remote frames differ from e.g. read/write operations on I2C?

I2C is single master while CAN is multi master (read: P2P). This means that both sending a remote or data frame is something any node on the network may do at any time. Sending a remote frame (and reading the answer) is actually somewhat close to an I2C read. But at the same time, any node might also send you data that you didn't request. This can_receive must support both reading of "expected data" (requested data) and this "unexpected data" (async data).

In the end it looks like you could have have different traits for standard and extended ID operation and separate methods for sending data and remote frames each of them taking a matching id and the data.

I like the idea of this approach but I'm not sure if it's worth it in practice.

One problem is that if you have an interface that supports CAN2.0A, CAN2.0B and CANFD frames. To receive all kind of frames you would have to call all three receive functions (from three different traits). Since they would share mailboxes/buffers CAN2.0A frames could effectively fill all the buffers and you wouldn't be able to empty them with the CAN2.0B receive function.

In theory not all CAN2.0A compatible interfaces needs to support CAN2.0B, but in practice they do. I don't think splitting the interface between extended and non-extended IDs is worth it.

For CAN-FD it probably makes more sense. I would still want the most general interface (CAN-FD) to return frames of less general types to avoid having to call different methods in different traits to read a can frame.

[kjetilkjeka]

Maybe this is a side track but I feel it's one thing that is beeing forgotten @pftbest.

I've started on a library for interfacing dynamixel servos in rust The protocol is on top of rs485 or rs232. The protocol requires things as timing out after 100ms and keeping track of what baudrate the different servos should be talked to with. (If the user was required to keep track of different baudrates and setting them manually on the interface between transmission it would be kind of horrible to use)

There are a lot of similar things that can be regarded as "user level stuff" but still needs to be modular between different architectures (because it's distributed as libraries). If you use the dynamixel library on desktop computers you can flip the "serialport" feature and use it with any serial<->RS485 adapter. The reason for this is that the serialport library contains the "set timeout" and "set baudrate" functions required to do so. I would love to make this possible for embedded as well.

---

My take on what this crate should be:
Everything that is generic with an interface/protocol should be included. If all can interfaces have some error counter. Then reading the error counter should be possible. If 95% of all X have Y then drop Y.

I like the drawing from @pftbest. Let me try explain what i mean using it as a basis:
Strictly speaking, (a) needs nothing from (5). It can already get everything it needs from (3). So how do we decide what should go through (5)?

My answer is: Everything generic (as much as possible) should go through (5). Everything special (only the things required) should go directly to (3).

[therealprof]

@kjetilkjeka What is preventing you from implementing that? The only thing that is rather lame from an interface point of view is the interrupt handling which needs to be done manually per platform instead of an generic way. But it still is possible to use interrupts e.g. to implement timeouts or to react on incoming data, and the baudrate is something you can usually specify during initialisation.

If you think anything is missing for RS485, please open a new ticket and let's extend the Serial trait(s) or define new ones.

[dunmatt]

Hoo boy, go AFK for a few hours and the thread explodes! I'll respond to each in order (starting from my last post):

@therealprof re: "Does the hardware need to know about the framing?" Yes. On STM32s, for example, there is a hardware priority queue that transmits packets according to specific in the framing (the id). Type parameters are insufficient for this, unless you want to have 2^29 types. They also have maskable filter banks in hardware (again, as described in the CAN spec, not just an STM thing), which means you'd need 2^58 types to get around representing the information as a couple u32s in a struct.

@kjetilkjeka You mentioned having transmit return a frame that has been bumped from the priority queue. Why wouldn't we just return WouldBlock in that case?

[dunmatt]

@pftbest There's more than made it into your diagram. There should be an item (4.5) that is the CAN driver and network stack. As is CAN takes care of OSI levels 1 and 2, but in practice everyone uses higher level protocols when they're interacting on a CAN bus, so there needs to be code there that acts as a CAN driver (ie controlling the hardware by way of the HAL) and dipatches incomming traffic to the appropriate OSI level 3(+) protocol, which would then be interacted with by (5) in an attempt to communicate with (1).

[therealprof]

@dunmatt Who said something about type parameters? Simply put it in the call signature.

Please have a look at the existing HALs, especially the I2C one. As far as I can see the write signature would work just fine for a send_basic_data method in a CAN trait, substitute addr by a type suitable type for a basic CAN ID and that'd be it.

[dunmatt]

@kjetilkjeka "I would still want the most general interface (CAN-FD) to return frames of less general types to avoid having to call different methods in different traits to read a can frame." Exactly, I strongly agree, let the driver sort out what sort of frame it was.

[therealprof]

@dunmatt

There should be an item (4.5) that is the CAN driver and network stack.

Absolutely! Go for it.

so there needs to be code there that acts as a CAN driver (ie controlling the hardware by way of the HAL)

No. A HAL is not a fancy proxy between any layer you desire, it is a set of basic primitives you can expect to always get from a driver implementing this HAL. If you want to hardcode a CAN implementation for a specific chip or a set of chips, please go ahead but this is NOT what embedded-hal is all about.

[dunmatt]

@therealprof Oop, sorry, I internally mishyp-henated the phrase "frame type parameter" to be "frame type-parameter" rather than "frame-type parameter". It could be made to work as you're suggesting (assuming that you also broke out the other relevant metadata into arguments), but it's less clear for the reader to have two separate values that have to be passed around in lockstep everywhere. In I2C land addresses are a different sort of thing than the data you store at them (one is a what, the other is a where), in CAN land the CAN header is not meaningfully separable from the data (both things are what, and the line between them blurs as you go up the network stack), at this level CAN doesn't even have a concept of where.

[therealprof]

@dunmatt I understand that an address is different from a CAN id. Just trying to point out that all HALs are implemented like that and that's the point of it.

Actually if you prefer to have a CAN structure that combines ID and data into one that'd be fine for me, too. As long it's not a fully assembled CAN frame because that doesn't translate well into typical implementations which automatically do things like CRC, length computation and other things so you would actually have to disable the frame and pull out the relevant bits in pretty much all cases which doesn't make any sense at all.

[dunmatt]

@therealprof Agreed, and if you look closely I think you will find that is exactly how it's implemented in can_utils. It is definitely not assembled. If there's a thing I can let the hardware do for me you can bet I'm going to let it.

[therealprof]

@dunmatt I never suggested otherwise, but I am suggesting that a HAL is the means by which the driver controls the hardware. Do you disagree?

Fully agree. But the interface needs to be simple and straightforward enough to allow almost any HAL implementation (and you may have noticed I'm trying to use implementation instead of driver because although strictly speaking a HAL implementation is a driver, the drivers we are talking about are consumers of the HAL, so either abstractions for a specific device sitting behind the the hardware or even the application itself). HAL implementations can range from MCU specific register accesses over domain specific controller chips using a different subsystem (like external SPI/CAN interfaces) over interfaces using a library on a real OS to a bitbanged GPIO implementations. Other aspects include that we want to cater for all possible shortcuts like hardware computation where available. If the HAL interface is too complex you're preventing implementations from happening and if you're operating at a too high level (like accepting frames fully assembled by an external crate) you're just wasting resources.

[dunmatt]

@therealprof It sounds like we're violently agreeing. I think the only point of disagreement is your assessment of how simple and straightforward my proposal is. The way I wrote this PR was by pulling up the CAN 2.0 spec in one tab, the CAN-FD 1.0 spec in another, and an editor and a terminal. Everything you see in this PR comes straight from the spec with one exception: maximum_timing_values, because the spec doesn't dictate that (although the hardware does, so the generic driver needs a way of finding out the hardware's capabilities in a generic way). To simplify past where this is is to start blocking functionality that the spec describes (and thus that people might be relying upon).

[therealprof]

@dunmatt In the end it's not up to me but I strongly dislike the fact that something happens in another crate and Frame is an opaque type I cannot immediately see behind. Using a Frame type also means that the HAL implementations need to drag that in and it is not obvious that this external crate is going to be stable (in the sense of the interface not changing because someone saw the need to), properly maintained and supported for any platform this works just fine on.

I would like to have something that is in one place, straight forward, nicely documented and simple enough that one can see on a glance: "Yes, this is so simple it's not going to break, ever!"

[therealprof]

@dunmatt I believe we're in agreement as soon as I see your proposed code changed to address my concerns. 😉

Specs are nice and great but that doesn't mean you have to model the whole thing into a single block.

[dunmatt]

As to your concerns about stability, upkeep and cross platform support, the structure of a CAN frame hasn't changed in more than 25 years, so I have my doubts that it's going to. I'll go so far as to say that the implementation as it is now in can_utils::frames can be considered stable. Would you feel better about it if I break it out into its own crate and promote that to 1.0.0?

The thing I'd like to avoid is having that struct in embedded-hal, because crates like can_utils would depend on it despite having nothing to do with hardware.

[therealprof]

@dunmatt I'm proposing the trait is changed/broken up into simpler methods using standard primitives and your can_utils can easily downcast/upcast those primitives to those required format by the HAL. And if you like you can create a "layer 4.5" driver that neatly interfaces between various HAL implementations (remember that setup is always and will always be system (or as we call it: target) specific) and CAN users.

[dunmatt]

@therealprof Huh? I think you're completely not understanding what I was saying about adding a box 4.5 to the diagram, but that doesn't matter.

Tell me if I'm wrong here, but it's starting to seem like what you're really objecting to is having any external dependencies in embedded-hal. Or perhaps dependencies you don't control? I don't know, but I'm grasping at straws trying to figure out how your proposal improves clarity, safety, correctness, or anything else. CAN interfaces really do have all of that functionality, it really is all in one place (both on the chip and in the datasheet), what challenge do you expect implementors to face that is somehow improved by fragmenting what it means to be a CanInterface?

[therealprof]

@dunmatt Yes, I am objecting to the external dependencies and to having a high level CAN bus interface in there. Do you have an HAL implementation already? Sometimes working on the implementation makes you realise whether an interface is suited or not.

[dunmatt]

I don't, I wanted to float the design to gauge support before dropping time on implementing it, but the response seems to be mostly positive. Give me a few hours.

What exactly to you mean "high level"? This is nothing like high level can, in most implementations the fields in these structs are going to map directly to register regions. (This representation is so stable they build it into hardware...) But don't take my word for it, I'll have a full implementation done in a couple of hours and you'll be able to see it for yourself.

[dunmatt]

@therealprof You're very right. I did learn a lot from the process of implementing it. But. On the other hand, considering that I had 3 hours and I've been programming in Rust for less than a month, here's your proof that the API isn't crazy pants: https://github.com/dunmatt/stm32f439-hal/blob/sampleCode/src/can.rs

The sample implementation is shit, it doesn't even build and I don't understand the errors at all. I'm sure that a lot of the repitition might be solvable via macros, and the whole E0117 thing caught me completely off guard... perhaps there's some more idiomatic way of doing it? But anyway. If a n00b can get the basics done in 3 hours it can't be that hard

[dunmatt]

... The more I work on this the less convinced I am that the protections provided by svd2rust are actually worth the trouble for more complicated peripherals. Basic things, like iterating over identically structured registers, or writing a function that perform an action on whatever register is passed in, seem to be impossible (or at least so far beyond my current skill that I can't even figure out what I might search for), and as a result I end up having 28 copies of many actions, one for each of the register type. Where's the right place to talk about this? I'm trying the #rust-embedded channel, but it's a ghost town...

[japaric]

I know nothing about CAN but given the discussion so far and the contents of this PR it seems to me that this is a complex enough topic that this should be iterated out of tree.

Once the out of tree implementation is mature enough that it has implementations for several devices and has been tested (with different multitasking models) and benchmarked on hardware then we can revisit this topic and decide what parts to land in embedded-hal.

How knows? Competing implementations with different tradeoffs may appear; in that case it would make sense to land an interface (traits) that lets you use all of them.

It has been mentioned that the CAN frame format has not changed in the past N years and will not change because it's part of the spec. The can-utils has a representation of a CAN frame that looks like this:

pub struct CanFrame {
    pub id: u32,
    pub dlc: u8,
    pub data: [u8; 8],
    pub rtr: bool,
    pub ide: bool,
    pub reserved0: bool,
    pub reserved1: bool,
}

But this "parsed" representation is not only possible representation of a CAN frame. Another valid representation looks like this (based off smoltcp packet types):

// simplified, i.e. not generic
pub struct CanFrame2<'a> {
    bytes: &'a mut [u8],
}

This CanFrame2 version would provide methods to get / set the different fields of the CAN frame (w/o incurring in bounds checks because the newtype has the invariant that the contents are a valid CAN frame). Semantically CanFrame and CanFrame2 are the same but they have different performance characteristics: e.g. CanFrame2 is cheaper to send across threads because it has a size of 2 words.

These are implementation choices that have implications on the performance characteristics. These choices may also affect things like Send-ness and Sync-ness which can make the CAN stack unusable with some multitasking models, etc. So I think it's premature to settle on any kind of concrete type at this time.

[dunmatt]

@japaric Out of curiosity, what does Out Of Tree mean? Is it like OOB for PRs... or for repos?

[japaric]

what does Out Of Tree mean?

Not in the master branch of this repo. So that could be a fork of this repo or some other crate.

[tib888]

I'm working on a can implementation for STM32F103x. It works already, but interrupt, error handling is missing, and I'm planning to refactor a few things.

There are lots of implementation specific details, so I agree with @japaric, that we need to have a few platform specific implementations before abstracting the things which all they have in common.

Probably the initialization / configuration part will be fully device specific.
The message filter / filterbank configuration also have many device specific details. For example

• you can chose which receive fifo should get the filtered message,
• you have id list and id mask mode,
• you may split the 32bit filter into two 16 bit ones, but then some bits of the extended ids will be ignored
• the filters are organized in filter banks, and only a full bank can be activated or deactivated
• organizing the filters into filter banks is not trivial.

On the other hand, traits for canid, canmessage/frame, transmit mailbox(es), receive fifos/mailbox(es) may be a good subject for embeded-hal.

I'll post my implementation here for review, as soon as i'm happy with it.

[mathk]

Ping from triage: No update since quite a long time. Should we close that and work on other branch?

[adamgreig]

Closed by #314. Thanks everyone for all the contributions and discussions!