Fix discovery silently stops after unknown msg type is found.

[llapx]

Signed-off-by: Lei Liu Lei.Liu.AP@sony.com

This PR fixed (#829)

[fujitatomoya]

@Barry-Xu-2018 would you mind taking time for review since this comes from us. I'll do the same.

[Barry-Xu-2018]

@fujitatomoya

would you mind taking time for review since this comes from us. I'll do the same.

Yeah. I will check this PR.

[Barry-Xu-2018]

I have no comments on the latest updating.

[jhdcs]

Looks good to me, as long as it passes CI

[emersonknapp]

Before I approve this PR - I am wondering if there is a simpler way to implement this. Does the existing behavior throw an exception when it tries to create a subscription to an unknown type, thus cancelling the discovery behavior? Can we simply catch this exception and print a warning message - instead of implementing complex filtering logic?

[llapx]

Does the existing behavior throw an exception when it tries to create a subscription to an unknown type, thus cancelling the discovery behavior?

Yes, when create a subscription with an unknown type, will throw a exception and lead to the discovery process stuck.

Can we simply catch this exception and print a warning message

The simple way is try to skip create_subscription with unknown type and print warning like this:

...
std::shared_ptr<rclcpp::GenericSubscription>
Recorder::create_subscription(
  const std::string & topic_name, const std::string & topic_type, const rclcpp::QoS & qos)
{
  // << catch the exception below >>
  auto subscription = this->create_generic_subscription(
    topic_name,
    topic_type,
    qos,
    [this, topic_name](std::shared_ptr<rclcpp::SerializedMessage> message) {
      auto bag_message = std::make_shared<rosbag2_storage::SerializedBagMessage>();
...

but this will print warning in a loop if someone continue sending the unknown type messages, this may flush the log.

instead of implementing complex filtering logic?

so I tried to filter out the unknown type messages, and keep the unknown types to make it only print warning once, I think this may good for performance.

[fujitatomoya]

looks good to me

[emersonknapp]

but this will print warning in a loop

Oh - good call. I didn't realize that because of get_missing_topics, it would keep trying to create the subscription repeatedly. OK, this sounds good to me

[emersonknapp]

• Linux
• Linux-aarch64
• macOS
• Windows