-
Notifications
You must be signed in to change notification settings - Fork 593
Fix for issue #270 #276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for issue #270 #276
Conversation
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.
Changed interrupt-based sensors to only copy volatile variables during interrupt-blocking sections, and do computations with them after re-enabling interrupts.
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. |
Looking into it a little bit I would say interrupts are normally delayed rather than just cancelled. To check the state of the interrupt flag, there could be options outside of Arduino framework. Thinking about whether it is needed:
|
@askuric do you still have reservations regarding the encoder? If not I'd like to merge it and have a chance to test it on the dev branch. |
Hey guys, I am OK with these changes, the influence to the execution should be minimal. However, we'll need to test thoroughly. :D Thanks again @dekutree64! |
Then lets merge it :-) |
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.
Sensor: Added friend class declaration for SmoothingSensor. Alternatively we could make prev_angle_ts public, and I could use getMechanicalAngle() and getFullRotations() to access the other state variables.