-
Notifications
You must be signed in to change notification settings - Fork 206
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
Added impl to Id make it more directly usable. #428
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eldruin (or someone else) soon. Please see the contribution instructions for more information. |
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.
Thanks for this PR!
This looks ok to me by itself but it seems a bit unidiomatic.
The whole StandardId
+ ExtendedId
+ Id::Standard(_) | Id::Extended(_)
and especially the matches added in the methods in this PR scream trait
to me. I did not think about an alternative, though.
Thoughts? @timokroeger, @adamgreig ?
Yeah, we're having a similar discussion about implementation of What I'm not sure about with Rust is whether adding a trait to a pure data type (that is/could be |
Overall I like the convenience this PR brings especially the constructors because their method name makes it explicit which type of ID we are dealing with. let raw = match id {
Id::Standard(id) => id.as_raw() as u32,
Id::Extended(id) => id.as_raw(),
}; The match pattern was intended because it makes it hard to accidentally mix standard and extended IDs. |
OK. Let me take a day or two to play around with this using a trait, and I'll update the PR. We're knee deep adding embedded hal/can support to the Linux SocketCAN crate, and it definitely looks like making this a little easier to use would be really helpful. |
Traits can be purely static. The whole generic dimension can be compiled away. See here for a nice overview |
Hey! Apologies, when I said:
...what I really meant was a month or two, or a year or two! @timokroeger, I see the concern about So either way, when forming the frame, you need to convert the ID to a 32-bit int and mask in the additional bits, if necessary. The I'll remove it from the PR. |
@eldruin: As for the idea of using a trait, I've been thinking about the pattern. Suppose you have an enum, and every variant in it implements a trait. Then couldn't/shouldn't the enum also implement the trait?!? The enum would just pass the behavior to the specific variant. I was wondering if this would be idiomatic, so asked on the Rust forum. (I was thinking also about doing this for the frames): It seems that there is precedence for this, and the enum_dispatch crate exists to automate it efficiently. With +100 dependent crates and +4M downloads, it sounds like it's an accepted pattern. But if I'm going to remove the
Simple enough. But is it worth it for just 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.
Sorry it took me a while to come back to this.
The code as-is looks fine to me.
I agree a trait like that for this would be overkill so I am fine with merging.
@fpagliughi Has anything changed in the mean time or should we go ahead with the merge?
embedded-can/src/id.rs
Outdated
pub const MAX: Self = Self(Self::MAX_RAW); | ||
|
||
/// Raw CAN ID `0x1FFFFFFF`, the lowest priority. | ||
pub const MAX_RAW: u32 = 0x1FFF_FFFF; |
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'd keep MAX_RAW
private and make as_raw
const
instead.
Added MAX_RAW to StandardId and ExtendedId (private).
Hey All. Again, sorry for the long delays on this. But I cleaned up the requested changes, then squashed and rebased. It's ready to merge, unless there's anything else to look at. Meanwhile, over in the SocketCAN crate, I've continued experimenting with ways to make Id's easy to use. One reason I wanted the generic A thought was rather than going in and out of raw values, instead, implement math operators, Thoughts? |
This adds an impl block to the
Id
enum which would make it a little easier to use in practical settings. Instead of:you can do:
Also added MAX_RAW to StandardId and ExtendedId.