Improve reliability, especially at low clock speed

tstarling commented 9 years ago

I found that the library was not working reliably with my 8 MHz Seeeduino Stalker. Some investigation with an oscilloscope showed that the input polling loop was quite slow (maybe 8us), and timer interrupts were taking ~10us. This meant that the polling loop commonly missed the first sensor-driven transition after pullup, and any transitions which occur close to timer interrupts. So:

Use pulseIn() instead of our own loop. This is a little bit faster than what was done before, is calibrated correctly, and could potentially be optimised upstream to provide better resolution in future. It makes the code a bit simpler and avoids the need to watch for the first sensor pulldown.
Disable interrupts during pulseIn(), to provide accurate timing.
Disabling interrupts, and the fact that I was using sleep mode, meant that millis() was not accurate. So instead of depending on millis() to determine whether to do a sensor read, require the caller to explicitly trigger a sensor read, and rely on the caller to wait the requisite sampling period. This means removing resetTimer() and making readSensor() public.

Also:

Use INPUT_PULLUP, which was introduced in Arduino 1.0.1.

Tested on DHT11 and RHT03 (like DHT22) in autodetect mode. Gathered data from the RHT03 for 6 days, 1800 samples, with no timeout or checksum errors.

markruys commented 9 years ago

Looks good, disabling interrupts to get a more precise timing. Your changes alter the core of the original code, so I want to test it on the hardware I have too before merging it. A pity that because you drop lastReadTime, you now have to explicitly call readSensor() before you call getHumidity() or getTemperature(). This will break existing code from others who use this library.

tstarling commented 9 years ago

I can split it up if you only want to merge part of it. It will more or less work with the old lastReadTime system, it's just that the sampling interval will be a few milliseconds longer due to the missed timer interrupts. And of course it won't work for me, since I am using sleep mode. In my application, the RTC wakes up the CPU every 5 minutes, with only a few milliseconds added to millis() each time. Explicitly triggering a read seems like a good policy, it is used by a lot of other sensor libraries, but if you really need backwards compatibility then I can fork instead.

Alternatively you could make a branch in your repo for the legacy interface, and merge the full patch into master.

matthijskooijman commented 9 years ago

I wonder if disabling interrupts is a good idea? Having interrupts disabled for a few ms isn't usually a good idea, one of the selling points of this particular DHT library is IMHO that it works with interrupts enabled. Of course, having interrupts disabled makes the readings sensitive to being interrupted. However, the difference between a short and long pulse is 68-30 uS, buying 48uS of leeway. Assuming that a digitalRead takes 10uS (measured on 8Mhz pro mini), that leaves 38uS of interrupt time.

I was going to argue that changing the low-high threshold from > 30 to, say, > 60 would drastically reduce the chance of interrupts messing up the readings. I tried using a bit of code like below to force the race condition to occur:

--- a/DHT.cpp
+++ b/DHT.cpp
@@ -134,6 +134,9 @@ void DHT::readSensor()
     delayMicroseconds(800);
   }

+  while(TCNT0 > 222) /* wait */;
+  while(TCNT0 < 222) /* wait */;
+
   pinMode(pin, INPUT);
   digitalWrite(pin, HIGH); // Switch bus to receive data

This didn't work as well as I'd expected. Additionally, raising the threshold didn't work either. Looking more closely at the code, I noticed there is a big chance that an interrupt happens between setting age and reading the pin, which would actually cause age to be an underestimate (there's also a smaller chance of the interrupt happening just after the reading, which, if the end of the pulse is also imminent, causes an over-estimation).

I think that some fiddlery with disabling interrupts for a part of the loop (to force interrupts to happen either before or after reading the pin) could help to ensure things are either underestimated or overestimated (though the slow-ness of digitalRead() can still swing both ways I think). I don't have any further time to investigate now (I really should be getting my actual work done), though.

Thinking on this more closely, I realize that ensuring over- or under- estimaton only helps if you apply the inverse on the start of pulse detection (e.g. if you make sure that interrupts always cause over-estimation of the timestamp of both the start and end edges, the end result can still be both higher and lower).

So, perhaps disabling interrupts is the right approach after all... If you do, I think using the pulseIn() function is a smart approach, since it contains very tight architecture-optimized code, which should make it accurate and portable.

A pity that because you drop lastReadTime, you now have to explicitly call readSensor() before you call getHumidity() or getTemperature(). This will break existing code from others who use this library.

Is this really needed? Disabling interrupts prevents millis() from counting, but a complete read shouldn't take more than a few ms?

markruys / arduino-DHT

Improve reliability, especially at low clock speed #3