[DP] Refactor away from diagnosticProtocolList

GwnDaan commented 1 year ago

Changes:

Major:

Refactor away from the diagnosticProtocolList and let the user create objects instead
Refactor away from extending CANLibProtocol to be more inline with newly added interfaces (Guidance/speed/maintain power) to have a more "The library should only do work when it is explicitly asked to" experience.
Add dedicated NetworkType configuration parameter
Introduced initialize / terminate as replacements for assign_... / deassign_....

Minor:

Use in-class initializers where possible
Fixed several typos
Apply reference-to-const variables and const functions where possible
Remove redundant case ' clauses'
Remove redundant parenthesis
Moved parse_j1939_network_states function from public to private

@ad3154, note that I don't have any way of testing if this actually works. I just refactored up to a point where I think I can better fix #161. Also we need to expand on the unit test cases for this diagnostic protocol, but I'm not really familiar with the documentation. Any chance you have time to take this over?

ad3154 commented 1 year ago

Any chance you have time to take this over?

I'll do what I can, I have some travel coming up soon-ish but I might be able to make headway before then. I have a reliable way to test this interface.

I think generally it's probably a good idea to make this more like our other interfaces. It needed some updates to be sure, especially to support #161. I guess the reason this was done different in the first place was that users basically need this interface to be ISOBUS compatible, so it felt like making it optional might result in people not knowing they need it. Many ISOBUS applications will query everyone's functionalities, software ID, and ECU ID, so if we're going down this path of refactoring it, we may need to be extra clear in the examples and tutorial docs that this is super strongly suggested haha.

GwnDaan commented 1 year ago

I guess the reason this was done different in the first place was that users basically need this interface to be ISOBUS compatible, so it felt like making it optional might result in people not knowing they need it.

I see what you mean here, and I agree, but as far as I can see it never has been not optional? As one still needed to use the assign_diagnostic_protocol_to_internal_control_function for the protocol to do anything at all. Or am I missing something here? I'm not against enabling the diagnostics functionality by default, but then we should probably let the CANNetworkManager handle it as the DiagnosticsProtocol requires at least a single ICF to be registered?

ad3154 commented 1 year ago

as far as I can see it never has been not optional?

Totally agree, that was a shortcoming in the old implementation, so I had made it a protocol to get automatic updates from the stack but failed to make it truly automatic...

I am not sure if we can make the network manager really handle it, unless it automatically attaches an instance of this class to every ICF, which is still not super intuitive then that users need to configure it... plus the manager is already so big... I guess for the moment I'm happy enough with making it like our other, nicer interfaces if you are.

Side note, I am working unit tests for this in my evening free time and found some other smaller issues that I'll open as other PRs soon...