Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CAN/CAN-FD API? #21

Closed
kjetilkjeka opened this issue Sep 24, 2017 · 14 comments
Closed

CAN/CAN-FD API? #21

kjetilkjeka opened this issue Sep 24, 2017 · 14 comments

Comments

@kjetilkjeka
Copy link

From working with the uavcan rust implementation I see the convenience of having a standardized CanFrame and CAN interface in Rust. Perhaps this is something this crate should aim to have?

I think it would make sense to both have a trait Can and a struct CanFrame.

@robomancer-or
Copy link

+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.

@therealprof
Copy link
Contributor

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

@kjetilkjeka
Copy link
Author

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.

@therealprof
Copy link
Contributor

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.

@robomancer-or
Copy link

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?

@dunmatt
Copy link

dunmatt commented Mar 1, 2018

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?

@EugeneGonzalez
Copy link

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.

@robomancer-or
Copy link

robomancer-or commented Mar 1, 2018

@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?

@EugeneGonzalez
Copy link

@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).

@kjetilkjeka
Copy link
Author

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

@dunmatt
Copy link

dunmatt commented Mar 2, 2018

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.

@dunmatt
Copy link

dunmatt commented Mar 2, 2018

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?).

@dunmatt
Copy link

dunmatt commented Mar 3, 2018

@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.

bors bot added a commit that referenced this issue Oct 28, 2021
314: Controller Area Network (CAN) Take 4 r=eldruin a=timokroeger

Updated to the latest HAL changes:
* Removed `try_` prefix
* Moved non-blocking implementation to `nb` module
* Removed default `blocking` implementaions

## Usage Example
[stm32-fwupdate](https://github.com/timokroeger/pcan-basic-rs/blob/eh-take-4/pcan-basic/examples/stm32-fwupdate.rs)

## Implementations
Updated for this PR:
* [pcan-basic](https://github.com/timokroeger/pcan-basic-rs/blob/eh-take-4/pcan-basic/src/lib.rs) on top of an existing software API
* [bxcan](https://github.com/timokroeger/bxcan/blob/eh-take-4/src/lib.rs#L460)

Based on the very similar predecessor traits `embedded-can` v0.3 ([diff v0.3 -> this PR](https://github.com/timokroeger/embedded-can/compare/eh-take-4))
* [candev](https://github.com/reneherrero/candev) implementing the traits for linux SocketCAN
* [socketcan-isotc](https://github.com/marcelbuesing/socketcan-isotp)

## Previous Discussion
* #212 
* #77
* #21
* #53 

Co-authored-by: Timo Kröger <timokroeger93@gmail.com>
@eldruin
Copy link
Member

eldruin commented Oct 29, 2021

Solved in #314

@eldruin eldruin closed this as completed Oct 29, 2021
peckpeck pushed a commit to peckpeck/embedded-hal that referenced this issue Nov 10, 2022
21: Fix code owners to be the embedded-linux team r=posborne a=therealprof



Co-authored-by: Daniel Egger <daniel@eggers-club.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants