RobTillaart / DHTNew

Arduino library for DHT11 and DHT22 with automatic sensor recognition
MIT License
98 stars 15 forks source link

general questions #3

Closed Mr-HaleYa closed 4 years ago

Mr-HaleYa commented 4 years ago

I would like to work on and improve this because I want to use it so I want to assure it is reliable. I would like to use a DHT22 but I'm starting with a DHT11 since currently, that's all I have.

When defining a type getType(); in the .h does not seem to do anything or refer to any functions. did you remove this? and if so how can a type be defined? will it still work with the 22 33 since it appears there is no way to define the type and it defaults to 11 12

You have the humidity readings floated but all humidity reading outputs are never decimals. Is this intentional?

In your examples, you call mySensor.humidity but the humidity should be private and instead, mySensor.getHumidity() should be called. same goes for temperature

I am just curious and have nothing to do during this "lockdown" and am offering to help if there is any way to do so.

RobTillaart commented 4 years ago

Hi Hale,

Help is always welcome,

If you look carefully you will see that getType returns _type. The implementation is in the .h file as it is so simple. This allows compilers to inline it. The value of _type is set by reading a sesnor, so before any read it just returns 0. I have found no way yet to discriminate automatically between the 22, 33 and 44. So I call them all 22 for now.

You can always make a pull request and if I agree on functionality and it is well tested etc I will merge it into the master.

Humidity for the DHT22 is a float as it has 1 decimal. To be able to cope with both (in fact 4++) types of DHT sensors it needs to be float.

I have been thinking of an integer only DHTNEW library as it would have a far smaller footprint and for most applications 1°C accuracy is enough.

Yes, the public members humidity and temperature are to become private, however I learned that you better publish the new interface before you remove the old one. But it is definitely on my list

So yes, please help to improve this library, maybe first a simple PR for the public -> private move. It would get things started.

Furthermore we could create the iDHTNEW library as a project. Keeping the interfaces the same would allow people to switch easily and all example sketches could be reused.

Regards, Rob

Mr-HaleYa commented 4 years ago

Great I would love to help!

so just to get it straight for the _type you fist call a read() and it goes to

  _type = 22;
  _wakeupDelay = DHTLIB_DHT_WAKEUP;
  int rv = _read();
  if (rv == DHTLIB_OK) return rv;

  _type = 11;
  _wakeupDelay = DHTLIB_DHT11_WAKEUP;
  rv = _read();
  if (rv == DHTLIB_OK) return rv;

  _type = 0; // retry next time
  return rv;

you start by defaulting to 22 then checking if the reply is good and if so you keep that as the type. if not you try the next type correct?

what are you meaning by a simple PR?

Regards, Hale

RobTillaart commented 4 years ago

PR stands for pull request. If you are not familiar with them please check youtube or google.

Mr-HaleYa commented 4 years ago

apologies I understand what a pull request is I had never seen it abbreviated before.

I am running the test sketch on an esp32 using the DHT11 and almost every line is a timeout error. is it failing to fast?

1. Type detection test, first run might take longer to determine type
STAT    HUMI    TEMP    TIME    TYPE
OK, 61.0,   25.8,   23227,  11
Time out error, -999.0, -999.0, 19099,  11
Time out error, -999.0, -999.0, 19099,  11
OK, 22.0,   25.8,   21875,  11

2. Humidity offset test
STAT    HUMI    TEMP    TIME    TYPE
Time out error, -999.0, -999.0, 19067,  11
Time out error, -999.0, -999.0, 19098,  11
OK, 16.5,   25.9,   21829,  11
Time out error, -999.0, -999.0, 19099,  11

3. Temperature offset test
STAT    HUMI    TEMP    TIME    TYPE
Time out error, -999.0, -999.0, 19067,  11
OK, 22.0,   31.3,   21876,  11
Time out error, -999.0, -999.0, 19099,  11
Time out error, -999.0, -999.0, 19099,  11

4. LastRead test
22.0,   25.8
22.0,   25.8
22.0,   25.8
22.0,   25.8
actual read
-999.0, -999.0
-999.0, -999.0
-999.0, -999.0
-999.0, -999.0
actual read
22.0,   25.9
22.0,   25.9
22.0,   25.9
22.0,   25.9
actual read
-999.0, -999.0
-999.0, -999.0
-999.0, -999.0
-999.0, -999.0
actual read
22.0,   25.9
22.0,   25.9
22.0,   25.9
22.0,   25.9
Mr-HaleYa commented 4 years ago

Maybe it would be beneficial if we were to add a delay function in the library that waits until the sensor is ready to take a reading to make sure it is ready. That way you couldn't "Spam" the function and get bad readings.

RobTillaart commented 4 years ago

ESP32 is a 3.3V device so you might need adjust the pull up resistors

Mr-HaleYa commented 4 years ago

I'm using a 3 pin DHT11 that is on a PCB with a 10k pull-up and it's connected to 5v, ground, and data pin. It's an esp32 board similar to a NodeMCU esp32 but built by TTGO

RobTillaart commented 4 years ago

Maybe it would be beneficial if we were to add a delay function in the library that waits until the sensor is ready to take a reading to make sure it is ready. That way you couldn't "Spam" the function and get bad readings.

I prefer no delay in my libs as these are blocking the execution of the code.
For that I added the lastRead() function so the user can wait if he wants.

RobTillaart commented 4 years ago

I'm using a 3 pin DHT11 that is on a PCB with a 10k pull-up and it's connected to 5v, ground, and data pin. It's an esp32 board similar to a NodeMCU esp32 but built by TTGO

Replace the resistor with a 3K3 or so, IIRC the pins are 3.3V, check the datasheet

Mr-HaleYa commented 4 years ago

your test function is calling readings to fast for the sensor. if I put a 2-sec delay between each call for readings it works as it's supposed to. If you look at my results it was giving me a reading every 3rd run. and you already had a delay for 500ms at the bottom of the code

Mr-HaleYa commented 4 years ago

I think a delay in the lib would be great if it where optional then the user could call it if they want but it is already in the library so its just needs to be defined as true

RobTillaart commented 4 years ago

Delays block the code and must be avoided in libraries. If people decide to use delays in sketches (and examples!) to get things working that is OK as it is their choice. Instead of a delay() users can decide to check IO light up leds or whatever. The responsiveness of libraries must be maximal. A delay of 2 seconds is an eternity in computer land.

What would be acceptable imho is using _lastRead. If the last read is less than 1 or 2 seconds just skip the read so people use the previous value. The skipping time is sensortype dependent, so at least 2 are needed.

RobTillaart commented 4 years ago

your test function is calling readings to fast for the sensor. if I put a 2-sec delay between each call for readings it works as it's supposed to. If you look at my results it was giving me a reading every 3rd run. and you already had a delay for 500ms at the bottom of the code

Sorry I misunderstood, the examples use delay(500), that could become 2000ms for teh DHT11 indeed. I thought you proposed it for the core library files. Still it should be sensor dependent.

Mr-HaleYa commented 4 years ago

I believe that I have got it working. I #defined a wait time in the .h. In the .cpp at the start of read() I have added

int DHTNEW::read()
{
  if (millis() - _lastRead < DHTLIB_DHT_READ_DELAY ) return DHTLIB_OK;
  if (_type != 0) return _read();

I had it being tested with some serial.print lines to tell when a reading was or wasn't taken

int DHTNEW::read()
{
  if (millis() - _lastRead < DHTLIB_DHT_READ_DELAY )
  {
    Serial.println("reading not taken");
    return DHTLIB_OK;
  }
  Serial.println("reading taken");

I changed the delay in the test.ino to the original 250 that was giving timeouts and my output was

1. Type detection test, first run might take longer to determine type
STAT    HUMI    TEMP    TIME    TYPE
reading taken
OK, 22.0,   26.8,   24702,  11
reading not taken
OK, 22.0,   26.8,   18, 11
reading not taken
OK, 22.0,   26.8,   17, 11
reading not taken
OK, 22.0,   26.8,   17, 11

2. Humidity offset test
STAT    HUMI    TEMP    TIME    TYPE
reading not taken
OK, 22.0,   26.8,   17, 11
reading not taken
OK, 22.0,   26.8,   17, 11
reading not taken
OK, 22.0,   26.8,   17, 11
reading not taken
OK, 22.0,   26.8,   17, 11

3. Temperature offset test
STAT    HUMI    TEMP    TIME    TYPE
reading taken
OK, 22.0,   29.0,   21842,  11
reading not taken
OK, 22.0,   29.0,   17, 11
reading not taken
OK, 22.0,   29.0,   17, 11
reading not taken
OK, 22.0,   29.0,   17, 11

4. Get LastRead test
22.0,   29.0
22.0,   29.0
22.0,   29.0
22.0,   29.0
reading taken
-------------
Taking reading
22.0,   26.5
-------------
22.0,   26.5
22.0,   26.5
22.0,   26.5
22.0,   26.5
22.0,   26.5
22.0,   26.5
22.0,   26.5
22.0,   26.5
reading taken
-------------
Taking reading
22.0,   26.5
-------------
22.0,   26.5
22.0,   26.5
22.0,   26.5
22.0,   26.5
22.0,   26.5
22.0,   26.5
22.0,   26.5
22.0,   26.5

Done...
RobTillaart commented 4 years ago

Thats the way it should be done however there are 2 different times so it needs fine-tuning

Mr-HaleYa commented 4 years ago

two different times?

RobTillaart commented 4 years ago

The time for a DHT11 to recover might be different than for a DHT22. Need to verify that in the datasheet.

Mr-HaleYa commented 4 years ago

yes I thought of that after I made the pull req, apologies

Mr-HaleYa commented 4 years ago

I'm having problems with the 1-second delay on the DHT11 tho... it keeps giving me a timeout on the sensor read unless I have the delay at 2 sec... I think maybe the delay caused by the function actually taking the read is not being taken into account with the _lastread.

After some testing, this looks like it is the case. The _lastRead = millis(); needs to be moved to return the time after the read has been taken otherwise the whole thing is thrown off... Is there any reason it is where it is? it should be as simple as moving it down 3 lines

Mr-HaleYa commented 4 years ago

I was able to get it working by moving the _lastRead and increasing the DHTLIB_DHT11_READ_DELAY to 1250 to account for the small things happening and each sensor is slightly different in capture time .96~1.2 sec from what I found. I also added a new function that can force the sketch to wait for a reading but it is default for false. It is working really well now and I have updated my pull req

RobTillaart commented 4 years ago

In your examples, you call mySensor.humidity but the humidity should be private and instead, mySensor.getHumidity() should be called. same goes for temperature

Just to come back to this one, as making these two fields is breaking the interface it would bring the version to 0.2.0 The examples could already be updated to use the getTemperature() and getHumidity() calls

Mr-HaleYa commented 4 years ago

if you agree with my changes to .h and .cpp I can go through and change over all the examples so you can release 2.0.

RobTillaart commented 4 years ago

if you agree with my changes to .h and .cpp I can go through and change over all the examples so you can release 2.0.

The update of the examples should be a separate pull request as that enhancement is independent of the "spam filtering" discussion

Mr-HaleYa commented 4 years ago

ok, I will revert back the test.ino to stick with the old interface until we have all examples on the new interface. I will do that on a different branch

RobTillaart commented 4 years ago

if you agree with my changes to .h and .cpp I can go through and change over all the examples so you can release 2.0.

The update of the examples should be a separate pull request as that enhancement is independent of the "spam filtering" discussion

Examples updated to use the functions for temp & hum

Mr-HaleYa commented 4 years ago

lol well, that was an easy fix. What else is there to do with the lib that isn't already done?

Now that all the examples are using the same function instead of the float value can you make that value private and release 2.0?

RobTillaart commented 4 years ago

Yes, it could. Maybe i have some time today or next week. I am moving all my libs to separate repos and have at least 30 to go that takes time and has more priority. Furthermore I have a business to run, seems that takes time too :)

RobTillaart commented 4 years ago

As the issues discussed are fixed in 0.1.7 I close this issue. for the open points I made separate issues

yesyesuk commented 4 years ago

Hi. I know this is closed but I hope you still read this.

Regarding an integer-only version, would you consider implementing a "scale factor" (for lack of a better term)? For example I would specify a factor of 10 (or rather 0.1 but that's not an integer) and the library would report 12.5degC as 125. This way you still get 0.1 degree precision but it's still integer.

RobTillaart commented 4 years ago

That is one way to do it, other options could be

0) integer field only, 9 bit res. Would mean always a fast sensor read.

1) integer value as is + a separate decimal field 0..15 as that is what the sensor gives.

2) variation on 1 with decimal field 0..9 . Would imply some rounding but perfectly doable.

3) integer value and use the internal decimal part for rounding only. No float math involved but would be accurate up to 0.5 degree. Very similar to 0

Your idea would be (2) with different representation. In fact one could add just another access function: Int getDeciCelcius() and Int getDeciHumifity()