Cannot publish action feedback in CANCELING state

[akir94]

Generated by Generative AI

No response

Operating System:

Linux IL01ON880001 6.14.0-37-generic #37~24.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu Nov 20 10:25:38 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

ROS version or commit hash:

lyrical

RMW implementation (if applicable):

No response

RMW Configuration (if applicable):

No response

Client library (if applicable):

rclcpp

'ros2 doctor --report' output

Steps to reproduce issue

ServerGoalHandle::publish_feedback() requires the goal to be in EXECUTING state.
This requirement was implemented here - #3118
As a result, feedback cannot be published when the goal is in CANCELING state.

Expected behavior

ServerGoalHandle::publish_feedback() should allow publishing in CANCELING state.
The action is still ongoing during this state, so its feedback is meaningful.
#3117 seems to agree since it only expects to block publishing feedback in terminal states, after calling succeed(), abort(), or canceled().

Actual behavior

Feedback is not published in CANCELING state.

Additional information

No response

[Rajat-Arora]

@akir94 Could you please explain the use case for this? Why is feedback required in the cancelling state, and why is the feedback needed if the client has already sent the request to cancel the goal?

[usamajahangir]

@akir94 I was reviewing the code and it is expected because the main function returns turn for all three states ACCEPTED, EXECUTING, CANCELLING which causes this

Fix is simple, but before I make the changes I have a question, is publishing required at Accepted state as well?

[akir94]

@Rajat-Arora I have an action that drives the robot on a sequence of waypoints.
The action's feedback also has the waypoint index that the robot is driving to.
On cancel, the action stops the robot.
However, the robot takes some time to brake and I want to know what waypoints it passes as it brakes.

@usamajahangir My specific case doesn't try to publish feedback in ACCEPTED state, so I'm indifferent to it.

[jmachowinski]

@akir94 Is it not sufficient to return the current reached waypoint in the result of the action ?

[jmachowinski]

@usamajahangir we forbid publishing in any state except executing for a reason. E.g. it is broken on a conceptional level and introduced technical issues.

As long as nobody can make a very good case for why this might be needed we will not accept PRs to change this behavior.

[akir94]

@jmachowinski You're right, and we are planning to adapt our code accordingly.

It's just that #3118 looks like a human error given that #3117 talks about terminal states.
Additionally, nav2's SimpleActionServer checks is_active(), not is_executing() - https://github.com/ros-navigation/navigation2/blob/7b9bcb4c2d6851414f0c8db3c1ef5a595221bd74/nav2_ros_common/include/nav2_ros_common/simple_action_server.hpp#L540
Any controller that tries to publish feedback in CANCELING will spam warning messages in the log.

I don't understand the objection that feedback during CANCELING is broken on a conceptual level.
I understand the objection for ACCEPTED state - the server really hasn't done anything with the goal yet.
But for CANCELING, the action server is still doing something, so it makes sense to report on it to the client.

[jmachowinski]

Let me double check the implementation tomorrow.
I actually misread the issue and now I agree with you that this is indeed a bug.
@fujitatomoya canceling is not a terminal state and it's not an automatic state transition. The last statement needs to be checked, but as far as I remember the transition happens when the server explicitly calls canceled on the goal handle. Therefore I don't see a reason to not publish feedback. Do you agree ?

[fujitatomoya]

@jmachowinski @akir94 @usamajahangir @Rajat-Arora

thanks for reporting this issue, and i am sorry that i could not attend the hackathon after ROSCon Toronto. (assuming that this came up there?)

anyway, looking back to the history...
yes i agree with you, publishing feedback in CANCELING is semantically valid according to the original design. (https://design.ros2.org/articles/actions.html)

#3117 was about feedback leaking after the goal had finished, the real concern was terminal states.
however, #3117 anchored the fix on the rclcpp Doxygen, which documents that it must only be called when the goal is executing and claims to throw std::runtime_error if the goal is in any other state. that Doxygen was itself stricter than the design doc. (this was where i made a mistake i think.)

i can apply the fix, and i will also apply the doc update inlcuding rclcpp/rclpy/rcl doc strings with message files.

thanks for raising the issue here 👍