-
Notifications
You must be signed in to change notification settings - Fork 41
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
Port zenoh_c to zenoh_cpp #327
Conversation
rclcpptest_executors
|
@ahcorde would it be possible to rebase this branch such that it only includes the newly introduced commits? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments to address from this first pass.
Signed-off-by: Alejandro Hernández Cordero <[email protected]> Fixed CI Signed-off-by: Alejandro Hernández Cordero <[email protected]> tokens Signed-off-by: Alejandro Hernández Cordero <[email protected]> removed zenoh_router_check Signed-off-by: Alejandro Hernández Cordero <[email protected]> removed rmw_init_options_impl.hpp Signed-off-by: Alejandro Hernández Cordero <[email protected]> Publisher Signed-off-by: Alejandro Hernández Cordero <[email protected]> Added subscribers Signed-off-by: Alejandro Hernández Cordero <[email protected]> Removed zid_to_str Signed-off-by: Alejandro Hernández Cordero <[email protected]> use zenoh::KeyExpr Signed-off-by: Alejandro Hernández Cordero <[email protected]> service data Signed-off-by: Alejandro Hernández Cordero <[email protected]> client data Signed-off-by: Alejandro Hernández Cordero <[email protected]> Removed unused signature Signed-off-by: Alejandro Hernández Cordero <[email protected]> cleanups and shm Signed-off-by: Alejandro Hernández Cordero <[email protected]> make linters happy Signed-off-by: Alejandro Hernández Cordero <[email protected]> make linters happy Signed-off-by: Alejandro Hernández Cordero <[email protected]> make linters happy Signed-off-by: Alejandro Hernández Cordero <[email protected]> make linters happy Signed-off-by: Alejandro Hernández Cordero <[email protected]> make linters happy Signed-off-by: Alejandro Hernández Cordero <[email protected]> make linters happy Signed-off-by: Alejandro Hernández Cordero <[email protected]> more changes Signed-off-by: Alejandro Hernández Cordero <[email protected]> Fixed segfault Signed-off-by: Alejandro Hernández Cordero <[email protected]> make linters happy Signed-off-by: Alejandro Hernández Cordero <[email protected]> make linters happy Signed-off-by: Alejandro Hernández Cordero <[email protected]> removed zenohc from CMakeLists.txt and warning Signed-off-by: Alejandro Hernández Cordero <[email protected]> Restored one method Signed-off-by: Alejandro Hernández Cordero <[email protected]> wrap the zenoh session with a shared pointer Signed-off-by: Alejandro Hernández Cordero <[email protected]> Changes missed Signed-off-by: Yadunund <[email protected]>
f8e85b1
to
196f30d
Compare
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
test_rclcpp
|
Fixed test_quality_of_service tests ros2/system_tests#555 |
This matches what we have observed so far. I'm still investigating the flaky test_executors. The test_n_nodes__rmw_zenoh_cpp has been identified #329. |
@ahcorde I made a suggested change here. diff --git a/rmw_zenoh_cpp/src/detail/rmw_client_data.cpp b/rmw_zenoh_cpp/src/detail/rmw_client_data.cpp
index bae0fe9..3cad22c 100644
--- a/rmw_zenoh_cpp/src/detail/rmw_client_data.cpp
+++ b/rmw_zenoh_cpp/src/detail/rmw_client_data.cpp
@@ -233,14 +233,13 @@ void ClientData::add_new_reply(std::unique_ptr<ZenohReply> reply)
reply_queue_.size() >= adapted_qos_profile.depth)
{
// Log warning if message is discarded due to hitting the queue depth
- std::string keystr = std::string(keyexpr_.value().as_string_view());
RMW_ZENOH_LOG_ERROR_NAMED(
"rmw_zenoh_cpp",
"Query queue depth of %ld reached, discarding oldest Query "
- "for client for %.*s",
+ "for client for %s",
adapted_qos_profile.depth,
- keystr.size(),
- keystr.c_str());
+ keyexpr_.value().as_string_view());
reply_queue_.pop_front();
}
reply_queue_.emplace_back(std::move(reply));
@@ -407,10 +406,11 @@ rmw_ret_t ClientData::send_request(
parameters,
[client_data](const zenoh::Reply & reply) {
if (!reply.is_ok()) {
+ auto reply_err_str = reply.get_err().get_payload().as_string();
RMW_ZENOH_LOG_ERROR_NAMED(
"rmw_zenoh_cpp",
"z_reply_is_ok returned False Reason: %s",
- reply.get_err().get_payload().as_string())
+ reply_err_str.c_str())
return;
}
const zenoh::Sample & sample = reply.get_ok();
@@ -420,7 +420,7 @@ rmw_ret_t ClientData::send_request(
RMW_ZENOH_LOG_ERROR_NAMED(
"rmw_zenoh_cpp",
"Unable to obtain ClientData from data for %s.",
- std::string(sample.get_keyexpr().as_string_view()));
+ sample.get_keyexpr().as_string_view());
return;
}
diff --git a/rmw_zenoh_cpp/src/detail/rmw_service_data.cpp b/rmw_zenoh_cpp/src/detail/rmw_service_data.cpp
index 5696e81..d24c965 100644
--- a/rmw_zenoh_cpp/src/detail/rmw_service_data.cpp
+++ b/rmw_zenoh_cpp/src/detail/rmw_service_data.cpp
@@ -156,7 +156,7 @@ std::shared_ptr<ServiceData> ServiceData::make(
RMW_ZENOH_LOG_ERROR_NAMED(
"rmw_zenoh_cpp",
"Unable to obtain ServiceData from data for %s.",
- std::string(query.get_keyexpr().as_string_view()));
+ query.get_keyexpr().as_string_view());
return;
}
diff --git a/rmw_zenoh_cpp/src/detail/rmw_subscription_data.cpp b/rmw_zenoh_cpp/src/detail/rmw_subscription_data.cpp
index 45ef1bf..a6d0114 100644
--- a/rmw_zenoh_cpp/src/detail/rmw_subscription_data.cpp
+++ b/rmw_zenoh_cpp/src/detail/rmw_subscription_data.cpp
@@ -212,7 +212,7 @@ bool SubscriptionData::init()
RMW_ZENOH_LOG_ERROR_NAMED(
"rmw_zenoh_cpp",
"Unable to obtain SubscriptionData from data for %s.",
- std::string(sample.get_keyexpr().as_string_view()));
+ sample.get_keyexpr().as_string_view());
return;
}
diff --git a/rmw_zenoh_cpp/src/detail/zenoh_utils.cpp b/rmw_zenoh_cpp/src/detail/zenoh_utils.cpp
index 795c4dc..3e0f6a4 100644
--- a/rmw_zenoh_cpp/src/detail/zenoh_utils.cpp
+++ b/rmw_zenoh_cpp/src/detail/zenoh_utils.cpp
@@ -49,7 +49,7 @@ zenoh::Bytes create_map_and_set_sequence_num(
int64_t source_timestamp = now_ns.count();
rmw_zenoh_cpp::AttachmentData data(sequence_number, source_timestamp, gid);
- return std::move(data.serialize_to_zbytes());
+ return data.serialize_to_zbytes();
}
///=============================================================================
|
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Thank you @YuanYuYuan, included here 6877a48 |
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left quite a few changes inline here, but this is looking pretty good.
@@ -2451,7 +2451,7 @@ rmw_get_gid_for_publisher(const rmw_publisher_t * publisher, rmw_gid_t * gid) | |||
RMW_CHECK_ARGUMENT_FOR_NULL(pub_data, RMW_RET_INVALID_ARGUMENT); | |||
|
|||
gid->implementation_identifier = rmw_zenoh_cpp::rmw_zenoh_identifier; | |||
pub_data->copy_gid(gid->data); | |||
memcpy(gid->data, pub_data->copy_gid().data(), RMW_GID_STORAGE_SIZE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is OK, because copy_gid
is now returning a copy.
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
0999f2d
to
6128468
Compare
@YuanYuYuan related to the string_view I need to use in std::string(keyexpr_.value().as_string_view()).c_str()); Otherwise I got a segfault in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, I did another pass on this. This is looking pretty close to me, with a few more relatively minor things to fix. The only remaining major issue is to see if we can reduce the latency with this vs. the rolling
branch.
RMW_SET_ERROR_MSG("Failed to get source_timestamp from client call attachment"); | ||
// Object that manages the raw buffer | ||
auto payload = sample.get_payload().as_vector(); | ||
if (payload.size() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can save an indentation level here by doing:
if (payload.size() == 0) {
RMW_ZENOH_LOG_DEBUG_NAMED(
"rmw_zenoh_cpp",
"ClientData not able to get slice data");
return RMW_RET_ERROR;
}
(and then unindenting the rest of the code)
@@ -2451,7 +2451,7 @@ rmw_get_gid_for_publisher(const rmw_publisher_t * publisher, rmw_gid_t * gid) | |||
RMW_CHECK_ARGUMENT_FOR_NULL(pub_data, RMW_RET_INVALID_ARGUMENT); | |||
|
|||
gid->implementation_identifier = rmw_zenoh_cpp::rmw_zenoh_identifier; | |||
pub_data->copy_gid(gid->data); | |||
memcpy(gid->data, pub_data->copy_gid().data(), RMW_GID_STORAGE_SIZE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's a good point.
I'm actually not in favor of having both overloads for copy_gid
; I think that is just confusing. I'll add a comment in the rmw_publisher_data.cpp
class.
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
I see. I observed similar issues before. Maybe the macro doesn't work well with the lifetime of the keyexpr. In this case, we need to store the string additionally. |
Fix the high latency on the subscriber due to zenoh bytes conversion
When compiling on Windows I'm getting this error just include the zenoh.hxx header Windows compile errorC:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\protocol_implementations\posix\../../provider/shm_provider.hxx(36,1): error C2236: token inesperado 'struct'. Compruebe si olvid� un ';' [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\protocol_implementations\posix\../../provider/shm_provider.hxx(36,1): error C2332: 'struct': falta el nombre de etiqueta [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\protocol_implementations\posix\../../provider/shm_provider.hxx(36,24): error C2513: '': no hay ninguna variable declarada antes de '=' [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\protocol_implementations\posix\../../provider/shm_provider.hxx(37,1): error C2332: 'struct': falta el nombre de etiqueta [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\protocol_implementations\posix\../../provider/shm_provider.hxx(37,25): error C2226: error de sintaxis: tipo 'zenoh::ShmProviderAsyncInterface::drop::' inesperado [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\protocol_implementations\posix\../../provider/shm_provider.hxx(41,1): error C2236: token inesperado 'struct'. Compruebe si olvid� un ';' [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\protocol_implementations\posix\../../provider/shm_provider.hxx(41,1): error C2332: 'struct': falta el nombre de etiqueta [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\protocol_implementations\posix\../../provider/shm_provider.hxx(41,24): error C2513: '': no hay ninguna variable declarada antes de '=' [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\protocol_implementations\posix\../../provider/shm_provider.hxx(42,1): error C2332: 'struct': falta el nombre de etiqueta [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\protocol_implementations\posix\../../provider/shm_provider.hxx(42,18): error C2059: error de sintaxis: '->' [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\provider\alloc_layout.hxx(38,10): error C2236: token inesperado 'struct'. Compruebe si olvid� un ';' [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\provider\alloc_layout.hxx(38,20): error C2332: 'struct': falta el nombre de etiqueta [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\provider\alloc_layout.hxx(38,20): error C2513: '': no hay ninguna variable declarada antes de '=' [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\provider\alloc_layout.hxx(39,14): error C2332: 'struct': falta el nombre de etiqueta [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\provider\alloc_layout.hxx(39,14): error C2059: error de sintaxis: '->' [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\provider\alloc_layout.hxx(43,10): error C2236: token inesperado 'struct'. Compruebe si olvid� un ';' [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\provider\alloc_layout.hxx(43,20): error C2332: 'struct': falta el nombre de etiqueta [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\provider\alloc_layout.hxx(43,20): error C2513: '': no hay ninguna variable declarada antes de '=' [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\provider\alloc_layout.hxx(44,21): error C2332: 'struct': falta el nombre de etiqueta [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] C:\ros2_ws\install\opt\zenoh_cpp_vendor\include\zenoh\api\shm\provider\alloc_layout.hxx(44,21): error C2226: error de sintaxis: tipo 'zenoh::shm::provider::closures::_z_alloc_layout_async_interface_drop_fn::' inesperado [C:\ros2_ws\build\rmw_zenoh_cpp\rmw_zenohd.vcxproj] Failed <<< rmw_zenoh_cpp [4.02s, exited with code 1] |
Signed-off-by: Alejandro Hernandez Cordero <[email protected]>
@clalancette windows issue fixed here ec42c81 |
They are a fixed size, so should use the fixed-size data structure. Signed-off-by: Chris Lalancette <[email protected]>
Since we aren't calling shared_from_this() in it anymore, we can do all of the work in the constructor. Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
This simplifies the code, and makes it less noisy. Signed-off-by: Chris Lalancette <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now looks good to me, so I'm going to go ahead and merge it. Thanks for all of the hard work here @ahcorde
For visibility reason I created a branch called
dev/1.0.0
in this repository, otherwise I can't open the PRThe goal of this PR is to port the
zenoh_c
API tozenoh_cpp
.I added a hack in the zenoh_cpp API to be able to make the transition easier. Edit the following file:
And add this change:
zenoh_cpp