tacho-motor class - push all polarity handling down to the driver

[rhempel]

Currently, the tacho-motor class does some manipulations on values passed to or returned from the lower level legoev3_motor driver. With the addition of new drivers for the rPi and EVB capes, I think it might be a good idea to make the tacho-motor class do all of its ramping and other calculations using the values provided by the user, and have the low level driver do the polarity handling.

As an example, consider this function:

static ssize_t duty_cycle_show(struct device *dev, struct device_attribute *attr,
                   char *buf)
{
    struct tacho_motor_device *tm = to_tacho_motor(dev);
    int err, duty_cycle;

    if (!tm->ops->get_duty_cycle)
        return -EOPNOTSUPP;

    duty_cycle = tm->ops->get_duty_cycle(tm->context);

    if (tm->ops->get_command(tm->context) == TM_INTERNAL_COMMAND_RUN_REVERSE)
        duty_cycle *= -1;
    if (tm->active_params.polarity == DC_MOTOR_POLARITY_INVERSED)
        duty_cycle *= -1;

    return sprintf(buf, "%d\n", duty_cycle);
}

This is a by-product of our code passing the absolute value of the duty cycle to the driver along with an internal command to run the motor in the forward or revese direction. The driver already knows all of this information.

Instead, I think we should be passing signed values down to the drivers, and letting the driver handle the inversion based on the polarity value. Similarly, the driver should pass us up a correctly signed value when we ask for the current value of duty cycle.

I think this will make writing the non-LEGO motor drivers a bit simpler - and will correctly remove any dependencies on driver operations from the tacho-motor class.

[dlech]

Why would we want to do the same polarity calculations in 3 different drivers when we can just do it in one place? And before I go into my opinion on the matter, I should probably ask how is it implemented with the dc-motor class.

My way of thinking about it is that the lower level driver implementation is as close to the real hardware as possible. In most cases, the low level driver is going to have a standard kernel pwm device which always uses a positive duty cycle. The tacho motor class, on the other hand, is what translates the low level hardware representation to the userspace representation.

So, although it is a bit messy to have these checks all in the tacho motor class, I think it is better to figure it all out once and then forget about it rather than trying to have to sort out polarity in each new motor driver.

[rhempel]

The trick is to be absolutely clear and do the inversions in exactly one place. I think you are saying that the underlying driver should not care about polarity, and that's fine for DC motors where the duty-cycle is a positive value - but speed regulation in the tacho driver DOES need to deal with polarity - otherwise the Firgelli motors (for example) would never work.

By the way, does this mean that the Figelli motors will probably work on the EVB but not on the rPi capes?

[dlech]

and that's fine for DC motors where the duty-cycle is a positive value - but speed regulation in the tacho driver DOES need to deal with polarity

Agreed. The example you gave above was for duty cycle, not speed. Since motor ports can operate in the dc-motor mode in addition to the tacho-motor mode, I think the duty cycle polarity should still be handled in the tacho-motor class because the same duty cycle related callbacks are used in the dc-motor class. Speed related stuff is a different story.

By the way, does this mean that the Figelli motors will probably work on the EVB but not on the rPi capes?

Indeed.

[rhempel]

OK, duty cycle polarity in the class level driver, ie tacho-motor and dc-motor handle duty cycle as a positive value and use "internal" commands to tell the motor to turn forward or reverse. The low level motor driver will handle the encoder polarity for speed calculations separately.

Darn those Firgelli guys for getting the encoder backwards!

[dlech]

Just start selling a cable that has a couple of inverters inline.

[dlech]

This needs a re-think with the new PRU tachometer driver, so closing. We can open a new issue if there is anyone still interested in using Firgelli actuators.