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

Open Beginner friendly
#742 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
75/100
Issue type
Refactor
Clarity
Clearly specified
Activity status
Quiet
Tech stack
rust
Domain
embedded-iot

Research direction

Locate the ExtendedId implementation and search for standard_id() references across the repository. Rename the method to base_id(), replace the incorrect bit-range comment, use as_raw() in the implementation, and update any affected references so the project still builds and tests pass.

Written by the indexing model from the issue text.

Description

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)
    }
Dominant language
Rust
Stars
2.7k
Forks
282
No merged PRs in 30d

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from rust-embedded/embedded-hal

All issues in rust-embedded/embedded-hal

Similar issues

More Rust issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.