Skip to content

Corrected control system derivative calculations#349

Merged
irwineffect merged 3 commits into
masterfrom
rsummers/control-derivative-fix
Jan 20, 2018
Merged

Corrected control system derivative calculations#349
irwineffect merged 3 commits into
masterfrom
rsummers/control-derivative-fix

Conversation

@ryan-summers

@ryan-summers ryan-summers commented Jan 3, 2018

Copy link
Copy Markdown
Contributor

This change corrects the bug as reported in #330 by adding in a boolean that indicates whether or not new measurements have arrived for a specific state. New errors are stored only if new state information has been received.

Integral information is still updated even if new state information has not been received. Do we want to change this?


This change is Reviewable

@ryan-summers

Copy link
Copy Markdown
Contributor Author

I believe that the failing CI tests are due to PalouseRobosub/robosub_simulator-2018#111

@ryan-summers

Copy link
Copy Markdown
Contributor Author

A PR has been made to correct the simulator PID values as necessary for this PR: PalouseRobosub/robosub_simulator-2018#112

@ryan-summers

Copy link
Copy Markdown
Contributor Author

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


src/movement/control_system.cpp, line 60 at r1 (raw file):

        current_integral = Vector6d::Zero();
        current_derivative = Vector6d::Zero();
        goals = Vector6d::Zero();

new_measurement_available needs to be set to false.


Comments from Reviewable

@skallaher

Copy link
Copy Markdown
Member

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/movement/control_system.h, line 169 at r2 (raw file):

     * Boolean vector indicating that new state measurements are available.
     */
    Matrix<bool, 6, 1> new_measurement_available;

Is there a reason that this is a 6x1 matrix of booleans rather than an array of 6 booleans?


src/movement/control_system.h, line 174 at r2 (raw file):

     * The previous errors calculated.
     */
    Matrix<std::deque<StateMeasurement>, 6, 1> previous_error;

Similar question here. Why not use an array or vector?


Comments from Reviewable

@ryan-summers

Copy link
Copy Markdown
Contributor Author

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/movement/control_system.cpp, line 60 at r1 (raw file):

Previously, ryan-summers (Ryan Summers) wrote…

new_measurement_available needs to be set to false.

Done.


src/movement/control_system.h, line 169 at r2 (raw file):

Previously, skallaher (Sean Kallaher) wrote…

Is there a reason that this is a 6x1 matrix of booleans rather than an array of 6 booleans?

See reply below


src/movement/control_system.h, line 174 at r2 (raw file):

Previously, skallaher (Sean Kallaher) wrote…

Similar question here. Why not use an array or vector?

It's keeping the class consistent since everything else is using Eigen vector/matrix types


Comments from Reviewable

@ryan-summers
ryan-summers force-pushed the rsummers/control-derivative-fix branch from 3e2e95c to bf06d3c Compare January 18, 2018 05:53
@skallaher

Copy link
Copy Markdown
Member

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@irwineffect
irwineffect merged commit bf06d3c into master Jan 20, 2018
@irwineffect
irwineffect deleted the rsummers/control-derivative-fix branch January 20, 2018 05:14
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