CAN/CAN-FD API? about embedded-hal HOT 14 CLOSED

+1 on this. My one request would be to not differentiate between frames that have been received and frames that are intended to be transmitted (this seems to be what you're proposing, I'm just making it explicit). ChibiOS does it this way and I find it creates a lot of duplicate code. Is this feature only lacking a champion? If so, I'd be glad to champion it.

from embedded-hal.

Please mock up the interface you have in mind for discussion. A PoC implementation would be even better but is not necessarily required.

from embedded-hal.

Here's my attempt https://docs.rs/embedded_types/0.3.2/embedded_types/can/index.html

i think a trait for sending these types is required as well.

from embedded-hal.

Those types seem to be just the framing, those should probably live in their own crate anyway. What is needed for a hal interface are generic methods for setting up CAN transfers (and doing so) over the CAN peripherals of a typical MCU. I don't know particularly much about CAN transmissions but from reading the STM32F042 reference manual it pretty much seems to build down to:

• CAN configuration
• Switching between different modes
• Stuffing data into a mailbox
• Getting data out of a mailbox

Maybe there's more to it and/or other MCUs do it in a complete different way, don't know.

from embedded-hal.

Yeah, CAN configuration is pretty straightforward. I'm going to take my own stab at writing a proposal over the next day or two. I do have a question before I dive in though,

In one of @japaric 's hal implementations (F3xx I want to say?) he has his own implementation of units, but the uom crate seems to already be for this purpose. Would it be preferable to roll my own representation of a frequency type, or would it be better to depend on uom?

from embedded-hal.

Quick preview for you guys: dunmatt@24f4922

It still needs documentation, obviously, but even without it I think it's pretty clear what's going on (for the most part). One thing I wasn't sure of (again, very new to rust) was if I'm doing the move correctly in CanInterface::receive... would it be better to use a mutable ref?

What do you all think?

from embedded-hal.

I think CanFrame would be better as an enum like:

pub enum CanFrame {
    DataFrame(id: u16, data: &[u8]),
    ExtDataFrame(id: u32, data: &[u8]),
    FDDataFrame(id: u16, data: &[u8]),
    ExtFDDataFrame(id: u32, data: &[u8]),
    RemoteFrame(id: u16),
    ExtRemoteFrame(id: u16),
   ....
}

I'm not sure about splitting extended identifiers yet, but it might be a good idea to be explicit. Also with this approach we can support Can FD, which gives us access to faster speeds. That said we need to add mechanism to query capabilities of CAN controller.

from embedded-hal.

@EugeneGonzalez For that enum what are you proposing as the signature of CanInterface::receive? Also, isn't the number of items in your enum exponential in the number of features supported?

from embedded-hal.

@robomancer-or there is a fixed number of messages that can be sent: data (only one that can be an FD frame), remote, error, and overload. I just got lazy with writing it out and expanding the enums fully. The current approach is pretty similar to SocketCan on Linux. That's not a bad thing, but I wanted to add some type safety to avoid shooting yourself in the foot. Like a remote frame doesn't have any data.

Ideas on how

1. Builder with read only CanFrame
2. Enums (would move the buffer into the enum)
3. Type Parameters (this would be messy)

More reference material [Uploading can_fd_spec.pdf…](Bosch CAN FD Document).

from embedded-hal.

One of the reason i want to write Rust on embedded is to leverage the type system in good ways. That's why I don't think the single struct socketcan approach is a good idea.

This is the same exact reason why CanFD types should be left out of can frames. If i try to send a CAN-FD frame to a ordinary CAN device it's no reason it shouldn't be caught in compile time.

I think this can be a nice starting point. The CanFrame enum should probably be expanded one layer to look like this:

pub enum CanFrame {
    BaseDataFrame(BaseDataFrame),
    ExtendedDataFrame(ExtendedDataFrame),
    BaseRemoteFrame(BaseRemoteFrame),
    ExtendedRemoteFrame(ExtendedRemoteFrame),
}

Having types such as ExtendedDataFrame allows compile time checks and simpler implementations for protocols that will only use one kind of frames. (like uavcan)

My one request would be to not differentiate between frames that have been received and frames that are intended to be transmitted

agreed

from embedded-hal.

One of the reason i want to write Rust on embedded is to leverage the type system in good ways. That's why I don't think the single struct socketcan approach is a good idea.

This is the same exact reason why CanFD types should be left out of can frames. If i try to send a CAN-FD frame to a ordinary CAN device it's no reason it shouldn't be caught in compile time.

Right, I agree with that whole heartedly. I would add that it's important to consider the consumers of this API. As a HAL, we're pretty much only going to be consumed by people writing drivers. That is, it's not the job of this layer to idiot proof anything past the physical layer, it's the job of this layer to make idiot-proofing higher levels portable and to not unduly restrict the options available at the implementer's and consumer's levels.

It's not in my previous preview, but your point about CAN-FD checking at compile time is spot on. It's not up yet, but my next preview is going to have a type parameter on transmit and receive for exactly that reason.

from embedded-hal.

Got another preview for you guys (in the PR above), what do you think? I found myself needing a CAN parameters calculator, but writing it started turning into a rabbit hole, so I'm going to spin it into a separate can_utilities crate and then depend on that (or maybe not depend on it but leave a pointer to let HAL implementors know it exists and will save them headache... which option is better?).

from embedded-hal.

@therealprof Ok, I've just updated #53 with a greatly cleaned up version. No sample implementation as of yet, but I'll be working on that today. In the mean time, I think it's ready to be seriously looked at.

from embedded-hal.

Solved in #314

from embedded-hal.