Fix for issue #270

[dekutree64]

Overhaul of interrupt-based sensors to eliminate bad readings due to interrupts modifying variables in the middle of get functions.

All sensor classes are now required to use the base class state variables, and do their angle reading once per frame in the update() function rather than using the virtual get functions to return real-time updated readings. I have chosen to make an exception for getVelocity() and allow it to access volatile variables as well, although it may be better to merge it into update() instead. Interrupts should be disabled/enabled as necessary in the update() and getVelocity() functions, and preferably nowhere else. The Arduino library provides no mechanism to restore the previous state of interrupts, so use of the unconditional enable should be kept to as few locations as possible so we can be reasonably sure that it won't be called at a time when interrupts should remain disabled.

Additional changes to specific sensors:

HallSensor: Removed the max_velocity variable because it was a quick fix for bad velocity readings that were coming from this interrupt problem, which should no longer occur. Also changed the condition for "velocity too old" from (_micros() - pulse_timestamp) > pulse_diff to (_micros() - pulse_timestamp) > pulse_diff*2, because any deceleration would cause inappropriate reporting of zero velocity.

MagneticSensorPWM: Unrelated to the interrupt problem, timestamp is now saved from the rising edge of the PWM pulse because that's when the angle was sampled, and communicating it takes a significant and variable amount of time. This gives more accurate velocity calculations, and will allow extrapolating accurate angles using the new SmoothingSensor class.

[askuric]

Hey @dekutree64,

Thanks for this, I can see the benefits from this addition in terms of velocity smoothness for sure. I had no idea that noInterrupts exists in Arduino IDE .

Now, I am not sure that the noInterrupts and interrupts is the best approach when it comes to Encoder. Hall sensor is fine because it is absolute and even if we miss some interrupts at the first next one after re-enabling the interrupts we will no the exact position of the motor.

However the encoder is not like that, and disabling interrupts will make it lose counts (provided high enough velocity) . I am not sure what would be best solution here be though. Maybe disable the interrupts only for fetching the necessary volatile variables pulse_counter and pulse_timestamp and then enabling them for the rest of the velocity calculation.

I am not sure, but I am a bit worried that it could introduce issues for encoders which are arguably the most used sensors with the simplefoc. What are your experiences, what kind of benefits have you seen for encoders of disabling the interrupts? Smoother velocity, less jitters?

[dekutree64]

Excellent catch. For sure, Encoder should copy the volatile variables in a minimal duration blocking section and then re-enable before doing the computations. If you can't even copy two variables between interrupts, then it's on the verge of losing position anyway due to not being able to process the interrupts at the rate they're coming in.

I may go ahead and do it for HallSensor as well. It has to miss 3 interrupts in a row before it loses position, but it doesn't hurt to be careful.

I only have hardware to test the hall sensors, so Encoder and MagneticSensorPWM are only theoretically working at this point.

[runger1101001]

The Arduino library provides no mechanism to restore the previous state of interrupts, so use of the unconditional enable should be kept to as few locations as possible so we can be reasonably sure that it won't be called at a time when interrupts should remain disabled.

I think we have to think about this one carefully - can it even happen? Also what exactly happens - is the interrupt merely delayed, or is it skipped? I need to look into this, I guess. The reason is that interrupts are used for example also in the current sensing in some cases, or by the I2CCommander, and of course they might be used in other user code.

[dekutree64]

The re-enabling problem would mainly show up if we put disable/enable blocks in getAngle() and similar functions. Then if user code ever called them from inside an interrupt handler, it would allow nested interrupts which would likely crash the system. Or if they were called during an interrupt blocking section of another function, it would re-enable interrupts sooner than intended. That's less bad, but still can cause loss of motor control in some cases like the HallSensor::getVelocity() divide by zero we patched previously.

Or if you were referring to the missed encoder pulses, then yes, a single blocked interrupt will only be delayed, not missed. Each interrupt source has a bit in a register that gets set when it triggers, and will interrupt the CPU if interrupts are enabled. You can have multiple interrupts from different sources pending, and they will fire off in a pre-determined priority each time interrupts are re-enabled. But if the same interrupt triggers again while its bit is already set, then by the time it gets handled there's no way to tell that it had been set more than once. That's when you get missed encoder pulses, which would only be at very high speed and resolution, but still conceivable.

And it's good practice to minimize interrupt blocking time in general, so I went ahead and did it for HallSensor too. It also allowed reformatting getVelocity() back closer to how it was originally (not that it makes any functional difference).

P.S. Was it a mistake to close this pull request and open a new one after making the changes? This is my first time using GitHub, and in hindsight there was probably a way to edit this one to keep the conversation all in one place...

[runger1101001]

No its fine but I've moved the conversation to the active PR... these things happen because we were both commenting and doing stuff at the same time :-)