Refactoring interrupt-based sensor code

[dekutree64]

When working on my experimental SmoothingSensor class, I became aware that many of the SimpleFOC sensor classes can give sporadic bad readings due to interrupts occurring in the middle of operations. And to make matters worse, the Arduino library does not provide any way to temporarily disable interrupts and then restore to the previous state (noInterrupts() does not return the previous state).

Note: I’m simultaneously working on getting more accurate timestamps for angle prediction by SmoothingSensor.

My proposed solution for HallSensor is to do away with all the specialized get functions except for getSensorAngle, so everything accesses the Sensor base class state variables except during update(), which is only ever called at a time when interrupts are enabled so I can call the disable/enable functions without worrying about the previous state. One option would be to make a specialized update() that blocks interrupts, like this:

void HallSensor::update() {
  noInterrupts();
  Sensor::update();
  angle_prev_ts = pulse_timestamp;
  interrupts();
}

So the call to HallSensor::getSensorAngle inside Sensor::update is protected from interrupts modifying electric_sector after electric_rotations is loaded from memory, and the timestamp is also guaranteed to be the correct one for the angle reading.

But due to the fact that two other sensors (MagneticSensorPWM and Encoder) need similar treatment, I think it would be better to move pulse_timestamp to the base class, and do like this:

void Sensor::update() {
    noInterrupts();
    float val = getSensorAngle();
    angle_prev_ts = _isset(pulse_timestamp) ? pulse_timestamp : _micros();
    interrupts();
    float d_angle = val - angle_prev;
    // if overflow happened track it as full rotation
    if(abs(d_angle) > (0.8f*_2PI) ) full_rotations += ( d_angle > 0 ) ? -1 : 1; 
    angle_prev = val;
}

So if the subclass uses pulse_timestamp, it is transferred to angle_prev_ts at the same time as the other state variables are updated, else _micros() is called as before.

But it’s a little iffy because it means you can’t use interrupt-based communication methods inside getSensorAngle. I don’t think the Arduino I2C and SPI libraries use interrupts, but it’s still something that could potentially cause trouble in the future, so I would add a note about it in the comment description of getSensorAngle.

With this approach, MagneticSensorPWM would only need this small change to handlePWM:

    if (!digitalRead(pinPWM)) {
        pulse_length_us = now_us - last_call_us;
        pulse_timestamp = last_call_us;
    }

That way the timestamp is set to the rising edge of the pulse, when the angle was sampled. It takes a good while to communicate it via PWM, but SmoothingSensor will still predict the true angle thanks to the accurate timestamp.

There is a slight issue with the non-interrupt mode of this class, that getSensorAngle() would be called with interrupts disabled, and it has a long blocking time which could potentially result in missed interrupts from other sources. But that blocking time is a problem for motor performance in general, so one option would be to remove the non-interrupt mode entirely.

The Encoder class mainly has interrupt trouble in the getVelocity function, due to accessing pulse_timestamp and pulse_counter. The other get functions only use pulse_counter, so on 32-bit processors loading it will be an atomic operation. However on 8-bit, you could potentially get the low bytes of one update and high bytes of another, and in extremely rare cases that would give the wrong value. So I would recommend removing the specialized get functions of this class as well.

I think the specialized getVelocity functions of HallSensor and Encoder can simply be removed and let the base Sensor::getVelocity handle it. HallSensor will lose the max_velocity check, but that was most likely patching one symptom of the greater problem being solved here. And Encoder may actually get a less noisy signal since there could be multiple ticks between updates, and this will calculate velocity based on all ticks since last update using the precise timestamps. The current code calculates based only on the most recent tick interval, which gives a more accurate instantaneous velocity, but that's not what we want since we're low-pass filtering the result.

If we do still want to allow specialized getVelocity functions, one option would be to make getVelocity non-virtual and just return the Sensor::velocity variable, and add a virtual getSensorVelocity that’s called inside the critical section of Sensor::update. But I think it will be fine to force all sensors to use the base class version.

[runger1101001]

Hi Deku!

I've a strong preference to keep the interrupt disabling out of the parent class - since not all the sensors need it. In the drivers library we have many more sensors that are like MagneticSensorSPI, and then there is the GenericSensor, where we can't say at all what the implementation is doing.

So in that sense I much prefer your first suggestion, where the update() method gets an override...

[dekutree64]

Copy-pasta it is, then! That will also allow me to skip disabling interrupts for MagneticSensorPWM if it's doing the long blocking wait.

It looks like I'll have to add interrupt disabling to the specialized getVelocity functions after all, because HallSensor also needs its check to return zero if it's been too long since the last pulse (although I will change it to last_pulse_diff*2, because as it is, any deceleration causes a bunch of inappropriate zero velocity reports).

Encoder::getVelocity should probably have a similar check to return zero if it's been too long since the last pulse. It has a sanity check to return zero if it's been more than half a second, but that's a long time to continue reporting a wrong velocity. EDIT: Reading more carefully, it later has a check for 100ms as well. I think the HallSensor approach of comparing against the last pulse interval would be better than a fixed value, but I'll leave it alone for now.

[runger1101001]

Hey, I'm just going to re-open this if its ok - we like to close the issues when we have released the code to a release version of the library...