Skip to content

Conversation

@Sokario
Copy link
Contributor

@Sokario Sokario commented Sep 26, 2025

This pull request adds multiple instance, triggers support and enhance data recovery for the MAXIM MAX30101.

MAXIM MAX30101 sensor multi-instance support:

  • Removed instance based Kconfig to device tree bindings, added parameter's recuperation from device tree.
  • More readable parameters for device tree.
  • Added missing configuration registers.
  • Updated nodes using max30101 compatible:
    • hexiwear_mk64f12 board (boards/nxp/hexiwear/hexiwear_mk64f12.dts)
    • i2c.dtsi tests drivers (test/drivers/build_all/sensor/i2c.dtsi)

MAXIM MAX30101 sensor triggers support:

  • Added new SENSOR_CHAN and SENSOR_TRIG:
    • SENSOR_CHAN_AMBIENT_LIGHT: because MAX30101 allows verification of ambient light cancellation.
    • SENSOR_TRIG_OVERFLOW: the MAX30101 allows interrupt to be raised when ambient light cancellation overflows.
  • Added interrupt related Kconfig options and irq-gpios device tree parameter.
  • Added max30101_trigger.c and updated CMakeLists.txt and max30101.c.
  • Added interrupt support in samples: samples/sensor/heart_rate
  • Updated sample.yaml and README.rst accordingly.
  • Updated sensor_shell.c and test_sensor_shell.py to match the tow newly added enumerators.
  • Updated test_sensor_shell.py to automatically adapt to futur modification of sensor_channel.

MAXIM MAX30101 sensor sample_fetch enhancement:

  • Added better data recovery for more complex multi-LED configurations.
  • New fifo/channel matrix mapping.
  • Added MAX30101_DIE_TEMPERATURE Kconfig to add die temperature acquisition (made it Kconfig in order to let the user remove the extra communication time on tight period acquisition).
  • Added Kconfig condition on temperature triggers.

MAX30101 datasheet.

@github-actions
Copy link

Hello @Sokario, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@Sokario
Copy link
Contributor Author

Sokario commented Sep 26, 2025

I had an error on twister pass (native_sim) for sensor_shell. Discovered that I needed to modify sensor_shell.c and test_sensor_shell.py to get twister working right with:

  • SENSOR_CHAN_AMBIENT_LIGHT
  • SENSOR_TRIG_OVERFLOW

Discovered an odity with test_sensor_attr_set/get wich are using channels instead of attributes ?
test_sensor_shel.py and sensor_shell.c uses hardcoded value and doesn't follow the sensor.h API.

@Sokario Sokario force-pushed the drivers/sensors/max30101 branch from 8086146 to 36ba139 Compare September 26, 2025 20:54
Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

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

Any chance you're planning to implement the sensor async API?

.slot = MAX30101_SLOT_CFG(n), \
}; \
static struct max30101_data max30101_data_##n; \
DEVICE_DT_INST_DEFINE(n, max30101_init, NULL, &max30101_data_##n, &max30101_config_##n, \
Copy link
Member

Choose a reason for hiding this comment

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

Use SENSOR_DEVICE_DT_INST_DEFINE

Comment on lines 81 to 130
smp-sr:
type: int
default: 50
enum:
- 50 # 50 Hz
- 100 # 100 Hz
- 200 # 200 Hz
- 400 # 400 Hz
- 800 # 800 Hz
- 1000 # 1000 Hz
- 1600 # 1600 Hz
- 3200 # 3200 Hz
description: |
Set the effective sampling rate with one sample consisting of one
pulse/conversion per active LED channel. In SpO2 mode, these means
one IR pulse/conversion and one red pulse/conversion per sample
period. Default set to 50 Hz.
led-pw:
type: int
default: 69
enum:
- 69 # 69 us | 15 bits resolution
- 118 # 118 us | 16 bits resolution
- 215 # 215 us | 17 bits resolution
- 411 # 411 us | 18 bits resolution
description: |
Set the pulse width for each LED to control the integration time
of the ADC in us. The ADC resolution is directly related to the
integration time. Default set to 69 us.
led-pa:
type: uint8-array
default: [0xff, 0xff, 0xff]
description: |
Set the pulse amplitude to control the LED current. The actual
measured LED current for each part can vary significantly due to the
trimming methodology. Default set to [0xff, 0xff, 0xff].
[0]: Red LED
[1]: IR LED
[2]: Green LED
Value range: 0x00 - 0xFF | 0.0 mA - 50.0 mA
led-slot:
type: array
default: [0, 0, 0, 0]
description: |
Set which LED are active in time each slot for Multi-LED mode only.
0: None (Disabled)
1: Red LED
2: InfraRed LED
3: Green LED
Default set to [0, 0, 0, 0].
Copy link
Member

Choose a reason for hiding this comment

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

Update the descriptions to explain why the specific default values were chosen. Often they are chosen to match hardware reset values

@Sokario Sokario force-pushed the drivers/sensors/max30101 branch from 36ba139 to f952e9f Compare September 30, 2025 15:14
@Sokario Sokario force-pushed the drivers/sensors/max30101 branch from 8eb7a25 to d3b946c Compare October 3, 2025 16:04
@Sokario
Copy link
Contributor Author

Sokario commented Oct 3, 2025

I had to modify once again the test_sensor_shell.py because of newly added commits adding flow rate.
I might be misunderstanding, but wasn't twister ran over the responsible commit in the CI @MaureenHelm ?
I changed the fixed channel number in test_sensor_shell_attr_set/get for its name, as it is supported in sensor_shell.c. It should allow anyone to add new channel whithout breaking the attr setter and getter of sensor_shell pytests.

EDIT: I understood why, it was because of the hard coded value of the channel used to test the attribute. My modification should fix any future issue.

EDIT 2: I also added a commit to automatically get the right number of channel to test in test_sensor_shell.py. This commit should prevent future issues. I updated the PR description.

@Sokario Sokario force-pushed the drivers/sensors/max30101 branch from 0309c38 to 76e2eb3 Compare October 7, 2025 09:45
@Sokario
Copy link
Contributor Author

Sokario commented Oct 7, 2025

While working on async API, I noticed SENSOR_TRIG_FIFO_FULL wasn't the best suited for FIFO_A_FULL interruption after all. I switched for SENSOR_TRIG_FIFO_WATERMARK wich better correspond to what FIFO_A_FULL interruption do (It allows to trigger to a set number of sample acquired in the max30101 fifo).

MaureenHelm
MaureenHelm previously approved these changes Oct 9, 2025
@kartben kartben requested a review from Copilot October 9, 2025 07:38
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for multiple instances, trigger functionality, and enhanced data acquisition for the MAX30101 heart rate sensor. It moves configuration from compile-time Kconfig options to device tree bindings, enabling multiple sensor instances with different configurations.

  • Replaced Kconfig-based configuration with device tree bindings for multi-instance support
  • Added interrupt/trigger support with new sensor channels and trigger types
  • Enhanced FIFO data handling with better channel mapping and optional die temperature acquisition

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
dts/bindings/sensor/maxim,max30101.yaml Device tree binding definition with configuration properties
drivers/sensor/maxim/max30101/max30101.c Refactored driver for device tree configuration and enhanced data handling
drivers/sensor/maxim/max30101/max30101.h Updated constants and data structures for multi-instance and trigger support
drivers/sensor/maxim/max30101/max30101_trigger.c New trigger/interrupt handling implementation
drivers/sensor/maxim/max30101/Kconfig Simplified Kconfig with trigger-specific options
include/zephyr/drivers/sensor.h Added new sensor channel and trigger type enums
samples/sensor/heart_rate/src/main.c Updated sample to demonstrate trigger functionality
tests/drivers/build_all/sensor/i2c.dtsi Added required device tree property for test
samples/sensor/sensor_shell/pytest/test_sensor_shell.py Made test more robust by dynamically detecting channel count

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

mask = MAX30101_INT_ALC_OVF_MASK;
index = MAX30101_ALC_CB_INDEX;
} else {
LOG_ERR("Only SENSOR_CHAN_LIGHT are supported for overflow trigger");
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

Corrected 'SENSOR_CHAN_LIGHT' to 'SENSOR_CHAN_AMBIENT_LIGHT' to match the actual check on line 95.

Suggested change
LOG_ERR("Only SENSOR_CHAN_LIGHT are supported for overflow trigger");
LOG_ERR("Only SENSOR_CHAN_AMBIENT_LIGHT is supported for overflow trigger");

Copilot uses AI. Check for mistakes.
return 0;
}

/* The work-queue is shared across all instances, hence it is initialized separatedly */
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

Corrected spelling of 'separatedly' to 'separately'.

Suggested change
/* The work-queue is shared across all instances, hence it is initialized separatedly */
/* The work-queue is shared across all instances, hence it is initialized separately */

Copilot uses AI. Check for mistakes.
Set the effective sampling rate with one sample consisting of one
pulse/conversion per active LED channel. In SpO2 mode, these means
one IR pulse/conversion and one red pulse/conversion per sample
period. Only one RED puls/conversion in HR mode.
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

Corrected spelling of 'puls' to 'pulse'.

Suggested change
period. Only one RED puls/conversion in HR mode.
period. Only one RED pulse/conversion in HR mode.

Copilot uses AI. Check for mistakes.
2: InfraRed LED
3: Green LED
Default set to [0, 0, 0, 0], no LED activated in multi-LED mode.
User needs to chose wich LED is active for each slot.
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

Corrected spelling: 'chose' should be 'choose' and 'wich' should be 'which'.

Suggested change
User needs to chose wich LED is active for each slot.
User needs to choose which LED is active for each slot.

Copilot uses AI. Check for mistakes.
The max30101 sensor driver doesn't support multiple instance.
Update Kconfig and maxim,max30101.yaml for instance based
configuration. Propagate changes over existing files.

Signed-off-by: Logan Saint-Germain <l.saintgermain@catie.fr>
@Sokario
Copy link
Contributor Author

Sokario commented Oct 9, 2025

Thank you @kartben for the review from copilot. I added the suggestions in the corresponding commits.

@Sokario Sokario force-pushed the drivers/sensors/max30101 branch 2 times, most recently from d2678b7 to ff33276 Compare October 13, 2025 08:55
The max30101 sensor driver doesn't support triggers.
Add `.trigger_set` API and corresponding Kconfig and
device tree parameters. Add `SENSOR_CHAN_AMBIENT_LIGHT`
and `SENSOR_TRIG_OVERFLOW`.

Signed-off-by: Logan Saint-Germain <l.saintgermain@catie.fr>
@Sokario Sokario force-pushed the drivers/sensors/max30101 branch from ff33276 to 7923c1f Compare October 13, 2025 09:01
MaureenHelm
MaureenHelm previously approved these changes Oct 17, 2025
positions are changed.
config MAX30101_DIE_TEMPERATURE
bool "Die temperature acquisition"
default n
Copy link
Member

Choose a reason for hiding this comment

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

default n is already the default and should therefore be omitted.

Copy link
Contributor Author

@Sokario Sokario Oct 20, 2025

Choose a reason for hiding this comment

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

Thank you, @maass-hamburg — it might have been an artifact from testing. Good catch, and thanks for taking the time to review it.
I’ve made the changes; now we just need to wait for the CI to finish.
EDIT: Thank you as well for triggering the CI — it’s much appreciated.

The max30101 allows to configure time slots in samples acquisition.
It is now supported by adding matrix mapping for the slot/fifo
indexing. When a channel is present multiple times, the resulting
sample from the `sensor_channel_get` is averaging each entry.
Added Die temperature sample acquisition with
`CONFIG_MAX30101_DIS_TEMPERATURE` Kconfig.

Signed-off-by: Logan Saint-Germain <l.saintgermain@catie.fr>
…tests

Use of the `sensor get` help. No channel provided allows the sensor_shell
to iterate through every channels. Getting the last channel gives the last
channel index and therefor the channel count. Provide futur proofing for
new channels.

Signed-off-by: Logan Saint-Germain <l.saintgermain@catie.fr>
@sonarqubecloud
Copy link

@cfriedt cfriedt merged commit 4ff0457 into zephyrproject-rtos:main Oct 20, 2025
27 checks passed
@github-actions
Copy link

Hi @Sokario!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

@Sokario
Copy link
Contributor Author

Sokario commented Oct 21, 2025

Thanks everyone for the reviews, feedback, and quick actions!
I really appreciate how efficiently the discussions and updates helped move this PR forward.
It’s a pleasure to contribute 👍

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.

5 participants