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 record, stop, start_discovery, stop_discovery and is_discovery_stopped services for rosbag2_transport::Recorder #1840

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

Conversation

sharminramli
Copy link

@sharminramli sharminramli commented Oct 17, 2024

Adds the following services to rosbag2_transport::Recorder:

  • record
  • stop
  • start_discovery
  • stop_discovery
  • is_discovery_stopped

Fixes some issues with starting record again after stopping:

  • Restarts the event publisher thread
  • Initialize stop_discovery_ to false and invert the check in start_discovery so that discovery is restarted when starting record again

Closes #1634

Signed-off-by: Sharmin Ramli <sharmin.ramli@enway.ai>
Signed-off-by: Sharmin Ramli <sharmin.ramli@enway.ai>
Signed-off-by: Sharmin Ramli <sharmin.ramli@enway.ai>
Signed-off-by: Sharmin Ramli <sharmin.ramli@enway.ai>
Signed-off-by: Sharmin Ramli <sharmin.ramli@enway.ai>
Signed-off-by: Sharmin Ramli <sharmin.ramli@enway.ai>
Signed-off-by: Sharmin Ramli <sharmin.ramli@enway.ai>
Signed-off-by: Sharmin Ramli <sharmin.ramli@enway.ai>
@sharminramli sharminramli requested a review from a team as a code owner October 17, 2024 17:10
@sharminramli sharminramli requested review from emersonknapp and jhdcs and removed request for a team October 17, 2024 17:10
Signed-off-by: Sharmin Ramli <sharmin.ramli@enway.ai>
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.

@sharminramli Thank you for your contribution.
IMO it will be more consistent and understandable if rename is_discovery_stopped() to the is_discovery_running().
Also it would be more usefull if service request such as start_discovery, stop_dicovery, record and stop will return true or false as result.
Please note that record and stop could throw an exception. It does make sense to wrap them in try-catch and return fail in case of catching exception.
The service requests like record and stop shall be initialized in constructor rather than in the record() method. Otherwise how we will call record service request.
We have some other service requests initializations inside record method only because they depend from opened writer and assumption that recorder running only once.
Since now we can run recorder multiple times need to safeguard them with check if writer is open. And consider moving them in constructor as well.

Comment on lines +271 to +283
TEST_F(RecordSrvsTest, record_stop)
{
auto & writer = recorder_->get_writer_handle();
auto & mock_writer = dynamic_cast<MockSequentialWriter &>(writer.get_implementation_handle());

EXPECT_FALSE(mock_writer.closed_was_called());

successful_service_request<Stop>(cli_stop_);
EXPECT_TRUE(mock_writer.closed_was_called());

successful_service_request<Record>(cli_record_);
EXPECT_FALSE(mock_writer.closed_was_called());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add recorder start after the stop and then again stop to make sure that we can start again after the stop

@@ -154,6 +159,10 @@ class Recorder : public rclcpp::Node
ROSBAG2_TRANSPORT_PUBLIC
bool is_paused();

/// Return the current discovery state
ROSBAG2_TRANSPORT_PUBLIC
bool is_discovery_stopped();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename to the is_discovery_running()


std::mutex start_stop_transition_mutex_;
std::mutex discovery_mutex_;
std::atomic<bool> stop_discovery_ = false;
std::atomic<bool> stop_discovery_ = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

stop_discovery_ shall be false by default. Otherwise, we will immediately exit from RecorderImpl::topics_discovery() when running it the first time.

Suggested change
std::atomic<bool> stop_discovery_ = true;
std::atomic<bool> stop_discovery_ = false;

const std::shared_ptr<rosbag2_interfaces::srv::StartDiscovery::Request>/* request */,
const std::shared_ptr<rosbag2_interfaces::srv::StartDiscovery::Response>/* response */)
{
start_discovery();
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to check if (in_recording_) before calling start discovery. Otherwise, it will be an undefined behavior since the writer may not be opened yet, but discovery can try to create a subscription with a callback calling writer->write(message).

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.

Add record, stop, start_discovery and stop_discovery services for rosbag2_transport::Recorder
2 participants