RCLC Actions Implementation

[pablogs9]

I propose here a draft implementation for ROS 2 Actions in the RCLC executor. By now I have integrated the Action server and client in the RCLC executor and created some utility functions.

The design should be discussed in the following points:

• Should this RCLC Action implementation be multithreaded or thread-safe? Or we are ok with handling one action call per time?
• Should some callbacks like the client's rclc_action_client_goal_callback_t be hidden from the user and be called directly from the executor?

Related:

• Add actions to examples micro-ROS/micro-ROS-demos#53
• RCL Actions static operation micro-ROS/rcl#10

TODO:

• Document code
• Client cancel goal
• Client status (Is this neccesary?)
• Tests

Please feel free to add comments about the design or the implementation.

CC: @Acuadros95

---

Update API need to be modified, in RCLCPP user provides the following callbacks:

• Action server: handle_goal, handle_cancel, handle_accepted
• Action client: goal_response_callback, feedback_callback, result_callback

In the client, goal_response_callback should be optional and the executor should be in charge of request the result if goals were accepted. Feedback callback also should be optional and the middleware layer should be in charge of discard non-read messages. Result callback should be compulsory in RCLC Actions API.

---

Related:

• RCL Action implementation using static memory: RCL Actions static operation micro-ROS/rcl#10

[ralph-lange]

Did a quick review and found minor points only. @JanStaschulat, could you please perform a thorough review as maintainer of rclc and developer of the Executor - but probably after the above TODOs are implemented.

[pablogs9]

@Acuadros95 I cannot add you as a reviewer, but at some point next week would be nice if you precheck this new API.

[pablogs9]

Hello @ralph-lange and @JanStaschulat could you review this? It is done from our side and we want to merge it.

@Acuadros95 could you detail the problems with the CI?

[Acuadros95]

Apart from my last comment, last CI failed on rclc_lifecycle_tests.
Also, I just updated micro-ROS rclc action demos with this API: micro-ROS/micro-ROS-demos#53

[pablogs9]

@mergify backport master

[mergify]

backport master

🟠 Waiting for conditions to match

• merged [:pushpin: backport requirement]

Hey, I reacted but my real name is @Mergifyio

[JanStaschulat]

As type structs of the executor (e.g. rclc_executor_handle_t and rclc_executor_handle_counters_t) are updated, this would break ABI stability for Galactic release. I propose to rebase and merge the "RCLC Actions Implementation" PR with main (Rolling).

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 68.97059% with 211 lines in your changes missing coverage. Please review.

Project coverage is 63.28%. Comparing base (0b90b3f) to head (2028066).
Report is 48 commits behind head on master.

Files with missing lines	Patch %	Lines
rclc/src/rclc/executor.c	72.80%	48 Missing and 51 partials ⚠️
rclc/src/rclc/action_goal_handle.c	59.87%	32 Missing and 31 partials ⚠️
rclc/src/rclc/action_client.c	68.75%	11 Missing and 14 partials ⚠️
rclc/src/rclc/action_server.c	69.62%	7 Missing and 17 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
+ Coverage   60.84%   63.28%   +2.44%     
==========================================
  Files          13       16       +3     
  Lines        1494     2160     +666     
  Branches      445      647     +202     
==========================================
+ Hits          909     1367     +458     
- Misses        379      475      +96     
- Partials      206      318     +112     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JanStaschulat]

Thanks you for updating the review. I am fine with all the changes except the verification of the while loops. Please document in more detail in which you show that the while loops always terminate (in semi-formal way) e.g. using
pre-condition:xxx
post-condition: xxx

Also rebase to main and create PR for Rolling.

[JanStaschulat]

How about using a for-loop with just a state-checking condition?

• better performance (currently with n elements in the list, the search function has to loop trough the list n times, with a for-loop you go through the list only once
• no argumentation for endless-while loop necessary

[Acuadros95]

I agree with the better performance argument, it will also simplify the code.

Edit: @JanStaschulat After looking into it, using a for-loop approach can be tricky. rclc_action_put_goal_handle is called conditionally and it removes an element from the linked list, messing with the iterator.

I think the while approach is the best for this case.

[JanStaschulat]

I was thinking about sometheing like this:

entity = xxx->action_client;
rclc_action_goal_handle_t * handle;
 for(handle =  entity->used_goal_handles; handle != NULL; handle = handle->next)
{
  if (handle->available_feedback)
  {
   // do sth
  }
 }

You might want to generalize this function with the condition-check as parameter function if you like.
But already with this restructuring using this for-loop instead of while-loopand the rclc_action_find_first_handle_with_XXX functions, which travers the linked list multiple times, the code does not have the problem of potential unbounded while loops.

[JanStaschulat]

I agree with the better performance argument, it will also simplify the code.

Edit: @JanStaschulat After looking into it, using a for-loop approach can be tricky. rclc_action_put_goal_handle is called conditionally and it removes an element from the linked list, messing with the iterator.

I think the while approach is the best for this case.

Edit: Are always the elements removed? Or only in some cases? At least in those cases, where elements are not removed, you could use a for-loop style traversal.

[Acuadros95]

Maybe we can modify rclc_action_put_goal_handle to return the next value on the list previous to the delete,
so it works more or less like iterators on c++:

entity = xxx->action_client;
rclc_action_goal_handle_t * handle;
 for(handle =  entity->used_goal_handles; handle != NULL;)
{
  if (handle->available_feedback)
  {
    if (...)
    {
        // ...
        handle = handle->next;
    }
    else
    {
        handle = rclc_action_put_goal_handle(...);
    }
  }
  else
  {
    handle = handle->next;
  }
 }

Anyway, this can lead to infinite loops too.

Edit: they are only removed in some cases, so yes, we can use the for-loop style in most places.

[JanStaschulat]

removed reset of data_available flag

[pablogs9]

DCO and we are done @Acuadros95

[pablogs9]

Please @JanStaschulat add your final review and we are ready to merge

[JanStaschulat]

LGTM

[mergify]

backport master

❌ No backport have been created

The new Mergify permissions must be accepted to create pull request with .github/workflows changes.
You can accept them at https://dashboard.mergify.com/

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/micro-ros-fully-supports-ros-2-features/23213/1

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-humble-hawksbill-released/25729/14