Clean recovery from starving the arm during streaming is currently not possible

[acmorrow]

Affected Client Library version(s)

2.14.0 2.14.1

What combination of platform is the Client Library running on.

Mac

How did you install the Client Library

Build the library from source

Which robot platform is the library connected to.

URSim in docker

Robot SW / URSim version(s)

master

How are you using the Client Library

Headless without using the teach pendant

Issue details

Summary

With external_control.urscript, If a streaming client fails to meet its obligation to keep the arm fed with points and allows the arm to "run off the end of the stream", the script is left in a state that makes safe recovery impossible. There is not a clear workaround at this time.

Issue details

The contract for streaming is that the client will end the stream with a point count, so that external_control.urscript knows how many more points to read off the socket. That works well, both for normal stream termination and for client-initiated cancellation. However, if the client fails to meet its obligation to keep the arm fed with points, the trajectory will report failure. But without a terminal message from the client containing the total point count, the external_control.urscript script can't know how many points may be sitting in the buffer that it should drain. Nor can it guess, or rely on a timeout, since the client may already be sending points for some future trajectory.

Steps to Reproduce

Run the arm off the end of a trajectory. I have written a number of unit tests which I can share on a branch that make the problem clear.

Expected Behavior

After a trajectory failure, the script should be able to safely drain any points already in the socket buffer.

Actual Behavior

The points remain in the buffer, and dangerously become part of the next trajectory.

Workaround Suggestion

I do not currently have a workaround suggestion. The most obvious approach would be to ask streaming clients to follow up a trajectory failure with an explicit STREAM_END or CANCEL containing a total point count. But that has some problems, in that it is likely to trigger a second "completion" on the same trajectory: confusing for both the script and the client. That could probably be suppressed in the external_control.urscript logic, but now things are starting to get a little messy. Another possibility would be to require streaming clients to attach an ID to each streamed trajectory point, which would allow the arm side script to drain points with matching ids, but this is imperfect as once you have read the first non-matching point you've already done the wrong thing. Another option would be to introduce a requirement on clients that when they get failure during a stream, they must drop and reconnect, but that's harsh and graceless.

Basically, I'd like to use this issue to talk through the issues. I have some unit tests that demonstrate the problem, please let me know if you would like me to publish them to a branch.

Finally, while investigating this issue, it looks like there may be some concurrency issues related to the trajectory point count between the trajectory thread and the main thread, which should probably be resolved at the same time. I can share more details on that, or we can wait until we have an agreed path forward on recovery from failure and then investigate the concurrency issue.

Relevant log output

Accept Public visibility

• I agree to make this context public

[urrsk]

Hi @acmorrow
Thanks for reaching out. And yes, it seems that you are pointing into a relevant issue.
The trajectory ID sound like a good idea, which will make it possible to ignore trajectory points that is not relevant to the new trajectory started.
Yes, please share your test. I will suggest you do that as a PR.

In regard to the concurrent issue, please note that all threads in urscripts are evaluated in a sequence (and not in parallel) per time step. Therefore, can all variables be considered as atomic. (The sequence order is the same as threads is created)
Though, there might be something in the cross section of an old been running and a new trajectory is started, as this is received from two different connections. Here an ID could potential also help here as well.

[acmorrow]

@urrsk -

I've opened #552 as a draft PR that contains the unit tests.

[acmorrow]

@urrsk - I'm going to take a look at what it would take to add support for trajectory ids. Before I do though, I could use a clarification on one thing. Presumably, this is going to be some sort of wire-format change. That'd mean both sides need to update in lockstep: both the URCL C++ code, and the external_control.urscript. I don't know enough about UR development practices or compatibility requirements to know if that is valid. If the assumption is that external_control.urscript and the URCL version must match, then the protocol can probably change easily. If there are compatibility requirements such as supporting an old external_control.urscript with a newer URCL or vice-versa, then the wire format change is going to be rather more complex.

Overall, though, I think improving the wire format here could simplify things considerably. If it is easy to identify both which motion a point on the socket belongs to and when the last point of a motion has arrived, then everything basically collapses into streaming, and much of the bookkeeping over in the control script may just evaporate away.

[urrsk]

Hi @acmorrow
We in general, we try to minimize and postpone wire changes to be released together as changes will affect people using a custom external_control.urscript.
Though, one of the reasons why we are having both the URCL C++ and the urscript together in one unit/package is that it makes it possible for us to change the protocol on both sides without giving problem for people using the URCL in the default way.
In addition, the streaming feature is also very new, and this is a problem we need to get addressed.

Regarding the ID approach. I will suggest:

• I will suggest that c++ defines the ID and is received to together with TRAJECTORY_MODE_STREAM_START as well as the other TRAJECTORY_MODE_STREAM command so we can detect and print out an error if this does not align.
• ID should be an arbitrary number, to make less likely that we meet corner case there two ID, received in a row,by the robot, is identically.
• The ID should also be sent back from the robot on complete, cancel etc, so we can detect problems here as well going forward.
• I hope we can limit the needed changes to the TRAJECTORY_MODE_STREAM scope. Though, Let's discuss it further if the scope is needed to grow.

@acmorrow thanks for looking into to this.

[acmorrow]

Hi @acmorrow We in general, we try to minimize and postpone wire changes to be released together as changes will affect people using a custom external_control.urscript. Though, one of the reasons why we are having both the URCL C++ and the urscript together in one unit/package is that it makes it possible for us to change the protocol on both sides without giving problem for people using the URCL in the default way. In addition, the streaming feature is also very new, and this is a problem we need to get addressed.

That all makes sense. Agree the streaming feature is new, but I'm the one that broke it (by creating it broken), so I definitely would like to see it get fixed! Hopefully this is an easy enough wire protocol change that it doesn't impose too heavily on users with customized control scripts.

In parallel with this discussion, I've been working on an implementation, and I have something that makes the tests in the PR pass (as well as adding a few other tests). So there is at least one possible solution. It doesn't align completely with your notes below, but is quite close.

Regarding the ID approach. I will suggest:

• I will suggest that c++ defines the ID and is received to together with TRAJECTORY_MODE_STREAM_START as well as the other TRAJECTORY_MODE_STREAM command so we can detect and print out an error if this does not align.

Yes, agree, URCL C++ creates the ID, attaches it on STREAM_START. There is no harm to attaching it to regular START too, from what I see.

• ID should be an arbitrary number, to make less likely that we meet corner case there two ID, received in a row,by the robot, is identically.

In my current solution, the URCL internals manage the number, and it is just an incremented atomic. Random could obviously work too. One advantage to incrementing is that it means we can detect some more interesting situations, like if we see points from a message in the "future", that indicates some sort of protocol violation, but you can't do that with random. Trade-offs either way, and I don't have strong feelings about it.

• The ID should also be sent back from the robot on complete, cancel etc, so we can detect problems here as well going forward.
• I hope we can limit the needed changes to the TRAJECTORY_MODE_STREAM scope. Though, Let's discuss it further if the scope is needed to grow.

I had considered doing this, but opted against it, because the value isn't actionable by a client of URCL: it never created the move id (URCL did that internally, automatically), so it can't interpret it or act on it. But again, this is easy to change if you like.

@acmorrow thanks for looking into to this.

Happy to! We are very excited about streaming support so I'm quite motivated to help get it solid. I feel a little bad that a broken version of it went out in a release that I pushed for. There is a bit of an interesting history on that, maybe. I had been wondering why I never encountered this issue during my testing. I think, but have not proven, that the nagle disable that arrived shortly after in the release is part of the story. When nagle was on, the stream got buffered client side until nagle got out of the way. That meant that the arm had more runway than expected, and starvation was less likely. Even when we tried to test starvation, it maybe didn't actually happen! So we didn't notice the misbehavior after starvation. Anyway, just a theory.

I'm going to push my current implementation to the PR with the tests and promote it out of draft, and we can iterate on the design there. But I think we are largely on the same page in terms of what needs to be done and why. Hopefully the wire protocol change isn't to onerous. Maybe it even gives you cover to slip in some other wire protocol changes you were interested in making!