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

Parameterized diff_drive_controller's odom_frame_id #205

Merged
merged 3 commits into from
Dec 15, 2015

Conversation

tappan-at-git
Copy link
Contributor

Addressing issue #204, made odom_frame_id_ a parameter-driven value for diff_drive_controllers.

@bmagyar
Copy link
Member

bmagyar commented Dec 14, 2015

May I ask you to provide 2 tests as well?
One where you don't specify the topic, and check that it shows up on /odom, and another one where you specify a different topic and the test checks that the new topic gets published and that there is no odom. Feel free to peek into the test folder for inspiration ;)

Updating the diff drive wiki should also be in order.

@tappan-at-git
Copy link
Contributor Author

Sorry, just so I'm sure I understand the tests you're asking for -- by topic do you mean frame id?

Currently working on:

  • Testing with odom_frame_id not specified: /odom tf exists, published odometry messages have their header frame_id set to odom.
  • Testing with odom_frame_id set to "new_odom": Original /odom tf does not exist, /new_odom tf does exist, and published odometry messages use new_odom in their header frame_id.

@tappan-at-git
Copy link
Contributor Author

Tests implemented, and passing.

ros::Duration(0.1).sleep();
}

// zero everything before test
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: you don't need to zero anything for this test

@efernandez
Copy link
Contributor

Just some minor comments related with the test.

I think it's good to have the ability to set the odometry frame_id, as in amcl.
The implementation LGTM. Thanks for the tests; maybe it was enough with the second one.

👍 @tappan-at-git Let us know if you either address my minor comments or not, to merge this.

@tappan-at-git
Copy link
Contributor Author

Changes made!

@bmagyar
Copy link
Member

bmagyar commented Dec 15, 2015

👍 here too

@efernandez
Copy link
Contributor

Thanks! Let's merge this 🎉

bmagyar added a commit that referenced this pull request Dec 15, 2015
Parameterized diff_drive_controller's odom_frame_id
@bmagyar bmagyar merged commit 429453e into ros-controls:indigo-devel Dec 15, 2015
@bmagyar
Copy link
Member

bmagyar commented Dec 15, 2015

Thanks for your contribution Eric!

@tappan-at-git
Copy link
Contributor Author

No problem! 🎉 Is there a good way to keep track of the release cycle so I can make sure the new feature gets documented on the wiki once it hits?

@bmagyar
Copy link
Member

bmagyar commented Dec 19, 2015

Not really. You can check back to see if there are new tags with release
version or take a look at the corresponding release discussion on the
issues list.
On 15 Dec 2015 19:33, "Eric Tappan" [email protected] wrote:

No problem! [image: 🎉] Is there a good way to keep track of the
release cycle so I can make sure the new feature gets documented on the
wiki once it hits?


Reply to this email directly or view it on GitHub
#205 (comment)
.

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

Successfully merging this pull request may close these issues.

3 participants