Add record, stop, start_discovery, stop_discovery and is_discovery_stopped services for rosbag2_transport::Recorder #1840
Conversation
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
requested review from
emersonknapp and
jhdcs
and removed request for
a team
Signed-off-by: Sharmin Ramli <sharmin.ramli@enway.ai>
There was a problem hiding this comment.
@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.
| 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()); | ||
| } |
There was a problem hiding this comment.
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(); | |||
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
stop_discovery_ shall be false by default. Otherwise, we will immediately exit from RecorderImpl::topics_discovery() when running it the first time.
| 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(); |
There was a problem hiding this comment.
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).
Adds the following services to rosbag2_transport::Recorder:
Fixes some issues with starting record again after stopping:
Closes #1634