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.

Sign up for GitHub

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

Include type hash in liveliness token and fill endpoint info #202

Merged
merged 1 commit into from
Jun 24, 2024
Merged

Include type hash in liveliness token and fill endpoint info #202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion rmw_zenoh_cpp/src/detail/graph_cache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
#include "rmw/validate_namespace.h"
#include "rmw/validate_node_name.h"

#include "rosidl_runtime_c/type_hash.h"

#include "graph_cache.hpp"
#include "rmw_data_types.hpp"

Expand Down Expand Up @@ -1173,7 +1175,21 @@ rmw_ret_t GraphCache::get_entities_info_by_topic(
return ret;
}

// TODO(Yadunund): Set type_hash, gid.
rosidl_type_hash_t type_hash;
rcutils_ret_t rc_ret = rosidl_parse_type_hash_string(
topic_data->info_.type_hash_.c_str(),
&type_hash);
if (RCUTILS_RET_OK == rc_ret) {
ret = rmw_topic_endpoint_info_set_topic_type_hash(
&endpoint_info,
&type_hash
);
if (RMW_RET_OK != ret) {
return ret;
}
}

// TODO(Yadunund): Set gid.
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions rmw_zenoh_cpp/src/detail/liveliness_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,11 @@ NodeInfo::NodeInfo(
TopicInfo::TopicInfo(
std::string name,
std::string type,
std::string type_hash,
rmw_qos_profile_t qos)
: name_(std::move(name)),
type_(std::move(type)),
type_hash_(std::move(type_hash)),
qos_(std::move(qos))
{
// Do nothing.
Expand All @@ -75,6 +77,7 @@ enum KeyexprIndex
NodeName,
TopicName,
TopicType,
TopicTypeHash,
TopicQoS
};

Expand Down Expand Up @@ -272,6 +275,7 @@ Entity::Entity(
const auto & topic_info = this->topic_info_.value();
keyexpr_parts[KeyexprIndex::TopicName] = mangle_name(topic_info.name_);
keyexpr_parts[KeyexprIndex::TopicType] = mangle_name(topic_info.type_);
keyexpr_parts[KeyexprIndex::TopicTypeHash] = mangle_name(topic_info.type_hash_);
keyexpr_parts[KeyexprIndex::TopicQoS] = qos_to_keyexpr(topic_info.qos_);
}

Expand Down Expand Up @@ -398,6 +402,7 @@ std::shared_ptr<Entity> Entity::make(const std::string & keyexpr)
topic_info = TopicInfo{
demangle_name(std::move(parts[KeyexprIndex::TopicName])),
demangle_name(std::move(parts[KeyexprIndex::TopicType])),
demangle_name(std::move(parts[KeyexprIndex::TopicTypeHash])),
std::move(qos.value())
};
}
Expand Down
5 changes: 4 additions & 1 deletion rmw_zenoh_cpp/src/detail/liveliness_utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,13 @@ struct TopicInfo
{
std::string name_;
std::string type_;
std::string type_hash_;
rmw_qos_profile_t qos_;

TopicInfo(
std::string name,
std::string type,
std::string type_hash,
rmw_qos_profile_t qos);
};

Expand Down Expand Up @@ -91,9 +93,10 @@ enum class EntityType : uint8_t
*
* For entities with topic infomation, the liveliness token keyexpr have additional fields:
*
* <ADMIN_SPACE>/<domainid>/<zid>/<id>/<entity>/<namespace>/<nodename>/<topic_name>/<topic_type>/<topic_qos>
* <ADMIN_SPACE>/<domainid>/<zid>/<id>/<entity>/<namespace>/<nodename>/<topic_name>/<topic_type>/<topic_type_hash>/<topic_qos>
* <topic_name> - The ROS topic name for this entity.
* <topic_type> - The type for the topic.
* <topic_type_hash> - The type hash for the topic.
* <topic_qos> - The qos for the topic (see qos_to_keyexpr() docstring for more information).
*
* For example, the liveliness expression for a publisher within a /talker node that publishes
Expand Down
28 changes: 20 additions & 8 deletions rmw_zenoh_cpp/src/rmw_zenoh.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,8 +686,11 @@ rmw_create_publisher(
rmw_zenoh_cpp::liveliness::EntityType::Publisher,
rmw_zenoh_cpp::liveliness::NodeInfo{
node->context->actual_domain_id, node->namespace_, node->name, context_impl->enclave},
rmw_zenoh_cpp::liveliness::TopicInfo{rmw_publisher->topic_name,
publisher_data->type_support->get_name(), publisher_data->adapted_qos_profile}
rmw_zenoh_cpp::liveliness::TopicInfo{
rmw_publisher->topic_name,
publisher_data->type_support->get_name(),
type_hash_c_str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels to me like there are two problems with this implementation:

  1. The ownership of type_hash_c_str in this function is very unclear. In particular, this function (rmw_create_publisher) allocates it (via calling rosidl_stringify_type_hash), but then it hands off ownership to TopicInfo to eventually free. While that works, it is really easy for the code to change and for a memory leak to happen later.
  2. We convert the type hash to a string in this function, and then convert it back to a rosidl_type_hash_t in GraphCache::get_entities_info_by_topic. That just seems like unnecessary work.

To fix this, I suggest two changes:

  1. Pass publisher_data->type_hash in here, and just have the TopicInfo hold onto that.
  2. Make sure to free type_hash_c_str in this function (probably via a rclcpp::make_scope_exit). This is actually an existing memory leak, but we may as well fix it while we are looking at things here.

The same goes for all uses of the type_hash below.

Copy link
Member Author

@Yadunund Yadunund Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was relying on implicit conversion to a string when passing type_hash_c_str to the TopicInfo constructor. That should create a copy right? I can also explcitly pass a std::string(type_hash_c_str).

We convert the type hash to a string in this function, and then convert it back to a rosidl_type_hash_t in GraphCache::get_entities_info_by_topic. That just seems like unnecessary work.

it's only unnecessary when the topics are created in the current session. However we still need to convert the string to a rosidl_type_hash_t for endpoints in other nodes in the graph since this information is gotten from liveliness tokens.

Correct me if i'm wrong but after I address #171 (comment), it should be clear that the pointer is freed and that we pass a newly instantiated string to TopicInfo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's only unnecessary when the topics are created in the current session. However we still need to convert the string to a rosidl_type_hash_t for endpoints in other nodes in the graph since this information is gotten from liveliness tokens.

Oh yes, good point. OK, never mind my comment on this.

I was relying on implicit conversion to a string when passing type_hash_c_str to the TopicInfo constructor. That should create a copy right?

Hm, I guess you are right. Converting a char * to a std::string does indeed seem to do a copy of the string. That's good to know. Personally, I prefer this to be explicit, and so I would pass the char * into the TopicInfo, and have TopicInfo be responsible for both copying and releasing the string. But I leave it to you.

publisher_data->adapted_qos_profile}
);
if (publisher_data->entity == nullptr) {
RCUTILS_LOG_ERROR_NAMED(
Expand Down Expand Up @@ -1519,8 +1522,11 @@ rmw_create_subscription(
rmw_zenoh_cpp::liveliness::EntityType::Subscription,
rmw_zenoh_cpp::liveliness::NodeInfo{
node->context->actual_domain_id, node->namespace_, node->name, context_impl->enclave},
rmw_zenoh_cpp::liveliness::TopicInfo{rmw_subscription->topic_name,
sub_data->type_support->get_name(), sub_data->adapted_qos_profile}
rmw_zenoh_cpp::liveliness::TopicInfo{
rmw_subscription->topic_name,
sub_data->type_support->get_name(),
type_hash_c_str,
sub_data->adapted_qos_profile}
);
if (sub_data->entity == nullptr) {
RCUTILS_LOG_ERROR_NAMED(
Expand Down Expand Up @@ -2191,8 +2197,11 @@ rmw_create_client(
rmw_zenoh_cpp::liveliness::EntityType::Client,
rmw_zenoh_cpp::liveliness::NodeInfo{
node->context->actual_domain_id, node->namespace_, node->name, context_impl->enclave},
rmw_zenoh_cpp::liveliness::TopicInfo{rmw_client->service_name,
std::move(service_type), client_data->adapted_qos_profile}
rmw_zenoh_cpp::liveliness::TopicInfo{
rmw_client->service_name,
std::move(service_type),
type_hash_c_str,
client_data->adapted_qos_profile}
);
if (client_data->entity == nullptr) {
RCUTILS_LOG_ERROR_NAMED(
Expand Down Expand Up @@ -2775,8 +2784,11 @@ rmw_create_service(
rmw_zenoh_cpp::liveliness::EntityType::Service,
rmw_zenoh_cpp::liveliness::NodeInfo{
node->context->actual_domain_id, node->namespace_, node->name, context_impl->enclave},
rmw_zenoh_cpp::liveliness::TopicInfo{rmw_service->service_name,
std::move(service_type), service_data->adapted_qos_profile}
rmw_zenoh_cpp::liveliness::TopicInfo{
rmw_service->service_name,
std::move(service_type),
type_hash_c_str,
service_data->adapted_qos_profile}
);
if (service_data->entity == nullptr) {
RCUTILS_LOG_ERROR_NAMED(
Expand Down
Loading