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

Directly publish to /diagnostics_agg on non-ok status #197

Open
wants to merge 4 commits into
base: noetic-devel
Choose a base branch
from

Conversation

MCFurry
Copy link

@MCFurry MCFurry commented Jun 4, 2021

I relate to #48 and internally we started using a custom aggregator for this purpose.
We thought it could be useful to share this.

Moreover, this aggregator also fills the /diagnostics_toplevel_state message with the message that caused the non-ok status. We found this particularly useful to feed this to a robot operator via a display for example to have a clue about what is going on, without him needing a computer with Robot Monitor open. (Think of a display in a remote control, or a simple LCD on the robot) But also for logging purposes it could be nice to have this.

(My editor automatically removes trailing whitespaces, so sorry for those diffs..)

@g-gemignani
Copy link
Collaborator

Hi @MCFurry and thank you for your PR.
Please correct me if I am wrong, but the changes you are proposing are an ABI-breaking change. I am not sure how this kind of changes should be handled in the future... Maybe @trainman419 or @Karsten1987 can comment here since they were involved in a PR with the same issue in #88?

@MCFurry
Copy link
Author

MCFurry commented Jun 7, 2021

Thanks for your response.

I don't know about ABI compatibility, what is the main issue when breaking this?

Please advice if you agree on the functionality but have an idea about staying ABI-compatible.

@g-gemignani
Copy link
Collaborator

The main issue with ABI compatibility is that, by modifying classes onto which other packages rely on, in order to ensure a proper functioning of the downstream packages, we would have to ask everyone to rebuild such packages again. This is why, usually, ABI breaking changes are introduced when switching versions. Since noetic is the last version that will be published for ROS1, I am not sure how we should handle such changes. That is why I was asking the opinion of people more expert than me.

@MCFurry
Copy link
Author

MCFurry commented Jun 21, 2021

Ahh thanks for the explanation. I see the issue now, especially with ROS1.

I guess it will be tricky getting this feature in without ABI breaking changes then, but please advice if someone sees a way.

If the feature itself is still considered useful to others I can of course try to put this in the ROS2 version?

@g-gemignani
Copy link
Collaborator

The feature itself is definitely useful in my opinion. What could be maybe done is to add another class that inherits and extends the class that you are modifying. A user could, in this way, use one class or another depending on if the new behavior is wanted or not. By implementing a new class and not touching the previous one, I believe that the ABI compatibility would be kept.

@MCFurry
Copy link
Author

MCFurry commented Jun 21, 2021

That sounds like a neat solution indeed. If someone can advice whether this could indeed keep ABI compatibility I'll look into it!

@rokusottervanger rokusottervanger force-pushed the fix/48_immediate_diagpub_on_nonok branch from e4a42ff to 32f31dc Compare April 8, 2022 09:35
@rokusottervanger rokusottervanger force-pushed the fix/48_immediate_diagpub_on_nonok branch from 32f31dc to f1b65d6 Compare April 8, 2022 09:49
@ralph-lange ralph-lange added the ros1 PR tackling a ROS1 branch label Nov 21, 2022
@rokusottervanger
Copy link

That sounds like a neat solution indeed. If someone can advice whether this could indeed keep ABI compatibility I'll look into it!

I have time for this now, but I need confirmation that this direction will indeed lead to an acceptable PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ros1 PR tackling a ROS1 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants