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

Draft of the Generic IO protocol extension of the Simple Message protocol #7

Closed

Conversation

gavanderhoorn
Copy link
Member

As per title.

It is probably easier to read the document rendered, you may use this url for that. I've also pushed a PDF rendered with restview: link.

Comments may be added in-line here (on the commit), or at the bottom of this PR.

Some points for discussion:

  1. should seq field of ros-i header be used instead of message_id
  2. should copied ROS Packet header structure be shown in more detail in this document (seeing as the original source doesn't explicitly give more detail on used types fi)
  3. should message structures be defined using shared_int and shared_real, instead of current C typedefs
  4. should a name field be added to IO_INFO ranges
  5. repeat timestamp field in IO_STREAM_PUB for each published range, instead of one per msg
  6. should streaming support be required
  7. should explicit support for IO timed with motion be added (ie: synchronise IO write/read execution with time_from_start in trajectories), or should that be a responsibility of the client

@ClayFlannigan
Copy link

Very thorough work as always. Thank you for this.

One thought: Many I/O interfaces support "forcing" or simulating of input I/O status. This is often done when the actual hardware is not available or for testing purposes. I've seen this used to temporarily override a broken or missing sensor. What this capability considered? Is it available through some other mechanism?

@gavanderhoorn
Copy link
Member Author

One thought: Many I/O interfaces support "forcing" or simulating of input I/O status. This is often done when the actual hardware is not available or for testing purposes. I've seen this used to temporarily override a broken or missing sensor. What this capability considered? Is it available through some other mechanism?

No, support for simulating IOs is currently not in the REP. If the intent is to force an input, do we want to do this on the simple_message level, from the ROS side? I can imagine forcing an Element being useful in a testing / debugging scenario, but I would think this would be done from the TP. Overriding physical IOs can be dangerous (when used for setting up mutual exclusion zones or confirmation signals fi), so it would be bad if a misdirected ros service call resulted in overriding an input in the wrong controller. Leaving this to the TP would remove / reduce that possibility.

I'd be happy to add it though, it would 'just' be an interface to the mechanism implemented by the controller.

@gavanderhoorn
Copy link
Member Author

Issues

were also used as input for this draft.

@ClayFlannigan
Copy link

Whether to add simulated I/O state may be a philosophical decision. I can see value in keeping the simple message interface as "simple" as possible. @shaun-edwards what think you?

@shaun-edwards
Copy link
Member

True IO forcing is not typically something that can be done programatically on the robot controller. Therefore, any "forcing" through simple_message would really just be some controller specific "soft" forcing. I don't see a real difference between supporting "soft" forcing on the controller or the ROS client, except that forcing from the ROS client would be applied to all robots. So my preference would be to support forcing on the ROS client side (i.e. ROS API, which isn't addressed by this REP directly).

@gavanderhoorn
Copy link
Member Author

@shaun-edwards: with 'soft forcing', are you thinking of only setting the local (ie: in the client) IO state of an Element to a certain value, without actually updating the server?

@shaun-edwards
Copy link
Member

@gavanderhoorn , 'soft forcing' is different for inputs (signals coming into the controller) and outputs (signals going out of the controller). Inputs could simply be 'soft forced' in the ROS client, the controller input would remain unchanged. Output 'soft forcing' would require the ROS client to intercept write requests and force the appropriate state.

of the controller – could either be made an extension to an already
running ROS-Industrial server component, or be made to run as a
separate task.
In all cases, the server is responsible for listening for incoming
Copy link
Member

Choose a reason for hiding this comment

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

Should we specify the port, or more generically that the IO messages should have a dedicated port. I think it is important for the port to be separate from the motion port (in cases where it can be).

Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably specify a port.

I don't necessarily agree that it is important to have a separate port though: the msg IDs can be used to dispatch incoming msgs. I can imagine this being important in situations where servers use blocking implementations of motion / io msg parsing & processing. That could add unwanted delay, which could be avoided by having a separate server task.

Copy link
Member

Choose a reason for hiding this comment

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

True...@ted-miller implemented IO control for motoman and it shares the motion channel. In some sense, this could be helpful because it could allow us to set IO synchronously with the path.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I did implement a quick message to fire a single I/O point... but I don't think it's a good idea for anything other than occasional / simple gripper control. (It was the quick/easy solution) Trying to implement large amounts of data will cause additional latency if sharing the motion channel.

See additional comments here: #6

@shaun-edwards
Copy link
Member

Overall, I think this is very well written. It's clear you have considered the alternatives, and I believe what you have described balances flexibility with performance.

should seq field of ros-i header be used instead of message_id

See my comment inline

should copied ROS Packet header structure be shown in more detail in this document (seeing as the original source doesn't explicitly give more detail on used types fi)

Can't hurt, although the source code does document these.

should message structures be defined using shared_int and shared_real, instead of current C typedefs

Yes

should a name field be added to IO_INFO ranges
repeat timestamp field in IO_STREAM_PUB for each published range, instead of one per msg

Do we anticipate different timestamps in a single message. This does not seem likely.

should streaming support be required

Maybe. I think the client side will emulate this with IO_READs if the functionality isn't there. As a result, controllers which can't handle the comms load will have to implement it.

should explicit support for IO timed with motion be added (ie: synchronise IO write/read execution with time_from_start in trajectories), or should that be a responsibility of the client

Yes, but I think we should embed IO messages in the waypoint data (i.e. the motion interface). This would be cleaner.

Some additional issues:

  1. We need to keep in mind the most common IO operations. In my experience this is read/writing digital IO on a single block (typically in groups of 16 bits), with most applications not having any more than one group per input or output. Does the spec provide an easy/efficient method to do this? (I think so, but I want to make sure).
  2. It probably goes without saying, but we should add for clarity. Robot outputs may be written or read. Inputs may only be read.
  3. I would argue that we should remove time-stamping of IO since the accuracy of such is pretty limited.

Thanks again for the effort.

@gavanderhoorn
Copy link
Member Author

@shaun-edwards wrote:

should copied ROS Packet header structure be shown in more detail in this document (seeing as the original source doesn't explicitly give more detail on used types fi)

Can't hurt, although the source code does document these.

I'll add the shared_int field declarations.

should message structures be defined using shared_int and shared_real, instead of current C typedefs

Yes

Then I think we need to add a shared_short and the unsigned variants of all those. Right now we're limited to shared_int and shared_real. Having to use 4 byte types everywhere will lead to some significant increase in message sizes.

should a name field be added to IO_INFO ranges
repeat timestamp field in IO_STREAM_PUB for each published range, instead of one per msg

Do we anticipate different timestamps in a single message. This does not seem likely.

When I wrote that down I was wondering whether subscriptions could become so large that the overhead of sampling all those Elements could cause such dT between the first and last values, that it might make sense. I admit, this will probably not really be an issue for a 'typical' application with some gripper and peripheral IO though.

should streaming support be required

Maybe. I think the client side will emulate this with IO_READs if the functionality isn't there. As a result, controllers which can't handle the comms load will have to implement it.

True. However implementing streaming (with subscription and configuration management, periodic publication) is a bit more complicated to implement than simple synchronous read/write. It also incurs more overhead. I don't really know the answer to this one, which is why I put it up as a point of discussion.

should explicit support for IO timed with motion be added (ie: synchronise IO write/read execution with time_from_start in trajectories), or should that be a responsibility of the client

Yes, but I think we should embed IO messages in the waypoint data (i.e. the motion interface). This would be cleaner.

Personally, I was thinking we could perhaps define something like an IO trajectory, which would exploit proper time_after_start values in both the motion and IO trajectory to synchronise motion and IO. Embedding IO data in motion trajectories doesn't feel right to me: a similar case could then be made to embed other types of messages in waypoint data, as long as we want their processing to be synchronised with motion.

Some additional issues:
1 . We need to keep in mind the most common IO operations. In my experience this is read/writing digital IO on a single block (typically in groups of 16 bits), with most applications not having any more than one group per input or output. Does the spec provide an easy/efficient method to do this? (I think so, but I want to make sure).

I think it does: the currently proposed system is supposed to support a superset of the usecase you describe. A single block is just one range, and IO_READ/IO_WRITE don't care where the IOs are located, as long as the indices are valid. Streaming would also be simply a case of subscribing to a single range, with (start : 0, len : 16) (and an appropriate type).

2 . It probably goes without saying, but we should add for clarity. Robot outputs may be written or read. Inputs may only be read.

Right, will add a note.

3 . I would argue that we should remove time-stamping of IO since the accuracy of such is pretty limited.

I'll wait a little longer, to gather some more input on this. But your vote is clear :).

Thanks again for the effort.

Thanks for your comments Shaun.

@gavanderhoorn
Copy link
Member Author

2 . It probably goes without saying, but we should add for clarity. Robot outputs may be written or read. Inputs may only be read.

Right, will add a note.

I just remembered: this is not necessarily true. The Fanuc R-30iA/B lets you write to inputs, as long as the Element is in simulate mode. This also ties in with the comment by @ClayFlannigan earlier. I'll see what the behaviour of other mfgs is.

@shaun-edwards
Copy link
Member

@gavanderhoorn, is there an update on this PR? Several people have asked me about this.

@gavanderhoorn
Copy link
Member Author

I suggest we close this PR - and withdraw the REP.

My rationale: I'd much rather we invest resources into proper fieldbus interfaces than design, implement and maintain yet another custom software-only approach. Community uptake of the extension described in this REP has also been low, with only one implementation existing so far.

Many of the ideas in this REP can be transferred to a ROS API-level version of an IO interface, which would be much more generic and reusable.

Please post a comment to let me know if anyone has any objections. If I receive none within a week, I'll close the PR.

@simonschmeisser
Copy link

@gavanderhoorn I'm a bit late to the party it appears ...

we used the ROS interface as implemented in the fanuc ReApp IO driver (I guess this is the one application you refer to) for interfacing multiple implementations (mitsubishi, hilscher based profinet (actually generic fieldbus) pci card, modbus/tcp).

Actually we currently only use the polling part, not the subscription based but that is on my oh-so-short todo-list ...

So I'm totally in favor of adding either these https://github.com/isys-vision/industrial_core/tree/mikado/industrial_msgs message definitions or similar to industrial_core (or a new industrial_io repo?). Is that what you referred to by

ROS API-level version of an IO interface

or could you elaborate some more? We could also discuss this in person at ROSCon I guess but I'd prefer to start it sooner rather than later.

Would you be in support of a PR that copies the message definitions from ReApp as above? Unless @antonxy objects of course

We could also contribute our QSerialBus based Modbus/Tcp client (and demo server) after some cleanup as a reference implementation. We used this to interface Wago IOs.

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.

5 participants