Closed emersonknapp closed 4 years ago
Publishing
Hmm, that's interesting. We never actually published battery state with the ros1 version. It did publish details on diagnostics though. I suspect that was all about optimising the streams of data - one sensordata stream at high rate, event streams at low rate, and one medium rate diagnostic stream ... all in preference of N streams at high rates.
Still, this is a super nice convenience and I like the idea of supporting a) convenience and b) efficiency if desired.
Turn it into a lazy publisher - i.e. check if there are subscribers at the beginning of publishBatteryState
, if not, don't do anything
Battery State
Also, battery state. Set power_supply_technology
to sensor_mgs.msg.BatteryState.POWER_SUPPLY_LION
and where you don't have relevant fields set, it looks like BatteryState.msg is recommending they be set to NaN.
Other
There's also the matter of differentiating charging while docked vs charging while plugged, but we don't need to solve that here today.
Would having the percentage (alongside the voltage) in the sensor stream data be useful to you?
Thanks for the feedback, I'll happily incorporate that.
I am most interested in the percentage and whether we are charging or not, from the perspective of my monitoring - it feels more generic than voltage across different robots, for setting alarms in dashboards (i'm using prometheus and grafana - there's a node that reads the batterystate and publishes it to prometheus alongside other system info).
I'm not that interested personally in the Diagnostics stream, I feel like that framework in ros1 had a lot to be desired, so i'm not sure about trying to use it again in ros2 - but that's just me.
Something I like about the BatteryState is that it's a well-defined schema as opposed to the free key-value of the diagnostic stream. I'm considering making a rqt or rviz visualization panel for the batterystate, which could be really helpful at a glance when monitoring a robot.
PR Update
Let's get these in, I'm happy with this as the source of the battery % provider. I did check over the sensor state message as an alternative, but it's really not the right place. It looks like a very deliberate copy of the raw data for debugging / rare use cases.
Diagnostics
I know how you feel - running into ROS1 diagnostics for the first time felt like regressing. However, we later founds ourselves with a fleet of robots (not kobukis), QA engineers running 9-5 and field testing for weeks - suddenly the ability to diagnose bringup or current health of the robot at a glance from a gui was invaluable, regardless of the awkwardness behind it. In addition, all that stringifying became useful - it let you translate numbers into the meaningful human-readable messages to be displayed on the gui without a nightmare dependency chain on message types. It's an ugly job, and there really aren't many alternatives (doing it from scratch even worse).
Having said that, there's not much use case for it if you're a developer and there's only a couple of robots. Just an extra layer in the onion.
Updated:
battery
to battery_state
for consistencyThanks @emersonknapp!
Ah, thanks for making the extra changes - I usually only work on the kobuki stuff over the weekend when I find time. Much appreciated :)
No worries, I had to push in some header changes and wanted to avoid creating merge conflicts.
I have existing infrastructure that likes to use BatteryState to generically understand robot status. It seems appropriate to publish it here. I am not sure if triggering it this frequently is necessary, but it seems fine as a start.