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

Improvement on the integral contribution implementation #33

Merged
merged 1 commit into from
Feb 3, 2015
Merged

Improvement on the integral contribution implementation #33

merged 1 commit into from
Feb 3, 2015

Conversation

carlosjoserg
Copy link
Contributor

The integral of the error is computed in a class member, and the integral contribution is computed as of it, instead of doing all together. Now, setting the integral gain to zero dynamically cancel the integral contribution.

Additional update:
Function getCurrentPIDErrors(...) now returns the integral of the position error, and not the contribution. Because of this, also the tests were modified. Tests integrationWindupZeroGainTest (actually, this one makes no sense now, since the error is directly given instead of obtaining by diving the term by the gain) and integrationWindupTest check the windup constraint on the computed command.

I don't know how this affects the dependant packages, but I believe the change is worthy.

Resolve #32.

@adolfo-rt
Copy link
Member

LGTM.

On getCurrentPIDErrors(...) returning the integral error (correct since this patch) vs the integral term (incorrect, existing behavior), I don't know if the existing broken behavior is used by anyone.

I'm going to let this one soak another day to see if somebody wants to comment on this particular issue, otherwise I'll give it a go.

adolfo-rt pushed a commit that referenced this pull request Feb 3, 2015
Improvement on the integral contribution implementation
@adolfo-rt adolfo-rt merged commit ded9ffd into ros-controls:indigo-devel Feb 3, 2015
@adolfo-rt
Copy link
Member

Thanks for the two fixes!.

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.

2 participants