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.

Sign up for GitHub

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

Jump to bottom

drivers: sensor: bmi08x: fix temperature interface and trigger setting #82405

Merged
merged 1 commit into from
Jan 15, 2025
Merged

drivers: sensor: bmi08x: fix temperature interface and trigger setting #82405

merged 1 commit into from
Jan 15, 2025

Conversation

ttwards
Copy link
Contributor

@ttwards ttwards commented Dec 2, 2024 •
edited
Loading

  1. Temperature Interface
    According to BMI08x datasheet, temperature reading
    requires both MSB and LSB bytes to be read and
    processed correctly.

Temp data processing should follow the formula:
Temp in °C = (temp_msb * 8) + (temp_lsb / 32)

This patch implements the correct reading
sequence and calculation method as specified
in the datasheet.

  1. Trigger Setting
    Previously we set handler and then trigger struct.
    However under some situation, as long as we set
    the handler, we get into ISR immediately and never
    set trigger struct.
    I simply changed the sequence.

Testing:

  • Verified temperature readings match datasheet
  • Tested on stm32f407igh board with BMI08x sensor

Datasheet:
截屏2024-12-01 16 21 40
截屏2024-12-01 16 21 59

@zephyrbot zephyrbot added the area: Sensors Sensors label Dec 2, 2024
@ttwards
Copy link
Contributor Author

ttwards commented Dec 3, 2024

Code Lints and commit lints are fixed, please check again

@XenuIsWatching
Copy link
Member

XenuIsWatching commented Dec 6, 2024 •
edited
Loading

remove the merge commit, you must always rebase, and this should all only be 1 commit

@ttwards ttwards force-pushed the fix-bmi08x-sensor-driver branch from 4ff0197 to 0e2d95d Compare December 9, 2024 03:13
@ttwards
Copy link
Contributor Author

ttwards commented Dec 9, 2024

remove the merge commit, you must always rebase, and this should all only be 1 commit

I've removed, thank you.

@ttwards ttwards force-pushed the fix-bmi08x-sensor-driver branch 2 times, most recently from 16c07a2 to 774add6 Compare December 9, 2024 05:45
XenuIsWatching
XenuIsWatching requested changes Dec 9, 2024
View reviewed changes
Copy link
Member

@XenuIsWatching XenuIsWatching left a comment

Choose a reason for hiding this comment

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

please fix the compliance issues

@ttwards ttwards force-pushed the fix-bmi08x-sensor-driver branch from 774add6 to 1c6d69e Compare December 10, 2024 06:30
XenuIsWatching
XenuIsWatching previously approved these changes Dec 10, 2024
View reviewed changes
@ttwards
Copy link
Contributor Author

ttwards commented Dec 13, 2024

Hmmmm, can anybody please tell me why there are 4 pending checks still...?

@kartben
Copy link
Collaborator

kartben commented Dec 13, 2024

Hmmmm, can anybody please tell me why there are 4 pending checks still...?

A maintainer needs to kick the CI manually for contributions coming from new contributors. I just did :) thanks!

@ttwards ttwards force-pushed the fix-bmi08x-sensor-driver branch from 1c6d69e to 30b8d10 Compare December 13, 2024 03:08
XenuIsWatching
XenuIsWatching previously approved these changes Dec 13, 2024
View reviewed changes
@ttwards ttwards force-pushed the fix-bmi08x-sensor-driver branch from 30b8d10 to 1c6d69e Compare December 14, 2024 08:33
XenuIsWatching
XenuIsWatching previously approved these changes Dec 26, 2024
View reviewed changes
ubieda
ubieda previously approved these changes Dec 31, 2024
View reviewed changes
@ubieda
Copy link
Member

ubieda commented Dec 31, 2024

Thanks for your contribution!

@XenuIsWatching
Copy link
Member

@MaureenHelm Please review

avisconti
avisconti reviewed Jan 8, 2025
View reviewed changes
Copy link
Collaborator

@avisconti avisconti left a comment

Choose a reason for hiding this comment

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

LGTM, but just few suggestions to improve the code

drivers/sensor/bosch/bmi08x/bmi08x_accel.c Show resolved Hide resolved
drivers/sensor/bosch/bmi08x/bmi08x_accel.c Show resolved Hide resolved
drivers/sensor/bosch/bmi08x/bmi08x_accel.c Show resolved Hide resolved
drivers/sensor/bosch/bmi08x/bmi08x_accel.c Show resolved Hide resolved
@ttwards ttwards dismissed stale reviews from ubieda and XenuIsWatching via 47d9e45 January 9, 2025 10:20
@ttwards ttwards force-pushed the fix-bmi08x-sensor-driver branch from d66b23f to 47d9e45 Compare January 9, 2025 10:20
avisconti
avisconti previously approved these changes Jan 9, 2025
View reviewed changes
@ttwards ttwards force-pushed the fix-bmi08x-sensor-driver branch from 47d9e45 to cc4241a Compare January 9, 2025 10:29
avisconti
avisconti previously approved these changes Jan 9, 2025
View reviewed changes
@avisconti
Copy link
Collaborator

Pls fix the CI

@ttwards ttwards force-pushed the fix-bmi08x-sensor-driver branch from cc4241a to 3200e00 Compare January 10, 2025 05:14
@ttwards
Copy link
Contributor Author

ttwards commented Jan 10, 2025

Pls fix the CI

Sorry for that, clang-format in my local workspace doesn't meet the zephyr project requirements very well.

avisconti
avisconti previously approved these changes Jan 10, 2025
View reviewed changes
@ttwards ttwards force-pushed the fix-bmi08x-sensor-driver branch from 3200e00 to 6a9c8cb Compare January 10, 2025 09:30
@ttwards
drivers: sensor: bmi08x: fix interfaceand trigger
13b045b 
1. Temperature Interface
According to BMI08x datasheet, temperature reading
requires both MSB and LSB bytes to be read and
processed correctly.

Temp data processing should follow the formula:
Temp in °C = (temp_msb * 8) + (temp_lsb / 32)

This patch implements the correct reading
sequence and calculation method as specified
in the datasheet.

2. Trigger Setting
Previously we set handler and then trigger struct.
However under some situation, as long as we set
the handler, we get into ISR immediately and never
set trigger struct.
I simply changed the sequence.

Testing:
- Verified temperature readings match datasheet
- Tested on stm32f407igh board with BMI08x sensor

Fixes: zephyrproject-rtos#82375

Signed-off-by: Wenxi Xu <xuwenxi0517@gmail.com>
@ttwards ttwards force-pushed the fix-bmi08x-sensor-driver branch from 6a9c8cb to 13b045b Compare January 10, 2025 09:34
@ttwards ttwards changed the title drivers: sensor: bmi08x: fix temperature data reading and processing drivers: sensor: bmi08x: fix temperature interface and trigger setting Jan 10, 2025
ubieda
ubieda approved these changes Jan 15, 2025
View reviewed changes
Copy link
Member

@ubieda ubieda left a comment

Choose a reason for hiding this comment

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

Refreshing my +1

@kartben kartben merged commit a490c90 into zephyrproject-rtos:main Jan 15, 2025
26 checks passed
@github-actions GitHub Actions
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ubieda ubieda ubieda approved these changes

@MaureenHelm MaureenHelm MaureenHelm approved these changes

@avisconti avisconti avisconti approved these changes

@jeppenodgaard jeppenodgaard Awaiting requested review from jeppenodgaard

@teburd teburd Awaiting requested review from teburd

@tristan-google tristan-google Awaiting requested review from tristan-google

@yperess yperess Awaiting requested review from yperess

@asemjonovs asemjonovs Awaiting requested review from asemjonovs

@kartben kartben Awaiting requested review from kartben

@XenuIsWatching XenuIsWatching Awaiting requested review from XenuIsWatching

Assignees

@MaureenHelm MaureenHelm

Labels
area: Sensors Sensors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ttwards @XenuIsWatching @kartben @ubieda @avisconti @MaureenHelm @zephyrbot