driver: sensor: icp101xx: adds driver and sample #69282
Conversation
zephyrbot
added
area: Devicetree Binding
|
Hello @rbuisson-invn, and thank you very much for your first pull request to the Zephyr project! |
zephyrbot
requested review from
MaureenHelm,
avisconti,
galak,
kartben,
nashif,
teburd,
tristan-google and
yperess
|
Hello @MaureenHelm, I'm new to Zephyr Twister, it looks like Twister decided to build the ICP101XX sample with board: tdk_robokit1, while the sample.yaml specifies nrf52dk_nrf52832 or arduino_zero. Thanks and regards, |
rbuisson-invn
force-pushed
the
sensor/invn/icp101xx
branch
6 times, most recently
from
a7f6ab2 to
4227499
Compare
rbuisson-invn
force-pushed
the
sensor/invn/icp101xx
branch
5 times, most recently
from
612e5dc to
8136a33
Compare
rbuisson-invn
force-pushed
the
sensor/invn/icp101xx
branch
3 times, most recently
from
e90afcd to
917602e
Compare
Adds official Invensense icp101xx driver. Also adds a sample for this driver. Validated with based on nrf52/nrf52832 + icp10100 daughter board Build ok: west twister -T samples\sensor\icp101xx -p nrf52dk/nrf52832 Signed-off-by: Remi Buisson <remi.buisson@tdk.com>
rbuisson-invn
force-pushed
the
sensor/invn/icp101xx
branch
from
917602e to
9318067
Compare
There was a problem hiding this comment.
unless this is to demonstrate sensor-specific API (which it doesn't look like this one has any), please do not introduce a new sample. Folks can use existing samples to interact with the sensor, for ex. sensor_shell
There was a problem hiding this comment.
I understand the request, however there are already many Zephyr pressure sensor samples implemented this way (while not having any specific code), such as:
- BME280 from Bosh
- BME680 from Bosh
- DPS310 from Infineon
- LPS22HB from STMicroelectronics
teburd
left a comment
teburd
left a comment
There was a problem hiding this comment.
A first pass of this, I will likely need to look it over again
| #ifndef _INV_BOOL_H_ | ||
| #define _INV_BOOL_H_ | ||
|
|
||
| typedef int inv_bool_t; |
There was a problem hiding this comment.
#include <stdbool.h> instead and use bool? Why do we need to redefine bool values
There was a problem hiding this comment.
The goal of his PR is to make sure Zephyr driver for ICP101xx sensors is using the official driver from TDK.
This code is part of this official TDK driver and cannot be modified for Zephyr.
|
|
||
| /** @brief Common error code definition | ||
| */ | ||
| enum inv_error { |
There was a problem hiding this comment.
Zephyr errors are typically errno values, e.g. ENOTSUP, EIO
There was a problem hiding this comment.
The goal of his PR is to make sure Zephyr driver for ICP101xx sensors is using the official driver from TDK.
This code is part of this official TDK driver and cannot be modified for Zephyr.
| #ifndef _INV_IDD_EXPORT_H_ | ||
| #define _INV_IDD_EXPORT_H_ | ||
|
|
||
| #if defined(_WIN32) |
There was a problem hiding this comment.
When would we be building this for windows as a shared lib
There was a problem hiding this comment.
The goal of his PR is to make sure Zephyr driver for ICP101xx sensors is using the official driver from TDK.
This code is part of this official TDK driver and cannot be modified for Zephyr.
| #define _INV_MESSAGE_H_ | ||
|
|
||
| /* Disable Message from the driver */ | ||
| #define INV_MSG(...) |
There was a problem hiding this comment.
This is a bit confusing, each driver has a log level adjustable already so this should not be needed
There was a problem hiding this comment.
The goal of his PR is to make sure Zephyr driver for ICP101xx sensors is using the official driver from TDK.
This code is part of this official TDK driver and cannot be modified for Zephyr.
| extern "C" { | ||
| #endif | ||
|
|
||
| #define ICP101XX_I2C_ADDR (0x63) |
There was a problem hiding this comment.
I'm not sure I agree in this case that the vendor code is that much of a benefit. The regmap and functionality here to manipulate the device could easily be made to better fit zephyr's sensor drivers already in the tree without a lot of effort.
Please take a look at drivers like the bmi160 and bme680 which are much closer to how I'd expect to see a sensor driver for Zephyr look
There was a problem hiding this comment.
The goal of his PR is to make sure Zephyr driver for ICP101xx sensors is using the official driver from TDK.
This code is part of this official TDK driver and cannot be modified for Zephyr.
| static const struct sensor_driver_api icp101xx_api_funcs = { | ||
| .sample_fetch = icp101xx_sample_fetch, | ||
| .channel_get = icp101xx_channel_get, | ||
| .attr_set = icp101xx_attr_set, |
There was a problem hiding this comment.
I'd really recommend implementing the submit and decoder APIs as well, as those are the future of Zephyr sensor drivers. In which case for this device on a request to read you would...
- Enable the device to sample
- Start a k_timer
- In the k_timer callback start a i2c read to directly read into a user given buffer
- On i2c read complete you'd mark the read complete
In the decoder you'd implement the ability to then decode the sensor data from its internal register format, stashed in the buffer, to useful fixed point q values.
| typedef struct { | ||
| int raw_pressure; | ||
| int raw_temperature; | ||
| float pressure; |
There was a problem hiding this comment.
floats require some special care in Zephyr as floats are not always stashed on context swaps, not always supported by the hardware (cortex-m0), and generally not accepted in sensor drivers for those reasons
There was a problem hiding this comment.
Do you confirm that using the k_float_enable/k_float_disable within the driver would make things safer?
As said in other comments, the official TDK driver for ICP101xx sensor relies on float.
|
|
||
| #define ICP101XX_BUS_I2C DT_ANY_INST_ON_BUS_STATUS_OKAY(i2c) | ||
|
|
||
| typedef struct { |
There was a problem hiding this comment.
We don't typedef structs in zephyr typically
There was a problem hiding this comment.
Agreed, will be changed.
| @@ -0,0 +1,49 @@ | |||
| .. _icp101xx: | |||
|
|
|||
| ICP101xx: Barometric Pressure and Temperature Sensor | |||
There was a problem hiding this comment.
We have a dht_polling sample that covers a similar case, I don't think we need a device specific sample in this scenario
There was a problem hiding this comment.
DHT is a sensor from another manufacturer (Aosong Electronics) and is NOT a pressure sensor, this sample can definitely not be used with the TDK ICP101xx pressure sensor.
|
|
||
| int main(void) | ||
| { | ||
| const struct device *dev = get_icp101xx_device(); |
There was a problem hiding this comment.
const struct device *dev = DEVICE_DT_GET(DT_ALIAS(barom_temp_altitude));
Any board overlay that supplies this devicetree alias then works with this sample. The sample could be renamed to something along those lines. BMP180 from bosch would also fit in this use case and sample, nothing icp101xxx specific should be needed.
There was a problem hiding this comment.
I understand the request, however there are already many Zephyr pressure sensor samples implemented this way (while not having any specific code), such as:
- BME280 from Bosh
- BME680 from Bosh
- DPS310 from Infineon
- LPS22HB from STMicroelectronics
There was a problem hiding this comment.
I think the point here is: does this sample really provide more than sensor_shell sample to an end-user in allowing them to easily exercise their sensor?
Arguably, the only added value is showing how one's DT overlay would look like but this is something that can also easily be done in the documentation of the binding directly if you really want to give users an example of how things could look like ; for example: https://github.com/zephyrproject-rtos/zephyr/blob/553a1c0063d81a6ab088f6632e5803b1e1d43418/dts/bindings/sensor/sensirion%2Cshtcx.yaml#L10-L21)
There was a problem hiding this comment.
I think the point here is: does this sample really provide more than sensor_shell sample to an end-user in allowing them to easily exercise their sensor?
Arguably, the only added value is showing how one's DT overlay would look like but this is something that can also easily be done in the documentation of the binding directly if you really want to give users an example of how things could look like ; for example: https://github.com/zephyrproject-rtos/zephyr/blob/553a1c0063d81a6ab088f6632e5803b1e1d43418/dts/bindings/sensor/sensirion%2Cshtcx.yaml#L10-L21)
There was a problem hiding this comment.
and to be clear, the "generic" samples you're referring to should probably just go away
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
5 participants
Adds official Invensense Inc. driver for icp101xx barometric pressure and temprature sensor. Also adds a sample for this driver.
Validated with custom setup based on nrf52dk_nrf52832 + icp10100 daughter board
Build ok by running: west twister -T samples\sensor\icp101xx -p nrf52dk_nrf52832