Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 28, 2024

Conversation

GwnDaan
Copy link
Member

@GwnDaan GwnDaan commented Jan 26, 2024

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

@GwnDaan GwnDaan added the iso: data link Related to the ISO-11783:3 standard label Jan 26, 2024
@GwnDaan GwnDaan requested a review from ad3154 January 26, 2024 00:38
@GwnDaan GwnDaan self-assigned this Jan 26, 2024
Copy link
Member

@ad3154 ad3154 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems OK, probably makes sense to require these values up front anyways. Sonar's complaint about new we should maybe look at if/when we refactor fast packet protocol.

This should make it a bit more self-explanatory what the message needs to be functional, instead of some loosely-required functions you have to call
@GwnDaan GwnDaan force-pushed the daan/can-message-constructor branch from 3e07e0a to f43ee59 Compare January 28, 2024 17:40
@GwnDaan GwnDaan merged commit d2d47dd into main Jan 28, 2024
9 checks passed
@GwnDaan GwnDaan deleted the daan/can-message-constructor branch January 28, 2024 17:40
Copy link

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
Copy link
Member

ad3154 commented Jan 29, 2024

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
Copy link
Member Author

GwnDaan commented Jan 30, 2024

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:

void test_wrapper_process_rx_message(const CANMessage &message, void *parentPointer)
{
VirtualTerminalClient::process_rx_message(message, parentPointer);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iso: data link Related to the ISO-11783:3 standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants