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.

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

Bluetooth: Mesh: Fix dim to dark behavior #12506

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

ludvigsj
Copy link
Contributor

@ludvigsj ludvigsj commented Oct 3, 2023

Fixes issues with Move Set and Delta Set handlers not
handling dimming starting from 0 correctly.

  • Clamp targets to 0 when dimming down from 0, instead of clamping to
    RANGE_MIN, to avoid a message requesting a dim down causing the light
    to actually turn on.
  • Take the gap between 0 and RANGE_MIN into account when computing the
    transition time for a move set before this is sent to the application
    callback, to avoid a slow transition for dims starting at 0
  • Update the samples and tester to correctly transition the
    lightness state when dimming up from zero when a RANGE_MIN is set.
  • Update the documentation for the callback to mention this
    requirement.
  • Small bug fixes
    • Range converted to actual in delta set to match with the delta and
      target_actual
    • Do not compute transition in move set if target == current

@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. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Oct 3, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Oct 3, 2023

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 force-pushed the develop/NCSDK-23472_Dim_to_dark branch from 2f01e4b to 51bdc9f Compare October 3, 2023 13:10
@ludvigsj ludvigsj changed the title Develop/ncsdk 23472 dim to dark Bluetooth: Mesh: Fix dim to dark behavior Oct 3, 2023
@ludvigsj ludvigsj force-pushed the develop/NCSDK-23472_Dim_to_dark branch from 51bdc9f to 2712d9b Compare October 3, 2023 13:43
@ludvigsj ludvigsj added the doc-required PR must not be merged without tech writer approval. label Oct 3, 2023
@ludvigsj ludvigsj requested a review from mia-ko October 3, 2023 13:44
@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.

tests/bluetooth/tester/src/model_handler.c Outdated Show resolved Hide resolved
subsys/bluetooth/mesh/lightness_srv.c Outdated Show resolved Hide resolved
@ludvigsj ludvigsj force-pushed the develop/NCSDK-23472_Dim_to_dark branch from 2712d9b to 19765fc Compare October 4, 2023 07:30
@github-actions github-actions bot removed the doc-required PR must not be merged without tech writer approval. label Oct 4, 2023
@ludvigsj ludvigsj requested a review from PavelVPV October 4, 2023 07:31
@ludvigsj ludvigsj added the DNM label Oct 4, 2023
@ludvigsj
Copy link
Contributor Author

ludvigsj commented Oct 4, 2023

There is an ongoing discussion for specs about the expected behavior here, will leave this PR DNM until discussion about specs is done

Comment on lines 78 to 81
* When performing a non-instantaneous state change, the applicaton must call
* @ref bt_mesh_lightness_start_value. The change must move instantaneously to the start
* value returned by this function and start gradually changing from that point over the
* time specified by the transition.
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
* When performing a non-instantaneous state change, the applicaton must call
* @ref bt_mesh_lightness_start_value. The change must move instantaneously to the start
* value returned by this function and start gradually changing from that point over the
* time specified by the transition.
* When performing a non-instantaneous state change, the applicaton must call
* @ref bt_mesh_lightness_start_value. The change must move instantaneously
* to the start value returned by this function, and then gradually change from
* that value over time specified by the transition.

/** @brief Get the starting point for a non-instantaneous change in lightness.
*
* When performing a non-instantaneous change of the lightness state, the change
* should immediately jump to this starting value and then start gradually changing.
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
* should immediately jump to this starting value and then start gradually changing.
* should immediately jump to this starting value and then gradually start changing.

Comment on lines 592 to 593
/* Do not clamp to range when dimming down from zero, to avoid
* a dim down to cause the light to turn on.
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
/* Do not clamp to range when dimming down from zero, to avoid
* a dim down to cause the light to turn on.
/* Do not clamp to range when dimming down from zero to avoid
* the light to turn on when dimmed down.

Comment on lines 597 to 598
/* Clamp the value to the lightness range to prevent it be moved
* back to zero in binding with Generic Level state.
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
/* Clamp the value to the lightness range to prevent it be moved
* back to zero in binding with Generic Level state.
/* Clamp the value to the lightness range to prevent that it moves
* back to zero in binding with Generic Level state.

@ludvigsj ludvigsj force-pushed the develop/NCSDK-23472_Dim_to_dark branch from 19765fc to a408ae7 Compare October 10, 2023 11:26
This commit fixes issues with Move Set and Delta Set handlers not
handling dimming starting from 0 correctly.

 * Clamp targets to 0 when dimming down from 0, instead of clamping to
   RANGE_MIN, to avoid a message requesting a dim down causing the light
   to actually turn on.
 * Take the gap between 0 and RANGE_MIN into account when computing the
   transition time for a move set before this is sent to the application
   callback, to avoid a slow transition for dims starting at 0
 * Small bug fixes
    * Range converted to actual in delta set to match with the delta and
      target_actual
    * Do not compute transition in move set if target == current

Signed-off-by: Ludvig Samuelsen Jordet <[email protected]>
This updates the samples and tester to correctly transition the
lightness state when dimming up from zero when a RANGE_MIN is set.

It also updates the documentation for the callback to mention this
requirement.

Signed-off-by: Ludvig Samuelsen Jordet <[email protected]>
@ludvigsj ludvigsj force-pushed the develop/NCSDK-23472_Dim_to_dark branch from a408ae7 to 4910bdf Compare October 10, 2023 11:34
@ludvigsj ludvigsj added this to the 2.5.0 milestone Oct 10, 2023
@ludvigsj ludvigsj removed the DNM label Oct 10, 2023
@rlubos rlubos merged commit 6976648 into nrfconnect:main Oct 10, 2023
14 of 15 checks passed
@ludvigsj ludvigsj deleted the develop/NCSDK-23472_Dim_to_dark branch April 18, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ble mesh Label for ble mesh PRbot. Add this if PR is related to ble mesh and you need to get review. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants