CAN Architecture Update

[BlakeFreer]

Relating to #242

This discussion was copied from @samparent97 and my discussion on MSTeams. Comments starting with (SAM) are his comments.

[BlakeFreer]

I think this is it https://github.com/BlakeFreer/Playground/tree/e555038833af6384ae83c76c705acf8f5d65bfde/cpp/can-handler

It has very similar functionality to the current system (no queue, just the most recent message) but is more performant by using template + concepts instead of virtual classes, completely removes the registry layer, and makes the generated code simpler

[BlakeFreer]

(Sam)

Messages are not split into Rx and Tx messages so they don't need to repeat their structure.
Alternatively (and even better) we could split the Message concept into an Rx which supports ::decode() and Tx which supports .encode(). Then the type system will prevent the developer from perfoming illegal operations, but the code for any message can still exist in a single struct, even if it is both Tx and Rx.

My question is, why does it matter to have the Tx and Rx struct definition in one?

IMO this was a feature since it was really clear which messages were injested and which were sent by the system

My question is, why does it matter to have the Tx and Rx struct definition in one?

[BlakeFreer]

(SAM)
That’s not the point I’m making. There’s info that can be important in more than one place. It’s such a simple design choice that gives the user the flexibility

[BlakeFreer]

Even a one time event message? We aren’t compacting our signals that tightly to need to send different events to different tasks

[BlakeFreer]

(SAM)
Why are we talking about events? Messages can be so much more than events

[BlakeFreer]

Yea but if it’s just live data then why does it matter if you’ve seen it before or not

[BlakeFreer]

(SAM)

Bc you might want to set flags if you’re not getting new data. I feel certain that the system will get to a complexity where this simple decision will become obvious

[BlakeFreer]

(SAM)

Less methods are defined in the interfaces / abstract classes at all layers. A greater focus is placed on the generated code. ex unpack_shift and pack_shift are not defined anywhere. We will let the cangen template decide how to unpack rather than coupling it to some methods on the Message class.

These get inlined anyway so it just helps readability, any CAN C/C++ code library uses a similar structure so if you've seen it before it makes a lot of sense very quickly

[BlakeFreer]

(SAM)

By moving to public .encode() and ::decode() methods on Message, we no longer need to make Msgs and Bus friends and can get rid of the protected method-relaying methods (like
Clone and ID in MsgRegistry).

This is huge

[BlakeFreer]

(SAM)

Since the Base is able to directly add messages to Bus, we can remove Bus.Update(). All messages are handled and made available in the background.

This is also huge

[BlakeFreer]

(SAM)

Are you planning to get rid of timestamps? I think they can be important in a lot of places

[BlakeFreer]

How do you use the timestamp besides determining if a message is new or old? Like if(timestamp>last_timestamp) {process message}

If that’s the only reason then an entire timestamp is more information than necessary. A “already_processed” Boolean flag would be more appropriate

[BlakeFreer]

(SAM)

Its 32bits it’s totally worth it imo

[BlakeFreer]

https://github.com/BlakeFreer/Playground/tree/64feab529efb43873f078874f69e4530ce823872/cpp/can-handler

Updated. Changed my mind on a lot.

• Split Tx and Rx. Added the timestamp to Rx because yeah tuples made it messy.
• Focused heavily on data invariants, ie what do I want to guarantee is true?
  • For RxMessages, the invariant is RxMessage data came from the CAN line. This means that only the Bus can instantiate an Rx message, and members are private so you need to use a getter(). Obviously no setters are provided.
  • For TxMessages, the invariant is that TxMessage members are valid values for that message. This allows us to enforce the DBC signal limits. I use a factory method that returns an optional TxMessage which returns NONE if parameters are invalid. See the implementation and this explanation
    An assortment of small coding projects. Contribute to BlakeFreer/Playground development by creating an account on GitHub.

To enforce the TxMessage invariant, you are unable to zero initialize the struct and populate the fields one-by-one as our current code does. You have to use a static create(param1, param2...) method to instantiate it. This means your code either has a complete and valid TxMessage, or it doesn't have one at all. this means that you can only send valid data, so your receiver will not be confused by an unpopulated signal.

[BlakeFreer]

(SAM)
for the tx thing, does that mean i have to instantiate a new message type every time?

[BlakeFreer]

You don’t need to allocate new memory each time, but you do need to call ::create to change the values in the old spot. zero memory cost for guaranteed validity