[RSDK-8574][RSDK-8184]: return an error if user tries to move a sensor controlled base without copy/pasting tuned PID values to the config #4291
Conversation
viambot
added
the
safe to test
There was a problem hiding this comment.
I think what you have makes sense. Didn't realize we weren't saving the gains in the struct already(outside of within the loop/config).
would honestly be fine with having the gains be linGains and angGains instead of the array of gains, just to match the config card. would make some of the tuning/reconfig logic easier to parse.
only other thought is making sure the sensorbase can close the wait for tuning loop thread
|
have implemented some of your feedback @JohnN193, but pausing on this to finish up the rover canary so might not update for a bit |
viambot
added
safe to test
martha-johnston
force-pushed
the
rsdk-8574
branch
from
0c32505
to
9d77c2c
Compare
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
|
lots of changes here, but I did some basic starter hardware testing and everything seems to work as expected, but I need to do some more searching for edge cases. also need to update software tests still but feel free to review changes because I think the issues are in the test files. |
viambot
added
safe to test
|
update: software tests fixed |
|
@randhid tagging you if you're interested in taking a look |
viambot
added
the
safe to test
|
okay should be way more simple now, and added everything to motor. have tested tuning in progress and the tunedPIDError on every sensor base and motor api. all that should be left is to add do command get pid values to motor which I'll do in the next commit |
| } | ||
| } | ||
|
|
||
| // unlock the mutex before setting up the control loop so that the motors | ||
| // are not locked, and can run if any auto-tuning is necessary | ||
| sb.mu.Unlock() | ||
| if err := sb.setupControlLoop(linear, angular); err != nil { | ||
| if err := sb.setupControlLoop(sb.configPIDVals[0], sb.configPIDVals[1]); err != nil { |
There was a problem hiding this comment.
there is an error a couple lines above when we are assigning the values, is that sufficient? or do you want this logic moved to the validate function? to me it seems like we should only loop through the control parameters once but I'm open to moving it
| if (sb.configPIDVals[0].NeedsAutoTuning() && !(*sb.tunedVals)[0].NeedsAutoTuning()) || | ||
| (sb.configPIDVals[1].NeedsAutoTuning() && !(*sb.tunedVals)[1].NeedsAutoTuning()) { |
control/setup_control.go
Outdated
| @@ -100,15 +102,16 @@ func SetupPIDControlConfig( | |||
| pidLoop := &PIDLoop{ | |||
| Controllable: c, | |||
| PIDVals: pidVals, | |||
| TunedVals: &[]PIDConfig{}, | |||
There was a problem hiding this comment.
true let me add some more checks for indexing
viambot
added
safe to test
| // if loop is tuning, return an error | ||
| // if loop has been tuned but the values haven't been added to the config, error with tuned values | ||
| if sb.loop != nil && sb.loop.GetTuning(ctx) { | ||
| return control.TuningInProgressErr(sb.Name().ShortName()) | ||
| } else if (sb.configPIDVals[0].NeedsAutoTuning() && !(*sb.tunedVals)[0].NeedsAutoTuning()) || | ||
| (sb.configPIDVals[1].NeedsAutoTuning() && !(*sb.tunedVals)[1].NeedsAutoTuning()) { | ||
| return control.TunedPIDErr(sb.Name().ShortName(), *sb.tunedVals) | ||
| } | ||
|
|
There was a problem hiding this comment.
let's make this a bit more readable. You can:
1 - use a case structure.
2—Write an in-line, anonymous function that checks if the PID values need Autotuning and passes in the four PID values for your second case.
There was a problem hiding this comment.
tbh i think these should just be two separate if statements with their own comments. Don't see a reason for them to be grouped together and I personally find it weird when switch statements have a bunch of logic for cases(fine if peeps disagree tho). Agree with the helper function, its something I wanted to add as part of the refactors
| } | ||
| } | ||
|
|
||
| // unlock the mutex before setting up the control loop so that the motors | ||
| // are not locked, and can run if any auto-tuning is necessary | ||
| sb.mu.Unlock() | ||
| if err := sb.setupControlLoop(linear, angular); err != nil { | ||
| if err := sb.setupControlLoop(sb.configPIDVals[0], sb.configPIDVals[1]); err != nil { |
There was a problem hiding this comment.
Is there a test (manual or software) that shows you what the experience looks like for a user right now, if they try to configure with 1) one badly named "type" 2) two badly named types? Are we hitting the tuning loops, and what happens if one loop is tuned but not the other in your current logic? What errors are returned?
| // if loop is tuning, return an error | ||
| // if loop has been tuned but the values haven't been added to the config, error with tuned values | ||
| if sb.loop != nil && sb.loop.GetTuning(ctx) { | ||
| return control.TuningInProgressErr(sb.Name().ShortName()) | ||
| } else if (sb.configPIDVals[0].NeedsAutoTuning() && !(*sb.tunedVals)[0].NeedsAutoTuning()) || | ||
| (sb.configPIDVals[1].NeedsAutoTuning() && !(*sb.tunedVals)[1].NeedsAutoTuning()) { | ||
| return control.TunedPIDErr(sb.Name().ShortName(), *sb.tunedVals) | ||
| } | ||
|
|
There was a problem hiding this comment.
Ah, is this the same case structure? Then scratch what I said in the above comment. Let's make it a private helper method for the sensor-controlled base.
components/motor/gpio/controlled.go
Outdated
| // if loop is tuning, return an error | ||
| // if loop has been tuned but the values haven't been added to the config, error with tuned values | ||
| if cm.loop != nil && cm.loop.GetTuning(ctx) { | ||
| return control.TuningInProgressErr(cm.Name().ShortName()) | ||
| } else if cm.configPIDVals[0].NeedsAutoTuning() && !(*cm.tunedVals)[0].NeedsAutoTuning() { | ||
| return control.TunedPIDErr(cm.Name().ShortName(), *cm.tunedVals) | ||
| } | ||
|
|
There was a problem hiding this comment.
Since this is another package and this logic is only used once, I'd keep it as is, we may want to look out for this pattern to consider a shared method in the control package if we need this logic across more controlled components.
components/motor/gpio/controlled.go
Outdated
| // if loop is tuning, return an error | ||
| // if loop has been tuned but the values haven't been added to the config, error with tuned values | ||
| if cm.loop != nil && cm.loop.GetTuning(ctx) { | ||
| return control.TuningInProgressErr(cm.Name().ShortName()) | ||
| } else if cm.configPIDVals[0].NeedsAutoTuning() && !(*cm.tunedVals)[0].NeedsAutoTuning() { | ||
| return control.TunedPIDErr(cm.Name().ShortName(), *cm.tunedVals) | ||
| } | ||
|
|
There was a problem hiding this comment.
Ah maybe I spoke too soon, but still, a private method in this package as well, if they're exactly the same for GoFor and SetRPM and have no reason to ever be different, let's make them the same check.
There was a problem hiding this comment.
I'll try and add a control helper
There was a problem hiding this comment.
FWIW I think these should be helper in the scope of this package, not the control package. They care about things passed into the driver code at the driver level; it just makes it a little bit less repeated, so if you ever need to change its check in this package, you don't end up with a mismatch within the driver code itself.f
There was a problem hiding this comment.
I think the controls package could own the helper, since the controls package kinda owns the tunedVals in the first place. That was something I wanted to setup when proposing the refactors. fine with leaving it in the driver for this pr
There was a problem hiding this comment.
we can incrementally add to the scope to the eventual PR, don't want to go too DRY in case we limit ourselves while addressing a driver functionality request, which is this ticket.
viambot
added
safe to test
|
Okay I think I should have covered everything we discussed but let me know if I misunderstood anything and there's more changes you'd like |
|
going to do a hardware test one more time to make sure everything still works as expected |
| } | ||
| } | ||
|
|
||
| // unlock the mutex before setting up the control loop so that the motors | ||
| // are not locked, and can run if any auto-tuning is necessary | ||
| sb.mu.Unlock() | ||
| if err := sb.setupControlLoop(linear, angular); err != nil { | ||
| if err := sb.setupControlLoop(sb.configPIDVals[0], sb.configPIDVals[1]); err != nil { |
There was a problem hiding this comment.
^ If the config is not nil, and there are incorrect types, and I copy over my PID values to those incorrect types, that's not great for a user, I'm assuming it'll just keep re-tuning even if I populate the fields of the malformed types?
Let's error in Validate for that case, tell people they've misspelt the types if the control part of the config is populated.
| } | ||
| } | ||
|
|
||
| // unlock the mutex before setting up the control loop so that the motors | ||
| // are not locked, and can run if any auto-tuning is necessary | ||
| sb.mu.Unlock() | ||
| if err := sb.setupControlLoop(linear, angular); err != nil { | ||
| if err := sb.setupControlLoop(sb.configPIDVals[0], sb.configPIDVals[1]); err != nil { |
There was a problem hiding this comment.
Also, for Validation, add a test for all the Valid and erroring cases.
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
martha-johnston
changed the title
This isn't 100% ready but I want some early feedback on the setup. Things that still need to be done:
Update: