Adding more unit tags for yaml files

[BrunoB81HK]

Hi!

I was browsing to the code and found that tags such as !degrees or !radians can be used to set a certain value as a certain unit. I think this is great! I expended this feature to include other units such as !meters, !turns, !inches or !foot.

I also adapted the tests to include these new units.

Let me know what you think!

[rhaschke]

Thanks for this generalization. Looks good. Could you please rebase this onto the noetic-devel branch? I will merge into ROS2 later.

Please document the new feature in the wiki.

[gavanderhoorn]

I realise xacro is -- sort of -- not tied to ROS, but would it not be better to keep this all to SI units only? We have REP 103 for a reason.

Personal perhaps, but I would argue against being able to specify my robots position limits in turns.

[rhaschke]

I agree that the list of added units is somewhat overshooting. On the other hand, is the use of those units optional 😉
Eventually, they are converted to meters or radians.

[gavanderhoorn]

In my experience, if something is supported, people will start to use it.

There is value in freedom-from-choice. Sticking to a limited, standardised set of supported units keeps things predictable, understandable and homogeneous -- which is sort of what the motivation for the creation/invention of SI units seems to have been.

If someone really wants to use imperial units, they could use xacros built-in support for math expressions. Yards, feet and inches aren't even SI derived units :)

[rhaschke]

I can fully understand your argumentation, Gijs. I'm tending to reject this PR given your concerns.
@BrunoB81HK, please argue why do you need all these extra units. Maybe we missed something.

[BrunoB81HK]

First of all, thanks a lot for your time reviewing this PR.

I tend to agree with @gavanderhoorn that SI units should always be prioritized and that defining your robot positions in turns is absurd. I also agree that, given the option, some people will start to use it other non-SI units and that there can be value in freedom from choice.

I guess where I disagree is the context in which this feature will be used. From experience, configuration files tend to inherit their units from the situation, not the other way around. I work in an industry where everything is designed inside the imperial unit system. The fact that xacro doesn't let me use this system in configuration files is more of an annoyance than anything else.

I also think that there is value in explicitly declaring the units of configurations. This removes any ambiguity or any need to remember wether the base unit is a meter or a millimeter, especially when using multiple software that may have different conventions. While using xacro's built-in support for math expressions for unit conversion is possible, I believe such an approach would lead to even less predictable and less understandable files and code.

I think this PR is more about adding a "quality of life" feature to xacro.

[gavanderhoorn]

Maybe adding support for conversions for the most used imperial units should be allowed. I'm not convinced, but I'm obviously in "SI land" already.

I'm also just one user who happened to randomly decide to post some comments on a PR I happened to come across, so in the end, it's really @rhaschke's decision.

Having said that:

I also think that there is value in explicitly declaring the units of configurations. This removes any ambiguity or any need to remember wether the base unit is a meter or a millimeter, especially when using multiple software that may have different conventions.

there really isn't (or shouldn't be) any possibility for ambiguity: ROS uses SI units everywhere. It's always metres for distances, radians for angles, etc, etc.

That's exactly what REP-103 standardises.

I understand the annoyance of having to convert imperial to metric, but I don't believe we can use ambiguity as a rationale for introducing these additional conversions. If you're using ROS messages, services, actions, etc, use SI units.

[BrunoB81HK]

To be clear, I hate that I have to vouch to include support for imperial system units in a software. I tend to agree on all your points theoretically. In practice, I think it is different, especially in North America.

Maybe adding support for conversions for the most used imperial units should be allowed. I'm not convinced, but I'm obviously in "SI land" already.

I agree that I went overboard with the unit choices. inches and foot should really be the only imperial system options for distances.

there really isn't (or shouldn't be) any possibility for ambiguity: ROS uses SI units everywhere. It's always metres for distances, radians for angles, etc, etc.

That is true for purely ROS software environments. Existing software or configuration files that need to interface with ROS could be using the imperial system. I can easily foresee someone misremembering the units that are needed for this file. Specifying the units helps with the ambiguity here.

Also, it take for granted that the persons who write the configuration files is familiar with ROS. I intend to use yaml configuration files as an easy way for other/new users/designers to quickly set up a robotic cell. In that context, specifying the units also helps with the ambiguity. That being said, this specific use-case is maybe too niche or unique to me.

[rhaschke]

I can fully understand both your positions. What about restricting the units to the following:

• radians
• degrees
• meters
• millimeters
• foot
• inches

[BrunoB81HK]

I think this is a pretty good compromise!

[rhaschke]

Great. Would you adapt your PR accordingly?

[BrunoB81HK]

Yes I will as soon as I can. Also, I will also rebase it unto the noetic-devel branch and add documentation in the wiki.

Thanks a lot!

[gavanderhoorn]

Please also document the fact using these new constructors would make the .yaml files incompatible with the ROS parameter loader constructors, which is where this feature originally 'started' (#251).

[BrunoB81HK]

Please also document the fact using these new constructors would make the .yaml files incompatible with the ROS parameter loader constructors, which is where this feature originally 'started' (#251).

Wait, I didn't know that. Is it not kind of a big deal?

[gavanderhoorn]

Wait, I didn't know that. Is it not kind of a big deal?

in my opinion? Yes, it would be :)

But I doubt we'd have any chance of getting any new functionality into ros_comm, seeing the state of maintenance there.

[rhaschke]

Any more objections, @gavanderhoorn?

[gavanderhoorn]

Nope (apologies for the delayed response).