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

drivers: htu21d relative humidity and temperature sensor driver #10110

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Oct 4, 2018

Contribution description

This PR adds a driver for the TE Connectivity HTU21D humidity and temperature sensor. The driver could also be used with Sensirion SHT21 and with SI7021.

The sensor only supports single measurements and polling. This driver provides also drivers/saul capabilities.

Please note:
Even though the HTU21D driver also works with SI7021 sensor for which drivers/si70xx already exists, there are the following differences:

  • drivers/si70xx checks some registers during intialization that do no exist in HTU21D sensor (heater control register and revision registers).
  • drivers/si70xx uses clock stretching which blocks the bus and the master during the measurement which can take up to 85 ms. drivers/htu21d does not use clock stretching to avoid this blocking.
  • drivers/si70xx does not implement the CRC check while drivers/htu21d implement and use it.

As an alternative, I could provide a PR that

  • defines a pseudomodule for HTU21D,
  • modifies drivers/si70xx so that it would also work also with HTU21D,
  • includes wrapper files drivers/include/htu21d.h and drivers/htu21d/include/htu21d_params.h which just redefine symbols with prefix htu21d_, and
  • includes a tests/driver_htu21d as test application.

I have it already realized also in that way and could provide it. The same could be done for SHT21.
But please keep in mind that a pseudo/wrapper module would use drivers/si70xx which blocks the bus and the master while waiting for measurement results.

Testing procedure

USEMODULE=htu21d make flash -C tests/saul BOARD=...

@PeterKietzmann PeterKietzmann added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers Area: SAUL Area: Sensor/Actuator Uber Layer labels Oct 4, 2018
@maribu maribu requested a review from basilfx November 13, 2018 09:23
@maribu
Copy link
Member

maribu commented Nov 13, 2018

@gschorcht: I could review this PR, but I don't have the hardware to test it. Should I still review?
@basilfx: Maybe you could get involved as author of the si70xx.

To me the advantage of not using clock stretching to allow using multiple I2C device on the same bus is obvious. Is there any technical argument to not use this approach in the si70xx? If not, I personally would favor a unified driver for all devices that would work in a non-blocking fashion, unless one of the following would result:

  • The resulting unified driver would be bigger than two separate drivers
  • The resulting driver would perform significantly poorer (e.g. regarding performance, features, ROM size) than each of the two separate driver
  • The resulting driver would be harder to maintain than two separate drivers (e.g. code gets way more complex)
  • Simultaneous use of si70xx, htu21d, and sht21 devices would not be possible by one unified driver, but it would be by separate drivers.

Note: I did not look yet into either this code or the si70xx drivers. So maybe the above questions are obvious.

@gschorcht
Copy link
Contributor Author

gschorcht commented Nov 17, 2018

@maribu Thanks for your offer to review the driver. We should wait some more time for an answer from @basilfx. Once he has answered we can decide whether to redesign the si70xx or to continue with the separate driver.

@basilfx
Copy link
Member

basilfx commented Nov 26, 2018

I'm in the middle of a moving, so I won't be able to test this soon.

@aabadie aabadie added this to the Release 2019.01 milestone Dec 3, 2018
@kb2ma kb2ma removed this from the Release 2019.10 milestone Oct 8, 2019
@stale
Copy link

stale bot commented Apr 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Apr 10, 2020
@gschorcht gschorcht removed the State: stale State: The issue / PR has no activity for >185 days label Apr 10, 2020
@gschorcht
Copy link
Contributor Author

PR is waiting for review.

@aabadie
Copy link
Contributor

aabadie commented Apr 10, 2020

Needs rebase

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced by the proposed API.

I don't see a real benefit from the user point of view in having a couple functions htu21d_measure_tmp/htu21d_fetch_tmp over a single function htu21d_read_temperature. It would make the API more natural, compared to other sensors. Then USE_HTU21D_BLOCKING could also be removed.

In a way, htu21d_read could be split into 2 functions htu21d_read_temperature and htu21d_read_humidity.

Otherwise, the driver implementation looks good. Maybe you could also add a README.md file with the test application.

Comment on lines +1 to +2
MODULE = htu21d

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MODULE = htu21d

} \
}

/** Forward declaration of functions for internal use */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** Forward declaration of functions for internal use */
/* Forward declaration of functions for internal use */

@@ -175,6 +175,10 @@ ifneq (,$(filter hts221,$(USEMODULE)))
FEATURES_REQUIRED += periph_i2c
endif

ifneq (,$(filter htu21d,$(USEMODULE)))
FEATURES_REQUIRED += periph_i2c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FEATURES_REQUIRED += periph_i2c
FEATURES_REQUIRED += periph_i2c
USEMODULE += xtimer

Because there's a strong dependency on xtimer in the driver implementation.

return HTU21D_OK;
}

static const uint8_t _times_tmp[] = { 85, 22, 43, 11 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have some defines for this constants ?

return HTU21D_OK;
}

/** Functions for internal use only */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** Functions for internal use only */
/* Functions for internal use only */

@maribu
Copy link
Member

maribu commented Apr 10, 2020

This PR adds a driver for the TE Connectivity HTU21D humidity and temperature sensor. The driver could also be used with Sensirion SHT21 and with SI7021.

Could this replace the sth2x and the si70xx driver? If so, I really would love to have less code to maintain to provide the same features.

This would also be greatly beneficial in case someone (for whatever reason) connects a bunch of temperature+humidity sensors of the various flavors, as with the single driver only ROM space for one implementation would be needed.

@aabadie
Copy link
Contributor

aabadie commented Jun 23, 2020

Please rebase. There are also some pending review comments.

@benpicco benpicco added this to Waiting for Author in New Drivers Sep 11, 2020
@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Dec 25, 2020
@gschorcht gschorcht removed the State: stale State: The issue / PR has no activity for >185 days label Jan 4, 2021
@gschorcht gschorcht added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 4, 2021
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022
@gschorcht gschorcht removed the State: stale State: The issue / PR has no activity for >185 days label Mar 3, 2022
@stale
Copy link

stale bot commented Sep 21, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@gschorcht gschorcht removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: SAUL Area: Sensor/Actuator Uber Layer Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
New Drivers
  
Waiting for Author
Development

Successfully merging this pull request may close these issues.

None yet

8 participants