Closed Mr-HaleYa closed 4 years ago
I changed the way that it checks for spam to be dependent on the sensor type and looked up the datasheet and it says that DHT11 is 1 sec and DHT22 is 2 sec so I added and fixed that.
The delay was to allow mills() to get above 2000 so that it would take a reading because it would boot and try and take a reading but the delay counter would say it is getting spammed because the device had not been on long enough. I fixed this by only introducing the anti-spam after the _type
has been defined.
The read delay does not look good to me.
we come in at line 33 type != 0 ==> line 35 evaluates false ==> line 46 evaluates true ==> it blocks in line 48-50 ==> it SKIPS line 59 ==> it sets the type to 22 and tries to read the sensor ==> that will fail and it will retry as type 11. ==> then it succeeds
Requirements
Think this are the main requirements (loosely formulated) can you agree on these requirements?
The move of _lastread = millis()
after the read is a correct move, good catch..
Your previous version was closer to the solution and in line with the requirements.
So I wrote this proposal, added 3 lines to the function: it meets the above requirements.
What do you think of this proposal? Can you verify it works correctly? (no DHT sensor nearby)
int DHTNEW::read()
{
uint32_t delta = millis() - _lastRead;
if ((_type == 22) && (delta < DHTLIB_DHT22_READ_DELAY)) return DHTLIB_OK;
if ((_type == 11) && (delta < DHTLIB_DHT11_READ_DELAY)) return DHTLIB_OK;
if (_type != 0) 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;
}
- It adds complexity to the user as it has a flag (wait for read) to set.
I don't think it adds complexity but the opposite, instead of having to add delays and whatnot in there code all they have to do is sett a flag to true once and leave it.
- It blocks execution for up to DHTLIB_DHT_READ_DELAY time, making the execution not predictable in terms of timing.
since it is optional then if they need it to be predictable they can leave it to the default false and write their own delays.
- Code wise it is even not correct as by the following scenario assume type is 11 and we do 2 successive read calls
we come in at line 33 type != 0 ==> line 35 evaluates false ==> line 46 evaluates true ==> it blocks in line 48-50 ==> it SKIPS line 59 ==> it sets the type to 22 and tries to read the sensor ==> that will fail and it will retry as type 11. ==> then it succeeds
I fixed it to not skip so now it is
if (_type != 0) //If sensor _type is define we come in here
{
//---------------------------
if ((_type == 22) && (millis() - _lastRead < DHTLIB_DHT_READ_DELAY)) //if the _type is 22 AND the delay has NOT been satisfied
{
if (_waitForReading) //if TRUE contine (default FALSE)
{
while (millis() - _lastRead < DHTLIB_DHT_READ_DELAY){} //wait until delay IS satisfied
return _read(); // Now get a reading since sensor is ready
}
else //if _waitForReading is FALSE
{
return DHTLIB_OK; // returns previous reading if 22 and delay not met (2 sec) and _waitForReading was false
}
}
//---------------------------
else if ((_type == 11) && (millis() - _lastRead < DHTLIB_DHT11_READ_DELAY)) //if the _type is 11 AND the delay has NOT been satisfied
{
if (_waitForReading) //if TRUE contine (default FALSE)
{
while (millis() - _lastRead < DHTLIB_DHT11_READ_DELAY){} //wait until delay IS satisfied
return _read(); // Now get a reading since sensor is ready
}
else //if _waitForReading is FALSE
{
return DHTLIB_OK; // returns previous reading if 11 and delay not met (1 sec)
}
}
//---------------------------
else
{
return _read(); // this runs if the the delay was satisfied and then it will take a reading
}
//---------------------------
}
so what instead would happen is assume type is 11 and we do 2 read calls back to back and wait flag is set to TRUE
we come in at line 33 type != 0 ==> line 35 evaluates false ==> line 46 evaluates true ==> it blocks in line 48-50 ==> it takes a reading
Requirements
- the object must read the sensor if the sensor is read longer than "sensor_read_delay" time ago
- the object may not read the sensor if it is read less than "sensor_read_delay" time ago the user has to reuse the last read values.
- the object may not block execution longer than the time to read the sensor so overhead should be minimal.
I partially agree. That's why I made it fully optional. all that adding the delay does is make it easier to get reading that is guaranteed fresh and not old without the user having to write delays in there code all they have to do is set a flag as true.
- the behavior should be as automatic as possible for the user of the library.
The move of
_lastread = millis()
after the read is a correct move, good catch..
Thank you for agreeing with this ^^^ :)
I partially agree. That's why I made it fully optional. all that adding the delay does is make it easier to get reading that is guaranteed fresh and not old without the user having to write delays in there code all they have to do is set a flag as true.
It is not fully optional as the user is confronted with a setting it has to understand, but OK.
we come in at line 33 type != 0 ==> line 35 evaluates false ==> line 46 evaluates true ==> it blocks in line 48-50 ==> it takes a reading
That scenario is better, but still code is way too complex.
Shorter with the same functionality (added yield() function to keep it usable in embedded OSs-es, a requirement forgotten, sorry )
int DHTNEW::read()
{
if (_waitForReading)
{
while ((_type == 22) && (millis() - _lastRead < DHTLIB_DHT22_READ_DELAY)) yield();
while ((_type == 11) && (millis() - _lastRead < DHTLIB_DHT11_READ_DELAY)) yield();
}
else
{
if ((_type == 22) && (millis() - _lastRead < DHTLIB_DHT22_READ_DELAY))) return DHTLIB_OK;
if ((_type == 11) && (millis() - _lastRead < DHTLIB_DHT11_READ_DELAY))) return DHTLIB_OK;
}
if (_type != 0) 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;
}
think code can even be shorter
int DHTNEW::read()
{
uint32_t waitTime = (_type == 22) ? DHTLIB_DHT22_READ_DELAY : DHTLIB_DHT11_READ_DELAY;
while ( _waitForReading && (_type != 0) && (millis() - _lastRead < waitTime)) yield();
if ((_type != 0) && (millis() - _lastRead < waitTime)) return DHTLIB_OK;
if (_type != 0) 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;
}
have you tested it? if it works then it doesn't matter what it looks like 😄
have you tested it? if it works then it doesn't matter what it looks like 😄
I have no sensor nearby, so I did not test it working
What it looks like does matter to me.
it must be understandable
True for all versions imho
it must be maintainable Less code is easier to maintain
Keep footprint as small as possible. Refactored my code (see below) and did a size test based upon - dhtnew_test.ino
lib version | code size | delta |
---|---|---|
1.6 (reference) | 5870 | 0 |
your code | 6074 | +204 |
my code | 6054 | +184 |
my code refactored | 6018 | +148 |
refactored version code
int DHTNEW::read()
{
if (_type != 0)
{
uint16_t readDelay = (_type == 22) ? DHTLIB_DHT22_READ_DELAY : DHTLIB_DHT11_READ_DELAY;
uint32_t lr = _lastRead;
if (_waitForReading) while ((millis() - lr < readDelay)) yield();
else if ((millis() - lr < readDelay)) return DHTLIB_OK;
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;
}
~150 bytes extra is quite a lot memory and just acceptable for now. Note the user can implement waitForRead functionality in a oneliner with 0.1.6
while (milis() - sensor.lastRead() < 2000) {};
sensor.read();
A quick compile with a user oneliner was equal in size as my refactored version
Ok so what have we learned from this and what do you want to go forward with? I do like the thought of the oneliner that can be added by the user to there code now that I see it. can add that to the documentation to show the uses of sensor.lastRead()
You could write an example sketch with a catchy name "DHTNEW_catchTimeout.ino ?" that shows how to handle the timeout with the one liner and add a small description/comment in the code. People look more often to the content of the examples than to documentation or datasheets.
A night sleep makes always a difference
int DHTNEW::read()
{
if (_type != 0)
{
uint16_t readDelay = (_type == 22) ? DHTLIB_DHT22_READ_DELAY : DHTLIB_DHT11_READ_DELAY;
while (millis() - _lastRead < readDelay)
{
if (!_waitForReading) return DHTLIB_OK;
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;
}
lib version | code size | delta |
---|---|---|
1.6 (reference) | 5870 | 0 |
your code | 6074 | +204 |
my code | 6054 | +184 |
my code refactored | 6018 | +148 |
this version | 5976 | +106 |
This latter version is only 10 bytes more than a version (below) without the test for _waitForReading
if (_type != 0)
{
uint16_t readDelay = (_type == 22) ? DHTLIB_DHT22_READ_DELAY : DHTLIB_DHT11_READ_DELAY;
if ((millis() - _lastRead < readDelay)) return DHTLIB_OK;
return _read();
}
...
Think the code arrived at a level hard to optimize more. I will do a final check of the code and update the class.
Pushed the new code to the master branch, so I close this pull request as the functionality and improvements proposed are all in the 0.1.7 version now. It takes time to optimize code to the max but it is worth it.
Thanks again for helping to improve the code base.
I see that you have added the bypass in the .cpp file, that looks good to me
if (millis() - _lastRead < DHTLIB_DHT_READ_DELAY ) return DHTLIB_OK;
#define DHTLIB_DHT_READ_DELAY 2000
I do not know if that value is correct for both DHT11 and DHT22, have you checked the datasheet?Why do you add
delay(2000)
in the testcode when the library already guards against too fast reading. I think thosedelay(2000)
should not be in the example.