Fix typesupport bug.

[EvanLiUAV]

Currently when I was tried to use rules_ros2 in my bazel project. we found an issue as below No msg shown in listener logging.
The talker output:

1622923403.099532 [0]     talker: using network interface wlp0s20f3 (udp/192.168.0.X) selected arbitrarily from: wlp0s20f3, docker0
[INFO] [1622923403.604275233] [minimal_publisher]: Publishing: 'Hello, world! 0'
[INFO] [1622923404.104128462] [minimal_publisher]: Publishing: 'Hello, world! 1'
[INFO] [1622923404.604133903] [minimal_publisher]: Publishing: 'Hello, world! 2'
[INFO] [1622923405.104024189] [minimal_publisher]: Publishing: 'Hello, world! 3'
...

the listener output:

1622923408.435101 [0]   listener: using network interface wlp0s20f3 (udp/192.168.0.X) selected arbitrarily from: wlp0s20f3, docker0
[INFO] [1622923408.604919651] [minimal_subscriber]: I heard: ''
[INFO] [1622923409.104868214] [minimal_subscriber]: I heard: ''
...

I found that the typesupport problem is caused by the compare condition in this function.

static std::string get_type_name(const char * type_support_identifier, void * type_support)
{
  if (using_introspection_c_typesupport(type_support_identifier)) {
    auto typed_typesupport = static_cast<MessageTypeSupport_c *>(type_support);
    return typed_typesupport->getName();
  } else if (using_introspection_cpp_typesupport(type_support_identifier)) {
    auto typed_typesupport = static_cast<MessageTypeSupport_cpp *>(type_support);
    return typed_typesupport->getName();
  } else {
    return "absent";
  }
}

so I check the using_introspection_cpp_typesupport function I found that the return condition is always false, the reason is using a pointer comparison rather than using std::string::compare.

The problem is solved when the conditions are correctly determined.

talker

evan@in_docker:~/rules_ros2$ bazel run //examples/chatter:talker 
INFO: Analyzed target //examples/chatter:talker (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //examples/chatter:talker up-to-date:
  bazel-bin/examples/chatter/talker
INFO: Elapsed time: 0.113s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action
1623155607.229638 [0]     talker: using network interface eno1 (udp/172.18.20.17) selected arbitrarily from: eno1, docker0
[INFO] [1623155607.736482142] [minimal_publisher]: Publishing: 'Hello, world! 0'
[INFO] [1623155608.236450915] [minimal_publisher]: Publishing: 'Hello, world! 1'
[INFO] [1623155608.736465676] [minimal_publisher]: Publishing: 'Hello, world! 2'
[INFO] [1623155609.236454210] [minimal_publisher]: Publishing: 'Hello, world! 3'
[INFO] [1623155609.736454613] [minimal_publisher]: Publishing: 'Hello, world! 4'
[INFO] [1623155610.236439533] [minimal_publisher]: Publishing: 'Hello, world! 5'
^C[INFO] [1623155610.466806625] [rclcpp]: signal_handler(signal_value=2)

listener

evan@in_docker:~/rules_ros2$ bazel run //examples/chatter:listener 
INFO: Analyzed target //examples/chatter:listener (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //examples/chatter:listener up-to-date:
  bazel-bin/examples/chatter/listener
INFO: Elapsed time: 0.096s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action
1623155596.586644 [0]   listener: using network interface eno1 (udp/172.18.20.17) selected arbitrarily from: eno1, docker0
[INFO] [1623155607.736783469] [minimal_subscriber]: I heard: 'Hello, world! 0'
[INFO] [1623155608.236732927] [minimal_subscriber]: I heard: 'Hello, world! 1'
[INFO] [1623155608.736722786] [minimal_subscriber]: I heard: 'Hello, world! 2'
[INFO] [1623155609.236693576] [minimal_subscriber]: I heard: 'Hello, world! 3'
[INFO] [1623155609.736732649] [minimal_subscriber]: I heard: 'Hello, world! 4'
[INFO] [1623155610.236678127] [minimal_subscriber]: I heard: 'Hello, world! 5'
^C[INFO] [1623155631.231753811] [rclcpp]: signal_handler(signal_value=2)

rqt

[eboasson]

@ivanpauno I find this an interesting bug. I know there are ways in which the addresses of the two strings being compared can end up being different (though I don't know exactly why it is happening with this bazel build) and I think the proposed solution is a sensible one.

The only downside I can think of is that the check happens also when (de)serializing or freeing a message, but I doubt it would cause a noticeable slowdown. I think the information could be cached in the ddsi_sertype to avoid having to do the string compare often.

Another issue is that I scanned the sources and it looks like the dynamic versions of the fastrtps and connext RMW layers do the same thing, so those would presumably have to be changed as well.

[ivanpauno]

I think the information could be cached in the ddsi_sertype to avoid having to do the string compare often.

Yes, that sounds like a good idea.
Though I don't mind to introduce this fix first.

Another issue is that I scanned the sources and it looks like the dynamic versions of the fastrtps and connext RMW layers do the same thing, so those would presumably have to be changed as well.

@EvanLiUAV could you open a similar issue in rmw_fastrtps and rmw_connextdds?

[ivanpauno]

can we use std::strcmp()?
To avoid allocating a std::string unnecessarely.

[MiguelCompany]

Another issue is that I scanned the sources and it looks like the dynamic versions of the fastrtps and connext RMW layers do the same thing, so those would presumably have to be changed as well.

In the case of rmw_fastrtps_dynamic_cpp, we are keeping the const char* pointer returned by get_message_typesupport_handle, which should be the pointer exported by the introspection type support package (this one for instance).

I think the pointer should not change (at least in the case of rmw_fastrtps_dynamic_cpp)

[ivanpauno]

Another issue is that I scanned the sources and it looks like the dynamic versions of the fastrtps and connext RMW layers do the same thing, so those would presumably have to be changed as well.

In the case of rmw_fastrtps_dynamic_cpp, we are keeping the const char* pointer returned by get_message_typesupport_handle, which should be the pointer exported by the introspection type support package (this one for instance).

I think the pointer should not change (at least in the case of rmw_fastrtps_dynamic_cpp)

The same happens here.
I'm not sure what @EvanLiUAV is exactly doing, but there might be some linker issues were the global doesn't exactly have the same address.

IMO doing a pointer comparison is unnecessarily fragile, comparing the strings seem more reasonable and can be cached when the subscription is created if there's any performance concern.

[clalancette]

This one got stuck in review, but we now have #481 which does the same thing. So I'm going to close this one in favor of that.