-
Notifications
You must be signed in to change notification settings - Fork 65
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
Embedded HAL Traits #24
Conversation
Hi @nnarain! I have a bunch of questions about this:
|
|
I think regarding 2b. the alternative would implementing the can traits in the linux-embedded-hal crate and reference the socketcan crate for that. I just did that for the current socketcan master branch here in a prototype. It's basically just a wrapper around socketcan, 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 have moved 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. |
That seems like a reasonable approach to me |
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... |
@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? |
[dependencies] | ||
embedded-hal = "1.0.0-alpha.8" |
There was a problem hiding this comment.
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. The alpha.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"
@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. |
There was a problem hiding this 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.
@@ -0,0 +1,55 @@ | |||
// | |||
// constants.rs |
There was a problem hiding this comment.
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 in lib.rs
.
But, for that matter, all of these constants and more are now available from the libc
crate. We should probably import and re-export all of the ones related to CAN, like:
// lib.rs
pub use libc::{
AF_CAN,
PF_CAN,
...
};
|
||
impl Frame for CanFrame { | ||
/// Create a new frame | ||
fn new(id: impl Into<Id>, data: &[u8]) -> Option<Self> { |
There was a problem hiding this comment.
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 a Result<>
, not an Option
.
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()); |
There was a problem hiding this comment.
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 an ExtendedId
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());
|
||
impl CanFilter { | ||
/// Construct a new CAN filter. | ||
pub fn new(id: u32, mask: u32) -> Result<CanFilter, ConstructionError> { |
There was a problem hiding this comment.
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 the StandardId
and ExtendedId
types to be consistent? Or would that make things unnecessarily complicated?
Previously, I was thinking to add a From<(u32,u32)>
(from a u32 tuple) for this to make it easier to create a list of filters. Maybe do both?
|
||
Ok(CanSocket { fd: sock_fd }) | ||
} | ||
impl embedded_hal::can::nb::Can for CanSocket { |
There was a problem hiding this comment.
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 the socket
module? Is this the stuff that would be moved upstream into the embedded-hal/can crate?
@@ -0,0 +1,163 @@ | |||
use crate::err::{ConstructionError, CanError, CanErrorDecodingFailure}; |
There was a problem hiding this comment.
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?
@@ -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 { |
There was a problem hiding this comment.
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(),
}
}
}
There was a problem hiding this comment.
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
@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. |
I'll try to get to addressing the PR comments soon. |
This MR builds off of @fpagliughi 's netlink-update branch (#20).
I have implemented the following:
tests/
folderCould probably use some advice on the best way to map the errors.