Provide hold/unhol via action interface

[martiniil]

Original comment by @hslusarek

To be honest, I think the connection type is wrong. At least the hold function (probably the unhold function, too) should be provided via action. If an action is used as connection type, many of the problems discussed would not exist. Furthermore, the user interface would be easier to understand and would leave the decision to wait or not, to the user.

Full discussion: https://github.com/PilzDE/pilz_robots/pull/308#discussion_r402408833

[martiniil]

I agree that the user interface would be easier to understand.

many of the problems discussed would not exist

Depends on how you want to implement this. The decision to make unhold not waiting reduced the complexity in traj_mode_manager.h. So I am not sure, which problems you are talking about.

[hslusarek]

Depends on how you want to implement this. The decision to make unhold not waiting reduced the complexity in traj_mode_manager.h. So I am not sure, which problems you are talking about.

My way of thinking is actually pretty simple: If I'm the user it would "surprise" me if the hold service is blocking, while it's counterpart the unhold service is not blocking. This is confusing. It has only marginally something to do with code complexity. API's should never be surprising. They should be intuitive and easy to understand and reason about. This is, in my opinion, not the case here. The action server approach would significantly improve the situation.

[martiniil]

Well, then let's agree that this is only about the user interface. I thought you see more problems, that the action server approach can solve.

Trying to get a common view: A service call is always blocking. You probably mean, that the unhold is not blocking until it can be executed? Why should that change with the action server?

[hslusarek]

A service call is always blocking. You probably mean, that the unhold is not blocking until it can be executed?

Exactly.

Why should that change with the action server?

Actually, the code in the PilzJointTrajectoryController would not change that much. However, using an action instead of a service, we provide a Feedback and a Result. With the Feedback and the Result the user of our PilzJointTrajectoryController can decide for himself, if he wants to make his calling code blocking or not blocking. In other words, the decision if the handleHoldRequest() and handleUnHoldRequest() function should wait, is "outsourced" to the user who can best judge for himself, if his code should be blocking or not.

[martiniil]

the decision if the handleHoldRequest() and handleUnHoldRequest() function should wait, is "outsourced" to the user

No. The user only decides about his own thread, he cannot influence the goal execution. Or do you think he should have the possibility of cancelling a hold/unhold goal?

[hslusarek]

the decision if the handleHoldRequest() and handleUnHoldRequest() function should wait, is "outsourced" to the user

No. The user only decides about his own thread, he cannot influence the goal execution. Or do you think he should have the possibility of cancelling a hold/unhold goal?

No, the cancelling should not be possible. All I'm saying is: The user can decide for himself if he wants to wait in his own thread or not, using the Feedback and the Result. Or, in other words, that's the option we should provide the user with. Because we (at least in my opinion) cannot reasonably decide if the services should be blocking or not, therefore, I suggest leaving the choice to the user.

[martiniil]

Yeah, I agree about using the action interface. But in my opinion the unhold should never wait for its execution. I guess we have to consult more opinions in order to get clear requirements.