Check if message's "header" field is of type std_msgs::msg::Header

[scottmendessl]

Resolves #51
HasHeader function in received_message_age.hpp will now check to make sure that member of name "header" is also of type std_msgs::msg::Header to return true.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (rolling@bb824c5). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 9290368 differs from pull request most recent head ec04cc8. Consider uploading reports for the commit ec04cc8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             rolling      #54   +/-   ##
==========================================
  Coverage           ?   17.36%           
==========================================
  Files              ?       35           
  Lines              ?     1192           
  Branches           ?      190           
==========================================
  Hits               ?      207           
  Misses             ?      818           
  Partials           ?      167           

Flag	Coverage Δ
unittests	17.36% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[emersonknapp]

This looks great, thanks for the patch!

[emersonknapp]

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/emersonknapp/f8d93ad6d2cb227affd80b62c5bc47b3/raw/f401f71837454a11b62e39ccca3061da80a7fc4b/ros2.repos
BUILD args: --packages-up-to libstatistics_collector
TEST args: --packages-select libstatistics_collector
Job: ci_launcher

[emersonknapp]

• Linux
• Linux-aarch64
• macOS
• Windows

[dabonnie]

I think you'll need to add std_msgs as a <test_depend> given the CI errors.

Also when running CI for this package I would change the build arguments to:

BUILD args: --packages-up-to rclcpp
TEST args: --packages-select rclcpp

to ensure we don't break anything.

[scottmendessl]

Thanks for the comment. Pushed an update with std_msgs added as <test_depend> to package.xml

[dabonnie]

Gist: https://gist.githubusercontent.com/dabonnie/dad12f179bf93b8d597d747ea1d97be1/raw/f401f71837454a11b62e39ccca3061da80a7fc4b/ros2.repos
BUILD args: --packages-up-to rclcpp libstatistics_collector
TEST args: --packages-select rclcpp libstatistics_collector
Job: ci_launcher

• Linux
• Linux-aarch64
• macOS
• Windows

[dabonnie]

Thanks for the comment. Pushed an update with std_msgs added as <test_depend> to package.xml

Not sure why it's still failing, trying to reproduce it myself.

[scottmendessl]

Thanks for the comment. Pushed an update with std_msgs added as <test_depend> to package.xml

Not sure why it's still failing, trying to reproduce it myself.

Likewise... It seems like a dependency issue w/ the rclcpp. May need to explicitly include std_msgs/msg/header.hpp in received_message_age.hpp, but haven't had immediate luck with that.

[dabonnie]

Thanks for the comment. Pushed an update with std_msgs added as <test_depend> to package.xml

Not sure why it's still failing, trying to reproduce it myself.

Likewise... It seems like a dependency issue w/ the rclcpp. May need to explicitly include std_msgs/msg/header.hpp in received_message_age.hpp, but haven't had immediate luck with that.

FYI I'm seeing the same issue when checking out your PR. When I have bandwidth I'll try and track down the compile time issue. It seems that something may be incorrect with the header check.

[emersonknapp]

You will definitely need to #include std_msgs/msg/header.hpp in this file - any given file that is including this header will not necessarily have included it. "Include what you use".

You will probably also now need to include std_msgs as an ament dependency of libstatistics_collector in the CMakeLists, so that other packages that consume this library have access to its headers. It was included as an exec_depend and build_depend in package.xml, but is not currently mentioned in ament_target_dependencies for libstatistics_collector in CMakeLists.txt

[dabonnie]

I tried that yesterday here (just pushed), but something is still missing.

[emersonknapp]

I think the only problem in your attempt was that you included the C header header.h instead of the C++ header header.hpp, which declares the std_msgs namespace. I just pushed commit 83486f2 to your branch and I was able to successfully colcon build --packages-up-to rclcpp

[scottmendessl]

Thanks all for the assist. Went ahead and merged changes to my branch from 83486f2.

[scottmendessl]

Build looks ok but failing some tests in rcl and rclcpp. It looks like due to the fact they define Header.msg as a custom message rather than using std_msgs/Header. See: https://github.com/ros2/rclcpp/tree/master/rclcpp/test/msg

This begs the question, is this implementation an incorrect check? Should the check be instead:

• Check for member 'header'
• Check 'header' has member 'stamp'
• Check 'stamp' is of type 'time'

[emersonknapp]

That test in rclcpp was added alongside this functionality by the same authors. Since rclcpp already has an implicit dependency on std_msgs through this library, I don't see any harm to change that test to use std_msgs/Header instead of a custom test msg.

The more sophisticated check is probably also fine, as it does make this functionality more flexible.

[dabonnie]

That test in rclcpp was added alongside this functionality by the same authors. Since rclcpp already has an implicit dependency on std_msgs through this library, I don't see any harm to change that test to use std_msgs/Header instead of a custom test msg.

The more sophisticated check is probably also fine, as it does make this functionality more flexible.

We are essentially changing the API (expected message type then), no?

https://index.ros.org/doc/ros2/Concepts/About-Topic-Statistics/#types-of-statistics-calculated

I agree that a sophisticated check is good, but wonder how we can gain more input from the community for this change (e.g. are people using message timestamps and if so then how? - ROS 2 doesn't have a default header unlike ROS 1).

[emersonknapp]

I would say no - we would not be changing the API with the more sophisticated check. In fact, I think that the original attempt here changed the API whereas this new proposal would maintain it.

• The current API as implemented is "look for a field named header then use it as if it has a member stamp that is of type builtin_interfaces/Time"
• This PR initially is attempting to make that check more strict - "look for a field named header that is of type std_msgs/Header"
• we are now proposing "look for and guarantee a subfield header.stamp that is of type builtin_interfaces/Time"

This would (if i understand correctly) remove the requirement for std_msgs to be a <build/exec_depend> of libstatistics_collector which (IIUC) it shouldn't have been in the first place, as it's not used anywhere in the actual library implementation and was not previously a dependency of rclcpp, which means the introduction of this library added a new unneeded dependency to the client library. With such a change to only use builtin_interfaces we could remove std_msgs and restore the rclcpp dependency set.

[dabonnie]

My misunderstanding: changing the check to

"look for and guarantee a field header.stamp that is of type builtin_interfaces/Time"

is a good proposal.

[Timple]

So a lot of discussion on ABI breaks.

I would already be happy if this was merged for the rolling release. So at least the next Humble release does not have this issue for the next 5 years 🙂

[Timple]

Aaaand we missed the Humble release...

[jacobperron]

Anyone planning to pick this ticket up? It sounds like changing the logic to check for a builtin_interfaces type would be acceptable (and hopefully it could be backported to existing ROS distros).

[clalancette]

Anyone planning to pick this ticket up? It sounds like changing the logic to check for a builtin_interfaces type would be acceptable (and hopefully it could be backported to existing ROS distros).

So I spent a few minutes on this. I went in, rebased this change onto the latest, and made a change so that we are at least checking whether the message contains a field called header that contains a subfield called stamp. That isn't what the agreement in #54 (comment) was, but my grasp of C++ template meta-programming is weak and I think this is a strict improvement over what we have now. So I think this is worthwhile reviewing, even though it doesn't check to see if the stamp field is of type builtin_interfaces::msg::Time.

[jacobperron]

I guess this change reduces the chances of a collision with custom user messages, but it doesn't really fix the bug. E.g. if this test message was updated to use a type other than builtin_interfaces/msg/Time, then I guess we'd run into the same issue.

Explicitly checking for the message fields and types we expect, would ultimately fix the bug I think. IIUC, the code requires that the message has the following two members:

• header.stamp.sec
• header.stamp.nanosec
  Both of which should be casted to int64_t.

libstatistics_collector/include/libstatistics_collector/topic_statistics_collector/received_message_age.hpp

Line 80 in bb824c5

const auto nanos = RCL_S_TO_NS(static_cast<int64_t>(stamp.sec)) + stamp.nanosec;

It's probably best to check for either std_msgs/msg/Header or builtin_interfaces/msg/Time type to avoid any semantic errors though. We probably don't want custom user types to be accidentally picked up by libstatistics_collector.

[clalancette]

I guess this change reduces the chances of a collision with custom user messages, but it doesn't really fix the bug. E.g. if this test message was updated to use a type other than builtin_interfaces/msg/Time, then I guess we'd run into the same issue.

No, there's an explicit test in here for that now (see https://github.com/ros-tooling/libstatistics_collector/pull/54/files#diff-3fc787b3bbc36449500f207e1fec489c5b2d6e7c67d86b233c54f458fb4e318c). In particular, if the field called header doesn't have a sub-field called stamp, then we won't try to access it. This doesn't completely eliminate the bug, but it does make it more unlikely.

It's probably best to check for either std_msgs/msg/Header

I don't think we want to do this, based on the earlier conversation. It adds a new transitive dependency to rclcpp.

or builtin_interfaces/msg/Time type to avoid any semantic errors though.

That would be ideal, since rclcpp already depends on it. My metaprogramming-fu isn't good enough to figure it out, though.

[clalancette]

That would be ideal, since rclcpp already depends on it. My metaprogramming-fu isn't good enough to figure it out, though.

All right, it turns out I just needed to do some more reading on template meta-programming. I think I've got it in ca5cd77; it both checks to see if a type has .header.stamp, and that that type is builtin_interfaces::msg::Time . Arguably the fact that it is called HasHeader is now a misnomer, but we can bikeshed that particular detail later.

I'm going to run a full-up CI on it to see what happens, but this is promising.

• Linux
• Linux-aarch64
• Windows

[Timple]

it both checks to see if a type has .header.stamp, and that that type is builtin_interfaces::msg::Time.

To be complete, could we not add the same checks for frame_id and check if there are no other fields?
I can still image people having custom headers with do include a stamp.

[clalancette]

To be complete, could we not add the same checks for frame_id

Done in 929ad82.

and check if there are no other fields?

The only way I can come up with to do this is to count the number of fields in the struct. It looks like this probably requires C++20 (https://stackoverflow.com/questions/2589861/c-template-meta-programming-number-of-member-variables). So I'd say let's leave this particular check out for now, unless someone comes up with a way to do it.

[jacobperron]

Changes LGTM, one question about a dependency.

[clalancette]

Here is another full-up CI with all of the latest changes:

• Linux
• Linux-aarch64
• Windows

[jacobperron]

I think this change maintains ABI compatibility. If others agree, I think it is worth backporting to Humble, Galactic, and Foxy.

[clalancette]

To be complete, could we not add the same checks for frame_id

Done in 929ad82.

Actually, this was a mistake both conceptually and in CI.

Conceptually, it really doesn't matter if the header has a frame_id or not; the important bit is whether the header has a stamp field, as that is what we'll be accessing in TimeStamp::value.

And this shows up in CI in an rclcpp test, because it defines a custom Header with only a stamp, which is actually enough. So I'm actually going to revert 929ad82 here, then re-run CI.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows

[jacobperron]

LGTM

[clalancette]

With green CI and approvals, I'm going to go ahead and merge this despite the failing CI. I'll fix up the DCO problems during the squash since this PR has a complicated history.