init j1939 datalink #44
Conversation
socketcan will only be needed only inside the example so we can change it into dev-dependencies
|
Sorry for the late return, unfortunately I was quite ill last week. Here are the necessary commits to clear up the previous mess. Embedded Can is now used as the interface for the J1939 layer and the whole thing compiles so far. The clippy warnings are also out, except for the examples, where I will soon set up a suitable example for embedded_can. For the PGN structs I still have to write tests to secure the bit conversion. I also have to add an additional layer for ISO 11783 specific frames, so that the network part ISO11783-5 can be adapted correctly. But I have abstracted the PGNs, priorities and addresses as much as possible and checked all raw inputs to exclude user or input errors and to guarantee the correctness of the types. PGN can now only be used with a try_from of a u32 and an error is thrown if the value is incorrect (for example, if the 18 bits are exceeded). The same applies to the conversion of u32 to an ID and a u8 to a priority where I have already partially adopted @Notgnoshi 's work. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44 +/- ##
==========================================
+ Coverage 11.52% 17.06% +5.53%
==========================================
Files 17 25 +8
Lines 2664 2819 +155
==========================================
+ Hits 307 481 +174
+ Misses 2357 2338 -19 ☔ View full report in Codecov by Sentry. |
|
Here is a brief update. I have implemented all J1939-21 defined things (except TPs) and tried to document them as good as possible. I put a lot of emphasis on abstracting away raw values and checking them on compile time before they are used to avoid wrong inputs as far as possible or at least directly recognizable during execution. Many tests have already been implemented, but there is still a little work to be done for complete coverage. I'm not quite finished yet, as I would like to set up an example to demonstrate the functionality, but that will come soon. The network stuff will come after that and I'll take over the things that @ad3154 implemented. I would particularly appreciate a short feedback from @Notgnoshi on whether things are implemented well as actually intended for Rust. |
| High, | ||
| /// Normal messages are sent to the driver when no high priority messages are in the queue (todo) | ||
| /// Normal messages are sent to the j1939 when no high priority messages are in the queue (todo) |
There was a problem hiding this comment.
nitpick: Looks like a casualty of a mass find/replace?
| Priority::Default, | ||
| let request_id = ExtendedId::new( | ||
| StandardId::new(Priority::Three, source_address), | ||
| CommonParameterGroupNumbers::AddressClaim.get_pgn(), |
There was a problem hiding this comment.
out-of-scope: Is there a way to turn off the Codecov comments. They get in the way of code review :(
|
|
||
| #[derive(Debug)] | ||
| #[non_exhaustive] | ||
| pub enum DriverOpenError { | ||
| /// The driver failed to open with filesystem semantics | ||
| /// The j1939 failed to open with filesystem semantics |
There was a problem hiding this comment.
nitpick: Throughout this PR it appears driver was found/replaced with j1939, which doesn't appear to always have been intentional
| /// This trait does _not_ define how to construct and configure a driver, as the details are likely | ||
| /// to differ from driver to driver. | ||
| /// This trait does _not_ define how to construct and configure a j1939, as the details are likely | ||
| /// to differ from j1939 to j1939. | ||
| pub trait Driver { |
There was a problem hiding this comment.
Do you expect to continue using this Driver trait internally? Something we might be able to do is provide a blanket implementation that implements Driver for any nb::Can implementations.
That might give us the opportunity to build an API that we like, or it might also be just too much overhead / confusing to use. I'm not sure.
| #[derive(Debug, Clone, Default)] | ||
| pub struct Frame { | ||
| id: Id, | ||
| data: Vec<u8>, |
There was a problem hiding this comment.
I'm less familiar with the j1939 details, but my impression is that a frame is limited to a single 8-byte message. If that's correct, I wonder if data: [u8; 8] would be better?
By renaming CanMessage to Frame did you intend to make the difference between the 8-byte frames and the logical variable-byte PDUs that you reconstruct from TP, ETP, and Fast Packet sessions?
There was a problem hiding this comment.
If we want to align terminology with the industry and the C++ stack, frames should by 8 bytes. Messages are a more abstract thing composed of one to many frames' protocol payloads.
| Id::Extended(id) => { | ||
| // Process address claims and requests to claim | ||
| if NAME::default() == NAME::default() | ||
| /*TODO!: Replaced following code line by upper NAME::default(). There needs to be another abstraction of ISO 11783 specific frames which includes the NAME*/ |
There was a problem hiding this comment.
Does j1939 really not have a NAME concept? How does it do address claiming then?
In general, I don't really see the value in separating out j1939 from iso11783. I think people commonly believe that j1939 forms the "base" of iso11783, and that the ISO standard is "built on top of" j1939, but they're ultimately separate standards that happen to be mostly equivalent until you get into the higher level layers.
I guess, I only have exposure to the ISO standard, and don't work on j1939 vehicle busses, so I'm pretty biased?
How will it work to have network management, address claiming, etc. that are supposed to work for both standards? Do we need to duplicate them (essentially break the library in two?) or what would it look like to use one "business logic" part of the library with two different "data model" parts?
There was a problem hiding this comment.
To be honest, I can't even remember that I wrote that or what I wrote at all haha. But thats crap. The NAME concept exists and is the identical as in ISO11783
There was a problem hiding this comment.
I had often worked on this at night, so my language may be a little confusing. I suppose I meant that I still have to implement this in my new J1939 frame. I remember that you and Adrian had created two similar frame structures in parallel. I had then adopted mine, where the NAME concept is not yet implemented, but since the NAME concept is not defined in the data link layer, I haven't done that yet.
There was a problem hiding this comment.
Just catching up with all the comments around GitHub, as I am traveling at the moment... In the C++ stack we only mention J1939's backwards compatibility stuff, or when the ISO standard says "read J1939 and do what they say". For example, in J1939 the TP.BAM inter-message delay must be 50ms, so we mention that, and the diagnostic protocols work slightly differently, so we default to the ISO way, but allow the user to say "no I want the old J1939 way" if they want. As Jannes says, the NAMEs are the same. I prefer to always be an ISOBUS stack first, and a J1939 stack second, personally
Here is my start to implement the pure J1939 layer with embedded-can. Please note that the pushed files do not yet compile completely. I just wanted to provide Notgnoshi with something quickly so that he can continue with it.