Async/await and events executor in animation server #704
Conversation
…se async/await to avoid deadlocks.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the animation server and related HCM actions to use async/await with an EventsExecutor, improving resource utilization and reducing latency.
- Converted the animation action callback (
execute_cb) to anasync defand replaced blocking sleeps and service calls withawaitandasync_wait_for - Switched from
MultiThreadedExecutortoEventsExecutorand removed explicit callback groups - Added
async_wait_forandasync_run_threadutilities and updated the motor‐power service call to usecall_async
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| bitbots_motion/bitbots_hcm/bitbots_hcm/hcm_dsd/actions/change_motor_power.py | Replaced blocking call with call_async on the motor switch service |
| bitbots_motion/bitbots_animation_server/bitbots_animation_server/animation_node.py | Migrated to async def execute_cb, removed callback groups, replaced Duration sleeps, and switched to EventsExecutor |
| bitbots_misc/bitbots_utils/bitbots_utils/utils.py | Added async_wait_for and async_run_thread utilities for async support |
Comments suppressed due to low confidence (1)
bitbots_misc/bitbots_utils/bitbots_utils/utils.py:201
- New asynchronous helpers (
async_wait_forandasync_run_thread) lack dedicated tests. Add unit tests to verify timing accuracy, cancellation behavior, and exception handling.
async def async_wait_for(node: Node, rel_time: float):
bitbots_motion/bitbots_hcm/bitbots_hcm/hcm_dsd/actions/change_motor_power.py
Show resolved
Hide resolved
| @@ -194,3 +196,43 @@ def parse_parameter_dict(*, namespace: str, parameter_dict: dict) -> list[Parame | |||
| parameter = Parameter(name=full_param_name, value=param_value) | |||
| parameters.append(parameter.to_parameter_msg()) | |||
| return parameters | |||
|
|
|||
|
|
|||
| async def async_wait_for(node: Node, rel_time: float): | |||
There was a problem hiding this comment.
The signature async_wait_for(mynode, 10) can easily be misunderstood as waiting for mynode to become available, or similar. Therefore, I suggest the following solutions:
- rename to
async_wait - rename to
async_wait_for_seconds - change to
async_wait_for(rel_time: float, node: Node) - change to
async_wait_for(seconds: float, node: Node)
There was a problem hiding this comment.
The method sleep_until sounds like it expects a end-time, not a duration...
What do you think about using Duration instead of seconds: float? This might be cleaner and more ROSy, but is not really necessary and a bit overhead.
There was a problem hiding this comment.
I had it as a duration at one point, but I didn't like the verbosity. But I agree that it would be more like the rclpy API.
What do you think of sleep_for?
There was a problem hiding this comment.
YES! Does that need async as well in the name or is that clear through other means?
There was a problem hiding this comment.
I would add async, but it should show as a coroutine in the signature as well
| """ | ||
| Allows the usage of blocking functions in an async context. | ||
|
|
||
| Spawns a thread to run the function and returns a Future that will be set when the function is done. |
There was a problem hiding this comment.
This does not seem to return anything, so the comment is wrong. Otherwise, the type annotation is wrong and I have not yet understood async-await in Python...
There was a problem hiding this comment.
Yeah it doesn't return. But we should return, I will fix it.
Co-authored-by: Jan Gutsche <34797331+jaagut@users.noreply.github.com>
Summary
Use events executor in animation server. Due to the complex nature of an action server that is able to reject/cancel actions as well as the close communication with the hcm via service rpc calls this was not possible in a single threaded manner without the use of async and await. Which actually works great with ROS 2. Why is this not the default in the tutorials?????
This change brings the resource utilization from 30% (idle) to 2-6% (while playing animations ). Also the latency when calling animations (e.g. during falling) seems much lower.
Somehow ROS2 async does not has a async sleep function (using ros time), so I needed to build my own based on ros2/rclpy#1234. I also added a wrapper for blocking functions that was not needed in the end.
I tested many scenarios where e.g. the hcm cancels an animation in simulation and it seems to work without deadlocks.
Minor Updates:
bitbots_motion/bitbots_hcm/bitbots_hcm/hcm_dsd/actions/change_motor_power.py: Updated the motor switch service call to use the asynchronouscall_asyncas a blocking method is not needed and might be risky.Related issues
#682
Checklist
colcon build