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

Working async controllers and components [not synchronized] #1041

Merged
merged 14 commits into from
May 8, 2024

Conversation

VX792
Copy link
Contributor

@VX792 VX792 commented May 29, 2023

I haven't found a good way to synchronize the controllers / components with the ros2 control node's thread without changing a lot of stuff inside the handles (and controllers), and since they are getting reworked anyway, I thought I'll wait until then.

Before that, this solution assumes that all writes to the command interfaces' double values are atomic operations.

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2023

Codecov Report

Attention: Patch coverage is 40.00000% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 31.67%. Comparing base (35bb5f7) to head (ad1510a).
Report is 20 commits behind head on master.

❗ Current head ad1510a differs from pull request most recent head 9469f8b. Consider uploading reports for the commit 9469f8b to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1041       +/-   ##
===========================================
- Coverage   75.51%   31.67%   -43.84%     
===========================================
  Files          41       93       +52     
  Lines        3328    10854     +7526     
  Branches     1921     7428     +5507     
===========================================
+ Hits         2513     3438      +925     
- Misses        421      793      +372     
- Partials      394     6623     +6229     
Flag Coverage Δ
unittests 31.67% <40.00%> (-43.84%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
.../include/controller_manager/controller_manager.hpp 60.00% <ø> (+27.85%) ⬆️
controller_manager/src/ros2_control_node.cpp 0.00% <0.00%> (ø)
...lude/hardware_interface/loaned_state_interface.hpp 76.92% <66.66%> (+10.25%) ⬆️
...de/hardware_interface/loaned_command_interface.hpp 81.81% <50.00%> (-18.19%) ⬇️
...re_interface/include/hardware_interface/handle.hpp 80.00% <71.42%> (-20.00%) ⬇️
...ce/include/hardware_interface/async_components.hpp 0.00% <0.00%> (ø)
controller_manager/src/controller_manager.cpp 38.13% <0.00%> (-29.93%) ⬇️
hardware_interface/src/resource_manager.cpp 46.66% <0.00%> (-30.19%) ⬇️
hardware_interface/test/test_handle.cpp 46.00% <50.00%> (ø)

... and 77 files with indirect coverage changes

@VX792
Copy link
Contributor Author

VX792 commented Dec 3, 2023

I'll create the final version after the handle rework is merged, as it will be easier to support atomic types after that.

Do we wish to backport this to humble though? Because in that case, I think I can finish this PR by adding async systems / sensors / actuators, and if I'm not wrong it could be done with the current structure without breaking the API.

std::atomic<double> cmd_if_value = 1.337;

AsyncStateInterface state_handle{JOINT_NAME, FOO_INTERFACE, &state_if_value};
AsyncCommandInterface command_handle{JOINT_NAME, FOO_INTERFACE, &cmd_if_value};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you assure that threads get a snapshot of all current cmd_if_value(s) ?
In a situation where a sync controllers is writing to a bunch of cmd_if_value(s) and an async controller kicks in, the latter could end up with a mixture of cmd_if_value(s)

Shouldn't AsyncCommandInterface assure atomicity of the whole?
Without async controllers, data fetch was synchronized w.r.t. the controllers. This is not the case anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know it's not possible for two controllers to access the same cmd interfaces at once. But if you meant a sync hardware component, your point is valid, it's not guarranted that the controller will see the latest values. In fact, something like this could easily happen in case of using an asynchronous JTC on a 6-dof robot arm, where each joint is represented by a single command interface:

  • Joint1: async JTC sees the most recent value
  • Joint2: async JTC sees the most recent value
  • Joint3: async JTC sees the most recent value
  • Joint4: async JTC sees the most recent value
  • Joint5: async JTC sees the previous value
  • Joint6: async JTC sees the previous value

And the trajectory could get executed with joint5 and joint6 containing values from a previous tick. I guess it depends on the use case if this is acceptable or not.
I was thinking about a solution which solves this by notifying the async controller when the read is done (just as you've suggested previously), BUT the writes from the async controller to the command interfaces would still be non-blocking relaxed atomics. This would ensure that the controller works with the latest data, but the control loop is never blocked by the asynchronous controller if it takes forever to write its interfaces.

Would something like this fit your use case?

The AsyncCommandInterface in this example sadly can't assure atomicity of all used command interfaces, since it only represents a single command interface. I might have misunderstood you here though.

Copy link
Contributor

@atzaros atzaros Dec 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm, IMHO incongruent views of joint states/commands are quite undesirable.

// write
// read
component->write(clock_interface_->get_clock()->now(), measured_period);
component->read(clock_interface_->get_clock()->now(), measured_period);
}
next_iteration_time += period;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid that AsyncControllerThread::controller_update_callback and AsyncComponentThread::write_and_read can drift inexorably in long runs.

Again I don't feel comfortable having data dependencies between threads that awake independently.

@atzaros
Copy link
Contributor

atzaros commented Mar 13, 2024

After some internal discussion with @saikishor and @jordan-palacios, we are open to accept this PR.

Even though we still have concerns about data coherency and random sampling, maybe the community can benefit anyway of @VX792's effort.

destogl
destogl previously approved these changes Apr 24, 2024
Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Thanks!

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be needed for all the old distros as well. if possible, I propose to backport this.
For Jazzy, we should see if we should go with the new approach as proposed in #1489 or not.

@bmagyar bmagyar merged commit 2cbe470 into ros-controls:master May 8, 2024
9 of 11 checks passed
@destogl destogl added backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron. labels May 8, 2024
@destogl
Copy link
Member

destogl commented May 8, 2024

@Mergifyio backport humble iron

Copy link
Contributor

mergify bot commented May 8, 2024

backport humble iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 8, 2024
(cherry picked from commit 2cbe470)

# Conflicts:
#	controller_manager/include/controller_manager/controller_manager.hpp
#	controller_manager/src/controller_manager.cpp
#	hardware_interface/include/hardware_interface/async_components.hpp
#	hardware_interface/src/resource_manager.cpp
mergify bot pushed a commit that referenced this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants