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

Add Tdk invensense icp201xx pressure sensor support #83914

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rbuisson-invn
Copy link
Contributor

drivers: sensor: icp201xx: supports icp201xx sensors
dts: supports icp201xx invn pressure sensor
samples: sensor: pressure_polling: barometric pressure, temperature and altitude generic sample
samples: sensor: pressure_interrupt: barometric pressure sensor interrupts generic sample
module: HAL TDK module

Adds official TDK Invensense Inc. driver in TDK HAL module for icp201xx sensor.
Add generic samples supporting this driver.
Validated with custom setup: nrf52dk_nrf52832 + icp201xx EVB board
Build ok by running:
west twister -T samples\sensor\pressure_polling -T samples\sensor\pressure_interrupt -p nrf52dk/nrf52832

Signed-off-by: Remi Buisson remi.buisson@tdk.com

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 13, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff

All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_tdk DNM This PR should not be merged (Do Not Merge) labels Jan 13, 2025
@rbuisson-invn rbuisson-invn force-pushed the TDK_Invensense_icp201xx_pressure_sensor branch 7 times, most recently from 92248ff to 4e866f3 Compare January 16, 2025 09:36
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Jan 16, 2025
@rbuisson-invn rbuisson-invn force-pushed the TDK_Invensense_icp201xx_pressure_sensor branch from 4e866f3 to 90baab8 Compare January 16, 2025 13:19
@rbuisson-invn
Copy link
Contributor Author

Hi @MaureenHelm ,
This PR is waiting a review for 3 weeks.
It is blocking further developments at our side.
I know it's quite a big chunk of code, but we would be very grateful if you could take a look at it.
Regards,

Comment on lines 22 to 46
#ifdef CONFIG_CPU_HAS_FPU
#include <math.h>
#define ATMOSPHERICAL_PRESSURE_KPA ((float)101.325)
#define TO_KELVIN(temp_C) (((float)273.15) + temp_C)
/* Constant in altitude formula:
* M*g/R = (0,0289644 * 9,80665 / 8,31432)
* with M the molar mass of air.
* with g the gravitational force acceleration.
* with R the universal gaz constant.
*/
#define HEIGHT_TO_PRESSURE_COEFF ((float)0.03424)

/* Constant in altitude formula:
* R / (M*g) = 8,31432 / (0,0289644 * 9,80665)
* with M the molar mass of air.
* with g the gravitational force acceleration.
* with R the universal gaz constant.
*/
#define PRESSURE_TO_HEIGHT_COEFF ((float)29.27127)
/* Constant for altitude formula:
* logarithm of pressure at 0m
* ln(101.325)
*/
#define LOG_ATMOSPHERICAL_PRESSURE ((float)4.61833)
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please consider using the dsp.h q31_t here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point, it requires pure floats to compute logarithm.
I don't think dsp.h provides such support.

Copy link
Collaborator

@yperess yperess Feb 10, 2025

Choose a reason for hiding this comment

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

Does it? The DSP header provide mechanism for multiplying fixed points which can be just as accurate. I'm OK with you using floats, but it does make the driver much more compute header when CONFIG_FPU=n.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't think you actually need a float, you should be able to use a fixed point API.

@rbuisson-invn rbuisson-invn force-pushed the TDK_Invensense_icp201xx_pressure_sensor branch from 90baab8 to aedacd0 Compare February 10, 2025 09:52
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

please also add relevant entries to build_all/sensors dtsi's and conf files as this is the only way to be certain we'll have coverage independently of the existence of samples and respective overlays.

Thanks!

sample.sensor.pressure_sensor:
build_only: true
tags: sensors
filter: dt_alias_exists("pressure_sensor")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This currently doesn't run. Needs the following change, but also keep only of the two overlay and name it nrf52dk_nrf52832.overlay (otherwise neither gets picked up, hence no pressure-sensor alias is found, hence test is filtered :) )

Suggested change
filter: dt_alias_exists("pressure_sensor")
filter: dt_alias_exists("pressure-sensor")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

Copy link
Collaborator

@kartben kartben Feb 18, 2025

Choose a reason for hiding this comment

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

This currently doesn't run. Needs the following change, but also keep only of the two overlays and name it nrf52dk_nrf52832.overlay (otherwise neither gets picked up, hence no pressure-sensor alias is found, hence test is filtered :) )

not addressed and therefore test would effectively still not run

sample.sensor.pressure_interrupt:
build_only: true
tags: sensors
filter: dt_alias_exists("pressure_sensor")
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@@ -0,0 +1,72 @@
.. zephyr:code-sample: pressure_interrupt
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't show up in samples listing due to typo

Also, varisou formatting issues with the file, please check https://builds.zephyrproject.io/zephyr/pr/83914/docs/samples/sensor/pressure_interrupt/README.html -- applies to the other README too :)

Suggested change
.. zephyr:code-sample: pressure_interrupt
.. :zephyr:code-sample: pressure_interrupt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing colon is probably not the suggested one :), but it was actually missing!
Thanks, fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

lol oops

Comment on lines 10 to 21
This sample application uses a pressure sensor interrupt line to:
** Inform when a measure is available in the sensor FIFO.
Temperature and pressure data are read and displayed in terminal.
If floats are supported, estimated altitude is also displayed.
** Inform when the pressure value crosses a specified threshold.
Threshold corresponds to around a 50cm altitude increase.
A message is displayed in the terminal.
** Inform when the pressure value changed more than the specified
value between two consecutive samples.
Change value corresponds to a finger pressing the sensor.
A message is displayed in the terminal.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This sample application uses a pressure sensor interrupt line to:
** Inform when a measure is available in the sensor FIFO.
Temperature and pressure data are read and displayed in terminal.
If floats are supported, estimated altitude is also displayed.
** Inform when the pressure value crosses a specified threshold.
Threshold corresponds to around a 50cm altitude increase.
A message is displayed in the terminal.
** Inform when the pressure value changed more than the specified
value between two consecutive samples.
Change value corresponds to a finger pressing the sensor.
A message is displayed in the terminal.
This sample application uses a pressure sensor interrupt line to:
* Inform when a measure is available in the sensor FIFO.
Temperature and pressure data are read and displayed in terminal.
If floats are supported, estimated altitude is also displayed.
* Inform when the pressure value crosses a specified threshold.
Threshold corresponds to around a 50cm altitude increase.
A message is displayed in the terminal.
* Inform when the pressure value changed more than the specified
value between two consecutive samples.
Change value corresponds to a finger pressing the sensor.
A message is displayed in the terminal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

Comment on lines 55 to 72
## Default configuration

[00:00:00.266,479] <inf> PRESS_INT_SAMPLE: Starting ICP201xx sample.
[00:00:00.273,803] <inf> PRESS_INT_SAMPLE: temp 25.49 Cel, pressure 96.271438 kPa, altitude 447.208465 m
[00:00:00.280,914] <inf> PRESS_INT_SAMPLE: temp 25.50 Cel, pressure 96.271331 kPa, altitude 447.234161 m
[00:00:00.288,024] <inf> PRESS_INT_SAMPLE: temp 25.49 Cel, pressure 96.266685 kPa, altitude 447.636077 m
[00:00:00.295,135] <inf> PRESS_INT_SAMPLE: temp 25.50 Cel, pressure 96.267951 kPa, altitude 447.537078 m
[00:00:00.302,246] <inf> PRESS_INT_SAMPLE: temp 25.51 Cel, pressure 96.268577 kPa, altitude 447.488281 m
[00:00:00.309,356] <inf> PRESS_INT_SAMPLE: temp 25.50 Cel, pressure 96.269340 kPa, altitude 447.414978 m
[00:00:00.316,467] <inf> PRESS_INT_SAMPLE: temp 25.50 Cel, pressure 96.268562 kPa, altitude 447.473663 m
[00:00:00.323,547] <inf> PRESS_INT_SAMPLE: temp 25.50 Cel, pressure 96.267341 kPa, altitude 447.596496 m
<repeats endlessly>

<when sensor is pressed>
[00:00:09.819,061] <inf> PRESS_INT_SAMPLE: PRESSURE CHANGE INTERRUPT

<when the sensor pressure crosses defined threhold>
[00:00:09.859,039] <inf> PRESS_INT_SAMPLE: PRESSURE THRESHOLD INTERRUPT
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs 3-space indenting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

Comment on lines 37 to 42
/ {
aliases {
pressure-sensor = &icp201xx;
};
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/ {
aliases {
pressure-sensor = &icp201xx;
};
};
/ {
aliases {
pressure-sensor = &icp201xx;
};
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@rbuisson-invn rbuisson-invn force-pushed the TDK_Invensense_icp201xx_pressure_sensor branch 2 times, most recently from 444d9aa to 52640a9 Compare February 17, 2025 09:54
icp201xx_reg_write_fn write;
};

#if CONFIG_SPI
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ping

Comment on lines 22 to 46
#ifdef CONFIG_CPU_HAS_FPU
#include <math.h>
#define ATMOSPHERICAL_PRESSURE_KPA ((float)101.325)
#define TO_KELVIN(temp_C) (((float)273.15) + temp_C)
/* Constant in altitude formula:
* M*g/R = (0,0289644 * 9,80665 / 8,31432)
* with M the molar mass of air.
* with g the gravitational force acceleration.
* with R the universal gaz constant.
*/
#define HEIGHT_TO_PRESSURE_COEFF ((float)0.03424)

/* Constant in altitude formula:
* R / (M*g) = 8,31432 / (0,0289644 * 9,80665)
* with M the molar mass of air.
* with g the gravitational force acceleration.
* with R the universal gaz constant.
*/
#define PRESSURE_TO_HEIGHT_COEFF ((float)29.27127)
/* Constant for altitude formula:
* logarithm of pressure at 0m
* ln(101.325)
*/
#define LOG_ATMOSPHERICAL_PRESSURE ((float)4.61833)
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't think you actually need a float, you should be able to use a fixed point API.

@rbuisson-invn rbuisson-invn force-pushed the TDK_Invensense_icp201xx_pressure_sensor branch from 52640a9 to a37bc76 Compare February 18, 2025 09:48
@rbuisson-invn
Copy link
Contributor Author

@yperess, it's not clear to me how I could compute a logarithm only using fixed points.
The C logf() function is called there:
drivers/sensor/tdk/icp201xx/icp201xx_drv.c line 75.
static float convertToHeight(float pressure_kp, float temperature_C)
{
return PRESSURE_TO_HEIGHT_COEFF * TO_KELVIN(temperature_C) *
(LOG_ATMOSPHERICAL_PRESSURE - logf(pressure_kp));
}

@kartben
Copy link
Collaborator

kartben commented Feb 18, 2025

please also add relevant entries to build_all/sensors dtsi's and conf files as this is the only way to be certain we'll have coverage independently of the existence of samples and respective overlays.

this part hasn't been addressed - thanks!

@yperess
Copy link
Collaborator

yperess commented Feb 19, 2025

@yperess, it's not clear to me how I could compute a logarithm only using fixed points. The C logf() function is called there: drivers/sensor/tdk/icp201xx/icp201xx_drv.c line 75. static float convertToHeight(float pressure_kp, float temperature_C) { return PRESSURE_TO_HEIGHT_COEFF * TO_KELVIN(temperature_C) * (LOG_ATMOSPHERICAL_PRESSURE - logf(pressure_kp)); }

I see we haven't ported this part of the math functions to the DSP subsystem yet. I'll get on that for the next release. The API will look like https://github.com/zephyrproject-rtos/cmsis-dsp/blob/main/Include/dsp/fast_math_functions.h#L158.

Please address the issue in #83914 (comment) and I'll be happy to approve as is.

@rbuisson-invn rbuisson-invn force-pushed the TDK_Invensense_icp201xx_pressure_sensor branch 7 times, most recently from 1c54338 to e4607ac Compare February 20, 2025 08:52
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

looks great, thank you! Just a final nit

Comment on lines 150 to 151
<&test_gpio 0 0>;
<&test_gpio 0 0>; /* 0x31 */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the original intent here is to give a visual clue every 10 lines, so probably needs to be:

Suggested change
<&test_gpio 0 0>;
<&test_gpio 0 0>; /* 0x31 */
<&test_gpio 0 0>; /* 0x30 */
<&test_gpio 0 0>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the original intent here is to give a visual clue every 10 lines, so probably needs to be:

Thanks for active follow up ;)
I'll fix.

@rbuisson-invn rbuisson-invn force-pushed the TDK_Invensense_icp201xx_pressure_sensor branch from e4607ac to 8815ad9 Compare February 20, 2025 10:03
kartben
kartben previously approved these changes Feb 20, 2025
@rbuisson-invn
Copy link
Contributor Author

@yperess, it's not clear to me how I could compute a logarithm only using fixed points. The C logf() function is called there: drivers/sensor/tdk/icp201xx/icp201xx_drv.c line 75. static float convertToHeight(float pressure_kp, float temperature_C) { return PRESSURE_TO_HEIGHT_COEFF * TO_KELVIN(temperature_C) * (LOG_ATMOSPHERICAL_PRESSURE - logf(pressure_kp)); }

I see we haven't ported this part of the math functions to the DSP subsystem yet. I'll get on that for the next release. The API will look like https://github.com/zephyrproject-rtos/cmsis-dsp/blob/main/Include/dsp/fast_math_functions.h#L158.

Please address the issue in #83914 (comment) and I'll be happy to approve as is.

Sure, it's now done.
Thanks for the overall review and feedback!

icp201xx are barometric pressure and temperature sensors.
https://invensense.tdk.com/smartpressure

Signed-off-by: Remi Buisson <remi.buisson@tdk.com>
Use official TDK Invensense driver for icp201xx sensor in tdk_hal module.

Signed-off-by: Remi Buisson <remi.buisson@tdk.com>
Add I2C and SPI variants for icp201xx.

Signed-off-by: Remi Buisson <remi.buisson@tdk.com>
Polls and reports pressure, temperature and altitude data

Signed-off-by: Remi Buisson <remi.buisson@tdk.com>
Reports generic barometric sensor interrupts
* Data ready interrupt
* Pressure threshold interrupt
* Pressure changed interrupt

Signed-off-by: Remi Buisson <remi.buisson@tdk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants