RSDK-6282 [part 1]: add PID values to the gpio motor config #3435
Conversation
viambot
added
the
safe to test
martha-johnston
changed the title
martha-johnston
requested review from
a team,
randhid and
susmitaSanyal
and removed request for
a team
viambot
added
safe to test
components/motor/gpio/loop.go
Outdated
| if p == 0.0 && i == 0.0 && d == 0.0 { | ||
| // when tuning (all PID are zero), the loop has to exclude the trapz block, so the sum block is | ||
| // temporarily changed to depend on the constant block instead of the trapz block to pass over trapz | ||
| sumBlock := control.BlockConfig{ | ||
| Name: "sum", | ||
| Type: "sum", | ||
| Attribute: rdkutils.AttributeMap{ | ||
| "sum_string": "+-", | ||
| }, | ||
| DependsOn: []string{"set_point", "derivative"}, | ||
| } | ||
| conf.Blocks = append(conf.Blocks, sumBlock) | ||
| } else { | ||
| // if not tuning, set the sum block to once again depend | ||
| // on the trapz block instead of the constant bloc | ||
| sumBlock := control.BlockConfig{ | ||
| Name: "sum", | ||
| Type: "sum", | ||
| Attribute: rdkutils.AttributeMap{ | ||
| "sum_string": "+-", | ||
| }, | ||
| DependsOn: []string{"trapz", "derivative"}, | ||
| } | ||
| conf.Blocks = append(conf.Blocks, sumBlock) | ||
| } |
There was a problem hiding this comment.
We can go ahead and try to make this a helper function from the package. Might help with semantically calling that function from outside packages. What do you think?
Okay to do in a follow up PR.
There was a problem hiding this comment.
+1 , this portion can be a function.
control/control_loop.go
Outdated
| blk, err := createBlock(bcfg, logger) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| l.blocks[bcfg.Name] = &controlBlockInternal{blk: blk, blockType: bcfg.Type} | ||
| l.blocks[bcfg.Name].blockIndex = i |
There was a problem hiding this comment.
[q] Why are you adding the block index?
There was a problem hiding this comment.
also don't think this is necessary any longer with new changes
| if motorConfig.ControlParameters.P == 0.0 && | ||
| motorConfig.ControlParameters.I == 0.0 && | ||
| motorConfig.ControlParameters.D == 0.0 { | ||
| em.loop.SetTuning(context.Background(), true) |
There was a problem hiding this comment.
Is this background context canceled by anything in the loop? Or will it continue returning nil and blocking on a done call in the controls package?
There was a problem hiding this comment.
I don't think it's canceled by anything... I'm not sure it's necessary to even have a context for this function
There was a problem hiding this comment.
this function should be removed in a future pr
components/motor/gpio/loop.go
Outdated
| if p == 0.0 && i == 0.0 && d == 0.0 { | ||
| // when tuning (all PID are zero), the loop has to exclude the trapz block, so the sum block is | ||
| // temporarily changed to depend on the constant block instead of the trapz block to pass over trapz | ||
| sumBlock := control.BlockConfig{ | ||
| Name: "sum", | ||
| Type: "sum", | ||
| Attribute: rdkutils.AttributeMap{ | ||
| "sum_string": "+-", | ||
| }, | ||
| DependsOn: []string{"set_point", "derivative"}, | ||
| } | ||
| conf.Blocks = append(conf.Blocks, sumBlock) | ||
| } else { | ||
| // if not tuning, set the sum block to once again depend | ||
| // on the trapz block instead of the constant bloc | ||
| sumBlock := control.BlockConfig{ | ||
| Name: "sum", | ||
| Type: "sum", | ||
| Attribute: rdkutils.AttributeMap{ | ||
| "sum_string": "+-", | ||
| }, | ||
| DependsOn: []string{"trapz", "derivative"}, | ||
| } | ||
| conf.Blocks = append(conf.Blocks, sumBlock) | ||
| } |
There was a problem hiding this comment.
+1 , this portion can be a function.
| type motorPIDConfig struct { | ||
| P float64 `json:"p"` | ||
| I float64 `json:"i"` | ||
| D float64 `json:"d"` |
control/control_loop.go
Outdated
| if err := blk.blk.UpdateConfig(ctx, config); err != nil { | ||
| return err | ||
| } | ||
| l.cfg.Blocks[blk.blockIndex] = blk.blk.Config(ctx) |
There was a problem hiding this comment.
@randhid block index used here because UpdateConfig was not actually updating the config it was just updating the blocks , and the blocks aren't a map so I can't just use the name
There was a problem hiding this comment.
wait no other way. it updates the config but not the actual blocks
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
starting with just the motor config and will replicate in the sensor-controlled base
edit: also mildly fixes the RSDK-6308 bug. you can now run multiple motors with control loops at the same time, but it's still not blocking so you have to add sleeps if you want it to work as expected. going to fix that issue in another PR