Skip to content

Conversation

@jovicjakov
Copy link

This PR aims to add driver support for the Bosch BMP180 digital pressure and temperature sensor.

@zephyrbot zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Sensors Sensors area: Samples Samples labels May 23, 2024
@github-actions
Copy link

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

Copy link
Contributor

@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.

some early comments. thanks for this new driver!

@jovicjakov jovicjakov force-pushed the bmp180 branch 4 times, most recently from bc89237 to c7ec360 Compare May 24, 2024 11:49
@jovicjakov jovicjakov requested a review from kartben May 24, 2024 13:03
@jovicjakov jovicjakov force-pushed the bmp180 branch 2 times, most recently from becc776 to ebcc7ac Compare May 24, 2024 20:25
@kartben kartben dismissed their stale review May 27, 2024 09:44

my earlier comments have been addressed - thanks!

@ubieda
Copy link
Member

ubieda commented Jun 14, 2024

@MaureenHelm it does appear everything's addressed. There's a conflict resolution required but seems trivial to fix.

ubieda
ubieda previously approved these changes Jun 14, 2024
@kartben
Copy link
Contributor

kartben commented Jun 14, 2024

@MaureenHelm it does appear everything's addressed. There's a conflict resolution required but seems trivial to fix.

@ubieda feel free to directly push to the branch to save some time :)

ubieda
ubieda previously approved these changes Jun 14, 2024
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.

Re-approving after rebasing

@ubieda ubieda added this to the v3.7.0 milestone Jun 15, 2024
teburd
teburd previously approved these changes Jun 15, 2024
@aescolar aescolar modified the milestones: v3.7.0, v4.0.0 Jun 17, 2024
@aescolar
Copy link
Member

Bulk changing the milestone from everything which did not make it to the v3.7.0 freeze deadline
and which is not labeled as a bug or documentation (or does not appear clearly as only containing
a bug fix or a documentation change).
If you disagree about this change please add the milestone again.

Please note that until rc2 only PRs containing bug fixes, docs and tests can be merged.
After that and until release only bug fixes and docs changes.

Exceptions require approval by the release team and a vote by the TSC.
https://docs.zephyrproject.org/latest/project/release_process.html#stabilization-phase

@aescolar
Copy link
Member

@jovicjakov rebase needed

The Bosch BMP180 is a sensor that measures pressure and temperature.

Signed-off-by: Jakov Jović <[email protected]>
Add bindings for the Bosch BMP180 pressure and temperature sensor.

Signed-off-by: Jakov Jović <[email protected]>
Add the new BMP180 drivers to the test suite.

Signed-off-by: Jakov Jović <[email protected]>
@kartben
Copy link
Contributor

kartben commented Aug 13, 2024

rebased to resolve conflict

@aescolar
Copy link
Member

@MaureenHelm please re-review

@mariopaja
Copy link
Contributor

This driver can also be used on BMP085. Maybe it can be documented somewhere since #67612 was blocked

val->val2 = data->temp % 10 * 100000;
break;
case SENSOR_CHAN_PRESS:
/* Pressure is calculated in steps of 1 Pa */

Choose a reason for hiding this comment

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

CHAN PRESSURE for the Senor API should be in kPa not Pa.

int32_t b6 = b5 - 4000;

x1 = (data->b2 * ((b6 * b6) >> 12)) >> 11;
x2 = (data->ac2 * b6) >> 11;
Copy link

@ruehlchris ruehlchris Aug 18, 2024

Choose a reason for hiding this comment

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

you might want to simplify from the original data-sheet example to
x1 = ((b2 * (b6 * b6)>>12))>>11;
x1 = ((b2 * b6 * b6) / pow(2,12) )/ pow(2,11)
x1 = (b2 * b6 *b6) / (pow(2,12) * pow(2,11))
final:
x1 = (b2 * b6 * b6) / 0x800000;

} else {
p = (b7 / b4) * 2;
}

Choose a reason for hiding this comment

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

I wondered the if (b7< xx) statement here from the datasheet example
math wise it is exactly same only a data overflow can happen if b7 is too big
so b7/b4 *2 would always be safe and no need for a if()

@ruehlchris
Copy link

Hi @jovicjakov , it is a pity that I missed you PR request and did my own implementation.
Not a problem but wast a bit of time. I saw in your implementation a lot of the original code from the datasheet which are not optimal (IMHO). I add some comments to your code lately.
As well, you exit with an error if the command ready bit(5) not 0 rather wait for the convention is ready.
The Sensor API wants the Pressure return as kPa your implementation return Pa.
Please have a look into my PR and use it to your benefit. #77182

@jovicjakov jovicjakov closed this Sep 17, 2024
@jovicjakov jovicjakov deleted the bmp180 branch September 17, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Devicetree Binding PR modifies or adds a Device Tree binding area: Samples Samples area: Sensors Sensors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants