Response to #40 #42
Response to #40 #42
Conversation
|
To make issue 2 in this PR clearer, check that, in #38, only one of the |
|
|
||
| Pid pid(1.0, 0.0, 0.0, -1.0, 0.0); | ||
| //// Do not allow this construction |
There was a problem hiding this comment.
Please either remove dead code, or bring it to a functional state.
|
Could you please rebase against Many thanks for the extra tests. |
|
[update]
|
|
I'm about to press some green buttons here. It seems that this PR got a bit forgotten, please raise a thumb if you also think it should be merged. |
|
|
||
| // If call again with same arguments | ||
| cmd = pid.computeCommand(-0.5, ros::Duration(1.0)); | ||
| // Then expect the integration do it part doubling the command |
There was a problem hiding this comment.
Then expect the integral part doubles the command
There was a problem hiding this comment.
or
Then expect the integration do its part doubling the command
|
LGTM, but I've added a few comments regarding typos on the comments. @bmagyar Maybe you can fix them before merging. All the tests look good. Thanks for the contribution. |
|
Well I don't really have access to Carlos' branch, but I can fix it after I merged it! :) Oh btw, merging... |
|
Oh shoot, should have rebased this one... Nothing serious though, only that the history is not that pretty on this one. |
|
Hey, sorry I'm late for the party... thanks for the review and the merge! |
|
No problem, thanks for your contribution! |
Could be that #40 happened mainly due to 2 things (well, 3, if you count my awkwardness):
computeCommandmethod has two signatures with almost a replica of code within -> Same code in two places requires update in two places -> The chance of making a mistake when updating increases.In this initial PR both are addressed, there are 2 separated commits to easy the review.
First, in 9dc84fd, the coverage of the PID class test suite is increased with the following modification of current tests:
zeroITermBadIBoundsTest: I think that enteringi_max < i_minshould be prevented, or at least warned. A suggested change to avoid bad i-bounds is there commented because it would require a change in thesetGainsmethod return type toboolinstead ofvoidand the current implementation wouldn't pass the test. Another posibility would be to leave that return type and just throw a warning ifi_max < i_minis passed.integrationWindupTest: Now upper and lower bounds are tested.gainSettingCopyPIDTest: Makeup in names for readability + Added test onreset()method.And the following additions, a new set of tests under the name
CommandTest, hopefully well described with comments:proportionalOnlyTest,integralOnlyTest,derivativeOnlyTest: Check that the command computed using each contribution independently is correct. Note that, they depend on the integral and differentiation schemes, and if they change, the tests might not be valid anymore.completePIDTest: Few checks on the sum up of contributions. Same here, the test depends on the integral and differentiation schemes.Second, in 7fe5e12, only one method does the PID command computation and writes to class members (except for
p_error_last_), which is the one with the signature that allows users to pass a pre-computed error derivative. The other one calls this method passing the error derivative computed with an internal differentiation scheme. I couldn't find whether this is ROS-style compliant or not, and not sure if it affects critically the performance, but surely it will reduce the chance of making a mistake if the implementation is to be updated again.PS: I didn't do anything with the DEPRECATED command computation methods.