Update/Add Level control features based on lastest cluster revision #13133
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…unused defines. Add runtime attribute metadata checks to validate that the need attributes exist for that code. Replace the #ifdef gates to compiled that code by default but still allow toggling it off by defining the right IGNORE_LEVEL_CONTROL_XXX defines
…map. add behaviour tied with lighting
…h level controll ligthing feature Set max Level to 254 for all Set CurrentLevel And StatUpCurrentLevel to NVM storage to support Start up current level behaviour
…e startup functionnality. Update yaml lvl control test support lighting device using a minimum level of 1 instead of 0
pullapprove
bot
requested review from
anush-apple,
austinh0,
balducci-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis and
franck-apple
pullapprove
bot
requested review from
kghost,
kpschoedel,
LuDuda,
mrjerryjohns,
msandstedt,
mspang,
pan-apple,
robszewczyk,
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple and
xylophone21
|
PR #13133: Size comparison from 7283e3a to 2ade6b4 Increases above 0.2%:
Increases (14 builds for efr32, k32w, linux, mbed, p6, qpg, telink)
Full report (19 builds for efr32, k32w, linux, mbed, p6, qpg, telink)
|
… of the current level
|
PR #13133: Size comparison from 7283e3a to a7157d3 Increases above 0.2%:
Increases (20 builds for efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (29 builds for efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
jmartinez-silabs
changed the title
bzbarsky-apple
requested changes
Dec 18, 2021
Comment on lines
-56
to
+57
| #endif // EMBER_AF_PLUGIN_SCENES | ||
| #endif |
There was a problem hiding this comment.
Please leave the labels on #endif that indicate what it's for; otherwise it can get very confusing which #if is being ended....
| @@ -409,7 +409,7 @@ bool emberAfLevelControlClusterMoveToLevelCallback(app::CommandHandler * command | |||
|
|
|||
| emberAfLevelControlClusterPrintln("%pMOVE_TO_LEVEL %x %2x %x %x", "RX level-control:", level, transitionTime, optionMask, | |||
| optionOverride); | |||
| moveToLevelHandler(Commands::MoveToLevel::Id, level, transitionTime, optionMask, optionOverride, | |||
| moveToLevelHandler(emberAfCurrentEndpoint(), Commands::MoveToLevel::Id, level, transitionTime, optionMask, optionOverride, | |||
There was a problem hiding this comment.
Suggested change
| moveToLevelHandler(emberAfCurrentEndpoint(), Commands::MoveToLevel::Id, level, transitionTime, optionMask, optionOverride, | |
| moveToLevelHandler(commandPath.mEndpointId, Commands::MoveToLevel::Id, level, transitionTime, optionMask, optionOverride, |
and similar in other places in this PR.
|
|
||
| if (resolvedLevel == 0xFF) | ||
| if (resolvedLevel.Value() == 0xFF) |
There was a problem hiding this comment.
This is never going to be true, actually.
What can be true is that resolvedLevel.IsNull() is true, in which case this .Value() call is going to crash.
What this should probably look more like is:
if (resolvedLevel.IsNull()) {
resolvedLevel.SetNonNull(temporaryCurrentLevelCache);
}
| } | ||
| #else | ||
| resolvedLevel = temporaryCurrentLevelCache; | ||
| resolvedLevel.Value() = temporaryCurrentLevelCache; |
There was a problem hiding this comment.
Was this codepath tested? I would expect this to crash.... This should be using SetNonNull as above.
There was a problem hiding this comment.
You are right, I missed that code path. In both all cluster app and lighting app the attribute OnLevel is defined and used
I'll fix that
| #ifdef ZCL_USING_LEVEL_CONTROL_CLUSTER_START_UP_CURRENT_LEVEL_ATTRIBUTE | ||
| // StartUp behavior relies StartUpCurrentLevel attributes being tokenized. | ||
| if (areStartUpLevelControlServerAttributesTokenized(endpoint)) | ||
| // If Those read only attribute are enabled we use those values as our set minLevel and maxLevel |
There was a problem hiding this comment.
Suggested change
| // If Those read only attribute are enabled we use those values as our set minLevel and maxLevel | |
| // If these read only attributes are enabled we use those values as our set minLevel and maxLevel |
| @@ -556,4 +556,9 @@ limitations under the License. | |||
| <attribute side="server" code="0x006F" define="STATUS_FLAGS" type="BITMAP8" min="0x00" max="0x0F" writable="false" default="0x00" reportable="true" optional="false">status flags</attribute> | |||
| <attribute side="server" code="0x0100" define="APPLICATION_TYPE" type="INT32U" min="0x00000000" max="0xFFFFFFFF" writable="false" optional="true">application type</attribute> | |||
| </cluster> | |||
| <bitmap name="LevelControlFeature" type="BITMAP32"> | |||
| <field name="OnOff" mask="0x1"/> | |||
There was a problem hiding this comment.
Should have a <cluster> tag here with the right code="..." bit so it will actually be attached to this cluster and codegen for it will happen properly.
There was a problem hiding this comment.
I actually took examples on other bitmap for the featuremaps of other clusters that are done and they are never wrapped by a cluster tag. I believe I did put it in inside the cluster the first time while working on it and it wasn't generated correctly.
Any ideas? I will give it another try
There was a problem hiding this comment.
Not wrapped in a cluster tag. You want:
<bitmap name="LevelControlFeature" type="BITMAP32">
<cluster code="0x0008"/>
<field name="OnOff" mask="0x1"/>
etc
all outside the <cluster> tag that defines the cluster
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This stale pull request has been automatically closed. Thank you for your contributions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Some level control lighting behaviours were disable by ember and zll non existant defines
Change overview
Testing
Test suite TC_LVL_1_1 to 5_1