-
Notifications
You must be signed in to change notification settings - Fork 255
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.
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
[humble] Reduce message spam when topics to be recorded do not exist #1434
Conversation
* move topic_filter.hpp to expose to recorder.hpp * fix ros2#1011: persist TopicFilter to avoid warning spam * address pr comments Signed-off-by: Daniel Mesham <daniel@auterion.com>
e5766c2
to
1c93f79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanMesh Thank you for trying to backport this PR.
There is one major issue with this backport.
While the fix and backport look pretty trivial the reason why it was not backported before is because it introduces ABI breaking changes when adding the new std::unique_ptr<TopicFilter> topic_filter_;
member variable to the recorder class.
According to the ABI BREAKING CHANGES
- Add, remove, or re-order member variables in a class
is an ABI breaking change.
Unfortunately, we can't introduce API/ABI breaking changes to the stable release of the ROS2 core package.
Although, there is a chance that if we will add this new member variable to the end of the class it will still keep ABI compatibility. Need to try and run the ABI checker.
My understanding is that while this generally works, it is not guaranteed. That is, the compiler is free to rearrange the in-memory layout of structures/classes in certain cases (and optimization levels). So I don't think we can rely on that. |
Thanks for the feedback. |
Personally, I don't think so. The ABI checker will likely tell you it is fine, but again it is not guaranteed so I don't think we should rely on that. What we've done in other situations like this is to introduce a sort of global map that holds a mapping of the pointer to the new member we want to add. Then you can look up the new variable you need when you need it, without changing ABI. You can see an example of this kind of thing in ros2/rclcpp#2183 |
Thanks @clalancette. I'll give that a go and come back to this. |
@Moma98 No updates at the moment. At some point, I would like to try the suggestion above, but have not got round to it. |
Hey! I have been also struggling today with this issue. We had to build rosbag2 from source using @DanMesh's branch, because this spamming issue basically made the latest humble release unusable for us. We would greatly appreciate if this issue was somehow solved for Humble as well. |
Perfect! Tested it in Humble and it works as expected. Thanks for the fast solution. |
Closing this since #1469 seems to have solved the issue. |
This ports #1018 to Humble.
The message spamming reported in #1011 was occurring when using
0.15.5
as it seems the change above was never backported.FYI @bastianhjaeger