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

Remove unneeded delay in hspiread32 #14

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@matthijskooijman

matthijskooijman commented Aug 3, 2016

This reduces the time for a typical readCelcius call from over a
millisecond to a couple of dozen microseconds, making the library much
more suited for use in timing-critical applications and control loops.

This delay was present between the lowering of the chip select line and
the first transfer. Looking at the MAX31855 datashee, it shows that the
time needed between the chip select drop and the first clock rise (tCSS)
is 100ns. This delay will be present due to function call overheads many
times over, so there is no need for an explicit delay.

Similar delays exist for software SPI, which are probably also not
needed, but I did not test this.

Remove unneeded delay in hspiread32
This reduces the time for a typical readCelcius call from over a
millisecond to a couple of dozen microseconds, making the library much
more suited for use in timing-critical applications and control loops.

This delay was present between the lowering of the chip select line and
the first transfer. Looking at the MAX31855 datashee, it shows that the
time needed between the chip select drop and the first clock rise (tCSS)
is 100ns. This delay will be present due to function call overheads many
times over, so there is no need for an explicit delay.

Similar delays exist for software SPI, which are probably also not
needed, but I did not test this.
@rswift

This comment has been minimized.

Show comment
Hide comment
@rswift

rswift Oct 7, 2016

I am using an ESP8266 with this library (the Adafruit Feather Huzzah, product 2821) and am using the software SPI interface (didn't engage noggin early enough, pins now soldered up). I have been looking at the performance of this function too. My testing shows that a call to readInternal() then readCelsius() was taking approximately 0.2 seconds, and very occasionally up to 0.8 seconds.

I've made a small change to add a readTemperatures() function that only makes a single spiread32() call, which is where the delay is. However, I've also tested the software SPI calls by replacing the delay(1) entries with delayMicroseconds(25) so by adding the new function, and reducing the delay, I'm now getting a very consistent ~7600 microsecond processing time for the function, and the time taken to read and linearise is now roughly 0.01 seconds... a tremendous improvement.

I will submit a pull request with my changes, hopefully they are appealing. But suffice to say, I second @matthijskooijman's proposed change.

rswift commented Oct 7, 2016

I am using an ESP8266 with this library (the Adafruit Feather Huzzah, product 2821) and am using the software SPI interface (didn't engage noggin early enough, pins now soldered up). I have been looking at the performance of this function too. My testing shows that a call to readInternal() then readCelsius() was taking approximately 0.2 seconds, and very occasionally up to 0.8 seconds.

I've made a small change to add a readTemperatures() function that only makes a single spiread32() call, which is where the delay is. However, I've also tested the software SPI calls by replacing the delay(1) entries with delayMicroseconds(25) so by adding the new function, and reducing the delay, I'm now getting a very consistent ~7600 microsecond processing time for the function, and the time taken to read and linearise is now roughly 0.01 seconds... a tremendous improvement.

I will submit a pull request with my changes, hopefully they are appealing. But suffice to say, I second @matthijskooijman's proposed change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment