diff --git a/.gitignore b/.gitignore index 433049f9e15..865b2315010 100644 --- a/.gitignore +++ b/.gitignore @@ -92,3 +92,6 @@ web/cmd/server/*.core # direnv (optional dev tool) .envrc + +# codegpt +.codegpt/* diff --git a/components/motor/gpiostepper/gpiostepper.go b/components/motor/gpiostepper/gpiostepper.go index f20acd515e7..3739061b891 100644 --- a/components/motor/gpiostepper/gpiostepper.go +++ b/components/motor/gpiostepper/gpiostepper.go @@ -85,46 +85,42 @@ func (cfg *Config) Validate(path string) ([]string, error) { func init() { resource.RegisterComponent(motor.API, model, resource.Registration[motor.Motor, *Config]{ - Constructor: func( - ctx context.Context, - deps resource.Dependencies, - conf resource.Config, - logger logging.Logger, - ) (motor.Motor, error) { - actualBoard, motorConfig, err := getBoardFromRobotConfig(deps, conf) - if err != nil { - return nil, err - } - - return newGPIOStepper(ctx, actualBoard, *motorConfig, conf.ResourceName(), logger) - }, - }) + Constructor: newGPIOStepper, + }, + ) } -func getBoardFromRobotConfig(deps resource.Dependencies, conf resource.Config) (board.Board, *Config, error) { - motorConfig, err := resource.NativeConfig[*Config](conf) - if err != nil { - return nil, nil, err - } - if motorConfig.BoardName == "" { - return nil, nil, errors.New("expected board name in config for motor") +// TODO (rh) refactor this driver so that the enable and direction logic is at the beginning of each API call +// and the step -> position logic is the only thing being handled by the background thread. +// right now too many things can be called out of lock, this function is only called from the constructor, CLose +// the doCycle step routine, and should not be called elsewhere since there's no lock in to ptoect the enable pins. +func (m *gpioStepper) enable(ctx context.Context, high bool) error { + var err error + if m.enablePinHigh != nil { + err = multierr.Combine(err, m.enablePinHigh.Set(ctx, high, nil)) } - b, err := board.FromDependencies(deps, motorConfig.BoardName) - if err != nil { - return nil, nil, err + + if m.enablePinLow != nil { + err = multierr.Combine(err, m.enablePinLow.Set(ctx, !high, nil)) } - return b, motorConfig, nil + + return err } func newGPIOStepper( ctx context.Context, - b board.Board, - mc Config, - name resource.Name, + deps resource.Dependencies, + conf resource.Config, logger logging.Logger, ) (motor.Motor, error) { - if b == nil { - return nil, errors.New("board is required") + mc, err := resource.NativeConfig[*Config](conf) + if err != nil { + return nil, err + } + + b, err := board.FromDependencies(deps, mc.BoardName) + if err != nil { + return nil, err } if mc.TicksPerRotation == 0 { @@ -132,15 +128,13 @@ func newGPIOStepper( } m := &gpioStepper{ - Named: name.AsNamed(), + Named: conf.ResourceName().AsNamed(), theBoard: b, stepsPerRotation: mc.TicksPerRotation, logger: logger, opMgr: operation.NewSingleOperationManager(), } - var err error - // only set enable pins if they exist if mc.Pins.EnablePinHigh != "" { m.enablePinHigh, err = b.GPIOPinByName(mc.Pins.EnablePinHigh) @@ -208,8 +202,7 @@ type gpioStepper struct { // SetPower sets the percentage of power the motor should employ between 0-1. func (m *gpioStepper) SetPower(ctx context.Context, powerPct float64, extra map[string]interface{}) error { if math.Abs(powerPct) <= .0001 { - m.stop() - return nil + return m.Stop(ctx, nil) } if m.minDelay == 0 { @@ -219,7 +212,10 @@ func (m *gpioStepper) SetPower(ctx context.Context, powerPct float64, extra map[ m.Name().Name) } + // lock added here to prevent race with doStep + m.lock.Lock() m.stepperDelay = time.Duration(float64(m.minDelay) / math.Abs(powerPct)) + m.lock.Unlock() if powerPct < 0 { m.targetStepPosition = math.MinInt64 @@ -288,7 +284,9 @@ func (m *gpioStepper) doCycle(ctx context.Context) (time.Duration, error) { func (m *gpioStepper) doStep(ctx context.Context, forward bool) error { err := multierr.Combine( m.dirPin.Set(ctx, forward, nil), - m.stepPin.Set(ctx, true, nil)) + m.stepPin.Set(ctx, true, nil), + m.enable(ctx, true), + ) if err != nil { return err } @@ -319,13 +317,7 @@ func (m *gpioStepper) GoFor(ctx context.Context, rpm, revolutions float64, extra ctx, done := m.opMgr.New(ctx) defer done() - err := m.enable(ctx, true) - if err != nil { - return errors.Wrapf(err, "error enabling motor in GoFor from motor (%s)", m.Name().Name) - } - - err = m.goForInternal(ctx, rpm, revolutions) - if err != nil { + if err := m.goForInternal(ctx, rpm, revolutions); err != nil { return multierr.Combine( m.enable(ctx, false), errors.Wrapf(err, "error in GoFor from motor (%s)", m.Name().Name)) @@ -406,8 +398,7 @@ func (m *gpioStepper) SetRPM(ctx context.Context, rpm float64, extra map[string] defer m.lock.Unlock() if math.Abs(rpm) <= .0001 { - m.stop() - return nil + return m.Stop(ctx, nil) } // calculate delay between steps for the thread in the goroutine that we started in component creation. @@ -488,19 +479,6 @@ func (m *gpioStepper) IsPowered(ctx context.Context, extra map[string]interface{ return on, percent, err } -func (m *gpioStepper) enable(ctx context.Context, on bool) error { - var err error - if m.enablePinHigh != nil { - err = multierr.Combine(err, m.enablePinHigh.Set(ctx, on, nil)) - } - - if m.enablePinLow != nil { - err = multierr.Combine(err, m.enablePinLow.Set(ctx, !on, nil)) - } - - return err -} - func (m *gpioStepper) Close(ctx context.Context) error { err := m.Stop(ctx, nil) diff --git a/components/motor/gpiostepper/gpiostepper_test.go b/components/motor/gpiostepper/gpiostepper_test.go index 14c2d7757f6..177eab34b6c 100644 --- a/components/motor/gpiostepper/gpiostepper_test.go +++ b/components/motor/gpiostepper/gpiostepper_test.go @@ -9,6 +9,7 @@ import ( "go.viam.com/test" "go.viam.com/utils/testutils" + "go.viam.com/rdk/components/board" fakeboard "go.viam.com/rdk/components/board/fake" "go.viam.com/rdk/logging" "go.viam.com/rdk/resource" @@ -20,11 +21,6 @@ func TestConfigs(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - logger := logging.NewTestLogger(t) - c := resource.Config{ - Name: "fake_gpiostepper", - } - goodConfig := Config{ Pins: PinConfig{Direction: "b", Step: "c", EnablePinHigh: "d", EnablePinLow: "e"}, TicksPerRotation: 200, @@ -32,6 +28,12 @@ func TestConfigs(t *testing.T) { StepperDelay: 30, } + logger := logging.NewTestLogger(t) + c := resource.Config{ + Name: "fake_gpiostepper", + ConvertedAttributes: &goodConfig, + } + pinB := &fakeboard.GPIOPin{} pinC := &fakeboard.GPIOPin{} pinD := &fakeboard.GPIOPin{} @@ -97,8 +99,9 @@ func TestConfigs(t *testing.T) { test.That(t, err, test.ShouldBeError, resource.NewConfigValidationFieldRequiredError("", "board")) }) + deps := resource.Dependencies{resource.NewName(board.API, "brd"): &b} t.Run("initializing good with enable pins", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) s := m.(*gpioStepper) test.That(t, err, test.ShouldBeNil) @@ -113,11 +116,16 @@ func TestConfigs(t *testing.T) { }) t.Run("initializing good without enable pins", func(t *testing.T) { - mc := goodConfig - mc.Pins.EnablePinHigh = "" - mc.Pins.EnablePinLow = "" - - m, err := newGPIOStepper(ctx, &b, mc, c.ResourceName(), logger) + c := resource.Config{ + Name: "fake_gpiostepper", + ConvertedAttributes: &Config{ + Pins: PinConfig{Direction: "b", Step: "c", EnablePinHigh: "", EnablePinLow: ""}, + TicksPerRotation: 200, + BoardName: "brd", + }, + } + + m, err := newGPIOStepper(ctx, deps, c, logger) s := m.(*gpioStepper) test.That(t, err, test.ShouldBeNil) @@ -132,25 +140,43 @@ func TestConfigs(t *testing.T) { }) t.Run("initializing with no board", func(t *testing.T) { - _, err := newGPIOStepper(ctx, nil, goodConfig, c.ResourceName(), logger) + c := resource.Config{ + Name: "fake_gpiostepper", + ConvertedAttributes: &Config{BoardName: "some_board"}, + } + + _, err := newGPIOStepper(ctx, nil, c, logger) test.That(t, err, test.ShouldNotBeNil) - test.That(t, err.Error(), test.ShouldContainSubstring, "board is required") + test.That(t, err, test.ShouldBeError, resource.DependencyNotFoundError(resource.NewName(board.API, "some_board"))) }) t.Run("initializing without ticks per rotation", func(t *testing.T) { - mc := goodConfig - mc.TicksPerRotation = 0 - - _, err := newGPIOStepper(ctx, &b, mc, c.ResourceName(), logger) + c := resource.Config{ + Name: "fake_gpiostepper", + ConvertedAttributes: &Config{ + BoardName: "brd", + Pins: PinConfig{Direction: "b", Step: "c", EnablePinHigh: "d", EnablePinLow: "e"}, + TicksPerRotation: 0, + }, + } + + _, err := newGPIOStepper(ctx, deps, c, logger) test.That(t, err, test.ShouldNotBeNil) test.That(t, err.Error(), test.ShouldContainSubstring, "expected ticks_per_rotation") }) t.Run("initializing with negative stepper delay", func(t *testing.T) { - mc := goodConfig - mc.StepperDelay = -100 - - m, err := newGPIOStepper(ctx, &b, mc, c.ResourceName(), logger) + c := resource.Config{ + Name: "fake_gpiostepper", + ConvertedAttributes: &Config{ + BoardName: "brd", + Pins: PinConfig{Direction: "b", Step: "c", EnablePinHigh: "d", EnablePinLow: "e"}, + TicksPerRotation: 1, + StepperDelay: -100, + }, + } + + m, err := newGPIOStepper(ctx, deps, c, logger) s := m.(*gpioStepper) test.That(t, err, test.ShouldBeNil) @@ -159,7 +185,7 @@ func TestConfigs(t *testing.T) { }) t.Run("motor supports position reporting", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -177,13 +203,12 @@ func TestRunning(t *testing.T) { logger, _ := logging.NewObservedTestLogger(t) c := resource.Config{ Name: "fake_gpiostepper", - } - - goodConfig := Config{ - Pins: PinConfig{Direction: "b", Step: "c", EnablePinHigh: "d", EnablePinLow: "e"}, - TicksPerRotation: 200, - BoardName: "brd", - StepperDelay: 30, + ConvertedAttributes: &Config{ + Pins: PinConfig{Direction: "b", Step: "c", EnablePinHigh: "d", EnablePinLow: "e"}, + TicksPerRotation: 200, + BoardName: "brd", + StepperDelay: 30, + }, } pinB := &fakeboard.GPIOPin{} @@ -197,9 +222,10 @@ func TestRunning(t *testing.T) { "e": pinE, } b := fakeboard.Board{GPIOPins: pinMap} + deps := resource.Dependencies{resource.NewName(board.API, "brd"): &b} t.Run("isPowered false after init", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -218,7 +244,7 @@ func TestRunning(t *testing.T) { }) t.Run("IsPowered true", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) s := m.(*gpioStepper) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -254,7 +280,7 @@ func TestRunning(t *testing.T) { }) t.Run("motor enable", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) s := m.(*gpioStepper) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -283,7 +309,7 @@ func TestRunning(t *testing.T) { }) t.Run("motor testing with positive rpm and positive revolutions", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) s := m.(*gpioStepper) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -303,7 +329,7 @@ func TestRunning(t *testing.T) { }) t.Run("motor testing with negative rpm and positive revolutions", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) s := m.(*gpioStepper) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -323,7 +349,7 @@ func TestRunning(t *testing.T) { }) t.Run("motor testing with positive rpm and negative revolutions", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) s := m.(*gpioStepper) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -343,7 +369,7 @@ func TestRunning(t *testing.T) { }) t.Run("motor testing with negative rpm and negative revolutions", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) s := m.(*gpioStepper) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -363,7 +389,7 @@ func TestRunning(t *testing.T) { }) t.Run("motor testing with 0 rpm and 0 revolutions", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -375,7 +401,7 @@ func TestRunning(t *testing.T) { }) t.Run("Ensure stop called when gofor is interrupted", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) s := m.(*gpioStepper) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -423,7 +449,7 @@ func TestRunning(t *testing.T) { }) t.Run("enable pins handled properly during GoFor", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -470,11 +496,55 @@ func TestRunning(t *testing.T) { test.That(tb, err, test.ShouldBeNil) test.That(tb, l, test.ShouldBeTrue) }) + + ctx, cancel = context.WithCancel(context.Background()) + + err = m.SetRPM(ctx, 100, map[string]interface{}{}) + test.That(t, err, test.ShouldBeNil) + + // Make sure it starts moving + testutils.WaitForAssertion(t, func(tb testing.TB) { + tb.Helper() + on, _, err := m.IsPowered(ctx, nil) + test.That(tb, err, test.ShouldBeNil) + test.That(tb, on, test.ShouldEqual, true) + + h, err := pinD.Get(ctx, nil) + test.That(tb, err, test.ShouldBeNil) + test.That(tb, h, test.ShouldBeTrue) + + l, err := pinE.Get(ctx, nil) + test.That(tb, err, test.ShouldBeNil) + test.That(tb, l, test.ShouldBeFalse) + }) + + cancel() + + err = m.SetPower(ctx, 1, map[string]interface{}{}) + test.That(t, err, test.ShouldBeNil) + + // Make sure it starts moving + testutils.WaitForAssertion(t, func(tb testing.TB) { + tb.Helper() + on, _, err := m.IsPowered(ctx, nil) + test.That(tb, err, test.ShouldBeNil) + test.That(tb, on, test.ShouldEqual, true) + + h, err := pinD.Get(ctx, nil) + test.That(tb, err, test.ShouldBeNil) + test.That(tb, h, test.ShouldBeTrue) + + l, err := pinE.Get(ctx, nil) + test.That(tb, err, test.ShouldBeNil) + test.That(tb, l, test.ShouldBeFalse) + }) + + cancel() test.That(t, ctx.Err(), test.ShouldNotBeNil) }) t.Run("motor testing with large # of revolutions", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) s := m.(*gpioStepper) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -508,7 +578,7 @@ func TestRunning(t *testing.T) { }) t.Run("motor testing with SetRPM", func(t *testing.T) { - m, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + m, err := newGPIOStepper(ctx, deps, c, logger) test.That(t, err, test.ShouldBeNil) defer m.Close(ctx) @@ -541,7 +611,7 @@ func TestRunning(t *testing.T) { }) t.Run("test calcStepperDelay", func(t *testing.T) { - stepper, err := newGPIOStepper(ctx, &b, goodConfig, c.ResourceName(), logger) + stepper, err := newGPIOStepper(ctx, deps, c, logger) test.That(t, err, test.ShouldBeNil) defer stepper.Close(ctx)