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

Bug Fix rate inverting duration #26

Closed
wants to merge 1 commit into from
Closed

Bug Fix rate inverting duration #26

wants to merge 1 commit into from

Conversation

lgerardSRI
Copy link
Contributor

When constructing a Rate object with a duration as argument, it is the expected period duration, not a frequency.

@dirk-thomas
Copy link
Member

Thank you for making the pull request. Could you please also add a simple unit test covering this case?

This needs to be backported into all current ROS distros.

@lgerardSRI
Copy link
Contributor Author

Yes I am quite surprised that it went unnoticed for all that time.
Do you need me to do something for the backporting?
I am writing a simple unittest.

@dirk-thomas
Copy link
Member

No, for backporting I will just cherry-pick the commit once it has been released to the latest distro and has no regression.

Thanks for working on the unittest

@dirk-thomas
Copy link
Member

Please squash your commits into a single one.

@lgerardSRI
Copy link
Contributor Author

I did squash them (see the second commit attached to this report).
Please note that I haven't tested these changes, I don't have access to ROS on my mac.

@dirk-thomas
Copy link
Member

The PR contains two commits. Please run git rebase -i HEAD~2 and mark the second commit for squashing. Then the PR should only contain a single commit.

When constructing a Rate object with a duration as argument, it is the expected period duration, not a frequency.
@lgerardSRI
Copy link
Contributor Author

ok, the github web interface was tricking me, sorry.

@dirk-thomas
Copy link
Member

Have you tried to compile the code? I get:

error: variable ‘ros::Rate r’ has initializer but incomplete type

@lgerardSRI
Copy link
Contributor Author

No... see my comment above.
I am not working with ROS sources, this is a proposed code that I edited here on github.
It tricked me into doing this forking automatically etc, I should have just reported a plain bug.

@dirk-thomas
Copy link
Member

Thanks - manually merged in 9198a8c.

@trainman419
Copy link
Contributor

Is it possible to get this backported to Hydro? I'm running into it while trying to initialize a rate from an interval.

It looks like this hasn't caused any regressions in Indigo.

@dirk-thomas
Copy link
Member

Sounds viable to me. I will put it on "the list' for the next Hydro patch release round.

@dirk-thomas
Copy link
Member

Backported to Hydro: 0.4.4

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

Successfully merging this pull request may close these issues.

None yet

3 participants