Include type hash in liveliness token and fill endpoint info

[Yadunund]

Builds off #171 to include the type hash in the liveliness token and populate the same when filling out endpoint info. As a result the type hash is now available when introspecting topics. Previously the type hash would show INVALID.

Note: since the liveliness token is updated, please make sure to kill all ROS 2 nodes including the ros2 daemon prior to testing.

To test:

# terminal 1
ros2 run demo_nodes_cpp talker

# terminal 2
ros2 topic info -v /chatter

The type hash should be filled in the result

yadunund@ubuntu-noble-20240225:~/ros2_rolling$ ros2 topic info -v /chatter
Type: std_msgs/msg/String

Publisher count: 1

Node name: talker
Node namespace: /
Topic type: std_msgs/msg/String
Topic type hash: RIHS01_df668c740482bbd48fb39d76a70dfd4bd59db1288021743503259e948f6b1a18
Endpoint type: PUBLISHER
GID: 01.10.0e.70.a8.7f.35.22.74.35.d2.74.00.00.17.03
QoS profile:
  Reliability: RELIABLE
  History (Depth): KEEP_LAST (7)
  Durability: VOLATILE
  Lifespan: Infinite
  Deadline: Infinite
  Liveliness: AUTOMATIC
  Liveliness lease duration: Infinite

Subscription count: 0

[clalancette]

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.

[Yadunund]

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?

[clalancette]

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.

[clalancette]

I've left one comment, but it is non-blocking. I'll leave it to you whether to leave the code as-is or make a slight adjustment. Either way this is looking good to me.

[clalancette]

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.

[Yadunund]

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.

I think we can keep things as they are right now. The constructor args make it apparent that a copy of a string should be passed. I could convert some of them to const chat * const but then we'd be making a copy in rmw_zenoh.cpp for args like topic_type which we obtain as an std::string (now I just std::move() this string to the TopicInfo constructor.)