CAN: `ExtendedId::standard_id()` name, comment and implementation

[ocornu]

The CAN ExtendId implementation provides the standard_id() method:

impl ExtendedId {
    ...
    /// Returns the Base ID part of this extended identifier.
    pub fn standard_id(&self) -> StandardId {
        // ID-28 to ID-18
        StandardId((self.0 >> 18) as u16)
    }
}

However, contrary to what its misleading name suggests, and despite the fact that it returns a StandardId, this method is not about creating a StandardId from an ExtendedId (converting an ExtendedId into a StandardId): as its documentation rightfully states, it's about extracting its Base ID.
Therefore, standard_id() should really be named base_id().

Additionally, the included comment is wrong (there are no such things as a CAN ID-28 and ID-18).

Lastly, as we have at our disposal the following const method (i.e. zero run-time overhead):

    /// Returns this CAN Identifier as a raw 32-bit integer.
    pub const fn as_raw(&self) -> u32 {
        self.0
    }

We should use it to make base_id() implementation-agnostic (rather than relying on self.0).

Altogether, it should look like this:

    /// Returns the Base ID part of this extended identifier.
    pub fn base_id(&self) -> StandardId {
        // Extract the 11 most significant bits from this 29-bit identifier
        StandardId((self.as_raw() >> 18) as u16)
    }

[ocornu]

On second thought, the only legitimate reason i can picture for using this standard_id() method is in the context of prioritizing CAN frames.
Please correct me if i'm wrong: in what other context would we "pretend" that an ExtendedId is a StandardId?

For example, that is the case in the Ord implementation for Id, which by the way is the only time this method is ever used internally (excluding its own unit-test, obviously):

impl Ord for Id {
    fn cmp(&self, other: &Self) -> core::cmp::Ordering {
        let split_id = |id: &Id| {
            let (standard_id_part, ide_bit, extended_id_part) = match id {
                Id::Standard(StandardId(x)) => (*x, 0, 0),
                Id::Extended(x) => (
                    x.standard_id().0,        // <= USED HERE
                    1,
                    x.0 & ((1 << 18) - 1),    // <= complementary bit-mangling here
                ),
            };
            (standard_id_part, ide_bit, extended_id_part)
        };

        split_id(self).cmp(&split_id(other))
    }
}

Therefore, it should not return a StandardId: the Base ID part of an ExtendedId is not a StandardId itself, rather it is a raw value (its 11 most significant bits) directly comparable to a StandardId's own (11-bit) raw value.

This becomes obvious when we consider implementing the complementary method ―which is lacking for some reason, forcing our above cmp() implementation to bit-mangle it on the spot.
In other word, the method returning the Extended ID part this time around: should it return an ExtendedId? No: it only returns the 18 least significant bits of its raw value, which is not an Id at all (standard or extended).

Considering all this:

• standard_id() should rather be named something like base_part() and its complement method extended_part() out of consistency
• still out of consistency, both of these methods should exist, or none: again, correct me if i'm wrong, but i can't picture a context where only one of them would make sense
• these two methods should return appropriately-sized raw values: u16 (because we want it to be directly comparable to StandardId::as_raw()) and u32 respectively
• they should both be const and inline(always) as this is compile-time work, and the only method they depend on, as_raw(), is itself const (and should probably be inline(always) as well).

For example:

    /// Returns the Base ID part of this extended identifier.
    #[inline(always)]
    pub const fn base_part(&self) -> u16 {
        // Extract the 11 most significant bits from this 29-bit identifier
        (self.as_raw() >> 18) as u16
    }

    /// Returns the Extended ID part of this extended identifier.
    #[inline(always)]
    pub const fn extended_part(&self) -> u32 {
        // Extract the 18 least significant bits from this 29-bit identifier
        self.as_raw() & ((1 << 18) - 1) 
    }

Lastly, they should both be put to work in the Ord implementation for Id:

impl Ord for Id {
    fn cmp(&self, other: &Self) -> core::cmp::Ordering {
        let split_id = |id: &Id| {
            match id {
                Id::Standard(x) => (x.as_raw(), 0, 0),
                Id::Extended(x) => (x.base_part(), 1, x.extended_part()),
            }
        };
        split_id(self).cmp(&split_id(other))
    }
}