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

[WIP] CAN Interface take 2 #77

Closed
wants to merge 12 commits into from
Closed
190 changes: 190 additions & 0 deletions src/can.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
//! Controller Area Network

use nb;

/// A type that can either be `BaseId` or `ExtendedId`
#[cfg(feature = "unproven")]
pub trait Id {

Choose a reason for hiding this comment

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

I don't really get the value of not specifying concrete types (u16 and u32) for base and extended id.

How about adding a

fn id(&self) -> u32;

possibly even with a blanket implementation based on the base_id and extended_id fns, but I think right now there is no way to guarantee that not both are not None so this might need to return Result<u32, Error> which is kind of ugly.

/// The (11-bit) BaseId variant.
type BaseId;

/// The (29-bit) ExtendedId variant.
type ExtendedId;

/// Returns `Some(base_id)` if this Can-ID is 11-bit.
/// Returns `None` if this Can-ID is 29-bit.
fn base_id(&self) -> Option<Self::BaseId>;

/// Returns `Some(extended_id)` if this Can-ID is 29-bit.
/// Returns `None` if this Can-ID is 11-bit.
fn extended_id(&self) -> Option<Self::ExtendedId>;
}

/// A type that will either accept or filter a `Frame`.
/// The filtering is done solely on the `ID` of the `Frame`.
#[cfg(feature = "unproven")]
pub trait Filter {
/// The Id type this filter works on
type Id: Id;

/// Constructs a filter that only accepts `Frame`s with the provided identifier.
fn from_id(id: Self::Id) -> Self;

/// Constructs a filter that will accept any `Frame`.
fn accept_all() -> Self;

/// Create a `Filter` from a filter/mask combination.
///
/// - Bit 0..11 is used when matching against base id
/// - Bit 0..29 is used when matching against extended_id
/// - Bit 29 matches the extended frame flag (can be used for only matching against base/extended ids)
/// - Bit 30..32 *must* be `0`
///
/// *Note: When filtering base id any rule put on `bit_pos >= 11` will (for implementers: must) be ignored*
///
/// ### Panic
/// (for implementers: must) panic if mask have bits equal to `1` for bit_position `>= 30`.
fn from_mask(mask: u32, filter: u32) -> Self;
}


/// A Can Frame
#[cfg(feature = "unproven")]
pub trait Frame {
/// The Id type of this Frame
type Id: Id;
Copy link

Choose a reason for hiding this comment

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

I'm dubious of the value of having Frame type and the Id type be associated. It's very common for both Id types to be present on the bus at the same time.

Copy link
Author

Choose a reason for hiding this comment

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

This is not a problem as Id is defined as "A type that can either be BaseId or ExtendedId"

Copy link

Choose a reason for hiding this comment

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

I'm surely not understanding something then (please forgive me, I'm still pretty new to the language). For a bus that contains messages of both Id types, how many Frame types would you expect to come out of the driver?

Copy link
Author

Choose a reason for hiding this comment

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

Just one Frame type and one ID type. Some examples of how the Frame type could look (only one of them, is actually required):

pub enum Frame1 {
    BaseIdDataFrame(_),
    BaseIdRemoteFrame(_),
    ExtendedIdDataFrame(_),
    ExtendedIdRemoteFrame(_),
}

pub struct Frame2 {
    remote: bool,
    extended_id: bool,
    data: [u8; 8],
}

and the Id type could look like (only one of them, is actually required):

extern crate ux;
use ux::{u11, u29};

pub enum Id1 {
    Base(u11),
    Extended(u29),
}

pub struct Id2 {
    extended: bool,
    id: u32,
}

When implementing the trait each representation of a Frame would have to choose a representation of a nId.

Copy link

@dunmatt dunmatt Apr 21, 2018

Choose a reason for hiding this comment

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

So you would also have

impl BaseId for Id2 {...}
impl ExtendedId for Id2 {...}

? That is to say, you'd have Id2 be both Base and Extended?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, this will not work. I will need to remove the BaseId and ExtendedId and add approriate methods for distinguishing between ids.


/// 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;

/// Returns true if this `Frame` is a extended id frame
fn is_base_id_frame(&self) -> bool {
self.id().base_id().is_some()
}

/// Returns true if this `Frame` is a extended id frame
fn is_extended_id_frame(&self) -> bool {
self.id().extended_id().is_some()
}

/// Returns the Can-ID
fn id(&self) -> Self::Id;

/// Returns `Some(Data)` if data frame.
/// Returns `None` if remote frame.
Copy link

@dunmatt dunmatt Apr 21, 2018

Choose a reason for hiding this comment

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

This seems like a non-obvious way to encode remote frames. If I asked a random programmer "What would you guess is the difference between a CAN frame with None for the data, and a CAN frame with Some(empty_slice) as the data?" I'm skeptical they'd guess "Whether or not its a remote frame!"

Or to voice my concern from a different angle, Option means "may not exist", where as RTR means "I would like someone to do a thing", and those aren't well connected.

Copy link
Author

Choose a reason for hiding this comment

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

I agree. How can we change this to the better?

Returning a len 0 slice for a remote frame is worse i believe. At the same time we will need to support frame encodings where it's not statically known if they are remote or data frames (from the discussion in the last PR)

Would it be sufficient to introduce is_remote_frame(&self) -> bool and is_data_frame(&self) -> bool as well?

Copy link

@dunmatt dunmatt Apr 21, 2018

Choose a reason for hiding this comment

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

Why is a len 0 slice worse? On the wire the length field is indeed zero and zero bytes come before the checksum. It seems like a great modeling of what is actually happening on the bus. It seems like zero length slice is what Occam's Razor would call for

I'm not sure what you mean wrt not knowing if a frame is remote or data, it's a required bit, it will always be present and always be either 0 or 1...? What's a situation where you might not know?

But yes, is_remote_frame(&self) -> bool seems like a fine alternative to using Option here.

Copy link
Author

Choose a reason for hiding this comment

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

Why is a len 0 slice worse? On the wire the length field is indeed zero and zero bytes come before the checksum. It seems like a great modeling of what is actually happening on the bus. It seems like zero length slice is what Occam's Razor would call for

I feel like there is a conceptual difference between "this data frame didn't have any data" and "this frame was not a data frame". And more importantly, using the API must be easy. An interface where you first have to check if it's a data frame before reading the data is easier to mess up than an interface where you will not be able to get to the data unless it is a data frame.

I'm not sure what you mean wrt not knowing if a frame is remote or data, it's a required bit, it will always be present and always be either 0 or 1...? What's a situation where you might not know?

It's possible to use different types for Remote/data frames and know the difference between these types statically. Or use the same type for both and distinguish between them dynamically (using a variable). The Trait will need to support both.

Copy link

Choose a reason for hiding this comment

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

I think you'll find it challenging to come up with an API that is natural for both static and dynamic typing. Static typing would have it that the id types are different in some way, and dynamic would have it that everything from either id type is on both.

But I hear what you're saying about the conceptual difference. And you made a good point in the other PR, namely, that the compiler should enforce that RTR frames have no data. I would say, though, that this conceptual difference is a bit idealistic. Obviously it's best if invalid configurations can't be represented, but what if invalid configurations are what other devices on the bus are sending? Should the HAL prevent communication with such devices (that are, generally, not within the user's control, eg the ECUs in a car)?

Copy link
Author

Choose a reason for hiding this comment

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

Due to the nature of this crate is essential that it works for all reasonable representations of the can frame.

I would say, though, that this conceptual difference is a bit idealistic.

Sure, but I would also argue that it's an idiomatic use of Rusts typesystem.

Obviously it's best if invalid configurations can't be represented, but what if invalid configurations are what other devices on the bus are sending? Should the HAL prevent communication with such devices (that are, generally, not within the user's control, eg the ECUs in a car)?

The DLC field in a remote frame doesn't mean "how long is the data field for this message" it means "How much data do i want to receive". Therefore the CAN controller would never be able to assemble such frame and this would never be an issue. But even if it were, I would almost always go for the fail fast and loudly approach and not accept a malformed Frame.

Copy link

@jdemilledt jdemilledt Mar 9, 2019

Choose a reason for hiding this comment

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

enum /* (I don't know what this should be named)) */ {
    Remote,
    Data(&[u8])
}

This would be an idiomatic solution that clearly represents what this frame is.

fn data(&self) -> Option<&[u8]>;
}

/// A Can-FD Frame
///
/// A "ordinary" Can-Frame must also be representable by this type.
#[cfg(feature = "unproven")]
pub trait FdFrame {
/// The Id type of this Frame
type Id: Id;

/// Returns true if this frame would/has be(en) transmitted as a Can-Fd frame.
/// Returns false if this frame would/has be(en) transmitted as a "ordinary" Can frame.
fn is_fd_frame(&self) -> bool;

/// 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;

/// Returns true if this `Frame` is a extended id frame
fn is_base_id_frame(&self) -> bool {
self.id().base_id().is_some()
}

/// Returns true if this `Frame` is a extended id frame
fn is_extended_id_frame(&self) -> bool {
self.id().extended_id().is_some()
}

/// Returns the Can-ID
fn id(&self) -> Self::Id;

/// Returns `Some(Data)` if data frame.
/// Returns `None` if remote frame.
fn data(&self) -> Option<&[u8]>;
}


/// A CAN interface
///
/// May be a `Transmitter`, `Receiver` or both.
#[cfg(feature = "unproven")]
pub trait Interface {
/// The Id type that works with this `Interface`
type Id: Id;

/// The Can Frame this Interface operates on
type Frame: Frame<Id = Self::Id>;

/// The Interface Error type
type Error;

/// The Filter type used in this `Interface`
type Filter: Filter<Id = Self::Id>;
}

/// A CAN interface that is able to transmit frames.
#[cfg(feature = "unproven")]
pub trait Transmitter: Interface {
/// Put a `Frame` in the transmit buffer (or a free mailbox).
///
/// If the buffer is full, this function will try to replace a lower priority `Frame`
/// and return it. This is to avoid the priority inversion problem.
fn transmit(&mut self, frame: &Self::Frame) -> nb::Result<Option<Self::Frame>, Self::Error>;
}

/// A CAN interface that is able to receive frames.
#[cfg(feature = "unproven")]
pub trait Receiver: Interface {
/// Return the available `Frame` with the highest priority (lowest ID).
///
/// NOTE: Can-FD Frames will not be received using this function.
fn receive(&mut self) -> nb::Result<Self::Frame, Self::Error>;

/// Set the can controller in a mode where it only accept frames matching the given filter.
///
/// If there exists several receive buffers, this filter will be applied for all of them.
///
/// *Note: Even after this method has been called, there may still be `Frame`s in the receive buffer with
/// identifiers that would not been received with this `Filter`.*
fn set_filter(&mut self, filter: Self::Filter);

/// Set the can controller in a mode where it will accept all frames.
fn clear_filter(&mut self);
}

/// A CAN interface also supporting Can-FD
///
/// May be a `FdTransmitter`, `FdReceiver` or both.
#[cfg(feature = "unproven")]
pub trait FdInterface: Interface {
/// The Can Frame this Interface operates on
type FdFrame: FdFrame;
}

/// A CAN-FD interface that is able to transmit frames.
#[cfg(feature = "unproven")]
pub trait FdTransmitter: FdInterface + Receiver {
/// Put a `FdFrame` in the transmit buffer (or a free mailbox).
///
/// If the buffer is full, this function will try to replace a lower priority `FdFrame`
/// and return it. This is to avoid the priority inversion problem.
fn transmit(&mut self, frame: &Self::FdFrame) -> nb::Result<Option<Self::FdFrame>, Self::Error>;
}

/// A CAN-FD interface that is able to receive frames.
#[cfg(feature = "unproven")]
pub trait FdReceiver: FdInterface + Transmitter {
/// Read the available `FdFrame` with the highest priority (lowest ID).
fn receive(&mut self) -> nb::Result<Self::FdFrame, Self::Error>;
}
2 changes: 2 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,8 @@ pub mod prelude;
pub mod serial;
pub mod spi;
pub mod timer;
#[cfg(feature = "unproven")]
pub mod can;

/// Input capture
///
Expand Down