Change comment of percentage in BatteryState.msg

[RasmusLar]

For the BatteryState percentage to be correctly named, it should be in the range of 0-100. The current naming is misleading, as it suggests the percentage is in the range of 0-1, which is not a percentage, but a fraction.

Another option would be to change the name to ratio or charge_level and keep the value range to be 0-1, but that would be a breaking change.

[RasmusLar]

Seems to me that Jenkins is failing internally for the build, maybe just a retriggering is enough? I also assume a comment shouldn't cause that :)

[christophebedard]

The existing name + comment could indeed be confusing if you just see "percentage," but mentioning "0 to 100" is not correct, and changing the name isn't really trivial at this point.

You could modify the comment to make it clearer that it is a ratio between 0 and 1:

-float32 percentage # Charge percentage on 0 to 1 range (If unmeasured NaN)
+float32 percentage # Charge percentage as a ratio between 0 and 1 (If unmeasured NaN)

[christophebedard]

Seems to me that Jenkins is failing internally for the build, maybe just a retriggering is enough? I also assume a comment shouldn't cause that :)

Yeah, that's a infrastructure flake. You can retrigger it with (well I think you can do it, not sure):

@ros-pull-request-builder retest this please

[fujitatomoya]

Or something like this?

Suggested change

	float32 percentage # Charge percentage on 0 to 100 range (If unmeasured NaN)
	float32 percentage # Charge percentage as a normalized value in the range [0.0, 1.0] (If unmeasured NaN)

[christophebedard]

Yeah, that works for me!

[RasmusLar]

The issue isn't really the comment at all. The issue is that the variable is called percentage, literally "per hundred" from latin, and by definition a unit which ranges from 0 to 100 for any ratio 0 to 1. It would be exactly the same as calling a variable mph and then there is a comment that it should be in kilometers per hour.

When users unaware of this comment reads code using this, like happened to me a few days ago, it might just look like this:

BatteryStateCallback(msg::BatteryState::ConstSharedPtr const msg) {
	this->battery_state.battery_charge  = msg->percentage;

and just reading this, I would assume percentage is literally a percentage. Only reason I really started looking into it was cause I was reading the source code for another project and got thrown off by an unexpected added * 100 in the end.

[RasmusLar]

I dunno if I was clear about what I mean, I think the variable name is more important to be correct than the comments and documentation around it, with that in mind I only see a couple options:

• Change the documentation to reflect the actual naming of the variable, allowing implementations to correct over time
• Deprecate the variable and add a new one that reflects the intention, allowing implementations to change when they have time
• Rename the variable, forcing the implementations to change on update.

[christophebedard]

You were 100% clear, but like I said changing the name is not trivial, and I personally think it's not worth the headache. On the other hand, changing the comment to make this more obvious is totally doable.

Change the documentation to reflect the actual naming of the variable, allowing implementations to correct over time

Changing what the field represents (i.e., changing the range from [0, 1] to [0, 100]) while keeping the same name is not possible, because there is no way to ensure that downstream users "updated" their use of it. If you get a value of 0.95, how do you know whether this is 95% or 0.95%?

Rename the variable, forcing the implementations to change on update.

I don't think we would ever do a clean change like that unless it was a critical issue. We'd do something like a tick-tock deprecation, but that doesn't work well with interfaces.

1. Add a new field with a better name. If the old field is 0.0 (default) and the new field is not 0.0, use the new field. If the new field is 0.0 (default) and the old field is not 0.0, use the old field. If both are 0.0, use 0.0. Mark the old field as deprecated (which we can't really do for interfaces, unlike normal APIs).
2. For the next distro, remove the old field.

But again I personally don't think it's worth it.

[RasmusLar]

Mark the old field as deprecated (which we can't really do for interfaces, unlike normal APIs).

Hmm, this could be another improvement proposal for ROS2 project then? :)

If you get a value of 0.95, how do you know whether this is 95% or 0.95%?

But again I personally don't think it's worth it.

I would think it's worth it, cause right now, I would think downstream users using 0-100 already, only due to the naming itself, though I guess as soon as they tested it with established packages they would realize somethings off and they would read the docs.

But, agree to disagree, in the end it's up to you maintainers, so I will leave it to you to do as you like 😄

[christophebedard]

Do you still want to proceed with improving the comment?