Refactor away from CANNetworkManager singleton

[GwnDaan]

While refactoring some unit tests I saw that we always refer to the one singleton:

static CANNetworkManager CANNetwork; ///< Static singleton of the one network manager. Use this to access stack functionality.

I wonder if using only one CANNetwork is the optimal approach? I find it a bit hard to gasp what it stands for and how it's used. Namely the automatic address claiming when you initialize a control function seems like "magic". You never have to explicitly call the CANNetwork to claim/register this CF. This might be confusing to new users of the stack?
It also handles multiple can channels at once from what I can tell, which I feel like can be separated in different objects? I think separating each 'device' on the stack to use it's own manager would allow us to write unit tests more easily.

Though there probably was a good reason to write it like this which I would love to hear and think about! 😄

[ad3154]

I have had parts of this particular discussion with some peers before and would be happy to lay out my thoughts. I am not perfect, and I want the stack to be as friendly as possible to relative newbies with CAN, but powerful to experienced users, so suggestions are always good.

The Singleton

The network manager singleton is not my favorite - I would be OK with splitting it up 1 per channel and allowing users to instantiate them. It is the current way for a reason that doesn't matter so much now. Initially when I wrote the stack, I wanted the protocol classes to be dynamic. Basically, I wanted to be able to compile in or out single protocols without ifdef. For example, if you have a project that does not use Fast Packet, it would be cool if you could simply not compile the cpp file for fast packet, and the stack would still build, just without Fast Packet. In the end though, this is what causes the pain that we went through with that -whole-archive stuff, so it's maybe not worth it, and we've started to go away from that anyways with TP and ETP being now housed in side the network manager.

The current way also simplifies the hardware layer a fair bit. There's just one receive queue, and all messages are processed by the stack by a single thread in order of reception. If we break it up it may involve some additional complexity, but hopefully not too much? Since the network manager updates via a callback from the hardware layer, I think we could register multiple of them with relatively few issues.

So yeah, I guess I did it for a reason, but that reason is now not so relevant. If we do end up re-structuring it, I am sure there will be some issues with how the protocols expect there to be a singleton, so maybe each will need to hold a reference to it's parent network or something, but those are challenges we could overcome. Plus, I do agree we would get more flexibility in unit tests if we split it up. That especially would be nice.

Address Claiming

Sorry in advance for the wall of text.

Understanding the current state

Some of why address claiming is the way that it is comes down to wanting it to "just work". Nearly every OEM I've interacted with in some way in my job, (and I cannot stress this enough, even some of the largest ones 😑) seem to fail at address claiming in one way or another. It's the single most frustrating thing for me when integrating products on an ISOBUS. Some companies don't even know they need to do it, some lie about their support for arbitrary addressing in their NAME, some ignore requests for the address claim PGN after they send the first one, some randomly jump around addresses without claiming them, and the list goes on with each company committing a different ISOBUS sin. So, I really wanted to completely abstract that away from the user so it was impossible to mess up, fully automatic, and completely required to communicate on the bus. Currently if they make an ICF, we implicitly know they want to talk on the bus, so it automatically claims and manages its own address. If the bus is not in a usable state, it just keeps trying until it is in a usable state, with no function calls required by the user outside of constructing an ICF. Internally we take all control of this, besides preferred address, away from the user. For example, we check the bit in the NAME the user supplied for supporting a self configurable address and use that when we claim so it is literally impossible for a company to lie about it in their NAME.

So to summarize, I did it that way because I really did want it to be magic, to the extreme! 🧙‍♂️
It is maybe a bit strange for a pattern.

Related: I also have met people that have written J1939 stacks or even ISOBUS stacks before that absolutely do not understand the volatility of addresses at all, and that they can change at literally any time. As such they end up storing addresses or hardcoding addresses everywhere in their application which will 100% always cause bugs. So a minor part of why address claiming in this stack is so opaque is because I literally do not want people to know about them any more then they must. The info is still available, but it's sort of de-prioritized information.

Adding more switches and knobs

I have met a number of people in my time writing software who do not share the "just works" approach of course. Many people seem to want "it just works, when I tell it to work", meaning they want to manually start and configure every option and have their library do nothing until they explicitly turn things on like address claiming. I understand that mentality and it does make sense to me (we do this with the CAN hardware plugins due to all the threading involved), but I just don't think that way in this specific case. My line of thinking is, if the user made the object to perform address claiming with all information needed to perform that task, then why do they need to call a function to turn it on? It's just another line of code with more documentation that someone will inevitably forget to call. In my mind at least it makes more sense to allow them to explicitly turn it off, if needed, with the added caveat that they will be unable to transmit while it is disabled. It's a bit different with the hardware plugins, as we do not know when the user will be done creating their plugins, so we need an explicit start command.

TLDR

• Yeah, splitting up the network manager seems like a good idea, maybe just a lot of work
• Address claiming was meant to be magic, haha. Maybe it is too magic, I do not know. I would be open to changing the ICF's usage pattern if people think calling an extra function makes more sense, but would also counter-propose we instead add a way to "turn them off".

[GwnDaan]

Thankss! I really appreciate your detailed explanation. I also agree with everything you wrote, especially the address claiming part now... I haven't had any experience with developing isobus in a job like environment so I didn't know these issues were a thing, but your reasoning makes a lot of sense to me! My main knowledge with isobus is with just using it on the farm at home. It's there most important that stuff "just works", and by implementing the complete arbitrary address claiming in the background on the stack I guess we can indeed reduce the amount of times that something doesn't work (together).

The only reason I noticed it was magical is that I glanced at it to see if I could easily write a unit test. But I didn't notice it before, so I think the current approach is fine and maybe even optimal in the sense of making sure it just works!

For the singleton, I'm down to try to refactor it to support a separate instance for each can channel. Maybe even in a TDD approach (which sounds scary to me haha). Though maybe it's not really a priority right now, idk.

[GwnDaan]

If we do end up re-structuring it, I am sure there will be some issues with how the protocols expect there to be a singleton, so maybe each will need to hold a reference to it's parent network or something, but those are challenges we could overcome.

Hey @ad3154, I just made a start with this, and am facing this issue indeed. What do you think of housing a weak_ptr to the CANNetwork managing a CF in the CF itself? In most places where we use the current singleton CANNetworkManager, already a reference to a CF exists. Thereby, a CF must be linked to exactly one network I think?

The only downside I can currently think of, is that you'd have to pass a weak_ptr of a CANNetwork to the factory function of a CF. But that might be not too bad?

[ad3154]

Hmm, I think we have some options or things to consider... Here's a few to brainstorm:

• We could pass a network manager pointer of some kind into each protocol constructor, and have each protocol save that since we'd have one instance of each protocol per network? This doesn't solve the issue of sending most messages, but might be part of the puzzle.
• We could have a factory function for networks, and map our channel index values to a specific network manager using some static list or lookup table?
• We could replace channel index values with weak pointers to a network (in control functions, similar to how you suggested)
• We could make the user deal with it themselves (manage the network manager instances) outside of the protocols, which I don't really like since the goal is to make it as easy and simple as possible to the user, and then have them pass that network manager into a static send function? Probably not ideal.

The only downside I can currently think of, is that you'd have to pass a weak_ptr of a CANNetwork to the factory function of a CF.

Is that true? I feel like it might be doable to make a non-static factory function on the network manager that abstracts that away from a user. Not entirely sure though.
Like maybe the InternalControlFunction factory could be on the Network Manager so it knows implicitly what channel to use, IDK

[GwnDaan]

We could pass a network manager pointer of some kind into each protocol constructor, and have each protocol save that since we'd have one instance of each protocol per network? This doesn't solve the issue of sending most messages, but might be part of the puzzle.

Yes, I agree. This would definitely solve the case for the protocols. But looking at the broader picture, we either have to do the same for all other classes utilizing the network, or end up with some kind of overlap/inconsistencies when using a different approach for other classes I feel like. Though I might be missing something here.

We could have a factory function for networks, and map our channel index values to a specific network manager using some static list or lookup table?

Indeed another possibility. I can see a CANNetworkManager as singleton being used to manage multiple CANNetworks, and each network being accessible by some sort of getter function. The singleton can then be access by the entire stack.

Though I'd like to refute this with it not really being an improvement over the current approach, since we'd then have to lookup the network on each call to a function of the network. While in theory replacing all "channel indexes" with a pointer/reference of some sort to a CANNetwork would mean we can simply directly access it. Idk

We could replace channel index values with weak pointers to a network (in control functions, similar to how you suggested)

Yea, I see this working for the HardwareInterface as well, where we can also replace the channel index values with weak pointers to a network. The only places we then haven't covered yet are the ones where we are registering the add_global_parameter_group_number_callback or add_any_control_function_parameter_group_number_callback callbacks. But as far as I can remember these functions are all called in the intializers, hence passing also a pointer/reference to the CANNetwork there shouldn't be that big of a hassle I think.

We could make the user deal with it themselves (manage the network manager instances) outside of the protocols, which I don't really like since the goal is to make it as easy and simple as possible to the user, and then have them pass that network manager into a static send function? Probably not ideal.

I agree. The protocols should work out of the box without any need to touch them

I feel like it might be doable to make a non-static factory function on the network manager that abstracts that away from a user. Not entirely sure though.
Like maybe the InternalControlFunction factory could be on the Network Manager so it knows implicitly what channel to use, IDK

Ohh, good one! That is indeed doable and is a nice addition. I think we can just support both ways here, so you can either call the create factory on the CF's themselves, or use the non-static factory function in the network manager to call it for you. Though this is only needed if we do end up replacing the channel indexes in the CF's with a pointer to the CANNetwork.

Replacing all the channel indexes with pointers to CANNetwork is the approach my preference goes to, since it seems the one with the most maintainable result. Though actually programming it may show differently, you never know haha. Nevertheless I'm open to the other approaches, or any other one we can come up with!