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

Additional CAN trait for extended features #381

Open
mlsvrts opened this issue Apr 27, 2022 · 4 comments
Open

Additional CAN trait for extended features #381

mlsvrts opened this issue Apr 27, 2022 · 4 comments

Comments

@mlsvrts
Copy link

mlsvrts commented Apr 27, 2022

The current embedded-hal CAN trait seems to only support 'transmit' and 'receive', but commonly higher level applications will need support at least:

  • Opening and closing the node
  • Setting the baudrate

Additionally, it can be useful to have support for:

  • Reading the bus state flags
  • Putting the device into listen-only mode

For instance, consider the options supported by the linux can-utils slcand daemon: https://github.com/linux-can/can-utils/blob/master/slcand.c

I think that it would be reasonable to have some CanState trait, that supports open/close, alongside a CanSpeed trait that allows setting (+getting?) the device baudrate -- but I would be interested to hear other opinions/comments.

@ryankurte
Copy link
Contributor

howdy, thanks for the issue!

the main goal with e-h is to abstract what is required for drivers rather than how to interact with a given platform, because that differs so substantially between platforms and cross platform applications must address this configuration -anyway- (think mapping pins, clocks, etc.)

i have not seen a specific need for these yet, but if there are drivers with a demonstrable need for these APIs at runtime the first steps would be to ensure your proposed traits could be supported by a collection of platforms and used by a driver or two, then to open the appropriate PRs.

@mlsvrts
Copy link
Author

mlsvrts commented May 4, 2022

@ryankurte Thanks for the feedback!

Maybe I'm misunderstanding the goals of this project. I expect a stack like:

host system
------
comm. transport (usb, uart, i2c, etc.)
------
can hal  <------------- embedded hal goes here
------
can controller (embedded hardware specific)

I'm not actually aware of many CAN stacks that don't support programmable baudrates: kvaser, pcan, linux vcan, etc. all support programmable bitrates, and Zephyr RTOS also seems to consider it a integral CanBus API feature.

So if I want to write a uart --> can driver using e-h, I'll find myself unable to implement all of the supported features (namely, programming the bitrate and open/close).

If all you need is drivers + hardware that support a shared feature-set, I'm happy to provide a list and open a PR with my suggested traits. Or do you need something more concrete than that?

@timokroeger
Copy link
Contributor

Related discussion: #212 (comment)

@adamgreig
Copy link
Member

The traits in embedded-hal are meant to be implemented by device-specific HALs and consumed by drivers that communicate over CAN, or by higher-level CAN protocol stacks that might implement something like ISO TP or CANopen.

The implementors would use something like vcan, or talk over SPI to a CAN controller, or use a microcontroller's embedded CAN controller - there's a big diversity in possible implementations, many of them on bare-metal embedded platforms.

The users of the traits are then things like a driver for a motor controller that's controlled over CAN (which might be part of an application or a standalone crate), so that it can easily be generic over different underlying hardware. In general those drivers shouldn't need to perform configuration operations like opening/closing the node or setting the baud rate; we consider that sort of configuration as out of scope for embedded-hal traits. Instead, the specific implementations (eg on top of vcan, or the STM32's bxcan peripheral, etc) should provide their own methods for configuration of baud rate, enable/disable, etc etc; applications use those methods to bring the bus up, and then drivers can use the generic traits to actually send and receive frames.

bors bot added a commit that referenced this issue Sep 26, 2022
394: Split nb and can traits to separate crates. r=eldruin a=Dirbaio

Following up from #177 (comment), this PR splits `nb` and `can` traits to separate crates.

I propose keeping these crates (and `embedded-hal-async`) separate forever. (instead of merging EHA into EH when stable as we originally planned when we created EHA).

Reasons for splitting `nb`:
- its future is unclear. Some people who would like to see `nb` deprecated. By splitting, if we do deprecate it, we won't get stuck with it in the main `embedded-hal` crate forever.
- It makes the module paths in the main crate cleaner.
- Traits tied to a particular "execution model" in their own crates, which make the overall structure easier to understand IMO.

Reasons for splitting `can`:
- It has some open concerns: #381 #387 
- Even if we do solve these, it's nice to have separate crates for the "more specialized" traits so that they can be major-bumped in the future if more concerns arise, without having to major-bump the main `embedded-hal` crate.

TODO:

- [x] Name it `embedded-can`, not `embedded-hal-can`.
- [x] Move/remove docs/examples from the main crate that mention `nb`.
- [x] Mention the subcrates from the main crate's docs.
- [x] Add CI
- [x] move `embedded-hal` to a subdir as well
- [x] What should the nb, can crate versions be? 1.0? (no opinion on this from me).
   - [x] can: 0.4
   - [x] nb: 1.0

Co-authored-by: Dario Nieuwenhuis <[email protected]>
bors bot added a commit that referenced this issue Sep 26, 2022
394: Split nb and can traits to separate crates. r=eldruin a=Dirbaio

Following up from #177 (comment), this PR splits `nb` and `can` traits to separate crates.

I propose keeping these crates (and `embedded-hal-async`) separate forever. (instead of merging EHA into EH when stable as we originally planned when we created EHA).

Reasons for splitting `nb`:
- its future is unclear. Some people who would like to see `nb` deprecated. By splitting, if we do deprecate it, we won't get stuck with it in the main `embedded-hal` crate forever.
- It makes the module paths in the main crate cleaner.
- Traits tied to a particular "execution model" in their own crates, which make the overall structure easier to understand IMO.

Reasons for splitting `can`:
- It has some open concerns: #381 #387 
- Even if we do solve these, it's nice to have separate crates for the "more specialized" traits so that they can be major-bumped in the future if more concerns arise, without having to major-bump the main `embedded-hal` crate.

TODO:

- [x] Name it `embedded-can`, not `embedded-hal-can`.
- [x] Move/remove docs/examples from the main crate that mention `nb`.
- [x] Mention the subcrates from the main crate's docs.
- [x] Add CI
- [x] move `embedded-hal` to a subdir as well
- [x] What should the nb, can crate versions be? 1.0? (no opinion on this from me).
   - [x] can: 0.4
   - [x] nb: 1.0

Co-authored-by: Dario Nieuwenhuis <[email protected]>
Co-authored-by: Diego Barrios Romero <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants