Add record, stop, start_discovery, stop_discovery and is_discovery_stopped services for rosbag2_transport::Recorder #1840
Conversation
Signed-off-by: Sharmin Ramli <[email protected]>
Signed-off-by: Sharmin Ramli <[email protected]>
Signed-off-by: Sharmin Ramli <[email protected]>
Signed-off-by: Sharmin Ramli <[email protected]>
Signed-off-by: Sharmin Ramli <[email protected]>
Signed-off-by: Sharmin Ramli <[email protected]>
Signed-off-by: Sharmin Ramli <[email protected]>
Signed-off-by: Sharmin Ramli <[email protected]>
sharminramli
requested review from
emersonknapp and
jhdcs
and removed request for
a team
Signed-off-by: Sharmin Ramli <[email protected]>
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; |
There was a problem hiding this comment.
stop_discovery_ will be exchanged for false in start_discovery(), and if it was previously true, then topics_discovery will be started. it should not exit because of the exchange.
subsequently stop_discovery() will exchange stop_discovery_ to true and when start_discovery() is called again it can again exchange stop_discovery_ for false and start topics_discovery()
| 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).
|
@MichaelOrlov Thank you for the review. I've unfortunately been busy this week but I'm starting to work on the changes. I have a question regarding:
I am right now reusing the existing Stop.srv in rosbag2_interfaces which is being used by the player. If I create a new srv e.g. StopRecorder.srv to have the boolean response, I would also rename the existing Stop.srv to e.g. StopPlayer.srv to avoid confusion. That however might break a lot of existing code for users. Alternatively I could add the bool to the existing Stop.srv (and also set it in the player when called). What do you think would be the best way? |
|
@sharminramli Adding a return value to the existing service requests looks like more safer option. |
Adds the following services to rosbag2_transport::Recorder:
Fixes some issues with starting record again after stopping:
Closes #1634