Skip to content
This repository has been archived by the owner on Oct 9, 2019. It is now read-only.

ABI compatible use_sime_time fix for Dashing #21

Closed
wants to merge 1 commit into from

Conversation

sloretz
Copy link

@sloretz sloretz commented Jun 6, 2019

This is a companion PR to #20. #20 is a proper fix that should go into eloquent. This is a hack that may be acceptable for a Dashing patch release.

This creates a robot state publisher subclass that uses ROS time for static transforms to avoid breaking ABI. This PR does change the size of the class JointStateListener which is in a publicly installed header. This it is probably ok because joint_state_listener.cpp contains a main(). It seems unlikely that anyone would have included joint_state_listener.h.

Personally, after making this PR, it is probably worth breaking ABI in Dashing and do a patch release with #20 instead of merging this PR

This creates a robot state publisher subclass that uses ROS time for
static transforms to avoid breaking ABI. This PR does change the size of
the class JointStateListener which is a publicly installed header. This
is a different ABI break, but it is probably ok because joint_state_listener.cpp
contains a main(). Only libraries would have been able to import
joint_state_listener.h, and it seems unlikely that anyone would have done
so.

Signed-off-by: Shane Loretz<sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@clalancette
Copy link

Personally, after making this PR, it is probably worth breaking ABI in Dashing and do a patch release with #20 instead of merging this PR

I agree; as we know from ROS 1, there are very few (though not 0) users who subclass RobotStatePublisher. I think it is pretty unlikely that they have ported to Dashing already, so I think we should just break the Dashing ABI by merging #20 when it is ready. @sloretz I'll close this out so we can concentrate on #20. Thanks!

@clalancette clalancette closed this Jun 7, 2019
@sloretz sloretz deleted the ros2_rclcpp_time_dashing branch June 7, 2019 14:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants