Skip to content

Conversation

@morsisko
Copy link
Contributor

@morsisko morsisko commented Feb 22, 2022

This pull request adds support for BH1750 ambient light sensor along with sample application.
For now this driver features only one-time mode. If this version looks good for you, I can try to implement continuous mode in future pull request, although it will be tricky, because this sensor hasn't got physical GPIO INT, so the trigger would need to be "emulated" (I discussed it at Discord and it turns out that using delayed work could be a nice idea, but I don't think any other Zephyr sensor driver implements it)

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

Seems like some files have formatting issues (missing EOF blank line)

@mbolivar-nordic
Copy link
Contributor

We might want to combine efforts between the downstream NCS version of the BH1749 (nrfconnect/sdk-nrf#6799) and this if they are similar enough.

@morsisko
Copy link
Contributor Author

mbolivar-nordic From what I see BH1749 is far more sophisticated (allows to grab individual RGB and IR values, dedicated hardware INT), not sure if the sensors are similar enough to merge those drivers into one

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

-1 on the binding's use of defaults. I didn't review the rest.

@morsisko morsisko force-pushed the bh1750 branch 2 times, most recently from 15f972c to 9932539 Compare March 26, 2022 14:43
@morsisko
Copy link
Contributor Author

The justification is added, conflicts are resolved.
@mbolivar-nordic Could you check if the justification meets your expectation?

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

Looks reasonable, thanks

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.

Thanks for your contribution.

Please add this sensor to tests/drivers/build_all/sensor

@morsisko morsisko requested review from gmarull and removed request for teburd and yperess January 28, 2023 13:56
gmarull
gmarull previously approved these changes Jan 31, 2023
MaureenHelm
MaureenHelm previously approved these changes Jan 31, 2023
@stephanosio stephanosio added this to the v3.4.0 milestone Jan 31, 2023
@morsisko
Copy link
Contributor Author

morsisko commented Feb 3, 2023

Should I resolve the conflict or will it be resolved by the integrator? I think if I resolve it then another one will appear soon (as this is conflict in the test file)

@MaureenHelm
Copy link
Member

Should I resolve the conflict or will it be resolved by the integrator? I think if I resolve it then another one will appear soon (as this is conflict in the test file)

Yes, please rebase and resolve the conflict.

This commit adds support for BH1750 ambient light sensor.
The driver works using I2C peripheral in one-time mode.

Signed-off-by: Michal morsisko <[email protected]>
@morsisko
Copy link
Contributor Author

morsisko commented Feb 3, 2023

@MaureenHelm ok, rebased

@morsisko morsisko requested review from MaureenHelm and gmarull and removed request for avisconti, galak, gmarull, teburd and yperess February 3, 2023 22:47
@morsisko morsisko requested a review from gmarull February 13, 2023 21:44
@nashif nashif merged commit 2e4d876 into zephyrproject-rtos:main Feb 20, 2023
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: Devicetree area: Documentation area: Samples Samples area: Sensors Sensors area: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants