Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Controller Area Network (CAN) Take 4 #314

Merged
merged 10 commits into from
Oct 28, 2021
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
## [Unreleased]

### Added
- Added `Can` Controller Area Network traits.
- `Error` traits for SPI, I2C and Serial traits. The error types used in those must
implement these `Error` traits, which implies providing a conversion to a common
set of error kinds. Generic drivers using these interfaces can then convert the errors
Expand Down
17 changes: 17 additions & 0 deletions src/can/blocking.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//! Blocking CAN API

/// A blocking CAN interface that is able to transmit and receive frames.
pub trait Can {
/// Associated frame type.
type Frame: crate::can::Frame;

/// Associated error type.
type Error: crate::can::Error;

/// Puts a frame in the transmit buffer. Blocks until space is available in
/// the transmit buffer.
fn transmit(&mut self, frame: &Self::Frame) -> Result<(), Self::Error>;

/// Blocks until a frame was received or an error occured.
fn receive(&mut self) -> Result<Self::Frame, Self::Error>;
}
103 changes: 103 additions & 0 deletions src/can/id.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
//! CAN Identifiers.

/// Standard 11-bit CAN Identifier (`0..=0x7FF`).
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub struct StandardId(u16);

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

/// Creates a new `StandardId` without checking if it is inside the valid range.
#[inline]
pub const fn new_unchecked(raw: u16) -> Self {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does new_unchecked() actually need to be unsafe, i.e. might any memory safety rely on it later?

I'd actually argue that yes, it must be an unsafe function. Implementations and users of the traits should be able to rely on StandardId/ExtendedId always containing a valid ID. In my opinion, that's the entire point of using such wrapper types in the first place (ref "Parse, don't validate"). "Relying on" should then mean that even writing unsafe code based on this assumption should be possible - without the need for additional validation of the passed IDs.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, for StandardId it is not marked as unsafe but for ExtendedId it is:

/// Creates a new `ExtendedId` without checking if it is inside the valid range.
#[inline]
pub const unsafe fn new_unchecked(raw: u32) -> Self {
Self(raw)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unsafe on ExtendedId was an oversight, pushed a fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO both perspectives on the unsafe are appropriate here. Are there any "unsafe API guidelines" which could help us find a decision?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning for functions needing to be unsafe is as follows: As soon as there exists a way to create an instance of StandardId containing an invalid ID from safe Rust, any possible downstream code which wants to rely on it containing a valid ID must add its own validation checks to ensure that it does. The is necessary for logic purposes and UB-purposes alike.

If, however, the API "contract" of the StandardId type is to only ever contain a valid ID and from safe Rust only abiding instances can be created, then downstream code can rely on this guarantee. Again, this is useful for logic reasons and UB avoidance alike.

As an example from the Rust stdlib, from_utf8_unchecked() is also unsafe. The reason is that some code handling strs downstream might want to optimize on the guarantee that str always contains valid UTF-8 data. Analogous, a CAN peripheral driver might want to optimize on the assumption that the u16 representing an ID will only ever have some of its lower 13 bits set. By keeping these functions safe, we're disallowing any such downstream code to be sound without additional code for validation.


Asking the other way around: What is gained from making these functions safe in the first place? For statically initializing a variable of type StandardId, it is irrelevant: The check in ::new() will be constant-folded away in probably all conceivable cases. The only place where ::new_unchecked() is really needed is when an ID is dynamically retrieved from elsewhere and you can guarantee that it has already been validated before. And even then, it is only relevant for edge-cases where the last bytes of memory and cycles of CPU time need to be optimized.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @Rahix. It seems *_unchecked() APIs are typically marked as unsafe in rust std lib and also for example in heapless.

Are those CAN traits going to be in v1.0?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am generally against marking functions as unsafe unless they actually lead to UB, and especially against doing it just because they seem like "scary" functions or some extra care (unrelated to UB) is required. In this case though I think @Rahix is right: the list of Rust UB explicitly includes "Producing an invalid value... The following values are invalid: Invalid values for a type with a custom definition of invalid values".

So, if we do reasonably define this type as only containing valid IDs, then it's right to say producing an invalid valid could cause UB, if some other code later depended on this property in order to be sound. I can't think of any such situations but I guess they could exist.

Copy link

@andresv andresv Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just remove new_unchecked() for now?

One downside is that using unwrap and expect introduces format bloat and makes binaries bigger and in this case there is also a little performance hit by always checking those values.

let canid = read_can();
let id = ExtendedId::new(canid).expect("always valid");
vs
let id = unsafe { ExtendedId::new_unchecked(canid) };

Self(raw)
}

/// Returns this CAN Identifier as a raw 16-bit integer.
#[inline]
pub fn as_raw(&self) -> u16 {
self.0
}
}

/// Extended 29-bit CAN Identifier (`0..=1FFF_FFFF`).
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub struct ExtendedId(u32);

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

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

/// Tries to create a `ExtendedId` from a raw 32-bit integer.
///
/// This will return `None` if `raw` is out of range of an 29-bit integer (`> 0x1FFF_FFFF`).
#[inline]
pub const fn new(raw: u32) -> Option<Self> {
if raw <= 0x1FFF_FFFF {
Some(Self(raw))
} else {
None
}
}

/// Creates a new `ExtendedId` without checking if it is inside the valid range.
#[inline]
pub const fn new_unchecked(raw: u32) -> Self {
Self(raw)
}

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

/// 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)
}
}

/// A CAN Identifier (standard or extended).
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub enum Id {
/// Standard 11-bit Identifier (`0..=0x7FF`).
Standard(StandardId),

/// Extended 29-bit Identifier (`0..=0x1FFF_FFFF`).
Extended(ExtendedId),
}

impl From<StandardId> for Id {
#[inline]
fn from(id: StandardId) -> Self {
Id::Standard(id)
}
}

impl From<ExtendedId> for Id {
#[inline]
fn from(id: ExtendedId) -> Self {
Id::Extended(id)
}
}
122 changes: 122 additions & 0 deletions src/can/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
//! Controller Area Network

pub mod blocking;
pub mod nb;

mod id;

pub use id::*;

/// A CAN2.0 Frame
pub trait Frame: Sized {
/// Creates a new frame.
/// Returns an error when the data slice is too long.
fn new(id: impl Into<Id>, data: &[u8]) -> Result<Self, ()>;

/// Creates a new remote frame (RTR bit set).
/// Returns an error when the data length code (DLC) is not valid.
fn new_remote(id: impl Into<Id>, dlc: usize) -> Result<Self, ()>;

/// Returns true if this frame is a extended frame.
fn is_extended(&self) -> bool;

/// Returns true if this frame is a standard frame.
fn is_standard(&self) -> bool {
!self.is_extended()
}

/// Returns true if this frame is a remote frame.
fn is_remote_frame(&self) -> bool;

/// Returns true if this frame is a data frame.
fn is_data_frame(&self) -> bool {
!self.is_remote_frame()
}

/// Returns the frame identifier.
fn id(&self) -> Id;

/// Returns the data length code (DLC) which is in the range 0..8.
///
/// For data frames the DLC value always matches the length of the data.
/// Remote frames do not carry any data, yet the DLC can be greater than 0.
fn dlc(&self) -> usize;

/// Returns the frame data (0..8 bytes in length).
fn data(&self) -> &[u8];
}

/// CAN error
pub trait Error: core::fmt::Debug {
/// Convert error to a generic CAN error kind
///
/// By using this method, CAN errors freely defined by HAL implementations
/// can be converted to a set of generic serial errors upon which generic
/// code can act.
fn kind(&self) -> ErrorKind;
}

/// CAN error kind
///
/// This represents a common set of CAN operation errors. HAL implementations are
/// free to define more specific or additional error types. However, by providing
/// a mapping to these common CAN errors, generic code can still react to them.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
#[non_exhaustive]
pub enum ErrorKind {
/// The peripheral receive buffer was overrun.
Overrun,

// MAC sublayer errors
/// A bit error is detected at that bit time when the bit value that is
/// monitored differs from the bit value sent.
Bit,

/// A stuff error is detected at the bit time of the sixth consecutive
/// equal bit level in a frame field that shall be coded by the method
/// of bit stuffing.
Stuff,

/// Calculated CRC sequence does not equal the received one.
Crc,

/// A form error shall be detected when a fixed-form bit field contains
/// one or more illegal bits.
Form,

/// An ACK error shall be detected by a transmitter whenever it does not
/// monitor a dominant bit during the ACK slot.
Acknowledge,

/// A different error occurred. The original error may contain more information.
Other,
}

impl Error for ErrorKind {
fn kind(&self) -> ErrorKind {
*self
}
}

impl core::fmt::Display for ErrorKind {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
match self {
Self::Overrun => write!(f, "The peripheral receive buffer was overrun"),
Self::Bit => write!(
f,
"Bit value that is monitored differs from the bit value sent"
),
Self::Stuff => write!(f, "Sixth consecutive equal bits detected"),
Self::Crc => write!(f, "Calculated CRC sequence does not equal the received one"),
Self::Form => write!(
f,
"A fixed-form bit field contains one or more illegal bits"
),
Self::Acknowledge => write!(f, "Transmitted frame was not acknowledged"),
Self::Other => write!(
f,
"A different error occurred. The original error may contain more information"
),
}
}
}
28 changes: 28 additions & 0 deletions src/can/nb.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
//! Non-blocking CAN API

/// A CAN interface that is able to transmit and receive frames.
pub trait Can {
/// Associated frame type.
type Frame: crate::can::Frame;

/// Associated error type.
type Error: crate::can::Error;

/// Puts a frame in the transmit buffer to be sent on the bus.
///
/// If the transmit buffer is full, this function will try to replace a pending
/// lower priority frame and return the frame that was replaced.
/// Returns `Err(WouldBlock)` if the transmit buffer is full and no frame can be
/// replaced.
///
/// # Notes for implementers
///
/// * Frames of equal identifier shall be transmited in FIFO fashion when more
/// than one transmit buffer is available.
/// * When replacing pending frames make sure the frame is not in the process of
/// being send to the bus.
fn transmit(&mut self, frame: &Self::Frame) -> nb::Result<Option<Self::Frame>, Self::Error>;

/// Returns a received frame if available.
fn receive(&mut self) -> nb::Result<Self::Frame, Self::Error>;
}
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,7 @@
pub mod fmt;
pub use nb;
pub mod adc;
pub mod can;
pub mod capture;
pub mod delay;
pub mod digital;
Expand Down