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

Add CLI option to set compression threads priority for Rosbag2 recorder #1760

Closed
MichaelOrlov opened this issue Jul 25, 2024 · 2 comments · Fixed by #1768
Closed

Add CLI option to set compression threads priority for Rosbag2 recorder #1760

MichaelOrlov opened this issue Jul 25, 2024 · 2 comments · Fixed by #1768
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Jul 25, 2024

Description

We need to add a CLI parameter for the ros2 bag record parameter to be able to set up compression threads priority.

Related Issues

Completion Criteria

The CLI parameter for compression threads priority exists and is covered by unit tests.

Implementation Notes / Suggestions

We already have the corresponding member variable RecordOptions::compression_threads_priority which matches to the
CompressionOptions::thread_priority in the ReaderWriterFactory::make_writer(..) method.

This is going to be a relatively low-effort and straightforward task since changes are expected to be only at the ros2bag and maybe rosbag2_py packages. Perhaps without breaking API/ABI compatibility, and it will be possible to backport to the Jazzy branch.

Testing Notes / Suggestions

We will need to add new or modify existing test in the ros2bag/test/test_recorder_args_parser.py to verify the correctness of the CLI parameter parser with the new CLI option.

Also, it will be great to add one integration test to the ros2bag/test/test_record.py to simply start the rosbag2 recorder with the newly added parameter and verify the debug output in the console. To facilitate debug output verification in the test will need to add some ROSBAG2_COMPRESSION_LOG_INFO_STREAM(..) or ROSBAG2_COMPRESSION_LOG_DEBUG_STREAM(..) output in the constructor of the SequentialCompressionWriter::SequentialCompressionWriter(..) and run ros2 bag record with corresponding --loglevel parameter from integration test. Note. This is supposed to be just a sanity check that the recorder can start with the settled-up CLI option. No need to publish messages and do a real recording with verification.

Update:
Actually will be better to add debug output to the void SequentialCompressionWriter::compression_thread_fn()

// Every thread needs to have its own compression context for thread safety.
auto compressor = compression_factory_->create_compressor(
compression_options_.compression_format);
rcpputils::check_true(compressor != nullptr, "Could not create compressor.");
while (true) {

Somewhere before while (true) { loop

@MichaelOrlov MichaelOrlov added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Jul 25, 2024
@MichaelOrlov
Copy link
Contributor Author

@r7vme Feel free to take this issue if you have time for it.

@r7vme
Copy link
Contributor

r7vme commented Jul 26, 2024

@MichaelOrlov sure, you can assign to me. I'll work on this issue next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants