Clearer warning message, the old one lacked information and was perhaps misleading

[peter-mitrano-ar]

Description

The old error said "Error occurred while doing error handling." but I don't think that's accurate, or is at least confusing. To be honest I'm not sure we need any warning here at all, but if we do, my hope is that this is clearer.

I encountered this with ros2 control, so here's an example:

BEFORE

[ros2_control_node-1] [ERROR] [lookat_controller_head]: Non-zero gain pd_gains.trans_x.p=2 is not allowed for lookat controller
[ros2_control_node-1] [WARN] [lookat_controller_head]: Error occurred while doing error handling.
[ros2_control_node-1] [ERROR] [controller_manager]: After configuring, controller 'lookat_controller_head' is in state 'unconfigured' , expected inactive.

AFTER

[ros2_control_node-1] [ERROR] [lookat_controller_head]: Non-zero gain pd_gains.trans_x.p=2 is not allowed for lookat controller
[ros2_control_node-1] [WARN] [lookat_controller_head]: Callback returned ERROR during the transition: configure
[ros2_control_node-1] [ERROR] [controller_manager]: After configuring, controller 'lookat_controller_head' is in state 'unconfigured' , expected inactive.

But maybe I misunderstand, and this change does not make sense.
If this looks good, I can also make a PR to rolling, but I currently use humble so this is where I started.

Is this user-facing behavior change?

Changes the log they see, hopefully for the better.

Did you use Generative AI?

No.

[fujitatomoya]

@peter-mitrano-ar

thanks for bringing up this question with PR.

i believe this error message is meant to be the error during error handling callback, which is not related to the original transition. so i think original message Error occurred while doing error handling. is appropriate to print here.

btw i thinks error handling is missing some functionalities such as ros2/rcl_interfaces#97

[peter-mitrano-ar]

@fujitatomoya

i believe this error message is meant to be the error during error handling callback

This would be the on_error callback, right? It looks to me like that happens later and has not happened yet. This warning is printed regardless of whether on_error is successful or not.

so i think original message Error occurred while doing error handling. is appropriate

So if a user callback returns ERROR, but on_error has not been called yet, does this message really make sense? I don't think so.

I tested this just to confirm on_error is called after this warning is printed:

[ros2_control_node] [ERROR] [...]: Non-zero gain pd_gains.trans_x.p=1 ...
[ros2_control_node] [WARN] [...]: Callback returned ERROR during the transition: configure
[ros2_control_node] [WARN] [...]: on_error called in lookat controller.

Here is my on-error for testing, as you can see it returns SUCCESS.

  CallbackReturn CartesianLookAtController::on_error(const rclcpp_lifecycle::State& previous_state)
  {
    RCLCPP_WARN(get_node()->get_logger(), "on_error called in lookat controller.");
    return CallbackReturn::SUCCESS;
  }

But perhaps I still not understanding the code? Thank you for you prompt reply and for pointing out that error handling is still missing some things!

[fujitatomoya]

Ah, sorry you are right. i was the confused here.... thanks for pointing that out!

on_error is not yet called here, transition callback returned error. that said, i agree with you that the error message here is not appropriate. and the later, it will call the on_error callback to see if it can clean up the error state for this transition.

[fujitatomoya]

lgtm with green CI.

[fujitatomoya]

Ah, sorry you are right. i was the confused here.... thanks for pointing that out!

on_error is not yet called here, transition callback returned error. that said, i agree with you that the error message here is not appropriate. and the later, it will call the on_error callback to see if it can clean up the error state for this transition.

[fujitatomoya]

@peter-mitrano-ar as you suggested, can you target this PR against rolling? and then we can do the backports for the downstream distros.

[peter-mitrano-ar]

@fujitatomoya Yes I will change this to be for rolling

[jmachowinski]

Pulls: #2924
Gist: https://gist.githubusercontent.com/jmachowinski/c087a62ea49832c6da2069de8f301ec1/raw/2212f3334a750ad064abbac379e14e6ddbef1482/ros2.repos
BUILD args:
TEST args:
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16703

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[peter-mitrano-ar]

I think this can be merged, we already merged the ones for Rolling and Kilted. Looks like Jazzy might be ready also?