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

Added LeadLag controller #60

Closed

Conversation

progtologist
Copy link
Member

This PR should be able to be integrated in any version(branch), I am currently using indigo so I have only tested it against that.

@progtologist
Copy link
Member Author

This also partially addresses issue #35

@bmagyar
Copy link
Member

bmagyar commented Mar 14, 2017

  • Could you please squash the 2 commits so we don't have a broken state on the history?
  • Do you have examples of where you use this already perhaps? I'm looking for something visual people could try.

@progtologist
Copy link
Member Author

@bmagyar I just squashed all the commits into one.
As far as the examples go, I am planning on developing some controllers in the next days for ros_controllers using this LeadLag class.

@progtologist
Copy link
Member Author

The latest commit refers to issue #61 (would have preferred to branch them out, but I guess it's a little bit too late now).

@graiola
Copy link
Member

graiola commented Apr 13, 2017

Hello,

Thanks for the pr!
I went through the code and it seems good to me, but I would prefer to merge it on kinetic. Could you prepare a pr for it?

Some (very) minor notes about the documentation :)

  • \f$ G(s) = gain * \frac{1 + \frac{w}{w_z}}{1 + \frac{w}{w_p}} \f$
    in the formula should be s and not w, right?
  • It should be highlighted that dt has to be the same as the sample time in the controller.

Thanks again! 👍

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Please add a bit of explanation & move this line into a separate commit if it's needed.

@@ -338,9 +338,6 @@ double Pid::getCurrentCmd()

void Pid::getCurrentPIDErrors(double *pe, double *ie, double *de)
{
// Get the gain parameters from the realtime buffer
Gains gains = *gains_buffer_.readFromRT();
Copy link
Member

Choose a reason for hiding this comment

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

Oops?

@bmagyar
Copy link
Member

bmagyar commented Apr 13, 2017

It's never not too late to branch it out. You can easily roll back your PR branch to the previous commit and make a force push. I'd merge the travis update on the separate PR.

You should be able to change the Pull Request target to kinetic-devel.

@bmagyar
Copy link
Member

bmagyar commented Nov 9, 2017

@progtologist Could you please rebase this PR against kinetic? Now we've got travis working again

@progtologist
Copy link
Member Author

I will try to do it this weekend and update you ;)

@bmagyar
Copy link
Member

bmagyar commented Nov 30, 2017

@progtologist this is a kind ping. I'd like to merge this, believe me :)

@progtologist
Copy link
Member Author

I am closing this PR in favor of #68 (switched to kinetic, created separate feature branch to avoid polution if another fix emerges)

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