[Core]: Replace raw pointers with smart pointers for CFs

[GwnDaan]

Closes #257, but is based on #255

I choose to go with the shared_ptr approach instead of the weak_ptr approach. The main reason being that it might be really confusing and hard to debug why a control function suddenly stopped working when using weak_ptr and they invalidate when the created owner shared_ptr goes out of scope.

What changed

• Replaced all raw pointer occurrences to smart pointers (shared_ptr) for the internal/external/partnered control functions
• Removed the activeControlFunctions as we can already iterate over the array in the controlFunctionTable.
• Switched to factory functions for creating control functions. This allows us to intercept the shared_ptr being created and manage it in CANNetwork accordingly.
• Add destroy() function to CF's to allow for removal of the CF from the network manager

[GwnDaan]

Also as a more general note on tests, I noticed some unexpected behavior with the tests, where they sometimes fail and sometimes don't. I suspect the problem being that they currently all share the singleton CANNetwork and it doesn't really get reset in between test cases. For the same reason, we have to destroy all used CF's at the end of the test cases.

#161 might be able to fix this. Since it again will be a really disruptive change, I can work on it this weekend if wanted, that way we do all the major disruptive changes in a time-span as short as possible.

[ad3154]

I think the tests sometimes fail because the network manager is being updated from the hardware layer while tests are also manually updating it, causing some race conditions within the tests. I think many of those issues could be fixed by having a way to turn on and off the periodic updates for the network manager.

You've been writing a lot of code recently, haha! Glad to have you back. I am traveling right now, so I haven't been contributing a ton. If you are interested in taking a swing at that, I agree it might be good to get major refactors out of the way sooner rather than later.

[ad3154]

I'm still reviewing, but wanted to mention I'm not really having any luck running this. Here's the VT object pool example, where the debug output seems strange, and the Client never uploads a pool.

I'm hoping to have more time this week to help

[ad3154]

One warning here:

/home/administrator/Documents/gwndaan/isobus/src/can_network_manager.cpp: In member function ‘void isobus::CANNetworkManager::on_control_function_created(std::shared_ptr<isobus::ControlFunction>, isobus::CANLibBadge<isobus::ControlFunction>)’:
/home/administrator/Documents/gwndaan/isobus/src/can_network_manager.cpp:340:94: warning: unused parameter ‘controlFunction’ [-Wunused-parameter]
  340 |         void CANNetworkManager::on_control_function_created(std::shared_ptr<ControlFunction> controlFunction, CANLibBadge<ControlFunction>)
      |  

Can probably just delete the parameter name of controlFunction for now.

[GwnDaan]

I'm not really having any luck running this

I'll check. I was able to run the VT3 object pool example successfully before making this PR. But that was just loading it in as a saved version on the VT.

I'm hoping to have more time this week to help

Sounds good! Unfortunately, I have a very busy week and a half ahead of me, so that means less coding for me... But hopefully at the end I'll have some footage of a real world application using this stack :)

[GwnDaan]

In my case it fails on the ETP session:

but it seems to be caused by #255 where the CANLibManagedMessage was handling the callbackMessageSize when trying to set it's data as nullptr. The CANLibManagedMessage.get_length() then returned this callbackMessageSize when it is set. I apparently missed this when reworking the CANMessages. I'll try to make a hotfix for this now.

Though this still doesn't explain why you don't even get a transport session started... Maybe a candump will tell more?

[ad3154]

Side note, we might be able to remove the approve step on these workflows, as I enabled the ability to allow forks to run our workflows and pull in the variable needed for sonar

[ad3154]

Everything seems to be running okay with your latest code, I was able to run all the VT examples with and without transferring the pools. But I worry a little bit about requiring people to destroy their ICFs or Partners... if one goes out of scope without destroy being called it doesn't get replaced by an external CF in the case of partners, and remains in the table and functional in the case of an ICF... I am wondering if we need to consider calling destroy in the destructor if it wasn't called otherwise, or at the very least log an error that it was improperly handled? I am wondering what your thoughts are on this.

[GwnDaan]

I am wondering if we need to consider calling destroy in the destructor if it wasn't called otherwise, or at the very least log an error that it was improperly handled? I am wondering what your thoughts are on this.

Since CANManager uses shared_ptr for keeping track of the control functions, the CF objects won't get destroyed unless they are not managed by the CANManager anymore. Hence doing anything with the CF in the deconstructor has little to no effect, since at that point they are already out of scope in the CANManager.

We could opt for using weak_ptrs instead in the CANManager, but I initially didn't chose to do so:

I choose to go with the shared_ptr approach instead of the weak_ptr approach. The main reason being that it might be really confusing and hard to debug why a control function suddenly stopped working when using weak_ptr and they invalidate when the created owner shared_ptr goes out of scope.

I didn't like the confusion that arises when you need to keep the 'owner' shared_ptr in scope, like with the EventDispatcher where we need to keep the shared_ptr to the listener in scope.

What are your thoughts one this?

[ad3154]

Hence doing anything with the CF in the deconstructor has little to no effect, since at that point they are already out of scope in the CANManager.

Hmm, yeah we'd almost have to save a weak pointer or similar in every CF back to the manager to do anything useful... which is a lot of overhead.

We could opt for using weak_ptrs instead in the CANManager

Yeah, I don't think that would be worth it... adds overhead as well to scan the table for expired weak pointers to prune them.

I didn't like the confusion that arises when you need to keep the 'owner' shared_ptr in scope, like with the EventDispatcher where we need to keep the shared_ptr to the listener in scope.

Actually that's a fair point, it's probably more arcane that users have to keep the return values from EventDispatcher around than it is to say the the manager has to own the control functions... I suppose you've convinced me, haha.

On a side topic, if we're moving that way towards the manager being the owner, we'll probably have to do some work in #161 to make what is currently std::shared_ptr<InternalControlFunction> get_internal_control_function(std::uint32_t index) specific to one instance of the network manager, and add something similar for partners so that users can lookup objects they've previously created but not destroyed? I am sure there will be a number of things like that to adjust...

[GwnDaan]

On a side topic, if we're moving that way towards the manager being the owner, we'll probably have to do some work in #161 to make what is currently std::shared_ptr<InternalControlFunction> get_internal_control_function(std::uint32_t index) specific to one instance of the network manager, and add something similar for partners so that users can lookup objects they've previously created but not destroyed? I am sure there will be a number of things like that to adjust...

I have made a good start on refactoring the CANNetworkManager a few weeks ago, I can already make a draft PR for your initial thoughts on it

[GwnDaan]

Nevermind, since the base branch of this PR is still on my fork, we can't compare to it on a draft PR. This link should do the trick instead: GwnDaan/AgIsoStack-plus-plus@smart-pointer-cfs...GwnDaan:ISO11783-CAN-Stack:remove-network-manager-singleton

Note how it still is very much work-in-progress. There is still a lot of classes to adapt to the new approach, but the core functionality should be almost complete. Namely the CANNetworkManager/TP/ETP/FP/CF's/AddressClaimStateMachine

[GwnDaan]

Side note, we might be able to remove the approve step on these workflows, as I enabled the ability to allow forks to run our workflows and pull in the variable needed for sonar

IIRC forks shouldn't have access to those variables, because of security reasons, or has that changed since we initially introduced this 'approve' thingy? Though I image we can remove the approve environment step if I develop on branches directly in this repo instead.

[ad3154]

IIRC forks shouldn't have access to those variables, because of security reasons, or has that changed since we initially introduced this 'approve' thingy? Though I image we can remove the approve environment step if I develop on branches directly in this repo instead.

IIRC it just allows them to run our workflows from a fork, and doesn't allow them to see the actual values, but developing on this repo directly is probably the best/easiest and keeps that kind of stuff as safe as it can be. I'll go back through and change it. I agree that the environment approval can probably be removed. We will always get asked to approve workflows from new contributors at least.

[GwnDaan]

Hi @ad3154, this took a while, but I think I finally got it now. I made sure that #192 is still working properly. This PR will probably be a little harder to review, sorry for that. Any feedback is welcome. I felt like and kind of still feel like the CANetworkManager class can use some rework. Maybe it gets better after we refactor away from the singleton pattern?

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
20 Code Smells

43.5% Coverage
1.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[ad3154]

Looking pretty good I think! Pretty big change, but a step in a better direction overall.

I also agree with your comments about the network manager needing some cleanup at some point.