-
Notifications
You must be signed in to change notification settings - Fork 255
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 service recording and display info about recorded services #1480
Implement service recording and display info about recorded services #1480
Conversation
The comments are from MichaelOrlov (#1414 (review)) I have made a first pass of the review and here is my major notes and proposals:
|
rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp
Outdated
Show resolved
Hide resolved
By now, all your comments have been handled. |
One case would like to discuss it with you.
That is, the filtered topic or service must match 1 and 2 at the same time.
I'd like to hear your opinions. |
@Barry-Xu-2018 As regards
I would expect if the user explicitly specifies |
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 addressing the review comments.
I've run one more round of review. I found a few minor things to change.
Also, I have a concern about the way the tests are written in the test_info.cpp
.
On one hand, using the generated bag files in tests is a working approach, however on the other hand it will be difficult to support such test infrastructure in the future.
Sometimes we make changes in the bag format of metadata and have to regenerate all bags written for tests.
It would be better to write bags on the fly from each test to the temporary folder.
Could you please change tests this way?
You can find an example of how to write test bags on the fly in the other tests in the rosbag2_cpp
package. For instance this one.
TEST_P(ParametrizedTemporaryDirectoryFixture, info_for_standalone_bagfile) {
const auto storage_id = GetParam();
const auto bag_path = rcpputils::fs::path(temporary_dir_path_) / "bag";
{
// Create an empty bag with default storage
rosbag2_cpp::Writer writer;
rosbag2_storage::StorageOptions storage_options;
storage_options.storage_id = storage_id;
storage_options.uri = bag_path.string();
writer.open(storage_options);
test_msgs::msg::BasicTypes msg;
writer.write(msg, "testtopic", rclcpp::Time{});
}
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 I made one more round of review.
We are certainly getting close to the finish, but tests still need some "love".
In the current state, almost all newly added tests tend to be flaky and written in non-deterministic way and rely on the ambient delays between critical operations.
I decided to speed up our review and development process and contribute to the tests stability.
I've made a PR Barry-Xu-2018#1 with base to this branch.
Please review my changes and if you don't have any concerns or requests for changes please feel free to merge on your branch.
cc: @fujitatomoya
rosbag2_tests/test/rosbag2_tests/test_rosbag2_info_get_service_info.cpp
Outdated
Show resolved
Hide resolved
Thank you for your assistance. |
rosbag2_tests/test/rosbag2_tests/test_rosbag2_info_end_to_end.cpp
Outdated
Show resolved
Hide resolved
ab62249
to
6a39627
Compare
Do rebase since there are conflicts with rolling branch. |
case service_msgs::msg::ServiceEventInfo::REQUEST_SENT: | ||
case service_msgs::msg::ServiceEventInfo::REQUEST_RECEIVED: | ||
service_process_info[bag_msg->topic_name]->request[msg.client_gid].emplace( | ||
msg.sequence_number); | ||
break; |
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.
because of ros2/rmw#357, we cannot tell these messages at all, right? is this the current constraints or do we have work-around for that?
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.
Yes.
From the code perspective, we don't have a workaround.
We have to request that users do not enable service and client introspection simultaneously as a workaround.
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 Wouldn't you mind creating a follow-up issue to not forget to change behavior when ros2/rmw#357 will be resolved?
@Barry-Xu-2018 i think we are getting closer on this, well tested i think. there are a few |
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 thanks for the comments. i do not have further comments, lgtm with green CI.
CC: @MichaelOrlov
@Barry-Xu-2018 @fujitatomoya I need to recap what has been changed recently and what is yet unclear or unresolved from my questions. |
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 I made one more round of review.
I have a few major concerns:
- As per my comment in the Implement service recording and display info about recorded services #1480 (comment) The
service_in_list(..)
function is redundant and introduces some confusion and potentially could be a source of errors in the future. - As per my comment in the Implement service recording and display info about recorded services #1480 (comment) Relying on the assumption that embedded service dependencies will be always in the MSG files is totally wrong!
This is a valid scenario when the service's type dependencies could be in IDL files. Need to properly handle this case and correctly detect and save in what encoding message definition is stored for further proper decoding on playback. - There are some leftovers after rebase. Need to delete
rosbag2_transport/qos.cpp
. Since it was moved to therosbag2_storage
package and there are some delta from prior PRs. Delta from therosbag2_transport/qos.cpp
shall be disregarded as per our common agreement here Implement service recording and display info about recorded services #1480 (comment)
To avoid long ping-pong in review and speed up process I've made all fixes which I think shall be done for this PR in the separate branch and prepared a PR Barry-Xu-2018#2. Please review it. If you have no concerns or comments feel free to merge it on your branch or cherry-pick my commits.
Thank you for your comments.
Okay. The comparison is actually based on the topic name.
Yes, I didn't consider this scenario in my implementation.
Yes.
After review, LGTM. |
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, now LGTM with green CI.
Also please fix DCO. There is one missing signature for one commit.
@Barry-Xu-2018 Windows build fails with error message:
|
Signed-off-by: Barry Xu <barry.xu@sony.com>
02494e9
to
25653d0
Compare
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-for-2023-12-14/35153/1 |
Signed-off-by: Barry Xu <barry.xu@sony.com>
Re-run CI after an attempt to fix Windows build errors by adding visibility control to the new CPP library BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_test_common rosbag2_test_msgdefs rosbag2_transport rosbag2_tests |
@Barry-Xu-2018 @MichaelOrlov thank you very much! 🤞🤞🤞 |
@Barry-Xu-2018 Sorry the Windows CI is still unhappy.
and the same warning for the But before start fixing those warnings, I have a question. Why do you need those |
- Also put `format_service_info(..)` and `format_bag_meta_data(..)` under the rosbag2_py namespace Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@Barry-Xu-2018 I've made a quick prototype and it seems the code compiles without errors and warnings without Please note I've put newly exposed API under the |
@MichaelOrlov MichaelOrlov
As you changed, the same code
I considered this approach at the beginning, which would be simpler. However, it would result in duplicate code being compiled into different libraries, which could raise questions during review. So, I decided to compile this part of the code into libraries. |
Thank you. |
….hpp Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
…_service_recording Replace rosbag2_format_output by direct source files linkage
Barry-Xu-2018#4 has been merged. |
Re-run CI after removing BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_test_common rosbag2_test_msgdefs rosbag2_transport rosbag2_tests |
@Barry-Xu-2018 @fujitatomoya Thank you for your initiative and for driving this large PR. |
Merry Christmas and Enjoy your holiday 🎄 🎇 ! |
Split #1414 to 2 PRs.
This is first PR. This PR implement service record and recorded service info display.