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

Bluetooth: Mesh: Add timeout for ambient light level report #11579

Merged
merged 2 commits into from
Jun 30, 2023
Merged

Bluetooth: Mesh: Add timeout for ambient light level report #11579

merged 2 commits into from
Jun 30, 2023

Conversation

PavelVPV
Copy link
Contributor

Add timeout for the last ambient light level report received from a
sensor. This will make the regulator drive the lightness value high up
to avoid being stalled in some old value when no reports are coming from
the sensor.

Test that ambient light level timer times out correctly.

@github-actions github-actions bot added ble mesh Label for ble mesh PRbot. Add this if PR is related to ble mesh and you need to get review. doc-required PR must not be merged without tech writer approval. labels Jun 20, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jun 20, 2023 •
edited
Loading

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
test-fw-nrfconnect-ble_mesh X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

ludvigsj
ludvigsj requested changes Jun 20, 2023
View reviewed changes
subsys/bluetooth/mesh/Kconfig.models Outdated
@@ -289,6 +289,17 @@ config BT_MESH_LIGHT_CTRL_REG_SPEC_INTERVAL
Update interval of the specification-defined illuminance regulator (in milliseconds).

endif #BT_MESH_LIGHT_CTRL_REG_SPEC

config BT_MESH_LIGHT_CTRL_AMB_LIGHT_LEVEL_TIMEOUT
int "Ambient light level report timeout"
Copy link
Contributor

@ludvigsj ludvigsj Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Specify unit for this somewhere? Not obvious at all now that it is seconds. Also, a 5 second default sounds like it's very short? I would expect more like several minutes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your suggestion on the default value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the task in JIRA says "(say 5 minutes default, set by KConfig option)" which sound more sensible to me.

@PavelVPV PavelVPV force-pushed the add_amb_light_level_timeout branch 2 times, most recently from 55637f7 to 337f2d0 Compare June 20, 2023 13:18
@PavelVPV PavelVPV requested a review from ludvigsj June 20, 2023 13:18
mia-ko
mia-ko requested changes Jun 21, 2023
View reviewed changes
doc/nrf/releases/release-notes-changelog.rst Outdated
@@ -314,6 +314,10 @@ Bluetooth libraries and services

* The :kconfig:option:`BT_MESH_MODEL_SRV_STORE_TIMEOUT` Kconfig option, that is controlling timeout for storing of model states, is replaced by the :kconfig:option:`BT_MESH_STORE_TIMEOUT` Kconfig option.

* Added:

* Add a new Kconfig option, :kconfig:option:`BT_MESH_LIGHT_CTRL_AMB_LIGHT_LEVEL_TIMEOUT`, that configures timeout to reset the ambient light level to zero.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Add a new Kconfig option, :kconfig:option:`BT_MESH_LIGHT_CTRL_AMB_LIGHT_LEVEL_TIMEOUT`, that configures timeout to reset the ambient light level to zero.
* The :kconfig:option:`BT_MESH_LIGHT_CTRL_AMB_LIGHT_LEVEL_TIMEOUT` Kconfig option that configures a timeout to reset the ambient light level to zero.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, move Added list to the top, above Updated list.

subsys/bluetooth/mesh/Kconfig.models Outdated
Comment on lines 298 to 301
Defines a time since last received sensor report with the ambient light level after which
the Light Lightness Control Server sets the ambient light level to zero and passes it to
the regulator. This option helps to avoid an issue when the regulator gets stuck with an
old output value, which doesn't trigger new report from sensor.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Defines a time since last received sensor report with the ambient light level after which
the Light Lightness Control Server sets the ambient light level to zero and passes it to
the regulator. This option helps to avoid an issue when the regulator gets stuck with an
old output value, which doesn't trigger new report from sensor.
Defines time in seconds the Light Lightness Control Server waits after receiving the sensor report with the
ambient light level before setting the ambient light level to zero and passing it to the regulator.
This option helps avoiding an issue with the regulator getting stuck with an old output value, and not
triggering new report from the sensor.

@PavelVPV PavelVPV force-pushed the add_amb_light_level_timeout branch from 337f2d0 to 0f28eae Compare June 21, 2023 12:09
@PavelVPV PavelVPV requested a review from mia-ko June 21, 2023 12:09
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@PavelVPV PavelVPV force-pushed the add_amb_light_level_timeout branch from 0f28eae to 4c12292 Compare June 21, 2023 14:59
@PavelVPV PavelVPV requested a review from Balaklaka June 21, 2023 15:01
mia-ko
mia-ko reviewed Jun 22, 2023
View reviewed changes
subsys/bluetooth/mesh/Kconfig.models Outdated
range 0 16777
help
Defines time in seconds the Light Lightness Control Server waits after receiving the
Sensor Status message with the present ambient light level property id before setting
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Sensor Status message with the present ambient light level property id before setting
Sensor Status message with the present ambient light level property ID before setting

doc/nrf/releases/release-notes-changelog.rst Outdated
@@ -310,6 +310,10 @@ Bluetooth libraries and services

* :ref:`bt_mesh` library:

* Added:

* The :kconfig:option:`BT_MESH_LIGHT_CTRL_AMB_LIGHT_LEVEL_TIMEOUT` Kconfig option that configures a timeout to reset the ambient light level to zero.
Copy link
Contributor

Choose a reason for hiding this comment

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

timeout to reset
or timeout before resetting?

To me the second one sounds better. What do you think?
Strange I did not pick up on this yesterday.

@PavelVPV PavelVPV force-pushed the add_amb_light_level_timeout branch from 4c12292 to 967ce41 Compare June 22, 2023 07:39
@PavelVPV PavelVPV requested a review from mia-ko June 22, 2023 07:39
@PavelVPV
Bluetooth: Mesh: Add timeout for ambient light level report
3f824f1 
Add timeout for the last ambient light level report received from a
sensor. This will make the regulator drive the lightness value high up
to avoid being stalled in some old value when no reports are coming from
the sensor.

Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no>
@PavelVPV
tests: Bluetooth: Mesh: Test ambient light level timeout
6d3588b 
Test that ambient light level timer times out correctly.

Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no>
@PavelVPV PavelVPV force-pushed the add_amb_light_level_timeout branch from 967ce41 to 6d3588b Compare June 30, 2023 07:37
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 30, 2023
@PavelVPV PavelVPV removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 30, 2023
@rlubos rlubos merged commit ab208e3 into nrfconnect:main Jun 30, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ludvigsj ludvigsj ludvigsj approved these changes

@Balaklaka Balaklaka Balaklaka approved these changes

@mia-ko mia-ko mia-ko approved these changes

@carlescufi carlescufi Awaiting requested review from carlescufi

Assignees
No one assigned
Labels
ble mesh Label for ble mesh PRbot. Add this if PR is related to ble mesh and you need to get review. doc-required PR must not be merged without tech writer approval.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@PavelVPV @NordicBuilder @ludvigsj @Balaklaka @mia-ko @rlubos