[RSDK-9603] Move enable pin logic into `doCycle` to allow SetPower and SetRPM to enable the configured EN pins #4666

randhid · 2024-12-31T22:01:23Z

We do not set EnablePinHigh pins to high in SetPower and SetRPM, new APIs that expanded this driver's capabilities, which is a problem since that will not allow a stepper motor driver with an enable pin that needs to be drawn high, this PR moves the enable logic to the doStep helper method so that the pins are set in each cycle for a stepper motor.

I have also refactored the tests and the constructor a little. I can change it back if people don't like flatter constructor functions.

Tests conducted:

SetRPM with EnLow Pins configured on an A4988 moves the stepper motor forward with positive rpm and backwards with negative rpm.
SetRPM without EnLow Pins configured on an A4988 moves the stepper motor forward with positive rpm and backwards with negative rpm.
SetPower with EnLow Pins configured moves the stepper motor forward with positive poiwer and backwards with negative power.
SetPower without EnLow Pins configured moves the stepper motor forward with positive poiwer and backwards with negative power.
GoFor with EnLow pins moves the motor in the following directions with the following rpm/rev combos
forwards: rpm+/rev+ and rpm-/rev-
backwards: rpm-/rev+ and rpm-/rev+
SetPower, SetRPM, GoFor, GoTo all interrupt each other when buttons are clicked on app. Stop interrupts each API call and halts the motor from moving in each API call.
Erroneously setting the EnablePin on the A4988 motor to "High" stops the motor from moving completely - this is expected as is is setting the motor driver to its disabled state and can be moved by hand freely at all times.

Tested with a pi5 board!

randhid · 2024-12-31T22:02:32Z

components/motor/gpiostepper/gpiostepper.go

+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


Moved enable up with no changes.
Added a long warning not to use this outside of the constructor, Close and doCycle because we cowboy it and don't lock the pins stored on the motor struct.

randhid · 2024-12-31T22:03:03Z

components/motor/gpiostepper/gpiostepper.go

@@ -208,8 +204,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)


This calls Stop in lock.

randhid · 2024-12-31T22:21:36Z

.gitignore

+# codegpt
+.codegpt/*


I can remove this but I think it's a good addition so people don't commit their codegpt files

JohnN193 · 2025-01-03T16:22:50Z

components/motor/gpiostepper/gpiostepper.go

@@ -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),


so as long as the motor thinks it should be moving (m.stepPosition == m.targetStepPosition), the motor will be enabled.

seems like we might be able to put all of the m.enable calls inside of doCycle, which might make the code easier to follow, but def not necessary.

Yes, also +1 on making this code easier to read and reason about, it's in a state right now. I will follow up.

randhid added 2 commits December 31, 2024 16:46

fix enable bug

cf03558

add comment about the dangers of enable

c85a5b5

randhid requested a review from martha-johnston December 31, 2024 22:01

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Dec 31, 2024

randhid requested a review from JohnN193 December 31, 2024 22:01

randhid commented Dec 31, 2024

View reviewed changes

remove errant enables

43596d4