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

[Design] Add design on recording and replay action #1928

Open
wants to merge 4 commits into
base: rolling
Choose a base branch
from

Conversation

Barry-Xu-2018
Copy link
Contributor

The design for recording and replay action.

@Barry-Xu-2018 Barry-Xu-2018 deleted the review/topic-add-design-for-record-replay-action branch March 4, 2025 08:57
@Barry-Xu-2018 Barry-Xu-2018 restored the review/topic-add-design-for-record-replay-action branch March 4, 2025 09:03
@Barry-Xu-2018 Barry-Xu-2018 reopened this Mar 6, 2025
Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

just a typo, lgtm

@fujitatomoya fujitatomoya marked this pull request as ready for review March 6, 2025 23:23
@fujitatomoya
Copy link
Contributor

@MichaelOrlov we would really like to have some help here 🙇 could you take a look at the design overview about ros2bag action support. we are working on this feature to make it available to Kilted, related REP is ros-infrastructure/rep#405

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@fujitatomoya @Barry-Xu-2018 Thank you for the design document. Overall seems good. I found a few nitpicks and one major proposal.

I would reverse the logic for the --send-action-goal parameter and send by default the real action's service requests instead of the recorded service events. As far as I understand, this would be a desired behavior in most cases.
I also found that --send-action-goal is unclear about what it really means and difficult to clarify nuances in the comments.

| :-- | :-- |
| --actions action [action ...] | Space-delimited list of actions to play. |
| --exclude-actions action [action ...] | Space-delimited list of actions not to play. |
| --send-action-goal | Send recorded action goal instead of recorded service events and action internal topics. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I would reverse the logic for the --send-action-goal parameter and send by default the real action's service requests instead of the recorded service events. As far as I understand, this would be a desired behavior in most cases.

Suggested change
| --send-action-goal | Send recorded action goal instead of recorded service events and action internal topics. |
| --send-action-events | Send recorded action service events instead of real action's service calls. |

I also found that --send-action-goal is unclear about what it really means and difficult to explain in the comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would reverse the logic for the --send-action-goal parameter and send by default the real action's service requests instead of the recorded service events. As far as I understand, this would be a desired behavior in most cases.

There is currently a parameter for play service request. If we reverse the logic for the --send-action-goal parameter, users will notice that the logic for services and actions is different.

  --publish-service-requests
                        Publish recorded service requests instead of recorded service events

I also found that --send-action-goal is unclear about what it really means and difficult to explain in the comments.

How about the following description?
Send recorded action goal. If this command parameter is not set, replay the send_goal service event, get_result service event, cancel_service event, feedback topic, and status topic in the recorded order. You can use ros2 action echo to view it.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm, i do not remember exactly how we get to decide this default behavior...
probably user needs to explicitly specify the send the actual request to the service because service server needs to be running in ROS 2 network. compared to the topic playback, this is kinda different expectation for user. the same logic goes to the action playback, the action server needs to be running when user specifies.

@Barry-Xu-2018 --send-action-goal sounds like only sending the goal request, but it just replays all the data stored in rosbag database right? including result request and cancel request.

Copy link
Contributor

Choose a reason for hiding this comment

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

The naming is hard!
Now I am questioning myself if do really need one more extra parameter --send-action-goal?
It seems that --publish-service-requests is doing exactly what --send-action-goal is supposed to do.
i.e. transform action service events to the real service requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fujitatomoya

--send-action-goal sounds like only sending the goal request, but it just replays all the data stored in rosbag database right? including result request and cancel request.

Based on my idea, if the '--send-action-goal' is set, rosbag2 will replay the action goal requests in the order they were recorded. User need to launch action server.
When this option is not set, it will replay all recorded action data (in addition to the send_goal event, it also includes the timing of cancel_goal and get_result, as well as feedback and status data. Users can view this through ros2 action echo).

@MichaelOrlov

It seems that --publish-service-requests is doing exactly what --send-action-goal is supposed to do.
i.e. transform action service events to the real service requests.

Yes. The essence of these two parameters is the same: whether to replay request or event messages. I used the new parameter '--send-action-goal' with another consideration in mind, which is to separately control the playback for services and actions. For example, service data is replayed with event messages, while action data can replay goal requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

i am inclined to take one option with --publish-service-requests including actions, but i will leave this to you and @MichaelOrlov

Copy link
Contributor Author

@Barry-Xu-2018 Barry-Xu-2018 left a comment

Choose a reason for hiding this comment

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

@MichaelOrlov

Thank you for your review. Let's discuss some of the comments further.

| :-- | :-- |
| --actions action [action ...] | Space-delimited list of actions to play. |
| --exclude-actions action [action ...] | Space-delimited list of actions not to play. |
| --send-action-goal | Send recorded action goal instead of recorded service events and action internal topics. |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would reverse the logic for the --send-action-goal parameter and send by default the real action's service requests instead of the recorded service events. As far as I understand, this would be a desired behavior in most cases.

There is currently a parameter for play service request. If we reverse the logic for the --send-action-goal parameter, users will notice that the logic for services and actions is different.

  --publish-service-requests
                        Publish recorded service requests instead of recorded service events

I also found that --send-action-goal is unclear about what it really means and difficult to explain in the comments.

How about the following description?
Send recorded action goal. If this command parameter is not set, replay the send_goal service event, get_result service event, cancel_service event, feedback topic, and status topic in the recorded order. You can use ros2 action echo to view it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants