Max31855: stop using SPI from ISR #891

Open
wants to merge 2 commits into
from

Projects

None yet

5 participants

@greyltc
greyltc commented Mar 31, 2016

This addresses #711 by doing SPI comms to the Max31855 thermocouple ADC in on_idle() instead of in code called from an ISR.

@greyltc greyltc solve Smoothieware#711
first crack

second crack

reduce ram usage

rate limit max31855 SPI reads

fix read_flag bug

fix formatting
edd99f3
@greyltc
greyltc commented Mar 31, 2016

I've tested this PR in my setup (with Max31855 temperature sensing).

It should definitely not be merged until someone has tested it with thermistor-based temperature sensing. Could someone else help with that please?

@hakalan
Contributor
hakalan commented Mar 31, 2016

Hi!
Isn't there a synchronization issue between ringbuffer accesses from isr and on_idle?
The timer interrupt can occur while on_idle is modifying the ring buffer, leading to incorrect buffer access.

@triffid
Contributor
triffid commented Mar 31, 2016

The advantage of ringbuffers is that they're immune to concurrent access
problems if used correctly, as long as writing the index pointers is atomic
On 31 Mar 2016 8:58 pm, "Håkan Langemark" notifications@github.com wrote:

Hi!
Isn't there a synchronization issue between ringbuffer accesses from isr
and on_idle?
The timer interrupt can occur while on_idle is modifying the ring buffer,
leading to incorrect buffer access.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#891 (comment)

@greyltc
greyltc commented Mar 31, 2016

About the ring buffer: How important is it to have the moving average that the ring buffer gives us?
I was thinking about removing averaging here completely (in another PR).
I suspect it might not matter anyway since I think @wolfmanjm plans to redo all that using adc like for the thermistor.

@hakalan
Contributor
hakalan commented Mar 31, 2016

Ok, I accept that the ringbuffer access is safe if you say so. :)

The reason I added a moving average in the first place was to try to limit the effect of the 0.25 degree quantization of the Max31855 readouts. Since the PID regulator is updated often but does not filter the signal (at least not when I wrote the thermocouple code in the first place, the situation might have changed?) the derivative term became too unstable and made the autotuning fail unless some smoothing was applied.
So this should probably be solved in PID algorithm instead.

I don't use my Smoothie anymore, otherwise I'd be glad to help...

@wolfmanjm wolfmanjm commented on an outdated diff Mar 31, 2016
src/modules/tools/temperaturecontrol/max31855.h
@@ -21,9 +21,10 @@ class Max31855 : public TempSensor
~Max31855();
void UpdateConfig(uint16_t module_checksum, uint16_t name_checksum);
float get_temperature();
+ void on_idle();
+ struct { bool read_flag:1; } ; //when true, the next call to on_idle will read a new temperature value
@wolfmanjm
wolfmanjm Mar 31, 2016 Contributor

this needs to be moved to private.

@arthurwolf
Contributor

Anybody want to test this ?

@tkurbad tkurbad added a commit to tkurbad/Smoothieware that referenced this pull request Nov 11, 2016
@tkurbad tkurbad Merged patch from PR #891 to enable two thermocouples on the same SPI…
… bus
0f6bf42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment