Working async controllers and components [not synchronized] #1041
Conversation
github-actions
bot
requested review from
bmagyar,
christophfroehlich,
DasRoteSkelett,
destogl,
duringhof,
jaron-l and
rosterloh
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
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}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
|
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. |
controller_interface/include/controller_interface/async_controller.hpp
Outdated
Show resolved
Hide resolved
destogl
added
backport-humble
|
@Mergifyio backport humble iron |
✅ Backports have been created
|
(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
(cherry picked from commit 2cbe470)
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.