Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add debug broadcast enable/disable option and debug broadcast port option to config file #309
Add debug broadcast enable/disable option and debug broadcast port option to config file #309
Changes from all commits
f1a4e45
824eca8
0545484
1b4e28c
8cd6450
dea9c13
b3e5594
c6a639f
c46a8dc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Did you have a particular reason changing the phrasing here?
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.
There are now 2 different UserLan ports that the user can specify (monitor and debug broadcast), so I added the word "monitor" to clarify which one this condition checked. But then that put it above 32 characters (the alarm message limit), so I chose to shorten "Invalid" to "Bad" to fit in the character limit. I think that "invalid" is a better word for that, but it's more important to be specific about which config setting needs to be changed.
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.
So should we instead separate parsing and error reporting for monitoring and debug log port?
Would lead to some mild code duplication, but would also make parsing clearer and avoids this situation.
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.
I don't believe that this specific message is affected by combined parsing/error reporting for the two port config options. My preference would be to keep the parsing as combined, but I would be fine with changing it.
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.
I understand why you opted to reuse the
Ros_UserLan_Port_Setting
type, but it does make the parsing code a little harder to read.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 I tried doing it both ways, and this had much less duplicated code and was overall cleaner in my opinion. I don't especially like it either though.