NotificationsYou must be signed in to change notification settings
Fork81
Star173

Embedded HAL Traits#24

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fpagliughi merged 10 commits intosocketcan-rs:masterfromnnarain:embedded-hal

Dec 5, 2022

Merged

Embedded HAL Traits#24

fpagliughi merged 10 commits intosocketcan-rs:masterfromnnarain:embedded-hal

Dec 5, 2022

Conversation

Copy link

Contributor

nnarain commentedJul 31, 2022

This MR builds off of@fpagliughi 's netlink-update branch (#20).

I have implemented the following:

Embedded HAL blocking and non-blocking traits for CAN
Added examples for blocking read/write
Moved some code around into dedicated modules
Moved integration tests intotests/ folder

Could probably use some advice on the best way to map the errors.

fpagliughi mentioned this pull request

Nov 23, 2022

Unmaintained?#11

Closed

Copy link

Collaborator

fpagliughi commentedNov 23, 2022•
edited
Loading

Hi@nnarain! I have a bunch of questions about this:

Is the goal to makethis crate portable to other (non-Linux) systems? Or to make application source code that would be easier to port to other embedded crates?
- Would this be possible given the dependence on so many Linux and std crates (nix, etc)
What is the argument for doing this considering that none of the other Embedded Linux crates (gpio-cdev,i2cdev,spidev, etc) support embedded-hal interfaces?
Would we essentially get the embedded-hal compatibility "for free" or would we need to make tradeoffs with full Linux SocketCAN support? Meaning: Would it make implementing the full Linux capabilities more difficult to keep it compatible?
- Would this make supporting async/await (tokio, async-std, smol, etc) more difficult?
Assuming we do want to merge this, it seems there is a bunch of refactoring in this PR that might make merging a bunch of the older/stale PR's and forks more difficult. Should we try to merge those first and then rebase this? Or what would you suggest?

Copy link

ContributorAuthor

nnarain commentedNov 23, 2022•
edited
Loading

No this is not really related to cross platform support
The advantage of embedded-hal is being able to build platform agnostic drivers.
a. If I write a embedded-hal i2c IMU driver, for example, I can use that driver on ESP, NRF and STM32 devices that implement embedded-hal
b. It does look like there is a linux implementation in the same github org:https://github.com/rust-embedded/linux-embedded-hal
c. For CAN specifically I've started writing some CANopen related libraries (https://github.com/nnarain/can-rs). That can could work on Linux or embedded devices). Also platform agnostic CAN tools (candump, cansend, etc).
No trade off I'm aware of. The can::Frame trait is implemented on the type. Other socketcan specific data could be stored in the struct if necessary
a. I have a fork of tokio-socketcan for async supporthttps://github.com/nnarain/tokio-socketcan
I'd suggest merging other MRs first and I'd rebase my branch.

Copy link

Contributor

marcelbuesing commentedNov 24, 2022

I think regarding 2b. the alternative would implementing thecan traits in the linux-embedded-hal crate and reference the socketcan crate for that. I just did that for the current socketcan master branchhere in a prototype. It's basically just a wrapper aroundsocketcan, based on the current master branch. That's also the way it's done e.g. for the spi implementation, inverting the crate dependency relationship.

I noticed that on the master branch and latest alpha of embedded-hal 1.0.0-alpha.9 they seem to havemoved the CAN module again into a separate crate called embedded-can. But I guess that makes little difference in the end. They also seem to have started splitting the async and blocking part.

Copy link

ContributorAuthor

nnarain commentedNov 25, 2022

That seems like a reasonable approach to me

Copy link

Contributor

marcelbuesing commentedNov 25, 2022

I just went through your implementation and it looks very good, would you maybe copy those to a PR for the linux-embedded-hal crate or should I do that? I think it's kind of bad timing right now because either you update the linux-embedded-hal crate to embedded-hal alpha.9 / master which will not work because there seem to be some stuff missing or you implement it for alpha.8 which will never become stable and already changed ;)... In the example I went with alpha.8 because it at least that way it compiles...

Copy link

ContributorAuthor

nnarain commentedNov 26, 2022

@marcelbuesing seems like you've already done the work, so perhaps you can put up a PR? Or is there still something you need to implement on your branch?

marcelbuesing mentioned this pull request

Nov 29, 2022

Standard/extended identifier implementation incorrect?#15

Closed

This was referencedNov 29, 2022

Ideas for v2.0#28

Closed

Add CAN FD support#16

Merged

socketcan2-rs#29

Closed

fpagliughi reviewed

Dec 3, 2022

View reviewed changes

Cargo.toml

		[dependencies]
		embedded-hal = "1.0.0-alpha.8"

Copy link

Collaborator

fpagliughiDec 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Their release appears to be a moving target. Thealpha.9 has breaking changes incompatible with this PR. If you want to keep this as-is, you would need to pin this exact version:

embedded-hal = "=1.0.0-alpha.8"

Copy link

Collaborator

fpagliughi commentedDec 3, 2022•
edited
Loading

@nnarain , this should be a quick/clean rebase over master right now. Can you do that and push it? That way I can get started on the review while we wait for the other stuff. Maybe even pull this in first, as the other fixes might be easy enough to merge on top of this.

nnarain added10 commits

December 3, 2022 16:13

Initial implementation with embedded-hal CAN traits

b923058

embedded-hal 1.0.0 alpha traits

0290584

Use correct write function for blocking

21d4606

Map some errors into CAN HAL errors

0fe7820

Added example

c92bff8

Move CanFrame to itself own module

481f7b4

Move CanSocket to its own module

19d6a8b

Fix some tests. Move integration tests to folder

a3891bc

Implement embedded-hal non-blocking trait

7861e34

Export CAN filter

ab89ebe

nnarain force-pushed theembedded-hal branch fromdc21dd3 toab89ebeCompare

December 3, 2022 16:14

fpagliughi requested changes

Dec 4, 2022

View reviewed changes

Copy link

Collaborator

fpagliughi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Overall this looks really good. A lot of updates I would have done anyway. If you don't have time to make the requested changes, let me know. I can accept this as-is and then make the mods along with a bunch of other cleanup.

src/constants.rs

		@@ -0,0 +1,55 @@
		//
		// constants.rs

Copy link

Collaborator

fpagliughiDec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

In general, I don't think a bunch of constants warrant their own module. These should probably stay inlib.rs.

But, for that matter, all of these constants and more are now available from thelibc crate. We should probably import and re-export all of the ones related to CAN, like:

// lib.rspub use libc::{    AF_CAN,    PF_CAN,    ...};

src/frame.rs


		impl Frame for CanFrame {
		/// Create a new frame
		fn new(id: impl Into<Id>, data: &[u8]) -> Option<Self> {

Copy link

Collaborator

fpagliughiDec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If this can fail with an error, it should return aResult<>, not anOption.

src/frame.rs

		impl Frame for CanFrame {
		/// Create a new frame
		fn new(id: impl Into<Id>, data: &[u8]) -> Option<Self> {
		let raw_id = hal_id_to_raw(id.into());

Copy link

Collaborator

fpagliughiDec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You don't appear to be properly handling when someone passes in anExtendedId with a value <= SFF_MASK.

That was a known bug with the previous implementation. See#15.

We should be able to pass a unit test like:

let frame = CanFrame::new(ExtendedId::new(0x50), &[]).unwrap();assert!(frame.is_extended());

src/socket.rs


		impl CanFilter {
		/// Construct a new CAN filter.
		pub fn new(id: u32, mask: u32) -> Result<CanFilter, ConstructionError> {

Copy link

Collaborator

fpagliughiDec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Shouldn't this now be defined with theStandardId andExtendedId types to be consistent? Or would that make things unnecessarily complicated?

Previously, I was thinking to add aFrom<(u32,u32)> (from a u32 tuple) for this to make it easier to create a list of filters. Maybe do both?

fpagliughi reviewed

Dec 4, 2022

View reviewed changes

src/lib.rs


		Ok(CanSocket { fd: sock_fd })
		}
		impl embedded_hal::can::nb::Can for CanSocket {

Copy link

Collaborator

fpagliughiDec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why are these sitting out here, rather than in thesocket module? Is this the stuff that would be moved upstream into the embedded-hal/can crate?

fpagliughi reviewed

Dec 4, 2022

View reviewed changes

src/frame.rs

		@@ -0,0 +1,163 @@
		use crate::err::{ConstructionError, CanError, CanErrorDecodingFailure};

Copy link

Collaborator

fpagliughiDec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Perhaps put a small doc comment at the start of each new module describing it?

fpagliughi reviewed

Dec 4, 2022

View reviewed changes

src/util.rs

		@@ -79,3 +79,10 @@ pub fn duration_from_timeval(ts: timespec) -> Duration {
		pub fn system_time_from_timespec(ts: timespec) -> SystemTime {
		UNIX_EPOCH + duration_from_timeval(ts)
		}

		pub fn hal_id_to_raw(id: Id) -> u32 {

Copy link

Collaborator

fpagliughiDec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I wonder if this isn't something we could push upstream to the embedded-hal/can crate?

impl Id {    pub fn as_raw(&self) -> u32 {        match self {            Id::Standard(id) => id.as_raw() as u32,            Id::Extended(id) => id.as_raw(),        }    }}

Copy link

Collaborator

fpagliughiDec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I threw a PR at them. Let's see how it goes.
rust-embedded/embedded-hal#428

fpagliughi added this to thev2.0 milestone

Dec 4, 2022

fpagliughi approved these changes

Dec 5, 2022

View reviewed changes

Copy link

Collaborator

fpagliughi commentedDec 5, 2022

@nnarain The other big PR for CAN FD is actually branched off of the old v1.x line. So I will drop this into master now and then start working to manually merge that one in. If you have time to answer some of my PR questions, that might help us clean up down the road. But master will be a WIP for a few weeks, so feel free to send in any small PR's to do some cleanup after it has landed.

fpagliughi merged commitd882020 intosocketcan-rs:master

Dec 5, 2022

Copy link

ContributorAuthor

nnarain commentedDec 6, 2022

I'll try to get to addressing the PR comments soon.

fpagliughi mentioned this pull request

Apr 1, 2023

2.0 ?#37

Closed

Labels

None yet

3 participants

Movatterモバイル変換

Embedded HAL Traits#24

Embedded HAL Traits#24

Uh oh!

Conversation

nnarain commentedJul 31, 2022

Uh oh!

fpagliughi commentedNov 23, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

nnarain commentedNov 23, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

marcelbuesing commentedNov 24, 2022

Uh oh!

nnarain commentedNov 25, 2022

Uh oh!

marcelbuesing commentedNov 25, 2022

Uh oh!

nnarain commentedNov 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fpagliughi commentedDec 3, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

fpagliughi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fpagliughi commentedDec 5, 2022

Uh oh!

nnarain commentedDec 6, 2022

Uh oh!

Uh oh!

fpagliughi commentedNov 23, 2022•
edited
Loading

nnarain commentedNov 23, 2022•
edited
Loading

fpagliughi commentedDec 3, 2022•
edited
Loading