Update REP-I0001 with implementation-related changes/clarifications

[JeremyZoss]

I'm working to implement this REP, and had a few suggestions for how it might be changed to improve flexibility and ease-of-use. I also wanted to add a bit more explanation about intended usage and operation, to make sure we're all on the same page.

Summary of the updates:

1. add missing fields to Dynamic_Joint_State, to match ROS API
2. reorder/add fields in Dynamic_Group_Status, to match existing RobotStatus msg
3. rework configuration parameter, to minimize duplication
   1. don't specify topic names. Too much duplication for nominal case. Can achieve the same thing using remap.
   2. don't specify joint-list separately for each message type. Too much duplication.
   3. allow sharing a common (minimal) config structure between nodes
4. elaborate on operation, configuration, usage, and backwards compatibility
   1. single vs. multi-topic usage
   2. overlapping topics (e.g. left_arm, right_arm, both_arms)
   3. logic to aggregate state data

Not to re-open something that's already been finalized, but I'd rather have input before everything's been implemented...

[gavanderhoorn]

Could you clarify why you removed that assumption? I can see why it would make sense to remove the regardless of group bit, but now it allows for multiple joints in the same group being identically named. Is that intentional?

[gavanderhoorn]

@JeremyZoss wrote:

Not to re-open something that's already been finalized, but I'd rather have input before everything's been implemented...

The status of REP-I0001 is still Draft, not Active, so it wasn't finalized.

[gavanderhoorn]

Missing space between messages and [#simp_msg]_ (in "new set of simple messages[#simp_msg]_ for multi-arm control").

[gavanderhoorn]

In the Dynamic Group Status section, there is a missing space between message and [#rbt_stat]_ (in "The dynamic group status is meant to mimic both the ROS-I RobotStatus message. See the RobotStatus message[#rbt_stat]_ for field descriptions").

[gavanderhoorn]

Would it be an idea to increase the visibility of the sentence:

Invalid fields (see bit field above) are not included, resulting in a shorter message.

by putting it under Assumptions or giving it its own paragraph/section somewhere? That is a rather big change from how simple_message currently works, and it is currently 'hidden' in a comment about field lengths in the Dynamic Joint Point and Dynamic Joint State structures.

[gavanderhoorn]

This just caught my eye, and although I know this was already in the original, I thought this PR would be a good time to bring it up (also as there are no actual implementations currently using any of the structures defined in the REP).

Do we really need all of the X, X_desired and X_error fields? It would seem that any one of those sets could be calculated using the other two. Is there any specific reason that they were included?

@shaun-edwards?

[gavanderhoorn]

Also: all msg structures have a valid_fields field, but the REP does not specify any of the bit positions in them, nor their semantics.

[gavanderhoorn]

Also: no mention of units to be used in any of the fields. Suggest adding that to an Assumptions section, or stating it somewhere near the structure definitions.

I assume it's going to be something like:

• joint positions: rad
• joint velocity: rad/s
• joint acceleration: rad/s^2
• effort: Nm

[gavanderhoorn]

@gavanderhoorn wrote:

Would it be an idea to increase the visibility of the sentence:

Invalid fields (see bit field above) are not included, resulting in a shorter message.

by putting it under Assumptions or giving it its own paragraph/section somewhere? That is a rather big change from how simple_message currently works, and it is currently 'hidden' in a comment about field lengths in the Dynamic Joint Point and Dynamic Joint State structures.

@JeremyZoss: could you address this one? I think my email already made it clear that it's a good idea to give this a more prominent place in the REP :).

[JeremyZoss]

@gavanderhoorn wrote:
@JeremyZoss: could you address this one? I think my email already made it clear that it's a good idea to give this a more prominent place in the REP :).

I thought I had sufficiently clarified this in my last commit (af8a3ca, line 72). Let me know if you have suggestions for improvements. Maybe it needs to be shorter, like a bullet point? Or repeated under "assumptions"?

[gavanderhoorn]

@JeremyZoss: perhaps we could add subsections to the Simple Message Structures section that detail the differences between v1 and v2 (this REP). We could also document the bit positions defined for valid_fields in such a section.

[shaun-edwards]

Do we really need all of the X, X_desired and X_error fields? It would seem that any one of those sets could be calculated using the other two. Is there any specific reason that they were included?

@gavanderhoorn, you are correct. The reason they were all included was to mirror the JointTrajectoryAction feedback. In the interest of reducing the the packet size, we could remove one.

[gavanderhoorn]

@shaun-edwards: ok, then I propose we remove the X_error fields, as they can be calculated easily from X and X_desired by the ROS node(s).

Thoughts?

[gavanderhoorn]

Question: was not including any specs on the types of fields (sizes, signedness, etc) on purpose?

[gavanderhoorn]

Sorry for all the posts, but: both JOINT_TRAJ_PT_FULL and JOINT_FEEDBACK(_EX) included a time field, intended to be used as a timestamp. I'd be in favour of adding that to all the message structures described in this REP.

We've been conservative with timestamps in the protocol (as clocks with sufficient resolution might not be common), but as the valid_fields field can be used to indicate support for this optional field, I think it would be nice to add it in.

One issue with a timestamp is the requirement for common clocks, which may be unrealistic. ROS depends on NTP, which some robot controllers support, but that is probably not a good idea (I can imagine NTP not being usable in factory contexts).

An alternative to common clocks is to establish the offset between two clocks and compensate for that on reception of msgs. There are established algorithms for that, and it would involve some simple message exchanges. Adding the time field now would allow us to implement support for this at a later time. Adding it now should be possible, as there is no implementation currently using these messages, so we don't break any backwards compatibility.

[JeremyZoss]

@gavanderhoorn wrote:

both JOINT_TRAJ_PT_FULL and JOINT_FEEDBACK(_EX) included a time field, intended to be used as a timestamp. I'd be in favour of adding that to all the message structures described in this REP.

Actually, the time field in those messages was probably intended to be used for the time_since_start time parametrization of the ROS trajectory messages. I don't know that we've ever seriously considered timestamping the status data being broadcast off the robot controllers.

I don't know that it makes sense to timestamp the data being sent to the controllers (DynamicJointPoint), but I could conceive of some possible use for timestamping the feedback data coming off the controllers.

In the ROS side, this timestamp would be most-logically integrated into the header field. Other ROS nodes are going to expect this data to be stamped relative to the ROS clock (not controller clock). Things will get confusing unless all clocks are sync'd (as you suggest).

My vote would be to leave the time field out, until we actually have an implementation that needs it. We should be able to extend these new messages with additional data fields, as long as they are added to the end of the field-list.

[gavanderhoorn]

@JeremyZoss wrote:

@gavanderhoorn wrote:

both JOINT_TRAJ_PT_FULL and JOINT_FEEDBACK(_EX) included a time field, intended to be used as a timestamp. I'd be in favour of adding that to all the message structures described in this REP.

Actually, the time field in those messages was probably intended to be used for the time_since_start time parametrization of the ROS trajectory messages. I don't know that we've ever seriously considered timestamping the status data being broadcast off the robot controllers.

That is probably true for JOINT_TRAJ_PT_FULL, but the comments in JOINT_FEEDBACK seem to suggest it was actually for time stamping data (from here):

[..]
 * \brief Class encapsulated joint feedback data.  This data represents the
 * current state of each robot joint, including position/velocity/acceleration.
 * The specific interpretation of this data (actual vs. commanded, timestamp, etc.)
 * is up to the robot-controller implementation.
[..]

I don't know that it makes sense to timestamp the data being sent to the controllers (DynamicJointPoint), but I could conceive of some possible use for timestamping the feedback data coming off the controllers.

Yes, in the current bridging / client-server design, it would mostly make sense for data server->client.

In the ROS side, this timestamp would be most-logically integrated into the header field. Other ROS nodes are going to expect this data to be stamped relative to the ROS clock (not controller clock). Things will get confusing unless all clocks are sync'd (as you suggest).

Well, as I said, syncing is not required, offset calculation would also work. Another alternative could be to not sync anything, but to make sure that delta T between bridged messages is consistent. The ROS node would then use the incoming time field to calculate the dT between the current and previous msg, and use that to increment a ROS timestamp it's keeping track of itself. That would make things slightly easier, but would still improve the accuracy and thus usefulness of the ROS header.stamp field. It would at least remove the 'arbitrary' timestamping that is done by the industrial_robot_client nodes now.

My vote would be to leave the time field out, until we actually have an implementation that needs it. We should be able to extend these new messages with additional data fields, as long as they are added to the end of the field-list.

Hm, I'm not too much of a fan of adding fields later. Many controllers do not support things like byte buffers, which make it easy to 'ignore' data 'at the end' that they don't know how to serialize. For Fanuc fi, the deserialiser would have to be designed with that functionality in mind, as it will have to read any excess bytes out of the socket. If we want that, that is fine, but then this REP (or maybe a future one, documenting the bytestream itself) should explicitly state that.

[gavanderhoorn]

"meant to mimic both the ROS-I RobotStatus message": is something missing here? Mimic both RobotStatus and what other message?

[gavanderhoorn]

Another question: should we address ros-industrial/ros_industrial_issues#27 (add support for linear (or: non-revolute) axes) in this update to this REP? Or at least make sure there is nothing preventing us from supporting that in a future extension?

Linear axes should map into prismatic joints in a urdf, and joint states reported for them should be in meters. As the issue asks: is there any special infrastructure and / or functionality required for support of linear axes? I think on the ROS side, the urdf would contain the required information. The robot driver is out of scope for this document. Should the group config file format get any special elements? @JeremyZoss?

[gavanderhoorn]

And another question: should we support 'real' point-to-point streaming (ie: points not part of a trajectory, but generated on the fly by some process)?

The time_from_start field in that case would not seem to fit very well: what would it be relative to? We could say that a special 'start point' is needed and that everything would be relative to that. Alternatively, the time_from_start field could be interpreted as the segment time for a move to start when the controller receives the msg. Both of these would not need any special support in the protocol, just some conventions on the side of the ptp generator and the controller.

I'm thinking specifically of use cases like those covered by the fs100_motoman pkg, and the adaptations to the motoman_driver done in ros-industrial/motoman#16 and ros-industrial/motoman#37.

[gavanderhoorn]

By encapsulating the entire trajectory in a single message, synchronized motion is possible.

Isn't synchronised motion only dependent on the trajectory point containing info for all groups that are to be part of the motion? A Dynamic Joint Point is still just one point, right?

[gavanderhoorn]

Would there be interest in continuing / wrapping up this discussion? I think there was some really good progress made in this PR.