-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
[RP]: Remove internal static pgn request protocol list #285
Conversation
457bcf4
to
39c90ee
Compare
Apologies for my lateness on reviewing this - I will try to review this weekend! |
I have a few thoughts after reading this, let me know what you think. PGN requests are a very important part of participating on both an ISO11783 and J1939 bus, and the standards are very strongly worded with such things like:
And since we have to send messages as responses, even if the user of the stack doesn't do any PGN requests themselves, I think we have some opportunities here to make this whole thing less weird and more automatic. My initial thinking is that maybe each Internal Control function should create a dedicated PGN request object, and we could just have a getter on the ICF to retrieve it (maybe a weak ptr makes the most sense here? It will only rarely be accessed by those other objects like Diagnostic Protocol...) I think this might make the most sense, as then all those other objects that need it can simply get it from the ICF that is passed into them at construction time, and it ensures all ICFs will respond properly without the user configuring it. It also continues to allow the network manager access to them to call Does that make sense? Looking forward to your/anyone's opinion. |
I agree I think housing it inside the internal control function makes the most sense as well, I'll go ahead and make that change |
f705d28
to
d2bfc47
Compare
SonarCloud Quality Gate failed. 0 Bugs 68.2% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looking good I think. I'd like to test a on a real bus but it looks like it'll work.
I'll try an run a couple manual tests and as long as they work I'll merge!
Like with the DiagnosticProtocol's static list removal in #281, this PR removes the static list in the
ParameterGroupNumberRequestProtocol
and instead let the application handle the objects themselves.Though a drawback of this approach I found, is that every other interface that requires "pgn requests" support (e.g. the LanguageInterface/DiagnosticProtocol) need to be fed with a shared_ptr to a
ParameterGroupNumberRequestProtocol
object. We could instead house the list ofParameterGroupNumberRequestProtocol
inside the CANNetworkManager or assign to each InternalControlFunction an optionally dedicatedParameterGroupNumberRequestProtocol
object (that get's automatically constructed/deconstructed when needed)?On the other hand, we can also just leave it as is, but then we keep a difference between the interfaces/protocols. Any thoughts on all this are more than welcome