Skip to content

drivers: sensor: bma4xx: Implement streaming APIs#83575

Merged
kartben merged 6 commits into
zephyrproject-rtos:mainfrom
cpeggy-25:add_bma4xx_streaming_apis
Apr 2, 2025
Merged

drivers: sensor: bma4xx: Implement streaming APIs#83575
kartben merged 6 commits into
zephyrproject-rtos:mainfrom
cpeggy-25:add_bma4xx_streaming_apis

Conversation

@cpeggy-25

@cpeggy-25 cpeggy-25 commented Jan 6, 2025

Copy link
Copy Markdown

Add a streaming implementation for the BMA4XX using both FIFO watermark and FIFO full interrupts.
A batch duration of 3,000 have been used for verification.

uart:~$ sensor attr_set bma4xx@19 all batch_dur 3000
bma4xx@19 channel=all, attr=batch_dur set to value=3000
[00:00:03.111,602] <inf> bma4xx: FIFO ENABLED
uart:~$ sensor stream bma4xx@19 on fifo_wm incl
Enabling stream...
channel type=0(accel_x) index=0 shift=6 num_samples=11 value=12784741210ns (1.398213)
channel type=1(accel_y) index=0 shift=6 num_samples=11 value=12784741210ns (1.398213)
channel type=2(accel_z) index=0 shift=6 num_samples=11 value=12784741210ns (1.398213)
channel type=3(accel_xyz) index=0 shift=6 num_samples=11 value=12784741210ns, (1.398213, 0.315163, 9.528051)
channel type=0(accel_x) index=0 shift=6 num_samples=10 value=12879747314ns (1.403959)
channel type=1(accel_y) index=0 shift=6 num_samples=10 value=12879747314ns (1.403959)
channel type=2(accel_z) index=0 shift=6 num_samples=10 value=12879747314ns (1.403959)
channel type=3(accel_xyz) index=0 shift=6 num_samples=10 value=12879747314ns, (1.403959, 0.256658, 9.482953)
channel type=0(accel_x) index=0 shift=6 num_samples=10 value=12979814453ns (1.415452)
channel type=1(accel_y) index=0 shift=6 num_samples=10 value=12979814453ns (1.415452)
channel type=2(accel_z) index=0 shift=6 num_samples=10 value=12979814453ns (1.415452)
channel type=3(accel_xyz) index=0 shift=6 num_samples=10 value=12979814453ns, (1.415452, 0.342849, 9.525091)
…
…
…

@github-actions

github-actions Bot commented Jan 6, 2025

Copy link
Copy Markdown

Hello @PeggyCienet, 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. 😊

@teburd teburd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me, the board overlay for the shell is a bit off though. I don't think we have a great way of dealing with this sort of thing unfortunately. Maybe @MaureenHelm will have some other thoughts, happy to approve once the shell board overlay has been discussed as the RTIO and sensor related changes look good to me.

@cpeggy-25 cpeggy-25 force-pushed the add_bma4xx_streaming_apis branch 7 times, most recently from 5b17b1a to d1c62ce Compare January 16, 2025 08:09

@yperess yperess left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you include a sample output in the PR description?

Comment thread drivers/sensor/bosch/bma4xx/CMakeLists.txt Outdated
Comment thread drivers/sensor/bosch/bma4xx/bma4xx_interrupt.h Outdated
Comment thread drivers/sensor/bosch/bma4xx/bma4xx.c Outdated
Comment thread drivers/sensor/bosch/bma4xx/bma4xx.h Outdated
@cpeggy-25 cpeggy-25 force-pushed the add_bma4xx_streaming_apis branch from d1c62ce to e13a97e Compare March 4, 2025 03:50

@MaureenHelm MaureenHelm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Each commit should correspond to a self-contained, meaningful change. For example, adding a feature, fixing a bug, or refactoring existing code should be separate commits. Avoid mixing different types of changes (e.g., feature implementation and unrelated refactoring) in the same commit.
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#pr-requirements

Comment thread drivers/sensor/bosch/bma4xx/bma4xx_spi.c
Comment thread drivers/sensor/bosch/bma4xx/Kconfig Outdated
@cpeggy-25 cpeggy-25 force-pushed the add_bma4xx_streaming_apis branch 5 times, most recently from 4f6cb65 to 5b7378c Compare March 17, 2025 04:01
@cpeggy-25 cpeggy-25 marked this pull request as draft March 26, 2025 13:38
@cpeggy-25 cpeggy-25 force-pushed the add_bma4xx_streaming_apis branch 2 times, most recently from eff8dfc to 04d9c76 Compare March 29, 2025 12:05

@yperess yperess left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The last commit is still a bit large, it seems like we could split it up to

  1. moving the common code and decoder to the separate files. This would be an easy change to review since it should be all cut/paste
  2. implementing the streaming logic

This will allow reviewers to focus on the new logic during the review. Overall this is looking really good.

Comment thread drivers/sensor/bosch/bma4xx/bma4xx.c Outdated
Comment thread drivers/sensor/bosch/bma4xx/bma4xx_common.c Outdated
Comment thread drivers/sensor/bosch/bma4xx/bma4xx.h Outdated

@yperess yperess left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Previous comment should have been a request for changes, sorry.

@cpeggy-25 cpeggy-25 marked this pull request as ready for review March 30, 2025 04:13
@cpeggy-25 cpeggy-25 force-pushed the add_bma4xx_streaming_apis branch 3 times, most recently from beb5f64 to a540bc7 Compare March 30, 2025 08:13
Peggy Chen added 6 commits March 31, 2025 07:59
Added register definitions, bit masks, and related constants
for the BMA4XX sensor in a separate header file.

Signed-off-by: Peggy Chen <peggy.chen@cienet.com>
Updated the sensor configuration to use the bma4xx_safely_configure
function for safer and more reliable initialization.

Signed-off-by: Peggy Chen <peggy.chen@cienet.com>
Refactor the sensor decoder code into individual files for
improved maintainability and readability.

Signed-off-by: Peggy Chen <peggy.chen@cienet.com>
Refactor the sensor submit logic into individual files
to enhance maintainability and readability.

Signed-off-by: Peggy Chen <peggy.chen@cienet.com>
Enable the sensor emulator and address related issues
to ensure proper functionality.

Signed-off-by: Peggy Chen <peggy.chen@cienet.com>
Add a streaming implementation for the BMA4XX using both
FIFO watermark and FIFO full interrupts. A batch duration of
3,000 have been used for verification.

Signed-off-by: Peggy Chen <peggy.chen@cienet.com>
@cpeggy-25 cpeggy-25 force-pushed the add_bma4xx_streaming_apis branch from a540bc7 to 85c13f3 Compare March 31, 2025 00:13
@cpeggy-25

Copy link
Copy Markdown
Author

Hi @yperess, @MaureenHelm, @teburd,

I've updated this PR and separated the changes into 6 commits. However, it shows This workflow requires approval from a maintainer. Could you please help approve it so the remaining checks can be completed? Thank you!

@cpeggy-25 cpeggy-25 requested review from teburd and yperess March 31, 2025 07:44
@cpeggy-25

Copy link
Copy Markdown
Author

Hi @yperess, @MaureenHelm, @teburd,

I have made the necessary updates to the PR based on your feedback and have organized the changes into 6 separate commits. Whenever you have a moment, could you kindly review this PR?

Thank you!

@kartben kartben merged commit 9faa7b0 into zephyrproject-rtos:main Apr 2, 2025
@github-actions

github-actions Bot commented Apr 2, 2025

Copy link
Copy Markdown

Hi @PeggyCienet!
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! 🪁

@cpeggy-25 cpeggy-25 deleted the add_bma4xx_streaming_apis branch April 2, 2025 12:47
valeriosetti added a commit to valeriosetti/zephyr that referenced this pull request Apr 2, 2025
Since zephyrproject-rtos#83575 bma4xx driver relies on RTIO to work. This means that
CONFIG_I2C_RTIO should be defined together with CONFIG_I2C in the
Kconfig, otherwise I2C_DT_IODEV_DEFINE() won't be available at
build time.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
kartben pushed a commit that referenced this pull request Apr 4, 2025
Since #83575 bma4xx driver relies on RTIO to work. This means that
CONFIG_I2C_RTIO should be defined together with CONFIG_I2C in the
Kconfig, otherwise I2C_DT_IODEV_DEFINE() won't be available at
build time.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
abrarnasirjaffari pushed a commit to abrarnasirjaffari/zephyr that referenced this pull request Apr 9, 2025
Since zephyrproject-rtos#83575 bma4xx driver relies on RTIO to work. This means that
CONFIG_I2C_RTIO should be defined together with CONFIG_I2C in the
Kconfig, otherwise I2C_DT_IODEV_DEFINE() won't be available at
build time.

Signed-off-by: Valerio Setti <vsetti@baylibre.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