Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update REP-I0001 with implementation-related changes/clarifications #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JeremyZoss
Copy link
Member

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...

 - add missing fields to Dynamic_Joint_State, to match ROS API
 - reorder/add fields in Dynamic_Group_Status, to match existing RobotStatus msg
 - rework configuration parameter, to minimize duplication
 - elaborate on operation, configuration, usage, and backwards compatibility
@@ -62,8 +62,7 @@ The industrial robot controller motion interface shall meet the following requir

Design Assumptions
=========
The following assumptions are inherent in the client/server design:
* All joints (regardless of group) shall be uniquely named.
None at this time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@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.

@@ -42,7 +42,7 @@ The following definitions are used throughout:
Motivation
==========

The first version of the motion/status interface (loosely documented in code and wikis) worked well for single arm manipulators. Multi-arm (asynchronous or synchronous) was not supported in a generic manor that could be used across all platforms [#discuss]_. Some attempts were made to support multiple arms using the existing interface, but these often required multi-arm joint states which could exceed the max number of joints and did not allow for mixed multi-arm/single-arm motion. The intent of this REP is to define a new set of simple messages[#simp_msg]_ for multi-arm control. These messages will replace existing motion control message (they will be deprecated). The approach taken in the REP is one that creates a new set of clients for robot motion. The underlying simple message libraries will remain unchanged (except for the addition of new messages).
The first version of the motion/status interface (loosely documented in code and wikis) worked well for single arm manipulators. Multi-arm (asynchronous or synchronous) was not supported in a generic manner that could be used across all platforms [#discuss]_. Some attempts were made to support multiple arms using the existing interface, but these often required multi-arm joint states which could exceed the max number of joints and did not allow for mixed multi-arm/single-arm motion. The intent of this REP is to define a new set of simple messages[#simp_msg]_ for multi-arm control. These messages will replace existing motion control message (they will be deprecated). The approach taken in the REP is one that creates a new set of clients for robot motion. The underlying simple message libraries will remain unchanged (except for the addition of new messages).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gavanderhoorn
Copy link
Member

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
Copy link
Member

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.

 - update header lines to meet RST spec
 - re-add unique joint-name assumption
 - fix literal-text to RST syntax
 - misc spaces and typos
 - add example for namespace-mapping config/behavior
 - add more discussion of `valid_fields` feature
@gavanderhoorn
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member Author

@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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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

@gavanderhoorn
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member

@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.

---------
The dynamic group status is meant to mimic both the ROS-I RobotStatus message. See the RobotStatus message[#rbt_stat]_ for field descriptions.::
--------------------
The dynamic group status is meant to mimic both the ROS-I RobotStatus message. See the RobotStatus message [#rbt_stat]_ for field descriptions.::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gavanderhoorn
Copy link
Member

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
Copy link
Member

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.


Dynamic Joint Point
---------
-------------------
The dynamic joint point is meant to mimic the ROS JointTrajectory message structure [#traj_msg]_. A one-to-one mapping of the joints included in the ROS message to the simple message shall be created. By encapsulating the entire trajectory in a single message, synchronized motion is possible.::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants