Open-Agriculture / AgIsoStack-plus-plus

AgIsoStack++ is the completely free open-source C++ ISOBUS library for everyone
https://agisostack.com/
MIT License
168 stars 40 forks source link

[Core]: Require complete message representation at construction time #420

Closed GwnDaan closed 5 months ago

GwnDaan commented 5 months ago

Describe your changes

This should make it a bit more self-explanatory what the message needs to be functional, instead of needing to know which loosely-required functions you have to call before you get a functional message. Also did a few cleanups of the CANNetworkManager where older things were still present or unclear

How has this been tested?

Ran the virtual terminal examples to ensure core communication is still correct

sonarcloud[bot] commented 5 months ago

Quality Gate Failed Quality Gate failed

Failed conditions

E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

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

ad3154 commented 5 months ago

One note... removing receive_can_message(const CANMessage &message); made it impossible to receive long messages in the unit tests without actually sending all the transport messages I think, which is kind of unfortunate. I had a unit test in the TC server for receiving the entire DDOP but now have to leave it out I think.

GwnDaan commented 5 months ago

removing receive_can_message(const CANMessage &message); made it impossible to receive long messages in the unit tests without actually sending all the transport messages I think, which is kind of unfortunate. I had a unit test in the TC server for receiving the entire DDOP but now have to leave it out I think.

@ad3154, sorry for that. I think it's better to try to test the TC server without involving the NetworkManager all together, like I managed to do with the (extended) transport protocol tests. Although that would require making some callback for sending/receiving messages, exactly the problem you're facing right now.

Another much more simpler option is to just wrap the receive function of the TC server for testing, like you did with the VT client here: https://github.com/Open-Agriculture/AgIsoStack-plus-plus/blob/0192b52f8e441824ce6b37d5de0371eb8d58bae4/test/vt_client_tests.cpp#L21-L24