[TP]: Rewrite to aid maintainability and possiblity to expand with other features

[GwnDaan]

This is basically a proof of concept that works as far as the unit tests go. Now I seek some feedback if we actually want these changes or not.

What changes:

• added abstract transport data interface
• reworked the session storage to hold the objects themselves
• a lot of refactorizarion such that a session could in theory be initialized and completed for any ECU on the bus

[GwnDaan]

Part of me is wondering if we might want to make even a different class from TransportProtocolManager, that somewhat duplicates the functionality but could be optionally linked in to add a "transport protocol listener"

Yeah I see what you mean, when implementing I tried to keep in mind a third "SessionDirection" which would be something like "ListenOnly" but thats not yet in there. But an approach like yours might work as good as well, though it introduces some dupplication when maintaining the stack. Something we already experience quite a bit with the current 3 transport protocols being very similar in some places.

[ad3154]

though it introduces some duplication when maintaining the stack

It would of course, and each protocol is juuuust different enough to make sharing code between them troublesome. The good thing is, the transport layer is quite stable from an ISO/J1939 requirements perspective, and probably won't need to be messed with too often once a listener is working (especially if we can ever hit it with the AEF conformance test tool to get it 100% compliant).

Going back to your (paraphrased) goal with this:

I basically [wanted] to try to make the TP class better suited for sniffing TP sessions between other ecu's on the bus.

This strikes me as a, lets say, abnormal thing to want in a "normal" ISOBUS application but something many people will want regardless of course, so, one thing I really like about a separate object is that it makes it clear when you use it that you are doing things outside the "core" functionality of the stack - something extra with a clearly defined but separate data path. Like, you really have to opt-in. Plus, then the fairly substantial extra storage associated with it becomes more optional, especially if we control linking it with CMake, because sniffing could result in us storing (253 + (253 252)) = 63756 sessions!!!! (or, 253+(253!)/(2!(253-2)!)*2 if you want to be pedantic about your equations).

[ad3154]

I'm sure we can control the memory cost without making it optionally linkable of course, it's just something to think over and consider. No matter what path we go, we may want to not sniff anything unless specifically configured to do so one way or another.

[GwnDaan]

@ad3154, I still think this is a great addition. I changed a lot and in my opinion it is now more modular and future proof. I can easily implement a message 'sniffer' based on this format that doesn't hurt our performance/memory usage, and is opt-in without too much extra codebase. Though you might still think otherwise, and hence I'm curious for your thoughts on this :)

edit: it most-likely still has some bugs in it that need to be catched

[ad3154]

@ad3154, I still think this is a great addition. I changed a lot and in my opinion it is now more modular and future proof. I can easily implement a message 'sniffer' based on this format that doesn't hurt our performance/memory usage, and is opt-in without too much extra codebase. Though you might still think otherwise, and hence I'm curious for your thoughts on this :)

edit: it most-likely still has some bugs in it that need to be catched

Ah, ok, I can take another look and let you know my thoughts 🙂

[GwnDaan]

Was your thinking mainly to consolidate CANMessageDataCallback? It just feels like a lot of misdirection which at least kinda feels more confusing to me than just interacting with the native/standard types.

Yeah that's basically one of the two main reasons I think:

• It provides one common interface to get the data from. This can be the same interface throughout all protocols. When one needs to get some data, there is no need to do a cumbersome check to know where to get the data from. The data is just there, only one function away.
• By passing/moving around an instance of a class, you can't accidentally modify the data it holds. That is because you explicitly can't, unlike raw pointers. Ofcourse proper code reviews shouldn't let that happen in the first place, but we're still human after all and mistakes will happen haha.

I feel like it will reduce the chance of failures in the future, at least from inside the stack. Not that, that was a problem in the first place, but a little more robust structure should not harm.

My main mental sticking point is still that I don't enjoy too many levels of abstract data types on top of abstract data types. I'm still not sure I grasp the benefit of having things like CANMessageDataVector which inherits from CANMessageData which inherits from a vector, when the data, really, is just a vector. Or CANMessageDataView which inherits from CANMessageData which inherits from CANDataSpan which is really just a templated version of DataSpan, which is just an array of bytes and a size. (I know I'm being a little over-simplify-y here, appologies)

Yeah definitely, though I think it's better to think of it as a simple 'interface'. No need to worry about the levels of abstraction beneath it. How I like to think about it:

• CANMessageData: hey, a interface class that has a few functions to access some data (idc where it's from)
• CANMessageDataView: oh cool, a class implementing the CANMessageData interface that allows us to 'look' at data stored somewhere in memory.
• CANMessageDataVector: ahh, another class implementing the CANMessageData interface, but now instead of 'looking' at the data, we actually 'own' it. Allows us to unrestrictively modify the data.
• CANMessageDataCallback: another class implementing the CANMessageData interface, but instead of representing data in memory, we can just request chunks of data from somewhere.

So no need to worry about all the ADT's behind it, I know we still are low level developers by heart, but let's appreciate a bit of abstraction here hahah.

[ad3154]

I appreciate your explanations, they do help me see what you mean, and I do feel a bit better about it now...

Like I mentioned I'd like to run some tests just because I am paranoid about changing TP, but then I think I'm OK with it and will give it one last look over.

Sorry I'm stalling, haha

[GwnDaan]

Yeah no worries, I was planning on trying to cover it with unit tests first as well. But I'm not sure where to start to be completely honest. Since the TP probably won't change in the mean time, i'm fine with leaving it open a little longer to be sure there are no issues.

[GwnDaan]

In https://github.com/Open-Agriculture/AgIsoStack-plus-plus/pull/344/commits/b0301bd38441bdde02a35071957a737775f8767f I removed the CANNetworkManager dependency, this should make it a lot easier to unit-test the implementation.

There's still a list I want to do before thinking about merging this into main:

• rewrite ExtendedTransportProtocol to match the new style
• rewrite FastPacketProtocol to match the new style
• unit-test all implementations

[sonarcloud[bot]]

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

23 New issues
0 Security Hotspots
74.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[GwnDaan]

Replaced by #391