Skip to content

Level cluster Step/Move commands reverted to random value after transition #7879

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

Merged

Conversation

tima-q
Copy link
Contributor

@tima-q tima-q commented Jun 24, 2021

Problem

A Step command did not execute as expected.
The transition happens, but when reaching the desired value it fell back to the original one.

After examination of the level-control-server.cpp code it seems a variable storedLevel is the source.
This variable is part of the state for the LevelControl cluster in case an Effect is requested (07-5123-04 (ZigBee Cluster Library doc), section 3.10.2.1.1.). The currentLevel is backed up there to restore after the effect has finished.

Backup point:

moveToLevelHandler(ZCL_MOVE_TO_LEVEL_COMMAND_ID, minimumLevelAllowedForTheDevice, currentOnOffTransitionTime, 0xFF, 0xFF,

Restore point after transition:

if (state->storedLevel != INVALID_STORED_LEVEL)

An INVALID_STORED_LEVEL is set in other commands (MoveToLevel/...) to skip the revert step.
For example:

moveToLevelHandler(ZCL_MOVE_TO_LEVEL_COMMAND_ID, level, transitionTime, optionMask, optionOverride,

However, in case of Step/Move command this variable was not initialized to prevent this reverting/setting.

Change overview

Assigning INVALID_STORED_LEVEL to the storedLevel variable where missing.

Testing

Setup:

  • chip-device-ctrl on RPi4 (TE3 image) + qpg6100 lighting-application
    Scenario:
  • factory reset - move through commissioning of device
  • Issue step command - `zcl LevelControl Step 101 1 0 stepMode=1 stepSize=64 transitionTime=20 optionMask=0 optionOverride=0'
  • Read out CurrentLevel value - zclread LevelControl CurrentLevel 101 1 0
  • Check logging
    • transition is printed for each step
    • which levels are set to light
  • Check LED behavior visually - not jumping back/erratic

…hing transition.

Fixed by adding fill-in of unused 'storedLevel' for these commands.
@github-actions
Copy link

Size increase report for "gn_qpg6100-example-build" from ed677f0

File Section File VM
chip-qpg6100-lighting-example.out .text 16 16
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_line,0,21
.text,16,16
.debug_str,0,-1
[Unmapped],0,-16


Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

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

@selissia could you take a look at this, please?

@selissia
Copy link
Contributor

@selissia could you take a look at this, please?

Done, approved.

@bzbarsky-apple bzbarsky-apple merged commit 6fdfcb6 into project-chip:master Jun 25, 2021
@tima-q tima-q deleted the level_control_step_move_fix branch September 6, 2021 07:21
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
…ition (project-chip#7879)

* Level cluster Step/Move commands reverted to random value after finishing transition.
Fixed by adding fill-in of unused 'storedLevel' for these commands.

* Update src/app/clusters/level-control/level-control.cpp

typo fix

Co-authored-by: Boris Zbarsky <[email protected]>

Co-authored-by: Boris Zbarsky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants