Skip to content

Conversation

@jeronimoagullo
Copy link
Contributor

Product Homepage:

https://www.rohm.com/

DataSheet:
https://www.farnell.com/datasheets/1813320.pdf

Testing Environment:
STMicroelectronics B_L4S5I_IOT01A Discovery kit

Signed-off-by: Jeronimo Agullo Ocampos [email protected]

@zephyrbot zephyrbot added area: Sensors Sensors area: Devicetree Binding PR modifies or adds a Device Tree binding labels Jan 16, 2023
@zephyrbot zephyrbot requested review from avisconti and teburd January 16, 2023 08:52
@galak
Copy link
Contributor

galak commented Jan 18, 2023

DT binding looks ok.

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.

Thank you for the contribution!

Please add this sensor to tests/drivers/build_all/sensor to ensure it builds in CI, and also address requested changes by amending your comments and force pushing.

@MaureenHelm
Copy link
Member

Please implement requested changes by amending the original commit rather than adding new commits. You also need to rebase to resolve merge conflicts.

@jeronimoagullo
Copy link
Contributor Author

Please implement requested changes by amending the original commit rather than adding new commits. You also need to rebase to resolve merge conflicts.

I have added the modifitions according to comments and then stashed all commits into the original commit. Then, I have merge, however, the merge has provoked a new commit. I am not sure if it is rigth, may you confirm it? Thank you in advance

@MaureenHelm
Copy link
Member

Please implement requested changes by amending the original commit rather than adding new commits. You also need to rebase to resolve merge conflicts.

I have added the modifitions according to comments and then stashed all commits into the original commit. Then, I have merge, however, the merge has provoked a new commit. I am not sure if it is rigth, may you confirm it? Thank you in advance

No, you need to rebase on the command line, not in the GH web interface, to remove the merge commit

@jeronimoagullo
Copy link
Contributor Author

Please implement requested changes by amending the original commit rather than adding new commits. You also need to rebase to resolve merge conflicts.

I have added the modifitions according to comments and then stashed all commits into the original commit. Then, I have merge, however, the merge has provoked a new commit. I am not sure if it is rigth, may you confirm it? Thank you in advance

No, you need to rebase on the command line, not in the GH web interface, to remove the merge commit

Ok, I understood. I think that it is now correct. It has passed all automated tests. Please, check it out. Thank you.

@jeronimoagullo jeronimoagullo requested review from MaureenHelm and removed request for avisconti, galak and nashif January 24, 2023 08:20
Comment on lines 650 to 653
Copy link
Member

Choose a reason for hiding this comment

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

Please set the address to 0x55 to make it sequential. Note this is a build test that will never run on real hardware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I see it. However, the previous one test_i2c_as5600 is incorrect and should be 64 to follow the correct numbering. Thus, I propose to set the test_i2c_bh1750 address to 0x65.

@zephyrbot zephyrbot requested review from avisconti and galak January 24, 2023 17:36
@jeronimoagullo jeronimoagullo force-pushed the bh1750_sensor_driver branch 2 times, most recently from d9ab80e to 9efa211 Compare January 25, 2023 09:23
@jeronimoagullo jeronimoagullo force-pushed the bh1750_sensor_driver branch 2 times, most recently from 57dd6bb to 98ccee9 Compare January 25, 2023 16:10
MaureenHelm
MaureenHelm previously approved these changes Jan 26, 2023
@jeronimoagullo jeronimoagullo requested review from teburd and removed request for avisconti and galak January 26, 2023 15:59
@teburd teburd requested a review from yperess January 27, 2023 20:10
@jeronimoagullo
Copy link
Contributor Author

Hi @MaureenHelm and @teburd, it's some days since my latest changes and it passed all tests, I would like to know if something else is missing from my side. Thank you in advance.

teburd
teburd previously approved these changes Feb 2, 2023
@MaureenHelm
Copy link
Member

Hi @MaureenHelm and @teburd, it's some days since my latest changes and it passed all tests, I would like to know if something else is missing from my side. Thank you in advance.

We're currently in feature freeze and need to wait until the merge window reopens after the release.
https://github.com/zephyrproject-rtos/zephyr/wiki/Release-Management

@stephanosio stephanosio added this to the v3.4.0 milestone Feb 7, 2023
@fabiobaltieri
Copy link
Member

@jeronimoagullo can you rebase please?

@jeronimoagullo
Copy link
Contributor Author

@jeronimoagullo can you rebase please?

I have just rebased and pushed. I have merged my contribution with the commit of same sensor with which it was conflicting

@carlescufi carlescufi requested review from MaureenHelm and teburd March 1, 2023 07:57
@carlescufi
Copy link
Member

@jeronimoagullo can you fix the compliance checks:

TRAILING_WHITESPACE: trailing whitespace
26
File:drivers/sensor/bh1750/bh1750.c
27
Line:132
28
 SPACING: space required after that ',' (ctx:VxV)
29
File:drivers/sensor/bh1750/bh1750.c
30
Line:138
31
 SPACING: space required after that ',' (ctx:VxV)
32
File:drivers/sensor/bh1750/bh1750.c
33
Line:138
34
 SPACING: space required after that ',' (ctx:VxV)
35
File:drivers/sensor/bh1750/bh1750.c
36
Line:139
37
 SPACING: space required after that ',' (ctx:VxV)
38
File:drivers/sensor/bh1750/bh1750.c
39
Line:144
40
 TRAILING_WHITESPACE: trailing whitespace
41
File:drivers/sensor/bh1750/bh1750.c
[42](https://github.com/zephyrproject-rtos/zephyr/actions/runs/4295302648/jobs/7498324895#step:9:43)
Line:148
[43](https://github.com/zephyrproject-rtos/zephyr/actions/runs/4295302648/jobs/7498324895#step:9:44)
Error: Process completed with exit code 1.

Update Rohm bh1750 ambient light sensor driver.

Signed-off-by: Jeronimo Agullo <[email protected]>
@jeronimoagullo
Copy link
Contributor Author

@jeronimoagullo can you fix the compliance checks:

TRAILING_WHITESPACE: trailing whitespace
26
File:drivers/sensor/bh1750/bh1750.c
27
Line:132
28
 SPACING: space required after that ',' (ctx:VxV)
29
File:drivers/sensor/bh1750/bh1750.c
30
Line:138
31
 SPACING: space required after that ',' (ctx:VxV)
32
File:drivers/sensor/bh1750/bh1750.c
33
Line:138
34
 SPACING: space required after that ',' (ctx:VxV)
35
File:drivers/sensor/bh1750/bh1750.c
36
Line:139
37
 SPACING: space required after that ',' (ctx:VxV)
38
File:drivers/sensor/bh1750/bh1750.c
39
Line:144
40
 TRAILING_WHITESPACE: trailing whitespace
41
File:drivers/sensor/bh1750/bh1750.c
[42](https://github.com/zephyrproject-rtos/zephyr/actions/runs/4295302648/jobs/7498324895#step:9:43)
Line:148
[43](https://github.com/zephyrproject-rtos/zephyr/actions/runs/4295302648/jobs/7498324895#step:9:44)
Error: Process completed with exit code 1.

Thank you for your comments, it's now done.

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.

sensor: Rohm bh1750 ambient light sensor

Update Rohm bh1750 ambient light sensor driver.

Signed-off-by: Jeronimo Agullo <[email protected]>

Please explain why you are updating the driver in the commit message.

# BH1750 ambient light sensor configuration options

# Copyright (c) 2022 Michal Morsisko
# Copyright (c) 2023 Sngular People SL. Jeronimo Agullo
Copy link
Member

Choose a reason for hiding this comment

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

You haven't made any changes to this file; don't add your copyright.

* for more information how to convert raw sample to lx
*/
tmp = (drv_data->sample * 1000 / 12) * (BH1750_DEFAULT_MTREG * 100 / cfg->mtreg);
if (attr == SENSOR_ATTR_CONFIGURATION) {
Copy link
Member

Choose a reason for hiding this comment

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

This could benefit from a more meaningful name, something like MEASUREMENT_MODE

@github-actions
Copy link

github-actions bot commented May 1, 2023

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label May 1, 2023
@github-actions github-actions bot closed this May 15, 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: Sensors Sensors Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants