Add speed/direction motor by pigrammer3 · Pull Request #1147 · gpiozero/gpiozero

pigrammer3 · 2024-05-28T21:29:34Z

See #300 and #330. Add support for a motor driven by three pins: one that's speed and the other two like the two forward and backward pins on the regular Motor class. I couldn't think of a better name than SpeedDirectionMotor.

lurch · 2024-05-30T01:19:14Z

+        self.backward_device.off()
+        self.forward_device.on()
+        self.speed_device.value = speed


As a safety-measure, I wonder if this should first set self.speed_device.value to 0 before turning the forward_device on? I.e. if you do

my_speed_motor.backward(1) time.sleep(1) my_speed_motor.forward(0.2)

you probably don't want a momentary blip where the motor is running forwards at full speed?

lurch · 2024-05-30T02:03:55Z

Hrmmm, looking at my comments in #366 (specifically this one ), I wonder if this could be shoe-horned into the existing Motor() class by e.g. adding a pwm_the_enable_pin parameter (which defaults to False) to Motor.__init__ ?
And then if you use Motor(forward=2, backward=3, enable=4, pwm=True) you'd get the current behaviour of forward_device & backward_device being PWMOutDevices and enable_device being a DigitalOutputDevice; but if you instead did Motor(forward=2, backward=3, enable=4, pwm_the_enable_pin=True) then you'd get forward_device & backward_device being DigitalOutputDevices and enable_device being a PWMOutputDevice (the behaviour in this PR) ? 🤔
Would probably make sense to make passing both pwmn=True and pwm_the_enable_pin=True be treated as an error. Passing enable=None and pwm_the_enable_pin=True should also be treated as an error.

And I'm too tired right now to think of a better name than pwm_the_enable_pin 😆

bennuttall · 2024-05-30T09:29:04Z

I thought this felt familiar. Does it not work the same if you just use the enable pin on the Motor class?

pigrammer3 · 2024-05-30T11:35:42Z

Hrmmm, looking at my comments in #366 (specifically this one ), I wonder if this could be shoe-horned into the existing Motor() class by e.g. adding a pwm_the_enable_pin parameter (which defaults to False) to Motor.__init__ ?
And then if you use Motor(forward=2, backward=3, enable=4, pwm=True) you'd get the current behaviour of forward_device & backward_device being PWMOutDevices and enable_device being a DigitalOutputDevice; but if you instead did Motor(forward=2, backward=3, enable=4, pwm_the_enable_pin=True) then you'd get forward_device & backward_device being DigitalOutputDevices and enable_device being a PWMOutputDevice (the behaviour in this PR) ? 🤔
Would probably make sense to make passing both pwmn=True and pwm_the_enable_pin=True be treated as an error. Passing enable=None and pwm_the_enable_pin=True should also be treated as an error.

And I'm too tired right now to think of a better name than pwm_the_enable_pin 😆

Yeah, that makes sense. How about pwm_enable?

pigrammer3 · 2024-05-30T11:38:11Z

I thought this felt familiar. Does it not work the same if you just use the enable pin on the Motor class?

That wouldn't work because the enable pin is a DigitalOutputDevice in the Motor class. @lurch's suggestion would make the Motor class optionally make enable a PWMOutputDevice, which would make it work. (Unless I misunderstood and that was also your suggestion).

lurch · 2024-05-30T12:05:12Z

And I'm too tired right now to think of a better name than pwm_the_enable_pin 😆

Yeah, that makes sense. How about pwm_enable?

I did consider that, but thought that people might confuse pwm=True and pwm_enable=True 😕
I guess enable_is_pwm might work? 🤔

I thought this felt familiar. Does it not work the same if you just use the enable pin on the Motor class?

That wouldn't work... (Unless I misunderstood...)

I think @bennuttall was suggesting that rather than having enable be PWM and forward & backward be Digital (which is what you're doing in this PR), have you tried having enable be Digital and forward and backward be PWM? (which is something the Motor() class already supports)
Or does the particular motor-driver chip you're using not support that mode of operation?

bennuttall · 2024-05-30T17:03:37Z

Yes I'm pretty sure that's where we ended up last time

pigrammer3 · 2024-05-30T22:46:53Z

I think @bennuttall was suggesting that rather than having enable be PWM and forward & backward be Digital (which is what you're doing in this PR), have you tried having enable be Digital and forward and backward be PWM? (which is something the Motor() class already supports)
Or does the particular motor-driver chip you're using not support that mode of operation?

I'm using the L298N motor driver, which while it does work with PWM on the forward & backward pins, is better with PWM on the enable pin (see #366 (comment)):

For 'Fast Stop' to be engaged, enable pin must be high and and the drive pins must be equal. Therefore if the drive pins are being PWMed, each cycle of the PWM the 'Fast Stop" feature will kick in. If PWM is on the enable pin the motors will 'free spin' between cycles.

In critical applications this may be important, but in terms of hobby electronics, I'm not sure how important it is. It might reduce the life cycle of the motor driver or the motor. L298 data sheet attached, refer to page 6 for 'Fast Stop' feature.
en.CD00000240.pdf

Also see page 4 of https://www.st.com/resource/en/application_note/CD00003776-.pdf:

The situation is different for bidirectional motors driven by a bridge. In this case the two alternatives have different effects. If the channel inputs are driven by the PWM signal, with suitable logic, the recirculation path is through a diode, the motor and a transistor (figure 7a), givind [sic] a slow decay. On the other hand, if the enable input is controlled the recirculation path is from ground to supply through two diodes and the winding. This path gives a faster decay (figure 7b).

lurch · 2024-05-31T13:35:17Z

        stopped.
        """
+        if hasattr(self, "enable_device"):
+            return (self.forward_device.value - self.backward_device.value) * self.enable_device.value


Hah, clever 😉

lurch · 2024-05-31T13:59:00Z

Should the stop() method also do

    if isinstance(self.enable_device, PWMOutputDevice):
        self.enable_device.value = 1

after turning off the forward and backward pins, in order to activate the 'Fast Stop' feature you mentioned?

Thanks for adding such comprehensive tests 👍

pigrammer3 · 2024-05-31T14:44:54Z

Should the stop() method also do
    if isinstance(self.enable_device, PWMOutputDevice):
        self.enable_device.value = 1
after turning off the forward and backward pins, in order to activate the 'Fast Stop' feature you mentioned?

Good point, I did that.

lurch · 2024-05-31T15:31:16Z

                    'backward speed must be 0 or 1 with non-PWM Motors')
        self.forward_device.off()
-        if isinstance(self.enable_device, PWMOutputDevice):
+        if hasattr(self, "enable_device") and isinstance(self.enable_device, PWMOutputDevice):


I wonder if it'd be worth having a self.enable_is_pwm boolean attribute and checking that, rather than all of these hasattr(...) and isinstance(...) tests? 🤷‍♂️

how about self.control_type, an enum that can be NO_PWM, PWM, NO_PWM_WITH_ENABLE, PWM_WITH_ENABLE, or ENABLE_IS_PWM? that way no isinstance checks have to be done at all?

Sounds good. But I wonder if DIGITAL, PWM, DIGITAL_WITH_DIGITAL_ENABLE, PWM_WITH_DIGITAL_ENABLE, DIGITAL_WITH_PWM_ENABLE is a slightly clearer set of names?

I've done this and run unit tests. Thanks for all the feedback.

…trol modes

pigrammer3 added 3 commits May 28, 2024 17:09

Add speed/direction motor, see gpiozero#300 and gpiozero#330

8be6c85

Add SpeedDirectionMotor to docs

541ef2b

Add tests for SpeedDirectionMotor

af3d3a5

lurch reviewed May 30, 2024

View reviewed changes

Move speed/direction motor behavior into Motor class

ccb33e3