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

[Issue-300] Handle prior on last stamp #323

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

svwilliams
Copy link
Contributor

As discussed in Issue #300, there is a bug in the TimestampManager.

When a Transaction is sent to the TimestampManager, it is expected that the variables involved in that Transaction will be updated to the values predicted by the motion model. This was not happening in the case where the Transaction involved a single timestamp. The code to update the output Transaction with the motion model values was included in the loop that processes timestamp pairs. If there is only one timestamp, then there are zero timestamp pairs and that loop never executes.

This PR first creates a few new unit tests to capture this condition: 0ab7189
Then moves the code block out of the pair handling loop: 9729c83
The code block now exists in its own loop, after all of the involved timestamps have been located. This specific location allows us to reuse lookups into the motion model history.

… to ensure that transactions that involve only a single timestamp are correctly handled
@fabianhirmann
Copy link
Contributor

Hi @svwilliams,

I can confirm that these changes fix our issue. I look forward to this PR being merged.

Thank you very much,
Fabian

@fabianhirmann
Copy link
Contributor

@svwilliams @carlos-m159 Just a quick question, are there any blocking points preventing this PR to be merged?

@svwilliams
Copy link
Contributor Author

My biggest blocker is getting enough time to port this into the ROS2 implementation at the same time.

@fabianhirmann
Copy link
Contributor

@svwilliams Thank you very much for your feedback. Sorry that I was bothering you. I wasn't aware that the ROS2 implementation is desired before merging this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants