-
Notifications
You must be signed in to change notification settings - Fork 289
Implement action recording and display info about recorded action #1939
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
Implement action recording and display info about recorded action #1939
Conversation
|
@Barry-Xu-2018 thanks 👍 please let us know once this is ready to review. |
|
CC: @MichaelOrlov |
3b18d88 to
83b0d07
Compare
Okay. I will notify you once it's ready. |
|
Although the design (#1928) is still being finalized, there are no objections regarding the record and info sections, so I have implemented these parts according to the design. If you have time, please help review it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Barry-Xu-2018 Thank you for your contribution and hard work. The PR is huge. I've made a first round of review which is not thorough yet. However, let's address first my finding from the first round then will do one more round.
Among my findings and suggestions mentioned inline in the code, I have one major concern in regards to the design of the API changes in the RecordOptions structure.
In the current implementation a newly added fields as
std::vector<std::string> actions; // action internal topics and service event topicsand
std::vector<std::string> exclude_actions; // action internal topics and service event topicsimply to keep a list of extended "full" action's topic names and events.
i.e. we need to expand each action to a list of action topics list by calling for the rosbag2_cpp::action_name_to_action_topic_name(action); in a multiple places.
However, we are using this new parameters for actions and exclude_actions from RecordOptions struct only in the TopicFilter class.
It would be a more elegant and cleaner implementation if we would use non-expaned action names in the RecordOptions struct (the same as in the CLI options) and expand them to the real action topics inside TopicFilter constructor and keep them as a private member variables of the TopicFilter class.
This will also simplify the cases when we need to serialize-deserialize the RecordOptions structure and store it in config files or in the metadata.
|
Thank you for your review of the first round. I will handle your review comments. |
fujitatomoya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Barry-Xu-2018 this one is big... 😅 thank you very much for your effort. i got several comments, most of them are minor. please have them checked out!
rosbag2_transport/src/rosbag2_transport/config_options_from_node_params.cpp
Outdated
Show resolved
Hide resolved
Thanks. I agree with you. |
|
Thanks for your comments. I will handle them. |
During your review, my fix (4946f5f) hadn't been submitted yet. However, these issues were already fixed in my fix, so I marked it as resolved. |
|
I submitted below commit to handle your review comments. |
fujitatomoya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i confirmed all the comments are addressed, lgtm with green CI.
|
@Barry-Xu-2018 github action is not haapy, https://github.com/ros2/rosbag2/actions/runs/14080698263/job/39432789706?pr=1939 can you check this? |
|
@Barry-Xu-2018 github action is happy now, thanks! @MichaelOrlov could you check the your comments? in the meantime, i will start the CI. |
|
Pulls: #1939 |
|
The error log is I have no idea about this issue. The cause is related to an error in Gist. |
|
@Barry-Xu-2018 I see that there is existent |
My bad, I messed up and was wrong. We do handle feedback messages. |
| record_options.topic_types = args.topic_types | ||
| # Convert service name to service event topic name | ||
| record_options.services = convert_service_to_service_event_topic(args.services) | ||
| record_options.actions = args.actions if args.actions else [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to explicitly assign an empty list []?
It seems it could be simplified as a simple assignment
record_options.actions = args.actions
The same as topics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I change to this, running the ros2bag test will report this error.
[ros2bag-cli-7] File "/root/ros2_src_rolling/build/ros2bag/ros2bag/verb/record.py", line 361, in main
[ros2bag-cli-7] record_options.actions = args.actions
[ros2bag-cli-7] ^^^^^^^^^^^^^^^^^^^^^^
[ros2bag-cli-7] TypeError: (): incompatible function arguments. The following argument types are supported:
[ros2bag-cli-7] 1. (self: rosbag2_py._transport.RecordOptions, arg0: List[str]) -> None
I compared the code with the record_options.topics and did not find any differences in the types. Do you have any suggestion about this issue ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting.. I will take a look
| record_options.exclude_topics = args.exclude_topics if args.exclude_topics else [] | ||
| record_options.exclude_service_events = \ | ||
| convert_service_to_service_event_topic(args.exclude_services) | ||
| record_options.exclude_actions = args.exclude_actions if args.exclude_actions else [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same here it seems we don;t need conversion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have the same problem mentioned in #1939 (comment)
If I change record_options.exclude_topics, it also has the same problem.
hmmm, this is weird. i just use https://github.com/ros-tooling/ros-github-scripts, probably recent PR introduce some regression... anyway, for now, i just fixed the https://gist.githubusercontent.com/fujitatomoya/01ae3c5effe622750ff6f312bb94a203/raw/b2829009be047691533f0103136e64b5c27efbfa/ros2.repos, i will restart the CI |
i had a typo for the test target package. the new CI is started. |
Why do you think we need to handle GoalInfo.msg for recording or info command ? |
Signed-off-by: Barry Xu <[email protected]>
Signed-off-by: Barry Xu <[email protected]>
Signed-off-by: Barry Xu <[email protected]>
Signed-off-by: Barry Xu <[email protected]>
Signed-off-by: Barry Xu <[email protected]>
Signed-off-by: Barry Xu <[email protected]>
Signed-off-by: Barry Xu <[email protected]>
Signed-off-by: Barry Xu <[email protected]>
Signed-off-by: Barry Xu <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
- Rationale: We shall store the original message definition for recorded topics by design. This is also true for service events. Signed-off-by: Michael Orlov <[email protected]>
…action_info Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
da8fcd6 to
6bb2be1
Compare
|
Rebase was done. Gist: https://gist.githubusercontent.com/fujitatomoya/01ae3c5effe622750ff6f312bb94a203/raw/b2829009be047691533f0103136e64b5c27efbfa/ros2.repos |
|
@Barry-Xu-2018 The Windows CI Job fails with error messages: |
Thank you. I’ll investigate it. |
Signed-off-by: Barry Xu <[email protected]>
On the Windows platform, I couldn't find a successful way to make ContainsRegex support the "|" character ("\\|" doesn't make ContainsRegex work as expected. But this can avoid above error.). I had to switch to using std::regex for testing. e196b67 |
|
@MichaelOrlov CI is green, if you are good to go, can you merge this? |
Address ros-infrastructure/rep#405
Design #1928