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

Controllers not deactivated on error in update on Humble #1873

Open
jellehierck opened this issue Nov 13, 2024 · 7 comments · Fixed by #1969
Open

Controllers not deactivated on error in update on Humble #1873

jellehierck opened this issue Nov 13, 2024 · 7 comments · Fixed by #1969
Assignees
Labels

Comments

@jellehierck
Copy link

Describe the bug
When a controller returns return_type::ERROR in its update() function, it does not get deactivated.

For example, using following snippet:

controller_interface::return_type MyController::update(
  const rclcpp::Time & /*time*/, const rclcpp::Duration & /*period*/
) {
  if (something_bad == true) {
    RCLCPP_ERROR(get_node()->get_logger(), "Something bad happened");
    return controller_interface::return_type::ERROR;
  }
  ...
  return controller_interface::return_type::OK;
}

The controller will spam Something bad happened to the console continuously instead of stopping the controller.

Expected behavior
I expected a controller to get deactivated once it returns return_type::ERROR.

Environment (please complete the following information):

  • OS: Ubuntu 22
  • Version: Humble

Additional context
This functionality is already implemented in the master branch (#1499). Is the choice to not include in Humble conscious? Are there any workarounds? Or would I need to build from source from the master branch?

Thanks!

@christophfroehlich
Copy link
Contributor

@saikishor can we backport #1499?

@saikishor
Copy link
Member

@saikishor can we backport #1499?

It shouldn't be hard to backport it. The problem is this is a change in behaviour, Moreover, I'm not sure if @ros-controls/ros2-maintainers are fine with this kind of change

@christophfroehlich
Copy link
Contributor

It is a change of behavior only if a user implemented an ERROR return value, which was just ignored up to now. However, I would not put too much effort in the backport, users always can build the rolling version from source to have all the new features.

@jellehierck
Copy link
Author

However, I would not put too much effort in the backport, users always can build the rolling version from source to have all the new features.

That would work for me.

I can see how the change in behaviour is something to consider carefully. But arguably, the current behaviour is wrong ;) We can leave that up to @saikishor and the other @ros-controls/ros2-maintainers.

@christophfroehlich
Copy link
Contributor

We could log a warning if an ERROR was returned, but don't change the behavior of it?

@jellehierck
Copy link
Author

Yes that would also be an acceptable change which clearly lets the users know what is happening instead of (somewhat) silently failing on Humble!

@saikishor
Copy link
Member

I can see how the change in behaviour is something to consider carefully. But arguably, the current behaviour is wrong ;) We can leave that up to @saikishor and the other @ros-controls/ros2-maintainers.

I do agree that it is wrong, that why I proposed this change for the Jazzy. The problem is the Humble doesn't have the disabling of controller when an ERROR is received in read and write cycles. That's why I'm hesitant to the change. Moreover, if the user doesn't have a robust hardware Component, then any NaN in the Handles might trigger this

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

Successfully merging a pull request may close this issue.

3 participants