Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[RSDK-6164] Mimo pid updates #4290
[RSDK-6164] Mimo pid updates #4290
Changes from 2 commits
358fe3e
252a42a
21c3e33
31e3453
1cbb1a1
00644c8
b7900ee
63eeb2a
8e60a2d
9b2916a
9f80288
311e05c
8bf5301
343544f
5b86ac8
c2506d6
598e704
22be53f
0ca4594
d2cba49
756e5b8
372f540
12303b2
4528d58
a983cfa
87b1283
7956825
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove before merging. running
make lint-go
should also help catch theseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a comment that this attempts to tune multiple signals simultaneously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just as an fyi I don't think tuning simultaneously would work for the base. or if you wanted to tune motors and a base? although maybe this change will allow it to do so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i kinda agree with you. I think I might try changing this to tune signals one at a time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the code to tune one signal at a time. the tests are alittle weird with this change(have to turn off tuning after we reach the "end" of the test) but otherwise i think this should work the way we want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this function be used for the single input case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not going to expand this for the single input case. will hold off on making large changes to the single input case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventually I think yes and just index 0 any time it's single input. but that does seem like a problem for a future PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay so in reference to my above comment about mismatch in number of pids and number of tuners, it seems like that comment is just wrong, because we're making a tuner for every pid block right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I guess the comment is abit misleading. we make a structure that holds up to N tuners, based on the number of pid sets/gains/signals configured.
a tuner only gets initialized if a pid set needs tuning, otherwise the tuning boolean will be set to
false
I'll update the comment in getTuning to reflect this behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so in this case all pid's in a MIMO block need to use the same values? (excluding P, I, and D). otherwise how would it know which "tune_ssr_value" to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jk I see it in a comment below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional nit to move that comment from line 254 to line ~239, but nbd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be worth making a helper function for this code? since both the mimo and single input/output code use it. no is an okay answer since we eventually only want one code path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving this code as is, as we will eventually remove the single input/output path once MIMO is ready