Skip to content

CAN StandardId and ExtendedId: duplicating constant literals is not good practice #744

Description

@ocornu

This is how StandardId is currently implemented (ExtendedId is implemented the same way):

impl StandardId {
    /// CAN ID `0`, the highest priority.
    pub const ZERO: Self = Self(0);

    /// CAN ID `0x7FF`, the lowest priority.
    pub const MAX: Self = Self(0x7FF);

    /// Tries to create a `StandardId` from a raw 16-bit integer.
    ///
    /// This will return `None` if `raw` is out of range of an 11-bit integer (`> 0x7FF`).
    #[inline]
    #[must_use]
    pub const fn new(raw: u16) -> Option<Self> {
        if raw <= 0x7FF {
            Some(Self(raw))
        } else {
            None
        }
    }

Duplicating constant literals (0x7FF) is error-prone and not good practice overall.
Instead, it should be implemented somewhat like this:

impl StandardId {
    /// CAN Standard ID bit size
    const BITS: u8 = 11;
    /// Max 11-bit raw ID (`0x7FF`)
    const MAX_RAW_ID: u16 = (1u16 << BITS) - 1;

    /// Min CAN ID (highest priority).
    pub const ZERO: Self = Self(0);

    /// Max CAN ID (lowest Standard priority).
    pub const MAX: Self = Self(MAX_RAW_ID);

    /// Tries to create a `StandardId` from a raw 16-bit integer.
    ///
    /// This will return `None` if `raw` is out of range of an 11-bit integer (`> MAX_RAW_ID`).
    #[inline]
    #[must_use]
    pub const fn new(raw: u16) -> Option<Self> {
        if raw <= MAX_RAW_ID {
            Some(Self(raw))
        } else {
            None
        }
    }

Note: There is no real reason to keep MAX_RAW_ID private, except that it might saw confusion in users by colliding with MAX, seemingly the way we intend them to compare StandardIds with each other (rather than by raw ID proxy).

Optionally, if we elect to decouple BITS and MAX_RAW_ID as above, we can also make other bit-mangling methods in ExtendedId look less arbitrary. For example (assuming #742):

    /// 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() >> (ExtendedId::BITS - StandardID::BITS)) 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 << (ExtendedId::BITS - StandardID::BITS)) - 1) 
    }

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions