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

[JTC] Reject messages with effort fields #699

Merged

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Jul 14, 2023

We decided in the last WG meeting (see here for the referece) to reject incoming trajectory messages having effort entries for now. If someone needs this, we have to discuss how to interpolate effort.

Together with 0f84024 this closes #688

@gavanderhoorn
Copy link
Contributor

What sort of error / result code would clients see?

A single error line in the controller's terminal is probably not sufficient to prevent surprises / confused users.

@christophfroehlich
Copy link
Contributor Author

What sort of error / result code would clients see?

A single error line in the controller's terminal is probably not sufficient to prevent surprises / confused users.

The message will be rejected, which means that nothing moves and the user will have a look in the command prompt.
Using the topic interface, there is no chance for returning error codes. Using action interface, rclcpp_action::GoalResponse::REJECT is the result for the action client.

What else do you suggest?

Co-authored-by: Abrar Rahman Protyasha <a_protyasha@apple.com>
@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Jul 14, 2023

The message will be rejected, which means that nothing moves and the user will have a look in the command prompt.

I guess I would phrase this as:

The message will be rejected, which means that nothing moves and the user will have to look in the command prompt.

that's what I meant when I wrote:

A single error line in the controller's terminal is probably not sufficient to prevent surprises / confused users.

I've been such a confused user in the past, hence my comment.

Using action interface, rclcpp_action::GoalResponse::REJECT is the result for the action client.

Last time I checked, there was no support for setting the error_code or the error_string as part of the rejection, as you could in ROS 1 (ros2/rclc#271).

That complicates things, but a work-around I've seen is to accept the goal, then 'abort' by never processing it and immediately sending the final result, in which you set the error_code and error_string to a meaningful value.

Clearly a work-around, and I still consider the limitation discussed in the linked issue a serious design flaw, but it seems without some work, the best you can do.

The advantage of doing something like this is that clients would have machine readable information about what happened to their goal.

"the human needs to look at the command line" is not a viable error handling / recovery strategy in many cases, so should be avoided as much as possible. The error_code field could be used by clients to implement automated recovery, or at the very least promote the error returned by the JTC in a way easily visible to users (using some form of UI, or perhaps still an ERROR in a log somewhere, but at least not hidden in the rest of the output).

@christophfroehlich
Copy link
Contributor Author

The check introduced with this PR just follows the way the trajectory message gets rejected with any other errors. So I suggest discussing your point in a separate issue, because we then would have to change the overall behavior of the validate_trajectory_msg method, which is not linked to the particular issue with the effort fields.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Thanks! Especially like negative sum of changes!!!

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Thanks!

@bmagyar
Copy link
Member

bmagyar commented Jul 17, 2023

Thanks @gavanderhoorn for the insightful comments, it's worth tracking that on a separate issue.

@bmagyar bmagyar merged commit 5850be5 into ros-controls:jtc-features Jul 17, 2023
8 of 10 checks passed
bmagyar added a commit that referenced this pull request Jul 25, 2023
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
@christophfroehlich christophfroehlich deleted the jtc-reject-effort branch July 25, 2023 12:39
mergify bot pushed a commit that referenced this pull request Aug 3, 2023
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
(cherry picked from commit a572381)

# Conflicts:
#	joint_trajectory_controller/test/test_trajectory_controller.cpp
christophfroehlich pushed a commit to christophfroehlich/ros2_controllers that referenced this pull request Aug 7, 2023
…rols#719)

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
(cherry picked from commit a572381)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants