getType() detects sensor type

[budulinek]

getType() tries to detect sensor type if _type == 0 we can therefore simplify read()

[RobTillaart]

Will not work correctly as _readDelay must be set before _read() is called. As _read() is called in getType() it can fail as _readDelay can still be zero.

[budulinek]

getType() does not call read(). It calls _read() directly.

And you do not need _readDelay to be set before calling _read().

All I did was I moved your code for detecting sensor type (by calling _read) from read() to getType(). Calling _read() is the only way how to get _type. Since _readDelay depends on _type, the first call to _read() will always be with _readDelay set to zero.

But as I said, that is not a problem since you do not need _readDelay for _read()

[RobTillaart]

Sorry, you are right.

---

DHTLIB_ERROR_SENSOR_NOT_READY is an error code created for a specific error in the handshake with the sensor if the sensor does not "come online" in time.

I propose to create a unique error code if getType() fails, as that keeps debugging easier.

int DHTNEW::read()
{   
  if (getType() == 0) return DHTLIB_ERROR_TYPE_DETECT;   //   or DHTLIB_ERROR_GETTYPE ?

• the new error code will affect the examples
• the new error code must be added to keywords.txt + DHTLIB_WAITING_FOR_READ too

[budulinek]

New error code is not necessary, I think. In the end, "error type detect" in reality means that "sensor is not responding".

Or, you now what, you can pass the original error code from _read() to getType() and then to read(). Through _type. Valid type has only pozitive values, so we can use negative values of _type for passing original error codes. Just change uint8_t DHTNEW::getType() to int DHTNEW::getType() and then do _type = rv in case of error....

[budulinek]

What do you think? Too crazy to pass error codes via _type?

EDIT: Please forget about the two commits above. I think we should stick with DHTLIB_ERROR_SENSOR_NOT_READY as the only possible error leading to getType() failing. See my comments bellow.

[budulinek]

If I understand it correctly, the detection of DHT11 and DHT22 is based on different values of PULL LOW timeout timeouts. From your comments in _readSensor():

// DHT22 // SENSOR PULLS LOW after 20-40 // DHT11 // SENSOR PULLS LOW after 6000-10000 us

Error in this step leads to DHTLIB_ERROR_SENSOR_NOT_READY. So the presence or absence of DHTLIB_ERROR_SENSOR_NOT_READY is decisive for our decision about sensor type. Even if other errors occure on the subsequent steps (DHTLIB_ERROR_TIMEOUT_A, B, C, D) we already have a valid type.

If we agree on that, we can actually do:

uint8_t DHTNEW::getType()
{
  if (_type == 0)
  {
    _type = 22;
    _wakeupDelay = DHTLIB_DHT_WAKEUP;
    if (_read() != DHTLIB_ERROR_SENSOR_NOT_READY) return _type;

    _type = 11;
    _wakeupDelay = DHTLIB_DHT11_WAKEUP;
    if (_read() != DHTLIB_ERROR_SENSOR_NOT_READY) return _type;
    _type = 0; // retry next time
  }
  return _type;  
}

[budulinek]

And then in read():

if (getType() == 0) return DHTLIB_ERROR_SENSOR_NOT_READY;

Because we now that getType(); can only fail because of DHTLIB_ERROR_SENSOR_NOT_READY

[RobTillaart]

Because we now that getType(); can only fail because of DHTLIB_ERROR_SENSOR_NOT_READY

_read() can have 7 different errors during the handshake etc, so getType() can too

Adding the unique error code gives a much faster way to debug if code fails. Passing the error through _type gives interference of functionality, and this will making the code harder to debug.

[budulinek]

_read() can have 7 different errors during the handshake etc

Yes, but only the handshake phase of _read() is relevant for type detection. Error during handshake phase leads to _read() returning DHTLIB_ERROR_SENSOR_NOT_READY. That is why I think that getType() should return 0 only in case of DHTLIB_ERROR_SENSOR_NOT_READY (and return 11 or 22 in case of other error codes). So DHTLIB_ERROR_TYPE_DETECT == DHTLIB_ERROR_SENSOR_NOT_READY.

[budulinek]

OK, so lets solve the first question first. When is it OK for getType() to return 11 or 22? Is the absence of DHTLIB_ERROR_SENSOR_NOT_READY sufficient and necessary condition for getType() returning 11 or 22?

Or do you want getType() to return 11 or 22 only if the whole _read() sequence is ok, ( i.e. _read() returns DHTLIB_OK)?

[RobTillaart]

Or do you want getType() to return 11 or 22 only if the whole _read() sequence is ok, ( i.e. _read() returns DHTLIB_OK)?

It has always been when the whole sequence is OK as that is the strongest conjecture that the type detected is working

[budulinek]

OK, I accept that. In that case it makes sense to create DHTLIB_ERROR_TYPE_DETECT.

So we will have (correct me if I am wrong): DHTLIB_ERROR_TYPE_DETECT: Sensor not connected or unknown sensor (type can not be detected) DHTLIB_WAITING_FOR_READ: Sensor detected (or type specified manually via setType()), we are waiting for read to finish DHTLIB_ERROR_SENSOR_NOT_READY: Sensor detected (or type specified manually via setType()), readDelay passed but sensor still not responding. Either wrong type specified manually or _readDelay too short.

[RobTillaart]

OK, I accept that. In that case it makes sense to create DHTLIB_ERROR_TYPE_DETECT.

So we will have (correct me if I am wrong): DHTLIB_ERROR_TYPE_DETECT: Sensor not connected or unknown sensor (type can not be detected) DHTLIB_WAITING_FOR_READ: Sensor detected (or type specified manually via setType()), we are waiting for read to finish DHTLIB_ERROR_SENSOR_NOT_READY: Sensor detected (or type specified manually via setType()), readDelay passed but sensor still not responding. Either wrong type specified manually or _readDelay too short.

DHTLIB_WAITING_FOR_READ: _waitForRead flag is set and time passed since last read is smaller than _readDelay of the sensor. ==> action, retry later when enough time has passed

DHTLIB_ERROR_SENSOR_NOT_READY: sensor does not react on initial part of the handshake, the microprocessor in the DHT sensor itself has not been woken by the wake up pulse (see diagram in datasheets)

DHTLIB_ERROR_TYPE_DETECT: sensor could not be read correctly during type detection.

[budulinek]

DHTLIB_WAITING_FOR_READ: _waitForRead flag is set and time passed since last read is smaller than _readDelay of the sensor.

Just a small correction. DHTLIB_WAITING_FOR_READ is returned only when _waitForRead is NOT set and and time passed since last read is smaller than _readDelay. When _waitForRead is set, read() waits in the while loop and returns (OK or error) only when _readDelay is over.

---

I am still thinking about how to debug getType(). This function fails (returns zero) on any error. If that happens, read() returns DHTLIB_ERROR_TYPE_DETECT, telling us that getType() failed. However, we will never learn which of the 7 possible errors caused the failure. getType() will not tell us. read() will also keep returning DHTLIB_ERROR_TYPE_DETECT. It will never get a chance to return _read();

Imagine a persistent checksum error, for example. getType() fails, read() will keep returning DHTLIB_ERROR_TYPE_DETECT and we will never see read() returning DHTLIB_ERROR_CHECKSUM.

A possible solution is: getType() will update _type, but return error codes received from _read(), instead of returning _type. read() would still keep returning DHTLIB_ERROR_TYPE_DETECT, but getType() would be able to tell us more detailed error ccode. But that would be a serious break of interface (because at the moment getType() returns _type).

Alternative solution:

• leave getType() as it is now (simply return _type)
• do the automatic detection of sensor type in completely new function. This new function would update _type and return error codes which can be used for debugging.

[budulinek]

OK, I will start by apologizing... Sorry for spamming you with just another proposal. I have second thoughts about my original idea of moving sensor type detection code away from read() function. It created more problems than it solved. My final proposal is very simple. Leave read() as it is:

int DHTNEW::read()
{
  if (_readDelay == 0)
  {
    _readDelay = DHTLIB_DHT22_READ_DELAY;
    if (_type == 11) _readDelay = DHTLIB_DHT11_READ_DELAY;
  }
  if (_type != 0)
  {
    while (millis() - _lastRead < _readDelay)
    {
      if (!_waitForRead) return DHTLIB_WAITING_FOR_READ;
      yield();
    }
    return _read();
  }

  _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;
}

And do a very simple getType() function:

uint8_t DHTNEW::getType()
{
  if (_type == 0) read();
  return _type;
}

getType() calls the read() function only when _type is zero. Which means read() will not do any while loop (i.e. no delay), it will try to detect sensor type and update _type if successfull. getType() will return _type.

CONS:

• no new error code DHTLIB_ERROR_TYPE_DETECT

PROS:

• no new error code DHTLIB_ERROR_TYPE_DETECT blocking all other (more detailed) error returns from read()
• getType() attempts to detects sensor type if _type == 0

I promise, this is my final proposal, take it or leave it. I am considering this PR closed, no more comments from my side...

Good night.

[RobTillaart]

OK, I will start by apologizing... Sorry for spamming you with just another proposal.

No need to apologize, these discussions are good to improve the library. Not all proposals will make it and in the end I can agree with your conclusion that the solution explored added more complexity than it solved.

Your "final" proposal improves the getType() function and the user still can expect one of 3 values so the behavior is similar except it is taking more time. I will review later today as it is not yet in your branch.

---

hint by adding the text cpp behind the three backquotes, code will have syntax high light.

[RobTillaart]

@budulinek I do not see your final proposal in the code yet...

[RobTillaart]

Thanks, I'll review them now.

[budulinek]

sorry, my repo was a bit messy, I opened new PR

[budulinek]

There is new PR for getType(), so can I close this?