Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed getRawTemperature and getRawHumidity #11

Merged
merged 1 commit into from
Jul 10, 2022
Merged

Fixed getRawTemperature and getRawHumidity #11

merged 1 commit into from
Jul 10, 2022

Conversation

theandy94
Copy link
Contributor

The internal variables that the raw values are read from were swapped.

@theandy94
Fixed getRawTemperature and getRawHumidity
d72086b 
The internal variables that the raw values are read from were swapped.
@RobTillaart RobTillaart self-assigned this Jul 10, 2022
@RobTillaart RobTillaart added the bug Something isn't working label Jul 10, 2022
@RobTillaart
Copy link
Owner

Thanks for catching this bug
I will investigate it later today.

@RobTillaart
Copy link
Owner

RobTillaart commented Jul 10, 2022 •
edited
Loading

Looks like I was somewhere completely else with my mind when writing this code.

@theandy94
Copy link
Contributor Author

Happens to the best of us 😉
Nice to see you are this fast to react to pull requests 👍

Have a nice sunday :)

@RobTillaart RobTillaart merged commit 1d3a1bc into RobTillaart:master Jul 10, 2022
@RobTillaart
Copy link
Owner

Nice to see you are this fast to react to pull requests 👍

If they are this obvious one has no choice 😁

@RobTillaart
Copy link
Owner

RobTillaart commented Jul 10, 2022 •
edited
Loading

@theandy94

Triggered by your PR a new 0.2.0 release is prepared. See the develop branch

  • adds get / set resolution - based upon SHT20 datasheet.
  • check batteryOK (checks VDD)
  • some updates in readme.md.

If you have time to test with real hardware, would be appreciated. (especially resolution)
I have no hardware available at the moment, and still need to write an example sketch.

update: example added (loop over resolutions, should give all same temp and hum +- noise)

@theandy94
Copy link
Contributor Author

theandy94 commented Jul 10, 2022 •
edited
Loading

The only hardware I've got is a HTU21D connected to an ESP32 devboard using its 3.3v to supply the sensor.

I've downloaded the current dev branch with 0.2.0 and ran the SHT2x_resolution.ino sketch.
Upon starting I get:
STAT: 0
BATT: OK

I can confirm, that the temperature and humidity readings are the same for each resolution setting +-noise.

Always glad to help :)

UPDATE:
I also tested getEIDA() and getEIDB() which do return values, although I am not sure how to validate correctness of the values.
EIDA: 10275022 EIDB: 839993428
The getFirmwareVersion() call returns 0 for my HTU21D, not sure what to make of this... maybe a difference between SHT and HTU chips?

@RobTillaart
Copy link
Owner

Thanks for testing, I will dive into the datasheets tomorrow to check for diffs. The EIDA is less important than the setresolution.
To be continued

@RobTillaart
Copy link
Owner

updated the develop with crc8 test in the read()
it will set the error flag, but will not fail on invalid crc.
(don't know if it adds value as the error rate seems to be pretty low)

@RobTillaart
Copy link
Owner

RobTillaart commented Jul 11, 2022 •
edited
Loading

I read several datasheets and they look pretty the same.
However I noticed I made a mistake in the masking of the resolution bits, so expect a fix asap.

update: fixed, but needs another round of testing.

If you have time for testing, can you post the output of SHT2x_resolution.ino?
Thanks,

@theandy94
Copy link
Contributor Author

Output from SHT2x_resolution.ino from latest develop:

SHT2x_LIB_VERSION:      0.2.0
STAT:   0
BATT:   OK
RES:    0
        102448  23.9    58.2
RES:    1
        101452  23.8    58.2
RES:    2
        101452  23.8    58.3
RES:    3
        101452  23.8    58.1
RES:    0
        101452  23.8    58.0
RES:    1
        101452  23.8    58.0
RES:    2
        101452  23.8    58.1
RES:    3
        101452  23.8    58.1
RES:    0
        101452  23.8    58.2
RES:    1
        101452  23.8    58.1
RES:    2
        101452  23.8    58.1
RES:    3
        101452  23.8    58.1
RES:    0
        101452  23.8    58.1
RES:    1
        101452  23.8    58.1
RES:    2
        101452  23.8    58.1
RES:    3
        101452  23.8    58.1

@RobTillaart
Copy link
Owner

Thanks, i notice the timing is still hard-coded.
Will look into that tomorrow.
Without shorter delays there is no reason to set a lower resolution.
Tbc

@RobTillaart
Copy link
Owner

Just pushed a change with adjusted timing for the different resolutions. (based upon table 7 SHT21)
also changed the decimals to 3 in the test sketch to see the difference in resolution.

Please rerun the test sketch.

If it is OK, I am going to merge it into master and release version 0.2.0

@theandy94
Copy link
Contributor Author

theandy94 commented Jul 12, 2022 •
edited
Loading

Output from latest develop:

SHT2x_LIB_VERSION:      0.2.0
STAT:   0
BATT:   OK
RES:    0       116452  24.300  60.727
RES:    1       1113875 24.300  60.727
RES:    2       1084875 24.279  60.727
RES:    3       1102875 24.279  60.727
RES:    0       116452  24.268  61.291
RES:    1       1113874 24.268  61.291
RES:    2       1084875 24.268  61.291
RES:    3       1102875 24.268  61.291
RES:    0       116452  24.290  60.795
RES:    1       1113874 24.290  60.795
RES:    2       1084875 24.290  60.795
RES:    3       1102875 24.290  60.795
RES:    0       116452  24.279  60.689
RES:    1       1113874 24.279  60.689
RES:    2       1084875 24.268  60.689
RES:    3       1102875 24.268  60.689
RES:    0       116452  24.290  60.620
RES:    1       1113874 24.290  60.620
RES:    2       1084875 24.279  60.620
RES:    3       1102875 24.279  60.620
RES:    0       116452  24.279  60.559
RES:    1       1113874 24.279  60.559
RES:    2       1084875 24.279  60.559
RES:    3       1102875 24.279  60.559

I noticed, that the humidity value only ever changed after resetting the resolution setting to 0 while staying the same throughout the for loop.

During investigating your code if there might be some hidden reason, I noticed the different delays based on the resolution setting (if else if...) and was wondering why for temperature it was 3 then 1 then 2 but for humidity 1 then 2 then 3.
I guess it wouldn't hurt to add a comment from the datasheet for your own sanity and anyone reading the code:
"Default measurement resolution is 14bit (temperature) / 12bit (humidity). It can
be reduced to 12/8bit, 11/11bit or 13/10bit by command to user register."

Otherwise I foresee someone in the future trying to "fix" this code but actually breaking correctness.

Anyways, thanks for your work :)

@RobTillaart
Copy link
Owner

Thanks for testing,
Good tip to improve comments here.
Still the time used by the various resolutions make no sense except for zero.
Tbc

@RobTillaart
Copy link
Owner

RobTillaart commented Jul 13, 2022 •
edited
Loading

The expected times versus the measured times

RES HUM TEMP TOTAL REAL
0 29 85 114 116
1 4 22 26 1113
2 9 43 52 1084
3 15 11 26 1102

So only the resolution == 0 meets the expectation.
I do not see a pattern in the deviations and

Which processor do you use?

What I do notice is that the sensor gives 4x the same temperature in a row.
And humidity values seem to repeat too.
That could indicate that time between measurements is too short.

I am going to rewrite the example.
Please be so kind to run another test.

Example updated

  • it repeats 5 measurements for every resolution
  • it has longer delays between measurements

@theandy94
Copy link
Contributor Author

I've used an ESP32 which is known to not be very precise with its delays...

I now used an Arduino Nano with the latest develop version.
Output is the following:

SHT2x_LIB_VERSION:      0.2.0
STAT:   0
BATT:   OK
RES:    0       115368  23.946  59.887
RES:    0       115376  23.882  60.017
RES:    0       115364  23.871  60.193
RES:    0       115368  23.871  60.139
RES:    0       115364  23.861  60.101
RES:    1       112292  23.861  60.101
RES:    1       113008  23.861  60.101
RES:    1       113016  23.861  60.101
RES:    1       113012  23.861  60.101
RES:    1       113000  23.861  60.101
RES:    2       83300   23.839  60.101
RES:    2       84396   23.839  60.101
RES:    2       83360   23.828  60.101
RES:    2       83364   23.828  60.101
RES:    2       84388   23.818  60.101
RES:    3       101796  23.818  60.101
RES:    3       100716  23.818  60.101
RES:    3       101752  23.818  60.101
RES:    3       101748  23.818  60.101
RES:    3       101736  23.818  60.101

done...

@RobTillaart
Copy link
Owner

Looks better than last test but still most are longer than expected.
Either the datasheet is incorrect or ?
I will make a remark in the readme and start merging process later today. Enough time spent on it for now.
Thanks

@theandy94
Copy link
Contributor Author

Always glad to help.

@RobTillaart
Copy link
Owner

Note: that the timing in the last test is very similar per resolution MINUS 1 second ...
(Will dive into it later again as said before, but I want to keep this observation)

@RobTillaart
Copy link
Owner

Released 0.2.0

@RobTillaart RobTillaart mentioned this pull request Jul 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@RobTillaart RobTillaart

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@theandy94 @RobTillaart