Skip to content

CAN: returning Result instead of Option #741

Description

@ocornu

Please correct me if there is a bigger picture that i'm missing…

There seem to be a common habit all through-out embedded-can of returning an Option instead of a Result.
For example:

impl StandardId {
    ...
    /// 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`).
    pub const fn new(raw: u16) -> Option<Self> {
        if raw <= 0x7FF {
            Some(Self(raw))
        } else {
            None
        }
    }

Or:

pub trait Frame: Sized {
    /// Creates a new frame.
    ///
    /// This will return `None` if the data slice is too long.
    fn new(id: impl Into<Id>, data: &[u8]) -> Option<Self>;

In both cases, when:

  • creating a StandardId that is more than 11-bit long, or
  • creating a frame with a data payload bigger than 8 bytes

None is returned.
However, this is not like, e.g., searching an item in a collection where it's not, thus None is rightfully returned. In both cases user-code is attempting to use illegal values, which is a logic error: therefore, an explicit Error should be returned instead.

So far it kinda works, because each such method only has one reason to fail (and return None): thus the error case is implicit, but remains "deterministic". So this may all sound like a pedantic distinction…

However, consider the following:

/// Create a new Standard Format data frame
pub fn new_standard(raw_id: u16, data: &[u8]) -> Option<Frame> {
    let id = StandardId::new(raw_id)?;
    Frame::new(id, data)?
}

Returning None is no longer an option (pun intended), as we can (pun intended) no longer distinguish between error cases. We're going to have to:

  • use a Result
  • create two custom Errors
  • wrap each None fail-case to return the appropriate one.

It's ugly, verbose, and over-specific in that it forces us to do boilerplate work that really is embedded-can's job, and create custom errors (e.g. IllegalId and DataLengthTooLong(u8)) that really are embedded-can's methods errors…

Therefore i'm suggesting that:

  • all Option-returning methods return a Result instead
  • corresponding errors be added to ErrorKind

Thank you!

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions