[WIP] CAN Interface take 2 #77

kjetilkjeka · 2018-04-19T17:55:41Z

#21 and #53 has been quiet for a while, the natural step was to try again based on the feedback given in that discussion. I've not implemented the Traits yet, as i wanted to get some quick feedback before i bothered.

@japaric @therealprof is this something close to what you're comfortable merging?

@dunmatt @tib888 t can this work for you?

The thing i dislike the most with the current state of this interface is that there is no function to return both CanFd and Can frames. This might be a source of a few different bugs.

therealprof · 2018-04-19T18:04:13Z

@kjetilkjeka I'd need to check that in detail but on a first glance it looks good to me. I'm not sure I do understand the purpose (and quite different signature) of transmit_without_priority(), though.

kjetilkjeka · 2018-04-19T19:27:41Z

I'm not sure I do understand the purpose (and quite different signature) of transmit_without_priority(), though

Can is designed with RT systems in mind. This means that there is some priority of frames involved. This means that implementing a Can driver is not straight forward, the naive implementation will most likely include a potential unbounded priority inversion.

The idea is that transmit and receive will avoid this issue, while for systems where this isn't an issue (and you don't want the complexity of swapping frames) there also exists alternative functions. I see now that transmit_without_priority() is a terrible name, so this will need to be changed either way (there should perhaps also be a receive equivalent?). Some alternative ways to handle this.

Rename transmit_without_priority() to something sensible.
Remove transmit_without_priority() and force everyone to use the priority inversion free alternative.
Include a flag in the transmit function instead. I don't like this as it should be harder to use the error prone alternative (longer function name).

therealprof · 2018-04-19T19:49:26Z

I only see three minor issues with those methods:

The name is weird
The description doesn't properly explain when and how to use it
By putting it in the trait you're forcing every implementor to provide implementations for those methods and I have no idea how often they would be used and whether they usually can be sensibly implemented

If they're "special" I'd prefer having them separately in an extra- or super-trait.

therealprof · 2018-04-20T09:16:30Z

@kjetilkjeka Looks good to me.

I'd like to hear from a user (e.g. @dunmatt @tib888) whether that could work for them.

kjetilkjeka · 2018-04-20T09:18:32Z

I fixed my own concern by letting FdInterface return all types of frame.

Instead of having a second weird transmit function i included the transmit_buffer_full that can be checked before attempting to transmit.

I'll leave it for a day or two to let other users express concerns, after that I will make a test implementation.

dunmatt

Overall I'm not convinced, I think the associated types are taking it a bit far just to avoid having an "id type" field broken out.

dunmatt · 2018-04-21T15:18:42Z

src/can.rs

+    type Id: Id + From<Self> + TryInto<Self>;
+}
+
+/// A Can Extended (28-bit) ID


Minor nit: Should say "29-bit"

dunmatt · 2018-04-21T15:22:37Z

src/can.rs

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


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.

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

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?

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.

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?

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

dunmatt · 2018-04-21T15:25:29Z

src/can.rs

+#[cfg(feature = "unproven")]
+pub trait Interface {
+    /// The Can Frame this Interface operates on
+    type Frame: Frame;


Doesn't this associated type mean that an interface only works with one frame type? If so, and the frame type is associated with the Id type, you'd need two different interfaces for the same hardware in the event of a bus that contains both id types. That doesn't strike me as a good design.

Examples of Frame implementations follows:

pub enum Frame1 { BaseIdDataFrame(_), BaseIdRemoteFrame(_), ExtendedIdDataFrame(_), ExtendedIdRemoteFrame(_), } pub struct Frame2 { remote: bool, extended_id: bool, data: [u8; 8], }

So the same interface can be used for extended id and base id

In Frame2, what would be the type of the id?

Probably something close to:

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

dunmatt · 2018-04-21T15:32:56Z

src/can.rs

+    fn id(&self) -> Self::Id;
+
+    /// Returns `Some(Data)` if data frame.
+    /// Returns `None` if remote frame.


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.

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?

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.

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.

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)?

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.

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.

dunmatt · 2018-04-21T19:41:19Z

It's essential that it works for all reasonable uses of CAN, it's not clear that it needs to be representation agnostic, another way to support all reasonable uses of the hardware is to specify a representation that will always work. This is, for example, how the ChibiOS HAL works. I get that that's how dlc works, but since this is a HAL we can't assume the hardware will filter bad frames. I agree with you that failing fast and loud is usually the right call in these cases, but I'd argue that failing in that way is the job of the driver rather than the HAL, that the HAL should not contain any policy decisions. There are legitimate uses for drivers that don't fail in that case (eg, can pen testing, and interacting with poorly implemented devices (both of which I do at work))

…

On Apr 21, 2018 2:01 PM, "Kjetil Kjeka" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/can.rs <#77 (comment)>: > +pub trait ExtendedId: Sized { + /// A generic ID type that encapsulate this type + type Id: Id + From<Self> + TryInto<Self>; +} + +/// A Can Frame +#[cfg(feature = "unproven")] +pub trait Frame { + /// The Id type of this Frame + type Id: Id; + + /// Returns the Can-ID + fn id(&self) -> Self::Id; + + /// Returns `Some(Data)` if data frame. + /// Returns `None` if remote frame. 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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#77 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDfy6bzvlcf82m0LawPeVqYmMzRch8zks5tq3P9gaJpZM4TcOxW> .

kjetilkjeka · 2018-04-22T10:21:55Z

@dunmatt I've addressed your concerns about the difficulties of use due to associated types in the latest commit. Remember that things like id(&self) -> u32 is possible to impl on the struct without requiring every implementation of a CanFrame to contain one.

I get that that's how dlc works, but since this is a HAL we can't assume the hardware will filter bad frames. I agree with you that failing fast and loud is usually the right call in these cases, but I'd argue that failing in that way is the job of the driver rather than the HAL, that the HAL should not contain any policy decisions. There are legitimate uses for drivers that don't fail in that case (eg, can pen testing, and interacting with poorly implemented devices (both of which I do at work))

I think the policy from the maintainers of this hal has been that as long as not every implementation contains something, we should leave it out. Parsing malformed frames is not something every controller will do and we should therefore avoid supporting it (at all) in this trait. If this is something you rely on you should probably use some functions from the driver instead from the hal. I guess the hal must contain some policy decision if it is to behave approximately the same for all drivers, for instance return nb::WouldBlock instead of spin locking etc. Perhaps @therealprof can weigh in on this?

dunmatt · 2018-04-22T13:49:04Z

If this is something you rely on you should probably use some functions from the driver instead from the hal.

That seems to suggest that you believe that the drivers are providers of the HAL rather than consumers of the HAL. I assert it's the other way around, that the reason to have a HAL in the first place is to be consumed by drivers. My whole objection is that drivers such as I need cannot be written to consume the HAL you're proposing (which is fine, you need not cater to me, I'm an unusual case and I'm perfectly happy scratching this itch for myself).

kjetilkjeka · 2018-04-22T14:38:06Z

The device drivers are consumers of the HAL while HW drivers are "producers". The HW drivers will also implement things that are not included in the HAL, and is possible to call directly into from the device drivers.

Is it the assumption of no data in remote frames that doesn't work for you? Perhaps you can give a concrete example of when this happens so it's easier for me to understand the need?

dunmatt · 2018-04-22T15:12:38Z

Ah, I see what you're saying, what you're calling HW drivers I had been calling HAL implementations, but you're right that they need not be the same. That said, the functionality I'm talking about is send and receive, if I have to bypass the HAL in order to send and receive there is really no point depending on the HAL.

Sure, my exact use case is this: the control and security of autonomous cars. There are computers from a half dozen or so different vendors on the bus, of which some we have executive control, some we have a little control, and the remainder are black boxes (literally, in one case). Moreover for business reasons we don't want to marry ourselves to our current vehicle platform, so there's a level of uncertainty even in terms of the devices on the network (ie I can't assume that all devices I'll ever care about are standards compliant, but I can assume that if one isn't I won't be able to fix it).

So given that I'm trying to control and secure black boxes, how would I do it? Setting aside control, in security applications you always assume malicious input, so my software needs to be able to detect (receive) malicious input and respond to it.

The assumption that only valid data happens on the CAN bus is one problem I have. Another is that my design intuitions skew more towards KISS than "make it impossible to fuck up" (but you're right that the latter is more idiomatic). Another is that enforcing policies isn't the job of an abstraction layer.

kjetilkjeka · 2018-04-22T15:58:39Z

Sure, my exact use case is this: the control and security of autonomous cars. There are computers from a half dozen or so different vendors on the bus, of which some we have executive control, some we have a little control, and the remainder are black boxes (literally, in one case). Moreover for business reasons we don't want to marry ourselves to our current vehicle platform, so there's a level of uncertainty even in terms of the devices on the network (ie I can't assume that all devices I'll ever care about are standards compliant, but I can assume that if one isn't I won't be able to fix it).

In such network (with a lot of different can controllers) wouldn't at least some of them be standards compliant and inject Error frames? Wouldn't the node sending malformed messages go into bus-off state pretty quick? Have you encountered any concrete devices that behaves this way?

So given that I'm trying to control and secure black boxes, how would I do it? Setting aside control, in security applications you always assume malicious input, so my software needs to be able to detect (receive) malicious input and respond to it.

Putting a black box node on a cars can network sounds to me like installing software you don't know what do on your servers.

With CAN my solution would be:

Only allow standard compliant devices on the bus. CAN is quite robust in itself.
Keep malicious nodes physically away from the bus. Require strict specifications of every node.
Be vary of anything that can proxy stuff onto your can bus.
Use a redundant interface for mission critical nodes, and only keep mission critical nodes on the second CAN-bus. This way if a malicious node messes up your primary can bus you can still provide some fail safes.

dunmatt · 2018-04-22T16:28:30Z

Wouldn't the node sending malformed messages go into bus-off state pretty

quick? Have you encountered any concrete devices that behaves this way? Not if they're malicious they wouldn't, and I'm building one, respectively.

Putting a black box node on a cars can network sounds to me like

installing software you don't know what do on your servers. Quite right, it's a wonder anyone deploys Window or Linux servers without first auditing the whole thing for themselves. By which I mean, the black boxes come with the cars when we buy them, and for the sake of getting stuff done we do have to trust someone sometime... Trust but verify. Believe me, if we could get the auto makers to give us their source we'd do it.

…

On Apr 22, 2018 11:58 AM, "Kjetil Kjeka" ***@***.***> wrote: Sure, my exact use case is this: the control and security of autonomous cars. There are computers from a half dozen or so different vendors on the bus, of which some we have executive control, some we have a little control, and the remainder are black boxes (literally, in one case). Moreover for business reasons we don't want to marry ourselves to our current vehicle platform, so there's a level of uncertainty even in terms of the devices on the network (ie I can't assume that all devices I'll ever care about are standards compliant, but I can assume that if one isn't I won't be able to fix it). In such network (with a lot of different can controllers) wouldn't at least some of them be standards compliant and inject Error frames? Wouldn't the node sending malformed messages go into bus-off state pretty quick? Have you encountered any concrete devices that behaves this way? So given that I'm trying to control and secure black boxes, how would I do it? Setting aside control, in security applications you always assume malicious input, so my software needs to be able to detect (receive) malicious input and respond to it. Putting a black box node on a cars can network sounds to me like installing software you don't know what do on your servers. With CAN my solution would be: 1. Only allow standard compliant devices on the bus. CAN is quite robust in itself. 2. Keep malicious nodes physically away from the bus. Require strict specifications of every node. 3. Be vary of anything that can proxy stuff onto your can bus. 4. Use a redundant interface for mission critical nodes, and only keep mission critical nodes on the second CAN-bus. This way if a malicious node messes up your primary can bus you can still provide some fail safes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#77 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDfy1D93u-pNhSXTW8-CiBXJ5Nq0sNGks5trKiwgaJpZM4TcOxW> .

therealprof · 2018-04-22T16:38:13Z

@dunmatt Indeed the assumption for the HAL is to make it easy to write well-behaved applications across different hardware. If that means that special applications like pen testing are not possible then it is what it is.

My question to you would be why you want to use an HAL anyway? For such uses you'd typically want to use both the most capable hardware with the lowest level access possible. A HAL will never give you that kind of access... Even the existence of a HAL implementation doesn't necessarily mean that you have to use it, you can still directly access the hardware instead.

dunmatt · 2018-04-22T16:40:22Z

Actually I might have missed your point, you were saying that a node I might want to interact with should go to bus off. I agree that it should, but I don't have enough faith in the quality of automotive software in general to assume that even the bus off part of the spec will be correctly implemented. Even if I had proof that our current cars are standards compliant I'd be wary of committing to being incompatible with non standards compliant devices

…

On Apr 22, 2018 12:28 PM, "M@" ***@***.***> wrote: > Wouldn't the node sending malformed messages go into bus-off state pretty quick? Have you encountered any concrete devices that behaves this way? Not if they're malicious they wouldn't, and I'm building one, respectively. > Putting a black box node on a cars can network sounds to me like installing software you don't know what do on your servers. Quite right, it's a wonder anyone deploys Window or Linux servers without first auditing the whole thing for themselves. By which I mean, the black boxes come with the cars when we buy them, and for the sake of getting stuff done we do have to trust someone sometime... Trust but verify. Believe me, if we could get the auto makers to give us their source we'd do it. On Apr 22, 2018 11:58 AM, "Kjetil Kjeka" ***@***.***> wrote: > Sure, my exact use case is this: the control and security of autonomous > cars. There are computers from a half dozen or so different vendors on the > bus, of which some we have executive control, some we have a little > control, and the remainder are black boxes (literally, in one case). > Moreover for business reasons we don't want to marry ourselves to our > current vehicle platform, so there's a level of uncertainty even in terms > of the devices on the network (ie I can't assume that all devices I'll ever > care about are standards compliant, but I can assume that if one isn't I > won't be able to fix it). > > In such network (with a lot of different can controllers) wouldn't at > least some of them be standards compliant and inject Error frames? Wouldn't > the node sending malformed messages go into bus-off state pretty quick? > Have you encountered any concrete devices that behaves this way? > > So given that I'm trying to control and secure black boxes, how would I > do it? Setting aside control, in security applications you always assume > malicious input, so my software needs to be able to detect (receive) > malicious input and respond to it. > > Putting a black box node on a cars can network sounds to me like > installing software you don't know what do on your servers. > > With CAN my solution would be: > > 1. Only allow standard compliant devices on the bus. CAN is quite > robust in itself. > 2. Keep malicious nodes physically away from the bus. Require strict > specifications of every node. > 3. Be vary of anything that can proxy stuff onto your can bus. > 4. Use a redundant interface for mission critical nodes, and only > keep mission critical nodes on the second CAN-bus. This way if a malicious > node messes up your primary can bus you can still provide some fail safes. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#77 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABDfy1D93u-pNhSXTW8-CiBXJ5Nq0sNGks5trKiwgaJpZM4TcOxW> > . >

dunmatt · 2018-04-22T16:47:02Z

@daniel, I completely agree, the original question was if this would work for my use case, and the answer is: no and that's fine. I really don't mind scratching my own itch. As for why I'd want a hal, it's so that if later I find the can peripheral too limiting I can swap in a bit banged implementation without rewriting everything

…

On Apr 22, 2018 12:38 PM, "Daniel Egger" ***@***.***> wrote: @dunmatt <https://github.com/dunmatt> Indeed the assumption for the HAL is to make it easy to write well-behaved applications across different hardware. If that means that special applications like pen testing are not possible then it is what it is. My question to you would be why you want to use an HAL anyway? For such uses you'd typically want to use both the most capable hardware with the lowest level access possible. A HAL will never give you that kind of access... Even the existence of a HAL implementation doesn't necessarily mean that you have to use it, you can still directly access the hardware instead. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#77 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDfywp0krMyugG_XZQa1PKRAHh_-vAIks5trLH2gaJpZM4TcOxW> .

kjetilkjeka · 2018-04-22T17:44:30Z

I guess that means we're in agreement. Thanks for giving input even though it wont be all usable in your case @dunmatt . I'll give @tib888 a day or two to addresd concerns before I implement it

tib888 · 2018-04-22T20:56:30Z

Unfortunately nowadays I do not have much time to deal with this, but please find here a few things popped in my mind reading the proposed code:

I see that filtering is not addressed here (yet?), but from my experience the RTR should be part of the ID, since frame filtering involves the RTR bit also. I may be wrong, but please consider this.

I think this below will not work if you really want a non blocking implementation for more than one transmit mailbox, because it can not trace the state/progress of the transfer:
fn transmit(&mut self, frame: &Self::Frame) -> nb::Result<Option<Self::Frame>, Self::Error>;

You may have a function, which puts a frame in a transmit mailbox and returns a handle to the chosen mailbox, like this:
fn start_transmit(&mut self, id: &Id, data: &[u8]) -> Result<TxMailBoxHandle, Self::Error>;

Then you will have to pull that handle to competition with an other function.
fn wait_transmitting_of(&mut self, handle: TxMailBoxHandle) -> nb::Result<!, Self::Error>
Or each TxMailBox could have its own struct instance with a trait.

Furthermore this start_transmit(...) could have separate id and data parameters (as I written above) because you may reuse the same ID may times and change the data only. Wrapping these two always into a frame is just boilerplate... But again, maybe I'm wrong.

Also, the Interface trait should be splitted into RxInterface and TxInterface because those are almost independent. One part of the application may need only the ownership of the receiver and an other only the transmitter...

stm32f103 for example have two independent receive fifos (you can configure the filters where to put the received frames) and three independent mailboxes (but there is central a register which points to the empty or lowest priority tx mailbox).

I have a working, but still work in progress CAN implementation (not implements this HAL yet) here:
https://github.com/tib888/stm32f103xx-hal/blob/master/src/can.rs

kjetilkjeka · 2018-04-24T11:09:22Z

I see that filtering is not addressed here (yet?), but from my experience the RTR should be part of the ID, since frame filtering involves the RTR bit also. I may be wrong, but please consider this.

Yes filtering is core part of the can specification and will need to be added.

From the CAN specification (ISO-11898-1:2003, don't have access to the 2015 review, please mail me if you have):

A frame transaction initiated at the LLC sublayer shall be a single, self-contained operation independent of previous frame transactions. The content of a frame shall be named by its identifier. The identifier does not indicate the destination of the frame but describes the meaning of the data. Each receiver may decide by frame acceptance filtering whether the frame is relevant or not.

And the definition of identifier is:

The identifier field shall be composed of three segments: base identifier, extension flag and identifier extension. The length of the base identifier shall be eleven (11) bits (ID-28 to ID-18), the extension flag one bit, and the length of the identifier extension shall be eighteen (18) bits (ID-17 to ID-0). The identifier extension shall be ignored if the extension flag is logic zero (0).

I interpret this as a can controller is not required to filter on RTR to be compliant to specifications.

I think this below will not work if you really want a non blocking implementation for more than one transmit mailbox, because it can not trace the state/progress of the transfer:
fn transmit(&mut self, frame: &Self::Frame) -> nb::Result<Option<Self::Frame>, Self::Error>;

I think you're correct, I will rethink this.

- removed weird transmit function - Allow one function to receive all kinds of can frames

…th a given ID

kjetilkjeka · 2018-05-21T13:53:29Z

Rebased onto japaric/master.

I made a split between Receiver and Transmitter after @tib888 recommendation. This only makes the interface more flexible and i don't think there are many disadvantages to doing so.

The problem of following the progress of the transfer is a hard one. I don't think it's a good idea to hand out Mailbox handles for two reasons:

The can controller might implement some sort of priority queue instead of mailboxes.
It's not simple to invalidate/represent a handle to a Mailbox when the mailbox starts transferring a new Frame. Unless we force an inflexible API requiring to track mailboxes manually it will also be racy.

I instead provided a method to see if there are any pending transmissions matching an ID. I think this will be sufficient in most (all) cases. Perhaps anyone can think of a case where it is not? (@tib888 )

kjetilkjeka · 2018-12-20T14:54:17Z

I've been busy with a lot of other things so I'm afraid not. I expect to be able to use some time on this within the next couple of months, as I'm going to need a CAN interface for one of this microcontrollers myself by this time.

A couple of things that has made this not straight forward to implement is the following:

svd2rust is not able to generate code when there are registers without a reset value. This is allowed from the svd specification. This is the case for one of my example platforms.
Code reuse within a chip family using SVD files are not straight forward, making implementation of these drivers harder than it needs to be.

I've been investigating these two things recently, and will hopefully be able to suggest some imporvements soon.

jdemilledt · 2019-02-26T02:58:04Z

I've recently taken up an interest in this. What issues need to be resolved to get this moving forward? I am willing to commit time to this.

kjetilkjeka · 2019-03-09T11:55:33Z

I've recently taken up an interest in this. What issues need to be resolved to get this moving forward? I am willing to commit time to this.

Great!

First of all you should look at the traits I've proposed and see if they make sense to you or whether you think anything should be done in a different way.

If you think my current proposal make sense, you should attempt

Implement it for one or more hardware to locate friction points from the implementer side of things.
Use it for one or more applications to find any friction point from the application side of things.

jdemilledt · 2019-03-10T03:43:10Z

One thing I'll point out here that, IMO, seems a bit inefficient: Since ux::{u11,u29} aren't primitives, the Rust compiler and runtime treat them differently. Not to mention, the CAN registers on at least the STM32 are all 32-bit. To decrease complexity and code size, IDs should just be u32 and either truncate or error when they run out of limits.

kjetilkjeka · 2019-03-10T12:57:02Z

First I will mention some things I think the API should attempt to avoid:

When using the u32 form the uppermost bit flags may be interpreted differently on different hardware. As a minimum, it should be specified in code what kind of bits are used for what kind of flags.
When handing invalid data (out of range IDs etc) different platform may choose to behave differently. Some may saturate, some mask out bits and some may do nothing. This should be avoided.

One thing I'll point out here that, IMO, seems a bit inefficient: Since ux::{u11,u29} aren't primitives, the Rust compiler and runtime treat them differently.

The current traits are not designed specifically to work with ux::{u11,u29}, but rather to be flexible enough to work with "anything".

Not to mention, the CAN registers on at least the STM32 are all 32-bit. To decrease complexity and code size, IDs should just be u32 and either truncate or error when they run out of limits.

I agree that using a concrete type will be more ergonomic than the Id trait from this PR. Although it's idiomatic to this HAL to use minimal traits for a lot of things, also using this approach for ID is perhaps to overdo it and we should definitively discuss the advantages of making this a concrete type instead.

I'm not certain to what degree the bit format of the u32 is standardized over different hardware. If this is different, using a fixed layout u32 would only allow optimizing for one platform. This would, in turn, make the static dispatch trait approach more efficient.

If we want to go with a type instead of a trait, we should at least use a newtype of u32 instead just an u32. This will not cause any bloat in code size. Something like the following can be an alternative

#[derive(Debug, PartialEq, Clone, Copy, Hash /* etc */)]
pub struct CanId(u32);

impl From<CanId> for u32 { /* ... */ }

impl CanId {
    const ID_MASK: u32 = 0x7ff;
    const EXTENDED_MASK: u32 = 0x1fffffff;
    const EFF_MASK: u32 = (1<<31);

    pub fn try_from_id(u16) -> Result<Self, _> { /* ... */ }
    pub fn try_from_extended(u32) -> Result<Self, _> { /* ... */ }
    pub fn try_from_bits(u32) -> Result<Self, _> { /* ... */ }

    pub fn is_extended(self) -> bool { /* ... */ }
    pub fn id(self) -> u32 { /* ... */ }
}

The problem with this is of course that id.into() cannot be used directly for hardware that doesn't use the bit map specified by this type. As previously mentioned, I'm not sure to what degree the bit format is standardized on, but this is potentially a probelm.

A different alternative would be to use a bit more friendly and simpler trait. This will allow for using optimized bit layouts for all hardware, while perhaps being a bit less cumbersome to work with than the trait from the PR. Let me suggest something like the following:

pub trait CanId : Into<u32> {
    /// The mask of the ID when a standard frame is converted into an `u32`
    const ID_MASK: u32;

    /// The mask of the ID when an extended frame is converted into an `u32`
    const EXTENDED_MASK: u32;

    /// The mask of the extended frame flag when converted into an `u32`
    const EFF_MASK: u32;

    pub fn try_from_bits(u32) -> Result<Self, _>;

    pub fn try_from_id(u16) -> Result<Self, _> {
        /* Default implementation based on `try_from_bits` and consts */
    }

    pub fn try_from_extended(u32) -> Result<Self, _> {
        /* Default implementation based on `try_from_bits` and consts */
    }

    pub fn is_extended(self) -> {
        /* Default implementation based on `Into<u32>` and consts */
    }

    pub fn id(self) -> u32 {
        /* Default implementation based on `Into<u32>` and consts */
    }
}

The approach for creating a new platform would be to create a CanId newtype over u32 and trivially implement the trait in a way that matches with the hardware bit layout.

To gain more insight, we should look whether bit patterns of registers used in CAN hardware is standardized. I suggest we at least compare the following relevant platforms:

socketcan
Microchip MCP2515 and MCP2517
The STM implementation you were talking about
Bosch IP (MCAN) - Used by atmel/microchip atsam
NXP S32K implementation

jdemilledt · 2019-03-11T14:17:02Z

A possible solution if the CAN ID types aren't compatible could be to have HAL implementors specify CanId <-> u32 conversion methods.

EDIT: I just read again and saw you mentioned that. I'll go look at those implementations and get back to you.

jdemilledt · 2019-03-11T14:20:28Z

For the record, at least the ST implementation makes a signal that can be sent to an MCP2515 which translates it to the CAN differential signal.

jdemilledt · 2019-03-11T14:42:32Z

Oops, I misunderstood what the MCP2515/17 are. Ignore my comments about ST and the MCP.

jdemilledt · 2019-03-11T15:03:34Z

I've taken a look at the SAM and STM32 implementations, and so far, I'm already seeing differences in ID encoding, but they're along the lines of a mask and a bit shift from what I'm seeing. Nothing too fancy. Perhaps just add a bit shift const to the CanId trait?

jdemilledt · 2019-03-21T18:42:54Z

@kjetilkjeka I think an idiomatic way to solve the u32 ID issue would be to have unsafe methods that just shift out any extra bits, and safe methods that error out if an ID is too big. If there's enough interest, I could also implement a lint that warns users if they hardcode IDs that are too big.

jdemilledt

I've come up with a few changes. Let me know what you all think.

diff --git a/src/can.rs b/src/can.rs
index 47ed7ba..7c070fd 100644
--- a/src/can.rs
+++ b/src/can.rs
@@ -4,31 +4,44 @@ use nb;
 
 /// A type that can either be `BaseId` or `ExtendedId`
 #[cfg(feature = "unproven")]
-pub trait Id {
-    /// The (11-bit) BaseId variant.
-    type BaseId;
+pub enum Id {
+    Base(u32),
+    Extended(u32)
+}
+
+#[cfg(feature = "unproven")]
+#[derive(Debug)]
+pub struct IdError;
 
-    /// The (29-bit) ExtendedId variant.
-    type ExtendedId;
+#[cfg(feature = "unproven")]
+impl core::error::Error for IdError {}
 
-    /// 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>;
+#[cfg(feature = "unproven")]
+impl core::convert::TryFrom<u32> for Id {
+    type Error = IdError;
+    fn try_from(from: u32) -> Result<Self, Self::Error> {
+        if (!0x7FF | from) == 0 {
+            Id::Base(from)
+        } else if (!0x1FFFFFFF | from) == 0 {
+            Id::Extended(from)
+        } else {
+            IdError {}
+        }
+    }
+}
 
-    /// 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>;
+#[cfg(feature = "unproven")]
+pub enum FrameContents<'a> {
+    DataFrame(&'a [u8]),
+    RemoteFrame
 }
 
 /// 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;
+    fn from_id(id: Id) -> Self;
 
     /// Constructs a filter that will accept any `Frame`.
     fn accept_all() -> Self;
@@ -51,31 +64,12 @@ pub trait Filter {
 /// A Can Frame
 #[cfg(feature = "unproven")]
 pub trait Frame {
-    /// The Id type of this Frame
-    type Id: Id;
-
-    /// 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;
+    fn id(&self) -> Id;
 
-    /// Returns `Some(Data)` if data frame.
-    /// Returns `None` if remote frame.
-    fn data(&self) -> Option<&[u8]>;
+    /// Returns `DataFrame(Data)` if data frame.
+    /// Returns `RemoteFrame` if remote frame.
+    fn data(&self) -> FrameContents;
 }
 
 /// A Can-FD Frame
@@ -83,35 +77,16 @@ pub trait 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;
+    fn id(&self) -> Id;
 
-    /// Returns `Some(Data)` if data frame.
-    /// Returns `None` if remote frame.
-    fn data(&self) -> Option<&[u8]>;
+    /// Returns `DataFrame(Data)` if data frame.
+    /// Returns `RemoteFrame` if remote frame.
+    fn data(&self) -> FrameContents;
 }
 
 
@@ -120,17 +95,14 @@ pub trait FdFrame {
 /// 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>;
+    type Frame: Frame;
 
     /// The Interface Error type
     type Error;
 
     /// The Filter type used in this `Interface`
-    type Filter: Filter<Id = Self::Id>;
+    type Filter: Filter;
 }
 
 /// A CAN interface that is able to transmit frames.

kjetilkjeka · 2019-03-24T10:45:24Z

@jdemilledt I don't think your changes related to ID will work. When converting from u32 it's not valid to convert all IDs that will fit in the normal ID range as a BaseID. Using extended IDs with low numbers is a totally valid thing to do with CAN, and is done all the time in practice.

By making the ID an enum instead of a trait, we're also missing the opportunity of abstracting over the native bit layout of the CAN controller. This will be slightly less efficient in both memory and CPU, although this might be fine I think the trait I previously proposed might solve this in a good way.

I also don't like the removal of the small default implemented convenience functions. I think they are quite valuable in practice. And they are implemented by default, meaning no overhead for the implementer.

Returning the FrameContents is fine, but require a lot of convenience functions implementation to be as convenient as the Option.

jdemilledt · 2019-03-26T22:15:11Z

Those are some good points. I don't necessarily agree about making a trait instead of a fully implemented struct since CAN IDs are the same across platforms except the underlying implementation's bit requirements, which could be implemented in the Interface. Can you please elaborate on what you're saying about convenience functions?

kjetilkjeka · 2019-03-29T07:30:03Z

Can you please elaborate on what you're saying about convenience functions?

I'm talking about the functions like is_data_frame(&self), we should definitely have those kinds of functions to avoid always having to pattern match on enums. It's quite idiomatic to Rust. Take for instance Result, it implements is_ok(), is_err() and a ton of other "convenient" things.

jdemilledt · 2019-03-29T12:36:00Z

I see. I removed them because I thought they were redundant, but if you want to keep those, that's fine.

no111u3 · 2019-11-18T10:03:07Z

@kjetilkjeka I think you may update your pr to current version, some changes is good for me, some need clarification.

arrowcircle · 2020-01-23T10:43:39Z

Any updates on bringing can bus to embedded-hal?

marcelbuesing · 2020-04-27T12:33:33Z

src/can.rs

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


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.

314: Controller Area Network (CAN) Take 4 r=eldruin a=timokroeger Updated to the latest HAL changes: * Removed `try_` prefix * Moved non-blocking implementation to `nb` module * Removed default `blocking` implementaions ## Usage Example [stm32-fwupdate](https://github.com/timokroeger/pcan-basic-rs/blob/eh-take-4/pcan-basic/examples/stm32-fwupdate.rs) ## Implementations Updated for this PR: * [pcan-basic](https://github.com/timokroeger/pcan-basic-rs/blob/eh-take-4/pcan-basic/src/lib.rs) on top of an existing software API * [bxcan](https://github.com/timokroeger/bxcan/blob/eh-take-4/src/lib.rs#L460) Based on the very similar predecessor traits `embedded-can` v0.3 ([diff v0.3 -> this PR](https://github.com/timokroeger/embedded-can/compare/eh-take-4)) * [candev](https://github.com/reneherrero/candev) implementing the traits for linux SocketCAN * [socketcan-isotc](https://github.com/marcelbuesing/socketcan-isotp) ## Previous Discussion * #212 * #77 * #21 * #53 Co-authored-by: Timo Kröger <timokroeger93@gmail.com>

eldruin · 2021-10-29T06:31:00Z

Solved in #314

77: Adapt to embedded-hal-1.0.0-alpha.7 r=posborne a=eldruin For the iterator methods I just collected the results. I have not looked into really using an iterating interface but merging this would be a first step and if there is interest we can look into that later on. Fixes rust-embedded#76 Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>

dunmatt suggested changes Apr 21, 2018

View reviewed changes

kjetilkjeka added 8 commits May 21, 2018 15:23

Added CAN hal

29b920d

Removed weirdness in can interface

09887a0

- removed weird transmit function - Allow one function to receive all kinds of can frames

Rewrote Id trait to make it work with a wider range of representations

78b641c

First take on filters

c9a4f59

Split Interface into Transmitter and Receiver

99bf193

Remove method from FdInterface that have a redundant method in Interface

c9c4f38

Added a method that allows to check if there are pending transmits wi…

8099f74

…th a given ID

typo

0790470

kjetilkjeka force-pushed the can branch from c96ad8f to 0790470 Compare May 21, 2018 13:44

added some missing unproven

2200dbe

Disasm added S-waiting-on-author Author needs to make changes to address reviewer comments and removed S-waiting-on-author Author needs to make changes to address reviewer comments labels Feb 24, 2019

johnthagen mentioned this pull request Mar 19, 2019

Embedded Commercial Rust Gaps rust-embedded/wg#336

Closed

jdemilledt suggested changes Mar 22, 2019

View reviewed changes

hannobraun mentioned this pull request Oct 7, 2019

Controller Area Network (CAN bus) rust-embedded/cortex-m#170

Closed

marcelbuesing reviewed Apr 27, 2020

View reviewed changes

tib888 mentioned this pull request Apr 29, 2020

tib888 can impl stm32-rs/stm32f1xx-hal#101

Closed

zklapow mentioned this pull request May 9, 2020

CAN Support stm32-rs/stm32f3xx-hal#100

Merged

1 task

timokroeger mentioned this pull request May 23, 2020

Controller Area Network (CAN) Take 3 #212

Closed

timokroeger mentioned this pull request Sep 22, 2021

Controller Area Network (CAN) Take 4 #314

Merged

eldruin closed this Oct 29, 2021

[WIP] CAN Interface take 2 #77

[WIP] CAN Interface take 2 #77

Conversation

kjetilkjeka commented Apr 19, 2018 • edited Loading

therealprof commented Apr 19, 2018

kjetilkjeka commented Apr 19, 2018

therealprof commented Apr 19, 2018

therealprof commented Apr 20, 2018

kjetilkjeka commented Apr 20, 2018

dunmatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunmatt Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunmatt Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunmatt Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdemilledt Mar 9, 2019 • edited Loading

Choose a reason for hiding this comment

dunmatt commented Apr 21, 2018 via email

kjetilkjeka commented Apr 22, 2018

dunmatt commented Apr 22, 2018

kjetilkjeka commented Apr 22, 2018

dunmatt commented Apr 22, 2018

kjetilkjeka commented Apr 22, 2018

dunmatt commented Apr 22, 2018 via email

therealprof commented Apr 22, 2018

dunmatt commented Apr 22, 2018 via email

dunmatt commented Apr 22, 2018 via email

kjetilkjeka commented Apr 22, 2018

tib888 commented Apr 22, 2018

kjetilkjeka commented Apr 24, 2018

kjetilkjeka commented May 21, 2018

kjetilkjeka commented Dec 20, 2018

jdemilledt commented Feb 26, 2019

kjetilkjeka commented Mar 9, 2019

jdemilledt commented Mar 10, 2019

kjetilkjeka commented Mar 10, 2019

jdemilledt commented Mar 11, 2019 • edited Loading

jdemilledt commented Mar 11, 2019 • edited Loading

jdemilledt commented Mar 11, 2019

jdemilledt commented Mar 11, 2019

jdemilledt commented Mar 21, 2019

jdemilledt left a comment

Choose a reason for hiding this comment

kjetilkjeka commented Mar 24, 2019

jdemilledt commented Mar 26, 2019

kjetilkjeka commented Mar 29, 2019

jdemilledt commented Mar 29, 2019

no111u3 commented Nov 18, 2019

arrowcircle commented Jan 23, 2020

Choose a reason for hiding this comment

eldruin commented Oct 29, 2021

kjetilkjeka commented Apr 19, 2018 •

edited

Loading

dunmatt Apr 21, 2018 •

edited

Loading

dunmatt Apr 21, 2018 •

edited

Loading

dunmatt Apr 21, 2018 •

edited

Loading

jdemilledt Mar 9, 2019 •

edited

Loading

jdemilledt commented Mar 11, 2019 •

edited

Loading

jdemilledt commented Mar 11, 2019 •

edited

Loading