[Design] Add design on recording and replay service #1359
Conversation
Signed-off-by: Barry Xu <[email protected]>
Barry-Xu-2018
requested review from
gbiggs and
james-rms
and removed request for
a team
There was a problem hiding this comment.
@Barry-Xu-2018 i have several comments.
@ihasdapie @emersonknapp thanks for sharing the status, since we have been away from rosbag2 activity for a while, it would be really appreciated if you can share details and thoughts on this.
CC: @MichaelOrlov
Barry-Xu-2018
changed the title
|
While recording finish, a YAML file will be created to save some information for rosbag info. So, after recording, created YAML file only save service name without request/response info. While executing rosbag info command, record file will be scanned (deserialization and parse content for service event message) and get the number of request/response. And then update YAML file. That is, the next time you execute "rosbag info," you will have complete information without the need to scan the recording file again. Of course, this approach would result in users not being able to obtain the complete information of services when they immediately view the YAML file after recording. If you have any better suggestions, please share them with me. |
|
@Barry-Xu-2018 Thanks for initiating discussion about service introspection recording and replay. Meanwhile I have a few comments in regards to the design in general.
|
It is 12:00 am Saturday in my local time. |
Service type can be gotten while recording phase (according to service event type). So it can be stored to metadata.yaml file like service name. Besides, service type can be gotten from MCAP file.
Do you think the information in the metadata.yaml file and the mcap file is different after only modifying metadata.yaml file?
Yes. There is no API. |
|
@Barry-Xu-2018 Instead of modifying |
Totally agree. Thanks. |
|
BTW, there is a quick way to parsing the number of request/response information. Use AddTwoInts demo as an example. The information regarding the request and response can be obtained from auto message_info = service_msgs::msg::ServiceEventInfo();
const rosidl_message_type_support_t * type_support_info =
rosidl_typesupport_cpp::get_message_type_support_handle<service_msgs::msg::ServiceEventInfo>();
// `info` member is located in the first item in `example_interfaces::srv::AddTwoInts_Event`
ret = rmw_deserialize(
&(serialized_message->get_rcl_serialized_message()),
type_support_info,
&message_info);
if (ret == RMW_RET_OK) {
RCLCPP_INFO(this->get_logger(), "Receive an event message, rmw_deserialize with `ServiceEventInfo`, type is: '%d'", message_info.event_type);
} else {
RCLCPP_INFO(this->get_logger(), "failed to rmw_deserialize");
}Since ServiceEventInfo has a fewer items (that is, Deserialization is fast), it is possible to get the number of request/response while recording. |
|
@MichaelOrlov @Barry-Xu-2018 I cannot make it today's call, so if this takes place i will catch up with you later, thanks! |
|
@Barry-Xu-2018 Am i understand correctly that in
|
Yes. You are right. |
|
@fujitatomoya @emersonknapp @ihasdapie We met with @Barry-Xu-2018 on today's Rosbag2 and Tooling WG meeting and come to the conclusion about the following:
P.S. Your feedback and opinion are welcome. |
|
@MichaelOrlov @Barry-Xu-2018 thanks for leading this.
agree on not having it update metadata.yaml, and this behavior. (just a comment)
agree. if this option applies, it should collect all hidden topics.
IMO,
sounds good. |
|
About record parameter for service,I list them based on current discussion result.
If users want to only record all services, there is no single parameter that can fulfill this requirement. The closest parameter to the recent requirement is "--include-hidden-topics." |
|
In this case, it will arise the same question if the user will want to record only all topics without services, there is no single parameter that can fulfill this requirement. |
Yes.
|
Adding |
How to unwrap request part in service event message. Assume we know the below information
// Use type support introspection
std::shared_ptr<rcpputils::SharedLibrary> ts_lib = rclcpp::get_typesupport_library(
"example_interfaces/srv/AddTwoInts_Event", "rosidl_typesupport_cpp");
const rosidl_message_type_support_t * ts = rclcpp::get_typesupport_handle(
"example_interfaces/srv/AddTwoInts_Event",
"rosidl_typesupport_cpp",
*ts_lib);
const rosidl_message_type_support_t * ts_handle = get_message_typesupport_handle(
ts,
rosidl_typesupport_introspection_cpp::typesupport_identifier);
const rosidl_typesupport_introspection_cpp::MessageMembers * message_members = (rosidl_typesupport_introspection_cpp::MessageMembers *)ts_handle->data;
// Allocate memory for deseriliazed message
void * ros_message = (void *)malloc(message_members->size_of_);
message_members->init_function(
ros_message, rosidl_runtime_cpp::MessageInitialization::ZERO);
// serialized_msg_ptr (std::shared_ptr<rcutils_uint8_array_t>) points to one service event seriliazed message
// Deserialize service event message
rmw_deserialize(serialized_msg_ptr.get(), ts, ros_message);
// Now ros_message point to all deseriliazed service topic event
// Get the offset part of request
// Now the size of ServiceEventInfo is fixed value. So the offset can be only calculated once.
//
// The strcut of service event message
// {
// ServiceEventInfo info
// example_interfaces::srv::AddTwoInts_Request_ request
// example_interfaces::srv::AddTwoInts_Response_ response
// }
//
// Service Event Info structure
// {
// uint8 event_type
// builtin_interfaces/Time stamp
// char[16] client_gid
// int64 sequence_number
// }
size_t request_offset = 0;
size_t request_offset_tmp = 0;
for(size_t i=0; i < message_members->member_count_; i++) {
auto message_member = message_members->members_[i];
if (message_member.name_ == std::string("request")) {
request_offset = request_offset_tmp;
} else {
auto sub_members = (rosidl_typesupport_introspection_cpp::MessageMembers *)message_member.members_->data;
request_offset_tmp += sub_members->size_of_;
}
if (request_offset != 0) {
break;
}
}
// Create node
auto node = std::make_shared<rclcpp::Node>("send_request_topic");
rclcpp::QoS qos = rclcpp::ServicesQoS();
rcl_client_options_t options = rcl_client_get_default_options();
options.qos = qos.get_rmw_qos_profile();
// Create client
// GenericClient should be new created. It can accept service type string to create client
// async_send_request() can accept void * as input message.
auto client = std::make_shared<rclcpp::GenericClient>(
&*node->get_node_base_interface(),
node->get_node_graph_interface(),
"add_two_ints", // Get from service event topic
"example_interfaces/srv/AddTwoInts", // Get from service event type
options);
// Check if service is ready
while (!client->service_is_ready()) {
std::printf("wait for service ready !\n");
std::this_thread::sleep_for(std::chrono::seconds(1));
}
// Send request message
// Input data will be serilized according to type example_interfaces::srv::AddTwoInts_Request_.
client->async_send_request((void *)((uint8_t *)ros_message + request_offset));The definition of class GenericClient likes namespace rclcpp {
class GenericClient : public ClientBase {
public:
GenericClient(
rclcpp::node_interfaces::NodeBaseInterface * node_base,
rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph,
const std::string & service_name,
const std::string & service_type,
rcl_client_options_t & client_options)
: ClientBase(node_base, node_graph)
{
ts_lib_ = rclcpp::get_typesupport_library(
service_type, "rosidl_typesupport_cpp");
// Now rclcpp::get_typesupport_handle only support message.
// We should make it support service
auto srv_type_support = rclcpp::get_typesupport_handle_srv(
service_type,
"rosidl_typesupport_cpp",
*ts_lib_);
srv_type_support_handle_ = get_service_typesupport_handle(srv_type_support, "rosidl_typesupport_cpp");
auto request_typesupport = srv_type_support_handle_->request_typesupport;
request_members_ = (rosidl_typesupport_introspection_cpp::MessageMembers *)request_typesupport->data;
auto response_typesupport = srv_type_support_handle_->response_typesupport;
response_members_ = (rosidl_typesupport_introspection_cpp::MessageMembers *)response_typesupport->data;
rcl_ret_t ret = rcl_client_init(
this->get_client_handle().get(),
this->get_rcl_node_handle(),
srv_type_support_handle_,
service_name.c_str(),
&client_options);
if (ret != RCL_RET_OK) {
if (ret == RCL_RET_SERVICE_NAME_INVALID) {
auto rcl_node_handle = this->get_rcl_node_handle();
// this will throw on any validation problem
rcl_reset_error();
expand_topic_or_service_name(
service_name,
rcl_node_get_name(rcl_node_handle),
rcl_node_get_namespace(rcl_node_handle),
true);
}
rclcpp::exceptions::throw_from_rcl_error(ret, "could not create generic client");
}
}
std::shared_ptr<void>
create_response() override
{
auto response = (void *)malloc(response_members_->size_of_);
response_members_->init_function(response, rosidl_runtime_cpp::MessageInitialization::ZERO);
return std::shared_ptr<void>(response, [](void *p){free(p);});
}
std::shared_ptr<rmw_request_id_t>
create_request_header() override
{
// TODO(wjwwood): This should probably use rmw_request_id's allocator.
// (since it is a C type)
return std::shared_ptr<rmw_request_id_t>(new rmw_request_id_t);
}
void
handle_response(
std::shared_ptr<rmw_request_id_t> request_header,
std::shared_ptr<void> response) override
{
erase_pending_request(request_header->sequence_number);
}
int64_t
async_send_request(const void * request)
{
int64_t sequence_number;
std::lock_guard<std::mutex> lock(pending_requests_mutex_);
rcl_ret_t ret = rcl_send_request(get_client_handle().get(), request, &sequence_number);
if (RCL_RET_OK != ret) {
rclcpp::exceptions::throw_from_rcl_error(ret, "failed to send request");
}
pending_requests_.emplace(sequence_number);
return sequence_number;
}
protected:
void
erase_pending_request(int64_t request_number)
{
std::unique_lock<std::mutex> lock(pending_requests_mutex_);
auto it = pending_requests_.find(request_number);
if (it == this->pending_requests_.end()) {
RCUTILS_LOG_DEBUG_NAMED(
"rclcpp",
"Received invalid sequence number. Ignoring...");
} else {
pending_requests_.erase(request_number);
}
}
RCLCPP_DISABLE_COPY(GenericClient)
std::set<int64_t> pending_requests_;
std::mutex pending_requests_mutex_;
private:
std::shared_ptr<rcpputils::SharedLibrary> ts_lib_;
const rosidl_service_type_support_t * srv_type_support_handle_;
const rosidl_typesupport_introspection_cpp::MessageMembers * request_members_;
const rosidl_typesupport_introspection_cpp::MessageMembers * response_members_;
};
} // rclcpprclcpp::get_typesupport_handle_srv() should be wrapped to rclcpp::get_typesupport_handle(). const rosidl_service_type_support_t *
get_typesupport_handle_srv(
const std::string & type,
const std::string & typesupport_identifier,
rcpputils::SharedLibrary & library)
{
std::string package_name;
std::string middle_module;
std::string type_name;
std::tie(package_name, middle_module, type_name) = extract_type_identifier(type);
auto mk_error = [&package_name, &type_name](auto reason) {
std::stringstream rcutils_dynamic_loading_error;
rcutils_dynamic_loading_error <<
"Something went wrong loading the typesupport library for service type " << package_name <<
"/" << type_name << ". " << reason;
return rcutils_dynamic_loading_error.str();
};
try {
std::string symbol_name = typesupport_identifier + "__get_service_type_support_handle__" +
package_name + "__" + (middle_module.empty() ? "srv" : middle_module) + "__" + type_name;
const rosidl_service_type_support_t * (* get_ts)() = nullptr;
// This will throw runtime_error if the symbol was not found.
get_ts = reinterpret_cast<decltype(get_ts)>(library.get_symbol(symbol_name));
return get_ts();
} catch (std::runtime_error &) {
throw std::runtime_error{mk_error("Library could not be found.")};
}
} |
|
@MichaelOrlov Friendly ping. Please look at above description of property on how to send request according to recorded service event message. |
1. Add '/srv/' to judge if type is for service event topic 2. Update and add new parameters for record command 3. Add '-v'/'--verbose' to info command Signed-off-by: Barry Xu <[email protected]>
|
According to above discussion, I updated the design. Please refer to 13effd3 |
Signed-off-by: Barry Xu <[email protected]>
Signed-off-by: Barry Xu <[email protected]>
@Barry-Xu-2018 Thank you for spending your time prototyping possible implementation for the service playback.
And the rationale for that is that publishing service requests require support from the rclcpp layer and the level of the changes looks like a significant feature that will require some time to get it done. As regards the implementation. size_t request_offset = 0;
size_t request_offset_tmp = 0;
for(size_t i=0; i < message_members->member_count_; i++) {
auto message_member = message_members->members_[i];
if (message_member.name_ == std::string("request")) {
request_offset = request_offset_tmp;
} else {
auto sub_members = (rosidl_typesupport_introspection_cpp::MessageMembers *)message_member.members_->data;
request_offset_tmp += sub_members->size_of_;
}
if (request_offset != 0) {
break;
}
}Has to be rewritten in some better way. And actually, I think there is some mistake in calculating the offset. Basically, we need to check the service event type and based on it. Perhaps we can cast the beginning of the deserialized ros message to the Service Event Info structure to be able to get the As regards the |
|
Thank you for sharing the results of your discussion
Totally agree. The second phase relies on modifications to the rclcpp for implementation.
Yes. I also prefer to place them in rclcpp.
Yes. I will update it. It can be simplified as your suggestion
Yes. We should check if the type of event message is
Yes. I only do simply implementation for sending request. Finally, GenericClient should be implemented as you suggested. |
|
Small comment: I'm personally not a big fan of all the new options for selecting topics/services, after this lands we will have During initial prototyping for service introspection I recall having to do something similar with loading the typesupports at runtime, but for getting the request/response msg typesupports given a service. Not sure if it'll be useful, but here it is anyways: https://gist.github.com/ihasdapie/e3950d0802a23d23419947c3496980ca. Another caveat I recall is that there are differences between the I second the two-phase implementation, playback is a lot more rather unrelated work and it'd quite annoying to block what is essentially a rosbag2 cli syntactic sugar PR because of it. |
|
Thanks for your comments.
Yes, that would reduce the number of parameters. However, the style and usage of topic are different (topic uses '-e REGEX').
Thanks, Your example provides another way to get request/response msg typesupports.
Yes. |
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-06-15/32038/1 |
|
@Barry-Xu-2018 Sorry for my late response.
On one hand it would be good to have support for wild cards, although on another hand it is going to be non-trivial in implementation. I would go with dedicated CLI options first. |
Agree.
Agree. So in order to keep the same style, we have to add new parameter for the service
About Do you any comments on the above table? @MichaelOrlov @ihasdapie |
|
@Barry-Xu-2018 Looks good to me. |
Okay. |
Signed-off-by: Barry Xu <[email protected]>
There was a problem hiding this comment.
@Barry-Xu-2018 I think we are pretty close to being good to go with this design document.
Need to mention that services playback is gonna implemented as phase 2 after implementing relevant API on rclcpp layer (list API which we needed), also remove -s and --services from playback.
Yes. I have started implementing the main functionality of phase 1. Once the new relevant API is added to rclcpp, I will start the implementation of phase 2. |
Signed-off-by: Barry Xu <[email protected]>
|
f0f8ddd will be updated for play option part based on the disccusion result of https://github.com/ros2/rosbag2/pull/1359/files#r1244556452 |
Signed-off-by: Barry Xu <[email protected]>
1. Add the descrption of `--include-hidden-topics` 2. Update play options 3. Add descripton of implementation phase Signed-off-by: Barry Xu <[email protected]>
There was a problem hiding this comment.
@Barry-Xu-2018 LGTM.
Thanks for your patients and versioning.
|
@MichaelOrlov Thanks. I share the current status. |
|
@Barry-Xu-2018 As regards
The draft PR will be appreciated. |
Okay. I will create draft PR. |
|
@MichaelOrlov |
|
Thanks, much appreciated. |
Related to #773