Fix #2035: Initialize previous_publish_timestamp_ in on_configure #2084
Conversation
github-actions
bot
requested review from
MarqRazz,
arne48,
erickisos,
saikishor and
xguay
| tf2::Quaternion orientation; | ||
| orientation.setRPY(0.0, 0.0, odometry_.getHeading()); | ||
|
|
||
| bool should_publish = false; |
There was a problem hiding this comment.
I think this variable can/should be removed now.
There was a problem hiding this comment.
I guess it is still needed to be able to control the frequency of publishing the message
There was a problem hiding this comment.
You are right, we can combine the logic and we can remove it
saikishor
left a comment
saikishor
left a comment
There was a problem hiding this comment.
Combine the logic and remove should_publish variable
…e try-catch also removing should_publish variable
christophfroehlich
left a comment
christophfroehlich
left a comment
There was a problem hiding this comment.
This just removes the logic introduced with #585
Are we sure that this is not needed anymore?
| previous_publish_timestamp_ = time; | ||
| should_publish = true; | ||
| } | ||
| previous_publish_timestamp_ += publish_period_; |
There was a problem hiding this comment.
| previous_publish_timestamp_ += publish_period_; | |
| previous_publish_timestamp_ = time; |
@christophfroehlich I think this should fix #585 and also be better overall.
There was a problem hiding this comment.
no because the comparison above fails if the time source changed.
|
|
||
| bool should_publish = false; | ||
| try | ||
| if (previous_publish_timestamp_ + publish_period_ <= time) |
There was a problem hiding this comment.
| if (previous_publish_timestamp_ + publish_period_ <= time) | |
| if (previous_publish_timestamp_ + publish_period_ > time) |
I was misunderstanding the logic here. I think with this change and the earlier one everything should work. This should cause this if statement to pass as true the first time in simulation and from there on work correctly.
There was a problem hiding this comment.
still: This comparison will throw an exception if the timesource changes
4 participants
Description
This PR addresses issue #2035 by moving the initialization of
previous_publish_timestamp_into theon_configure()lifecycle method.Previously, this variable was initialized inside a
catchblock within the update loop, effectively using exception handling for normal control flow. Moving it toon_configurealigns it with howprevious_update_timestamp_is handled and ensures the timestamp is valid before the update loop begins.Related Issues
Closes #2035
Type of Change
Checklist