Added impl to Id make it more directly usable.

[fpagliughi]

This adds an impl block to the Id enum which would make it a little easier to use in practical settings. Instead of:

let id = Id::Standard(StandardId::new(0x110)?);

let raw = match id {
    Id::Standard(id) => id.as_raw() as u32,
    Id::Extended(id) => id.as_raw(),
};

you can do:

let id = Id::new_standard_id(0x110)?;
let raw = id.as_raw();

Also added MAX_RAW to StandardId and ExtendedId.

[rust-highfive]

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.

[eldruin]

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 ?

[fpagliughi]

Yeah, we're having a similar discussion about implementation of CanFrame and CanFdFrame in the socketcan crate. We want to keep them separate, but some common usage.

What I'm not sure about with Rust is whether adding a trait to a pure data type (that is/could be Copy) is zero-overhead at runtime? Does it introduce a dyn component or is it a purely static thing?

[timokroeger]

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.
Usually when standard and extended IDs are mixed on the same bus their messages are handled by unrelated subsystems.
Comparison and ordering is also not as you would naively expect (see #384) and always casting the ID to an u32 would erase those properties.

[fpagliughi]

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.

[eldruin]

What I'm not sure about with Rust is whether adding a trait to a pure data type (that is/could be Copy) is zero-overhead at runtime? Does it introduce a dyn component or is it a purely static thing?

Traits can be purely static. The whole generic dimension can be compiled away. See here for a nice overview

[fpagliughi]

Hey! Apologies, when I said:

OK. Let me take a day or two to play around with this using a trait, and I'll update the PR.

...what I really meant was a month or two, or a year or two!

@timokroeger, I see the concern about Id::as_raw() being misused for improper comparisons. I hadn't thought of that. My reason for wanting it was for use with Linux SocketCAN. In that API, the ID type, canid_t, is a 32-bit word with up to 29-bits for the raw ID and the upper three bits used for flags. One of them used to indicate that it's an extended ID.

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 as_raw() is really convenient. But I suppose that's pretty specific to SocketCAN, and I can work around it.

I'll remove it from the PR.

[fpagliughi]

@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):
https://users.rust-lang.org/t/representing-small-network-frames/91569

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 as_raw() function, what would the trait be other that a query for the type of ID?

pub trait CanId {
    fn is_extended(&self) -> bool;
    fn is_standard(&self) -> bool { !self.is_extended() }
}

impl CanId for StandardId {
    fn is_extended(&self) { false }
}

impl CanId for ExtendedId {
    fn is_extended(&self) { true }
}

impl CanId for Id {
    fn is_extended(&self) { matches!(self, Self::Extended(_)) }
}

Simple enough. But is it worth it for just this?

[eldruin]

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?

[reitermarkus]

I'd keep MAX_RAW private and make as_raw const instead.