-
Notifications
You must be signed in to change notification settings - Fork 247
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
Sort info output by topic name #1804
base: rolling
Are you sure you want to change the base?
Conversation
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.
it would be nice if format_service_with_type
has the same sorting interface aligned with topics.
this PR is failing to build with the following error and DCO is missing. could you address them?
07:59:08 /tmp/ws/src/rosbag2/rosbag2_py/src/rosbag2_py/format_bag_metadata.cpp:144:8: error: ‘iota’ is not a member of ‘std’
07:59:08 144 | std::iota(sorted_idx.begin(), sorted_idx.end(), 0);
07:59:08 | ^~~~
07:59:08 gmake[2]: *** [CMakeFiles/_storage.dir/build.make:90: CMakeFiles/_storage.dir/src/rosbag2_py/format_bag_metadata.cpp.o] Error 1
07:59:08 gmake[1]: *** [CMakeFiles/Makefile2:284: CMakeFiles/_storage.dir/all] Error 2
07:59:08 gmake: *** [Makefile:146: all] Error 2
Signed-off-by: Soenke Prophet <[email protected]>
657745f
to
8645046
Compare
Signed-off-by: Soenke Prophet <[email protected]>
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.
@Sanoronas Thank you for your contribution.
I agree with @fujitatomoya that sorting by the serialization format is unlikely to be useful. Because we are not supporting different serialization formats during recording by design.
I can only envision a possibility to have a different serilization formats if someone will convert the already recorded bag or record with modifyied rosbag2 recorder or using some different tool for recording.
Therefore I think it makes sense to have options for sorting:
- By topic or service name.
- By topic or service type
- By topic or service messages count
Could you please use C++ enum values for the sorting method?
Also, It will be appreciated if you would add the same sorting algorithm for the services in the format_service_with_type(..)
function located in the same file.
…ion format Signed-off-by: Soenke Prophet <[email protected]>
Signed-off-by: Soenke Prophet <[email protected]>
Signed-off-by: Soenke Prophet <[email protected]>
Thanks for the feedback @MichaelOrlov and @fujitatomoya I added --sort option for CLI, handling sorting of service topics and moved the sorting options to enum. |
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.
@Sanoronas, Thank you for addressing comments from the previous review and for adding a CLI option for the sorting method.
I've put some comments and requests for code changes. Also, in addition to that, could you please add sorting to the format_service_info(..)
function from the format_service_info.cpp
?
rosbag2/rosbag2_py/src/rosbag2_py/format_service_info.cpp
Lines 80 to 86 in cd7bd63
print_service_info(service_info_list[0]); | |
auto number_of_services = service_info_list.size(); | |
for (size_t j = 1; j < number_of_services; ++j) { | |
info_stream << std::string(indentation_spaces, ' '); | |
print_service_info(service_info_list[j]); | |
} |
The same way you did for services in another place.
For sorting by "count" you can use the sum of the
si->request_count
and si->response_count
.To add sorting for the
format_service_info.cpp
, the declaration of the enum class SortingMethod
will need to be moved to a separate header file, which makes sense anyway.Also, I would like to ask you to create the following
info_sorting_method_from_string(str)
helper function
SortingMethod info_sorting_method_from_string(std::string str)
{
static std::unordered_map<std::string, SortingMethod> sorting_method_map = {
{"name", SortingMethod::NAME},
{"type", SortingMethod::TYPE},
{"count", SortingMethod::COUNT}};
std::transform(str.begin(), str.end(), str.begin(), ::tolower);
auto find_result = sorting_method_map.find("str");
if (find_result == sorting_method_map.end()) {
throw std::runtime_error("Enum value match for \"" + str + "\" string is not found.");
}
return find_result->second;
}
Use it instead of rosbag2_py::stringToSortingMethod[sorting_method];
whenever conversion from string to enum is needed.
You can put a declaration of those info_sorting_method_from_string(str)
function to the same header file with the enum class. However, the definition should be in the cpp file to not violate the C++ ODR rule. Will need to create one more InfoSortingMethod.cpp
file for that.
And the last but not least request is to create a few unit tests to cover all these changes.
I know, creating integration unit tests from scratch sounds like a tedious and non-trivial task.
But we have got covered you 😉
We already have integration tests for the ros2 bag info
. However, they are in a different package. You can find them in the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_tests/test/rosbag2_tests/test_rosbag2_info_end_to_end.cpp.
All the infrastructure and sample bag files for tests already exist. Perhaps it will not be so difficult to write a few new tests similar to the existing ones.
…iles; move ServiceInformation and ServiceMetadata struct to storage package for clear include structure Signed-off-by: Soenke Prophet <[email protected]>
@MichaelOrlov , thanks for your input! I addressed some of your points in the recent commit (missing switch-case and tests). |
@Sanoronas Have you tried instruction from the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_py/README.md? |
@MichaelOrlov Yes, they were working fine until the recent changes |
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.
@Sanoronas Thanks for addressing most of my previous comments.
I don't think that moving ServiceInfo
with ServiceMetadata
to the rosbag2_storage
package is a good idea because it will cause a lot of confusion about why it is there if it is not used in any way in those package.
I belive we can do better.
First of all, there is some confusion about naming after the implementation of services support in Rosbag2. It seems I overlooked it on the review back then.
We already have a definition for the rosbag2_service_info_t
in the rosbag2_cpp
package, where we distinguish the service's requests and responses.
At the same time, we had a similar structure, ServiceInfo
, in the rosbag2_py
package defined in the format_bag_metada.cpp
. However, in the ServiceInfo
, we consolidate the service's requests and responses into the one event_message_count
field. In the Service Introspection feature, we name it as ServiceEvents.
To clean up the mess, we need to do the following:
- Rename
ServiceInfo
to theServiceEventInfo
to avoid further misunderstanding and confusion. - Move declaration of the
ServiceMetada
andServiceEventIno
in the separate header fileServiceEventInfo.hpp
and put it inside rosbag2_py/src, since we are not going to use it somewhere outside of therosbag2_py
package. This way, you will be able to refer to it from theformat_bag_metadata.cpp
and from the newly createdinfo_sorting_method.hpp{cpp}
files.
@Sanoronas As regards:
I've tried to checkout your latest version and follow up the instruction from the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_py/README.md
The error message is
I think this error message will go away as soon as you will move |
…erviceEventInformation; replace if-else by switch-case for differantiating between sorting methods; bugfix sorting method from string resolution and service info verbose not being sorted Signed-off-by: Soenke Prophet <[email protected]>
Signed-off-by: Soenke Prophet <[email protected]>
@MichaelOrlov Your other comments should also be resolved now. Regarding tests I used the existing resources for now. For more detailed tests we would probably need to create another resource with differing count, types and names to properly distinguish the available options. |
- Add missing const qualifier for the generate_sorted_idx(..) version with rosbag2_cpp::rosbag2_service_info_t - Small cleanups Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
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.
@Sanoronas I've managed to fix linker issues and was able to regenerate pyi stubs.
I also made small cleanups by moving things around and applying small nitpicks.
Please see my three latest commits on your branch.
However, I didn't have a chance to review unit tests yet.
Closes #1797