Only reset port settings for motor on first start()

[mutesplash]

This should mostly solve Issue #122

[OliverFaust]

That is a first order filter.
In a scenario with a constantly changing speed value, this code will fail to reduce the message rate. Therefore, the interface must sill be able to sustain the full message speed in order to rule out a random failure.

[mutesplash]

True, but but the Pi is fast compared to the serial port and you're going to need/want all the bandwidth you can get out of the serial port in your scenario. This is seems kind of cheap.

[OliverFaust]

Agreed, in this case there might be a minor benefit.

[OliverFaust]

That is the solution.

[OliverFaust]

Thank you, the patch works.

If this is transferred to main branch, you might want to consider extending the functionality with the following update_speed function in the Motor class:

    def update_speed(self, speed=None):
        """Update motor speed

        :param speed: Speed ranging from -100 to 100
        """
        if self.runmode == Motor.runmodes['Free']:
            if self.current_freerun_speed == speed:
                # Already running at this speed, do nothing
                return
        elif self.runmode != Motor.runmodes['None']:
       	    # Motor is running some other mode, wait for it to stop or stop() it yourself
       	    return

        if speed is None:
            speed = self.default_speed
        else:
            if not (speed >= -100 and speed <= 100):
                raise MotorException("Invalid Speed")
        cmd = "port {} ; set {}\r".format(self.port, speed)
        if self.runmode == Motor.runmodes['None']:
            raise MotorException("Motor not started")
        self.runmode = Motor.runmodes['Free']
        self.current_freerun_speed = speed
        self._write(cmd)

And the corresponding extension for the Pair class:

    def update_speed(self, speedl=None, speedr=None):
        """Start motors

        :param speed: Speed ranging from -100 to 100
        """
        if speedl is None:
            speedl = self.default_speed
        if speedr is None:
            speedr = self.default_speed
        self._leftmotor.update_speed(speedl)
        self._rightmotor.update_speed(speedr)

[mutesplash]

update_speed() would mostly duplicate start() and only save the elif branch from executing as you would just bail out if it weren't already running. Not sure if it's worth it even if you make start() call update_speed() to de-duplicate, as then it would make sense to require start() before update_speed() and then repeated start() calls would assume a re-initialization and then we'd be back to looping start() makes the motor serial line crash.

[OliverFaust]

From a technical point of view I agree with that assessment.
Once this functionality is transferred to the main branch, you might want to include an example which calls the start function in a hard loop. That might help users to create robot functionality.

[chrisruk]

Thanks a lot for this pull request @mutesplash, I've just been testing it with alternating between a speed of 0 and 1, and it seemed to work great :). I'll also add an extra test case soon for the motors that tests for this issue that your pull request fixes.

Do you mind altering this pull req. so that we use an enum for runmodes (I just checked enum was introduced in 3.4 and the minimum python version we're supporting is 3.7).

Edit: Also if possible could the ramp deduplication possibly go in a separate pull request, as that seems to be a separate improvement, and would probably make it a bit easier for me to merge this pull request first, then spend time separately reviewing that

[mutesplash]

There we go