RSDK-8926: Rover canary motor test fail because failure to set pins #4549
Conversation
…pping, and make stop all error clearer
viambot
added
the
safe to test
martha-johnston
requested review from
a team and
randhid
and removed request for
a team
robot/impl/local_robot.go
Outdated
| resourceErrs := make(map[string]error) | ||
| for _, name := range r.ResourceNames() { | ||
| res, err := r.ResourceByName(name) | ||
| if err != nil { | ||
| resourceErrs = append(resourceErrs, name.Name) | ||
| resourceErrs[name.Name] = err | ||
| continue | ||
| } | ||
|
|
||
| if actuator, ok := res.(resource.Actuator); ok { | ||
| if err := actuator.Stop(ctx, extra[name]); err != nil { | ||
| resourceErrs = append(resourceErrs, name.Name) | ||
| resourceErrs[name.Name] = err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(resourceErrs) > 0 { | ||
| return errors.Errorf("failed to stop components named %s", strings.Join(resourceErrs, ",")) | ||
| for k, v := range resourceErrs { | ||
| return errors.Errorf("failed to stop component named %s with error %v", k, v) |
There was a problem hiding this comment.
we should join all the errors first before returning them, right now you're returning at the first iteration of this for loop, and so only returning the first error. Not the same behaviour as before.
There was a problem hiding this comment.
oh meant to log this every time not return. is there a benefit to returning a joined error rather than just logging each error as we iterate through resourceErrs
There was a problem hiding this comment.
Since the behaviour before was to return an error, that would be a change in the behaviour of StopAll; now - if Stopall failed to stop your actuating components, you've got bigger problems than what the local robot does after. Session manager and App call stopAll for connectivity loss and moving away from the control page. If there are error checks there where they end up retrying if an error is encountered, then you're changing that behaviour of that caller.
There was a problem hiding this comment.
okay I'll wrap it all into one error
| @@ -306,7 +306,7 @@ func (m *Motor) Stop(ctx context.Context, extra map[string]interface{}) error { | |||
| m.opMgr.CancelRunning(ctx) | |||
| m.mu.Lock() | |||
| defer m.mu.Unlock() | |||
| return m.setPWM(ctx, 0, extra) | |||
| return m.setPWM(context.Background(), 0, extra) | |||
There was a problem hiding this comment.
Nice! Glad you identified the cause os the setPWM failure.
| @@ -193,23 +193,23 @@ func (r *localRobot) StopAll(ctx context.Context, extra map[resource.Name]map[st | |||
| } | |||
|
|
|||
| // Stop all stoppable resources | |||
| resourceErrs := []string{} | |||
| resourceErrs := make(map[string]error) | |||
There was a problem hiding this comment.
@cheukt okay with this change? I think it's good for the StopAll error map in general.
viambot
added
safe to test
viambot
added
safe to test
The context was getting canceled before the pins could be set correctly in order to stop the motor. by using a background context,
Stop()andsetPWM()can return whileturnOffis still setting the pins. Could this also be done withSelectContextOrWait?I also updated the error return from local_robot.go because it buried the error from
StopAllin order to see what was actually happening.