ROS 2 RFC: Rules for field names in Interface files

[Esteve Fernandez]

Hi,

we have a pending proposal to enforce stricter rules on the contents
of Interface files (.msg and .srv files), but this RFC is specifically
about the rules for field names in interface files. Currently we allow
upper case letters in interface files, e.g. CameraInfo has fields `D`,
`K`, `R`, and `P`. We'd like to restrict field names to all lowercase
and underscores (plus some other, less controversial rules). For a
full set of the proposed rules, you can look at this work in progress
design doc:

http://design.ros2.org/articles/interface_definition.html#conventions

The advantages to enforcing these rules (and consequently changing
field names in messages like CameraInfo) is that anyone can look at a
member of a generated message struct, e.g. `CameraInfo.D`, and tell if
the member is a field or constant. It also helps us avoid name
collisions in the generated code (similar to the rule about not having
trailing `_` nor double `_` in the field name). Also, having a style
and sticking to it has an inherent value and promotes good conventions
and more readable code.

The arguments against doing the change: An Indirect consequence of
these stricter rules is that it might encourage more non-trivial
changes to field names than would otherwise occur. For example, rather
than converting the field name `D` (an actual field name in
`CameraInfo`) to `d` as a result of these stricter rules, it might be
changed instead to `distortion_vector` for clarity. This is probably a
good thing, but it will add to the disruption of migrating to ROS 2.
For the ROS 1/ROS 2 bridge that would require some manually specified
conversion rule (similar to a rosbag migration rule) to perform the
conversion automatically. However, even with ROS 1.x, it’s relatively
easy to tell whether a member of a generated message is a field or
constant based on the context.

I think I've summarized the pros and cons of the issue well, but if
I've missed something please jump in and add to the conversation.

In the short term we will be turning on the strict enforcement and
changing the fields in the messages that are affected. In the
meantime, we'd like to also solicit feedback from you about this
issue, and if we reach a different conclusion based on pros or cons we
did not come up with, then we can change our policy and potentially
the message fields again.

Cheers.

[Jack O'Quin]

-1 The advantages listed above do not seem to justify the global hassle of updating all packages using these messages (not just core OSRF programs). 

--

 joq

[Jonathan Bohren]

On Fri, Jul 3, 2015 at 5:12 PM Jack O'Quin <jack....@gmail.com> wrote:

On Wed, Jul 1, 2015 at 9:05 PM, Esteve Fernandez <est...@osrfoundation.org> wrote:

I think I've summarized the pros and cons of the issue well, but if

I've missed something please jump in and add to the conversation. 

-1 The advantages listed above do not seem to justify the global hassle of updating all packages using these messages (not just core OSRF programs). 

I also don't think changing this for the sake of capitalization conventions is worth the hassle. I'm not sure anyone I've ever talked to has had problems determining whether an identifier is a field or a constant.

Meanwhile, the "disruptions" involved in migrating from ROS to ROS2 seem like they are accumulating day by day. It would be informative to have OSRF  maintain a ROS to ROS2 migration guide, and consider the effects of each proposed ROS2 change on the cost of migrating code from ROS.

-j

[Esteve Fernandez]

Hi,

Jack, Jonathan, thanks for your feedback. We believe these changes are
useful for making messages more expressive, but we share your concerns
about making it a strict requirement and all the effort it'd require
to update existing ROS packages.

How would you feel about allowing the current ROS 1.x naming rules
(i.e. current ROS 1.x messages would be supported), and us providing a
tool (i.e. a linter) that analyzes message definitions and informs
users about the proposed naming guidelines?

Thanks.

> --
> You received this message because you are subscribed to the Google Groups
> "ROS SIG NG ROS" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ros-sig-ng-ro...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Bob Dean]

Short version, esteve's idea about a warning would be good compromise. Perhaps With directions on how to modify the warbling logic so that a company could adapt it to their conventions?

Long version I posted in another thread by accident: "I would suggest allowing lower camel case as variable names. It is a very common convention, and allowed by ros1 msggen, and if a company has been using it for their messages due to company naming conventions, moving to ros2 becomes a substantially larger effort. Nearly every line of code may need to be touched in some cases. In an industry where engineer time is >$1000 a day that cost adds up very fast and is a factor in choosing ros2 over another solution. It would probably be cheaper for he company to hack the msggen scripts to allow it.

Lower camel case is not an excuse for non descriptively named variables."

Sent from my iPhone

[Jonathan Bohren]

I think if there was a tool that could produce a warning, such a tool should provide an option to disable the warning (as in Bob's case).

-j

[William Woodall]

Sorry, that should have been implied. The linter would either be opt-in or opt-out, but either way it can be disabled.

William Woodall

ROS Development Team

wil...@osrfoundation.org

http://wjwwood.io/

[Bill Morris]

If I understand the issue correctly. Maybe the rule could be that field
names must be 'Not all caps' , while constants must be named 'ALL CAPS'
and greater than 1 character. This way camel case and most everything
else stays valid.

[Jack O'Quin]

Unfortunately, that still breaks common messages like CameraInfo.

With all the effort required to convert packages to ROS 2, I see no adequate justification for introducing additional friction by changing the ROS 1 message rules. Why throw away part of the migration advantage we gained by supporting ROS 1 message definition files? 

I have no objection to the lint idea, although I agree with Bill: just flag non-constant fields that are all ALL CAPS and constant ones that are not. 

We have a lot more important issues to deal with than this.

+1 to continuously updating the migration guide as a design document whenever any breakage is introduced.

-- 

 joq

[Robert Dean]

I am going to disagree with Jack. Some of the common messages need to be broken. or technically it would be 'fixing' them. The names in CameraInfo specifically are horribly non descriptive. Changing the handful of core messages to make them more descriptive, and therefore the core easier to understand should the user base continue to expand, is a manageable migration risk. Having to change all of an entity's custom messages is higher risk.

--

[William Woodall]

A separate proposal/concern is whether or not to make changes to the field names in CameraInfo. The question at hand is whether or not the convention can be changed to be more inclusive but still provide benefit.

I happen to agree that CameraInfo's field names are not great. And though I agree with Jack that there are more important topics, there really isn't a better time to consider changes to messages than right now. However, we should propose those changes separately, as I did with the proposal to remove the seq field from Header.

[Jack O'Quin]

On Wed, Jul 8, 2015 at 3:49 PM, William Woodall <wil...@osrfoundation.org> wrote:

A separate proposal/concern is whether or not to make changes to the field names in CameraInfo. The question at hand is whether or not the convention can be changed to be more inclusive but still provide benefit.

I happen to agree that CameraInfo's field names are not great. And though I agree with Jack that there are more important topics, there really isn't a better time to consider changes to messages than right now. However, we should propose those changes separately, as I did with the proposal to remove the seq field from Header.

On Wed, Jul 8, 2015 at 11:21 AM, Robert Dean <bob....@gmail.com> wrote:

I am going to disagree with Jack. Some of the common messages need to be broken. or technically it would be 'fixing' them. The names in CameraInfo specifically are horribly non descriptive. Changing the handful of core messages to make them more descriptive, and therefore the core easier to understand should the user base continue to expand, is a manageable migration risk. Having to change all of an entity's custom messages is higher risk.

You are right to say that CameraInfo could use some improvements, perhaps even in ROS 2. I am *not* defending those silly field names: D, K, R, and P. I agree with Will that those changes should be handled separately.

I mainly object to making blanket changes to the syntax of every message definition.

--

 joq

[Bill Morris]

Besides the D, K, R, and P fields in CameraInfo, is anything currently
using ALLCAPS for any message field names that isn't already a constant?

./hydro/share/stereo_msgs/msg/DisparityImage.msg
float32 T # Baseline, world units
This was the only one I found in a quick search.

I would support throwing an error for field names (i.e. ID) in ALLCAPS
greater than 1 character in length
I would support throwing a warning for field names in the range of [A-Z]

[Vincent Rabaud]

ok, I thought it was more obvious for CameraInfo but apparently not: the message was created by vision / OpenCV people. To us, it is obvious that K is a calibration matrix, P a projection matrix, R a pose rotation, t a pose translation ... (cf https://www.ics.uci.edu/~majumder/vispercep/cameracalib.pdf). This is a convention that was popularized by the book "Multiple view geometry" by Hartley and Zisserman.

Now, this is a math/matrix notation and if there is a new msg standard, sure, let's make things more roboticsy or science agnostic, let's reach as many people as possible. I would totally be down for creating special IDL for converting messages from ROS1 <-> ROS2 messages (like protobuf does I think).

But I believe this is a larger debate that is independent from ROS2. Here are other points that I'd like to be tackled whenever:
- messages need to be homogeneized (e.g. several ones for object recognition)
- messages need to be deprecated (e.g. PointCloud)

- messages need to be re-thought (e.g. PointCloud2 : a mapping from channel to vector would make more sense (instead of mixing types like we do, which wastes space and complexifies the parsing))

- other messages are weird to me:
    - why is the quaternion in Pose.msg named "orientation", but in Transform "rotation"
    - to me Transform.msg should be named RigidTransform.msg.

    - why Point32.msg and Point.msg , why not Point64.msg

    - Quaternion.msg uses x, y, z, w and not w, x, y, z leading to some incomprehension (https://github.com/mrdoob/three.js/issues/2250)

  Maybe it is obvious to some of you, if so, let's quote the book/site reference/REP in the message itself (e.g., as I do help sometimes and not just complain :)  https://github.com/ros/common_msgs/commit/168e19b72afc53fb8aad5cff0b2a5796ace40a8d#diff-b1009d9f62f18bf8c491348b99660cb3)

Maybe a special message SIG ?

--

[Jack O'Quin]

On Thu, Jul 9, 2015 at 11:09 AM, Vincent Rabaud <vincent...@gmail.com> wrote:

ok, I thought it was more obvious for CameraInfo but apparently not: the message was created by vision / OpenCV people. To us, it is obvious that K is a calibration matrix, P a projection matrix, R a pose rotation, t a pose translation ... (cf https://www.ics.uci.edu/~majumder/vispercep/cameracalib.pdf). This is a convention that was popularized by the book "Multiple view geometry" by Hartley and Zisserman.

Now, this is a math/matrix notation and if there is a new msg standard, sure, let's make things more roboticsy or science agnostic, let's reach as many people as possible. I would totally be down for creating special IDL for converting messages from ROS1 <-> ROS2 messages (like protobuf does I think).

But I believe this is a larger debate that is independent from ROS2. Here are other points that I'd like to be tackled whenever:
- messages need to be homogeneized (e.g. several ones for object recognition)
- messages need to be deprecated (e.g. PointCloud)

- messages need to be re-thought (e.g. PointCloud2 : a mapping from channel to vector would make more sense (instead of mixing types like we do, which wastes space and complexifies the parsing))

- other messages are weird to me:
    - why is the quaternion in Pose.msg named "orientation", but in Transform "rotation"
    - to me Transform.msg should be named RigidTransform.msg.

    - why Point32.msg and Point.msg , why not Point64.msg

    - Quaternion.msg uses x, y, z, w and not w, x, y, z leading to some incomprehension (https://github.com/mrdoob/three.js/issues/2250)

  Maybe it is obvious to some of you, if so, let's quote the book/site reference/REP in the message itself (e.g., as I do help sometimes and not just complain :)  https://github.com/ros/common_msgs/commit/168e19b72afc53fb8aad5cff0b2a5796ace40a8d#diff-b1009d9f62f18bf8c491348b99660cb3)

Maybe a special message SIG ?

I prefer to discuss messages in the context of a SIG comprised of existing users. The drivers SIG (ros-sig-drivers) is pretty experienced at handling a lot of those kinds of messages, and generally has good judgement. Perhaps, the Perception SIG should discuss CameraInfo. PointCloud2 is mostly about perception, although it does affect a number of drivers. 

Nor should we go on a hunting expedition looking for messages to update. Most of them work fine across an impressive variety of systems. ROS 2 is going to be hard enough. Let's not create any unnecessary migration effort.

--

 joq

[William Woodall]

Vincent, I understand why CameraInfo is setup the way it is, and even though I don't find the fields particularly descriptive it isn't clear to me that they should be changed.

I think it's best to consider each of these proposals individually, like I did for the removal of `seq` (shameless plug, vote on it if you haven't already: https://github.com/ros2/common_interfaces/pull/2). There's always the concern of migration, but if we ever intend to correct any of these known issues/inconsistencies then now is the time to do so.

--

You received this message because you are subscribed to the Google Groups "ROS SIG NG ROS" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ros-sig-ng-ro...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Tully Foote]

Although some of these are non-optimal. For most of these the drivers SIG is probably a good place to discuss low level changes like these, there have been several similar discussions. We should still seek to only do highly valuable changes and avoid unnecessary changes. I don't think most of these reach that level.

I've answered some of you specific questions about datatypes below. Several of them were design decisions

Tully

On Thu, Jul 9, 2015 at 9:09 AM, Vincent Rabaud <vincent...@gmail.com> wrote:

ok, I thought it was more obvious for CameraInfo but apparently not: the message was created by vision / OpenCV people. To us, it is obvious that K is a calibration matrix, P a projection matrix, R a pose rotation, t a pose translation ... (cf https://www.ics.uci.edu/~majumder/vispercep/cameracalib.pdf). This is a convention that was popularized by the book "Multiple view geometry" by Hartley and Zisserman.

Now, this is a math/matrix notation and if there is a new msg standard, sure, let's make things more roboticsy or science agnostic, let's reach as many people as possible. I would totally be down for creating special IDL for converting messages from ROS1 <-> ROS2 messages (like protobuf does I think).

But I believe this is a larger debate that is independent from ROS2. Here are other points that I'd like to be tackled whenever:
- messages need to be homogeneized (e.g. several ones for object recognition)
- messages need to be deprecated (e.g. PointCloud)

- messages need to be re-thought (e.g. PointCloud2 : a mapping from channel to vector would make more sense (instead of mixing types like we do, which wastes space and complexifies the parsing))

That sounds a lot more like the original PointCloud. I don't think we have enough control in the msg idl to setup the optimal iterators. I think that the custom deserialization types into processing libraries native datatypes is actually a better solution.

 

- other messages are weird to me:
    - why is the quaternion in Pose.msg named "orientation", but in Transform "rotation"

 

These words are semantically different, just like their datatypes. An pose is a representation of the state of an object and can be broken down to the cartesian position and angular orientation. A transform defines a change in position which when broken down into cartesian and angular. A cartesian change in position is a translation and an angular change in position is a rotation.

    - to me Transform.msg should be named RigidTransform.msg.

it is a little ambiguous but I think that changing it will cost more than the reduction in ambiguity. When we get a non-rigid transform type we can call it out specifically.

 

    - why Point32.msg and Point.msg , why not Point64.msg

We chose 64bit as the default resolution for floating point message datatypes. The *32 datatypes were only created where space savings would make a significant difference. The main example being point clouds.

 

    - Quaternion.msg uses x, y, z, w and not w, x, y, z leading to some incomprehension (https://github.com/mrdoob/three.js/issues/2250)

  Maybe it is obvious to some of you, if so, let's quote the book/site reference/REP in the message itself (e.g., as I do help sometimes and not just complain :)  https://github.com/ros/common_msgs/commit/168e19b72afc53fb8aad5cff0b2a5796ace40a8d#diff-b1009d9f62f18bf8c491348b99660cb3)

As in the link there's no real standard as to which order you present them. We followed Bullet which uses xyzw. Though that's not actually in the msg as it requires named member based access.

[Jack O'Quin]

On Fri, Jul 10, 2015 at 8:06 AM, Tully Foote <tfo...@osrfoundation.org> wrote:

Although some of these are non-optimal. For most of these the drivers SIG is probably a good place to discuss low level changes like these, there have been several similar discussions. We should still seek to only do highly valuable changes and avoid unnecessary changes. I don't think most of these reach that level.

I don't see any need for most of those changes, either. 

As far as I know, PointCloud has been deprecated for years. Is it still actively used? We could drop it from ROS 2.

I don't understand the PointCloud2 issues like Vincent and Tully do, but I am reluctant to change it without some fairly compelling reason. 

--

 joq

[Jonathan Bohren]

What I'd like to see out of ROS2 is a future-proofing of these kinds of design problems. For example, for semantically-identical messages with different field labels, why can't we design a system where each user of the message can pick one of the versions of the field labels he or she wants to use. This could easily be done in dynamically-typed languages like python, and for statically-typed languages like C++ we could probably use namespace scoping or template metaprogramming to specify versions.

Then all we need to do is adjust the default message version as we release subsequent ROS distros, and migration becomes as easy as specifying the version when you include messages, specify their type names, or even just globally with compilation flags.

-j 

[Daniel Stonier]

I'm a little bit agnostic on this discussion - mostly just works for me and isn't a great bother to sort out what the fields are. A good ide will usually pop up a dialog showing you the code where it's defined and it's not complicated. And if it's still not enough, then I go directly to the message file where I can find comments.

That second point is pertinent I think. Even a good variable name is often not enough to describe how the variable should be used in more complicated messages. And it may be a variable from a nested message which finds itself under constraints in the message that is using it. At this point the syntax used for displaying the variable is mostly irrelevant and these comments become even more important.

I do like how the comments are shown in the MsgApi on the wiki links.  If things were to be easier what I'd like to see is `rosmsg show geometry_msgs/AccelWithCovariance` show me the comments as well, preferably in high fidelity colour!

Daniel.

--

You received this message because you are subscribed to the Google Groups "ROS SIG NG ROS" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ros-sig-ng-ro...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Phone : +82-10-5400-3296 (010-5400-3296)
Home: http://snorriheim.dnsdojo.com/

Yujin Inno: http://inno.yujinrobot.com/