Skip to content
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

use SteadyTimer in message_filters #1247

Merged
merged 1 commit into from
Dec 19, 2017

Conversation

flixr
Copy link
Contributor

@flixr flixr commented Nov 24, 2017

To make sure that the message filters also work if system time is set back, use SteadyTimer instead of the normal Timer

{
dispatch();
}

void init()
{
update_timer_ = nh_.createTimer(update_rate_, &TimeSequencer::update, this);
update_timer_ = nh_.createSteadyTimer(ros::WallDuration(update_rate_.toSec()), &TimeSequencer::update, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can WallDuration really not be initialized from a Duration? That seems like an API fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC it can't... but I didn't check that again and did it explicitly just to be sure...

@mikepurvis
Copy link
Member

LGTM. It's a behaviour change, but I'd vote for this go into Lunar. It's extremely unlikely someone would be depending on the current broken behaviour.

That said, there's a potential ABI issue would be if a third party had message_filter class members, those will now change in type (though not in size, so maybe it wouldn't actually be a problem, since there's nothing to link?).

@dirk-thomas
Copy link
Member

Thank you for the patch.

@dirk-thomas dirk-thomas merged commit ba502fa into ros:lunar-devel Dec 19, 2017
@flixr flixr deleted the message_filters_steady_timer branch December 19, 2017 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants