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

Use diagnostics_msgs/DiagnosticStatus.msg iso custom Status message #62

Open
Achllle opened this issue Mar 20, 2020 · 4 comments
Open
Labels
good first issue Good for newcomers help wanted We're especially looking for input from the public on this one

Comments

@Achllle
Copy link
Collaborator

Achllle commented Mar 20, 2020

Currently the message osr_msgs/Status contains:

int64     battery
int64[5]  error_status
int64[5]  temp
int64[10] current

I propose:

  • battery should have its own topic, with message sensor_msgs/BatteryState
  • error_status should be reported in /diagnostics using diagnostics_msgs/DiagnosticArray which would allow the use of diagnostics tools
  • temperature should have its own topic, with message type sensor_msgs/Temperature
  • current should be reported in the same sensor_msgs/JointState message used to report feedback on the wheel positions and velocities under 'effort'.
@ericjunkins
Copy link
Collaborator

My one thought about this change is that now if you want to look at the overall health of the robot you have to look in many different places. I would like to maintain the ability to query one single location to get debug information about the robot, however I think there is also benefit to having the topics you proposed as well for other tools. I think it might make sense to just publish to both. In addition to this i agree we should clean up the message types used for these.

@Achllle
Copy link
Collaborator Author

Achllle commented Mar 25, 2020

Aggregation is typically solved using diagnostics_aggregator and robot_monitor like systems. The nice thing about it is that people can choose what tool they want to use for that. For debugging, I typically will set the log_level of whatever node I'm debugging to DEBUG and I'll get all the relevant information that way. I can also see the value of having both, but that's more stuff to maintain, especially if things change.

@Achllle Achllle added good first issue Good for newcomers help wanted We're especially looking for input from the public on this one labels Jan 27, 2021
@itstalmeez
Copy link

Like definitely its my opinion you are not supposed to go with this so, separating 'battery' into its own topic with sensor_msgs/BatteryState is a great idea. This will provide more detailed and standardized information about the battery status.

Moving 'error_status' to /diagnostics using diagnostics_msgs/DiagnosticArray is a wise decision. It promotes a cleaner separation of concerns, enabling the use of diagnostic tools for better error management.

Creating a separate topic for 'temperature' using sensor_msgs/Temperature will enhance the clarity of data and its intended use.

Including 'current' in the sensor_msgs/JointState message for wheel positions and velocities is efficient, as it groups related data together.

These changes will not only enhance the modularity of your project but also foster interoperability with other ROS packages.

@Achllle
Copy link
Collaborator Author

Achllle commented Oct 24, 2023

@itstalmeez you're welcome to create a PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted We're especially looking for input from the public on this one
Projects
None yet
Development

No branches or pull requests

3 participants