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

[Diagnostics] - A non active controller should not be reported as error #1183

Closed
firesurfer opened this issue Nov 30, 2023 · 4 comments
Closed
Labels

Comments

@firesurfer
Copy link
Contributor

Describe the bug

Currently controllers that are loaded but are deactivated are reported as errors via ROS diagnostics.
grafik

I think this should be a warning. In my system for example I regularly switch controllers (e.g. from a compliant controller to the jtc and back). The other controller being inactive is desired behavior in that case and should not be reported as an error.

To Reproduce

Load a controller and deactivate it

Expected behavior

Report nothing or just a warning via ROS diagnostics

Environment (please complete the following information):

  • OS: Ubuntu 22.04
  • Version iron
@firesurfer firesurfer added the bug label Nov 30, 2023
@saikishor
Copy link
Member

Hello @firesurfer!

Yes, it makes sense that it shouldn't be reported as an error if the controllers are loaded but are deactivated. Feel free to open a PR fixing this part of the code:

void ControllerManager::controller_activity_diagnostic_callback(
diagnostic_updater::DiagnosticStatusWrapper & stat)
{
// lock controllers
std::lock_guard<std::recursive_mutex> guard(rt_controllers_wrapper_.controllers_lock_);
const std::vector<ControllerSpec> & controllers = rt_controllers_wrapper_.get_updated_list(guard);
bool all_active = true;
for (size_t i = 0; i < controllers.size(); ++i)
{
if (!is_controller_active(controllers[i].c))
{
all_active = false;
}
stat.add(controllers[i].info.name, controllers[i].c->get_state().label());
}
if (all_active)
{
stat.summary(diagnostic_msgs::msg::DiagnosticStatus::OK, "All controllers are active");
}
else
{
stat.summary(diagnostic_msgs::msg::DiagnosticStatus::ERROR, "Not all controllers are active");
}
}
.

I would say it would be an OK state to be in the inactive state, if you are interested, you can also log the information of the controller stats like their update_rate and if they are async or not etc.

Thank you

@firesurfer
Copy link
Contributor Author

@saikishor I created a PR which just makes the switch from Error to Warn. Not sure if there is any other error information to report about controller (such as ros2 control could not active the controller)

@christophfroehlich
Copy link
Contributor

Is this fixed with #1184?

@saikishor
Copy link
Member

@christophfroehlich yes it should be fixed. Now it won't report it as an error

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

No branches or pull requests

3 participants