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.

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

setResolution doesn't call SHT2x_WRITE_USER_REGISTER #18

Closed
madl3x opened this issue Mar 25, 2023 · 9 comments
Closed

setResolution doesn't call SHT2x_WRITE_USER_REGISTER #18

madl3x opened this issue Mar 25, 2023 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@madl3x
Copy link

madl3x commented Mar 25, 2023

I haven't checked the SHTx datasheet, but in the function setResolution, when calling writeCmd, the command used is SHT2x_READ_USER_REGISTER:

SHT2x/SHT2x.cpp

Line 480 in 71a1d94

if (writeCmd(SHT2x_READ_USER_REGISTER, userReg) == false)

Shouldn't it be SHT2x_WRITE_USER_REGISTER?

Also, this SHT2x_WRITE_USER_REGISTER compiler definition is not used anywhere in the code.
A bit weird.

@RobTillaart RobTillaart self-assigned this Mar 26, 2023
@RobTillaart
Copy link
Owner

Thanks for the issue. I will dive into it asap, might take some days.

@RobTillaart
Copy link
Owner

There is a (long open) issue with the resolution #13
It could be related. As I have not investigated that yet l cannot say for sure. Finally I do not have such sensor available for testing (no excuse, but it does not help either).
That said, you might have pinpointed the cause.

@RobTillaart RobTillaart added the bug Something isn't working label Mar 26, 2023
@RobTillaart
Copy link
Owner

Had a look and indeed the SHT2x_WRITE_USER_REGISTER should be used in both setResolution() and some heater functions.

I will create a develop branch + PR asap.
Are you able to test that branch?

@RobTillaart
Copy link
Owner

Created the develop branch + PR #19

@madl3x
Copy link
Author

madl3x commented Mar 26, 2023

Thank you for your fast response!

Finally I do not have such sensor available for testing

You should test all your library updates with the proper hardware.
Doing blind updates is never a good idea for open-source, especially in communities like Arduino where people are using libraries to avoid understanding too much of the underlying hardware.

RobTillaart added a commit that referenced this issue Mar 26, 2023
- fix setResolution #13, #18
- fix heater settings
- add GY21 as derived class
- fix derived constructors
- move code from .h to .cpp
- update readme.md
- update GitHub actions
- update license 2023
- minor edits
@RobTillaart
Copy link
Owner

Thank you for your fast response!

You're welcome!

You should test all your library updates with the proper hardware.
Doing blind updates is never a good idea for open-source, especially in communities like Arduino ...

I know,
you are 100 % right, also not a good idea in closed source, never,

However, I have about 200 libraries written (partly on request) and for most of them I have bought the hardware to test but not for all (for various reasons). For this library I don't have the hardware, so in the readme.md file is a disclaimer. Same is true for all different boards, there are hundreds of them with sometimes minor differences, and I only have a few to test.
(not an excuse, just reality)

@RobTillaart
Copy link
Owner

@madl3x
If there are no further related questions, you may close the issue.

@madl3x madl3x closed this as completed Mar 29, 2023
@madl3x
Copy link
Author

madl3x commented Mar 29, 2023

No further questions @RobTillaart, thank you for fixing this!

@RobTillaart
Copy link
Owner

Otherwise do not hesitate to ask questions, especially with a well done analysis/diagnosis - appreciated 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants