[samples/nrfconnect] Added updating ZCL cluster state to fit actual light/lock state. #3132

kkasperczyk-no · 2020-10-08T13:24:42Z

Problem

Initial value of ZCL ON/OFF cluster is not defined, while the lock/lighting is initially turned on in nRF Connect examples.

Summary of Changes

Added UpdateClusterState method called on the init and each completed on/off action.

Fixes #3131

bzbarsky-apple

Same comments apply to lock-app as to lighting-app.

bzbarsky-apple · 2020-10-08T14:32:51Z

examples/lighting-app/nrfconnect/main/AppTask.cpp

@@ -99,6 +102,9 @@ int AppTask::Init()
    InitServer();
    PrintQRCode(chip::RendezvousInformationFlags::kBLE);

+    // Initialize cluster state to meet with the real light state
+    UpdateClusterState();


This should happen inside emberAfPluginOnOffClusterServerPostInitCallback, presumably, to happen at the right place in the ZCL model layer initialization. It looks like the gen files for lighting-app (and lock-app) don't set up the CLUSTER_MASK_INIT_FUNCTION bits for the on/off cluster; they should presumably be regenerated with that bit set, so emberAfPluginOnOffClusterServerPostInitCallback will be called.

Ok and could you tell me where I can find how to regenerate those files, to check if emberAfPluginOnOffClusterServerPostInitCallback is called and everything works as expected?

At the moment you need to open up Simplicity Studio, create a configuration like the one we already have but with the "Cluster server init" box checked under the "Callbacks" tab, then do some hand-editing on the generated code to make it compile in CHIP and add licenses. And depending on how the version of Simplicity Studio and SDK you are using compares to what was used to generate the files, either hand-edit more stuff or end up with a bunch of extraneous changes.

We're getting close to having a better flow, but we're not there yet. :(

In the meantime, I created #3180 to change the generated files to ensure the callback is called. As far as testing, if you just add an assert(false) inside emberAfPluginOnOffClusterServerPostInitCallback in examples/lighting-app/lighting-common/gen/callback-stub.c on top of #3180 it should be obvious whether it's getting called or not. And then you'd want to remove it from callback-stub.c and move it to the relevant apps as needed.

I tested it with your changes from #3180 and it works, but I'm not sure about removing this implementation from callback-stub.c. Without removing it, the code obviously will not compile because of the multiple definition, but would you like me to remove it permanently from callback-stub.c and provide empty implementations for all platforms? Wouldn't be better to not remove the implementation, but give it weak attribute?

We've had some weird issues with the linker, so are unlikely willing to trust it here. Ok with you to do the necessary for now and revisit when we think attribute(weak) is robust and better enough to justify the work of proving it robust? our build system for these generated files is still a WIP...

In practice, apps really should implement this callback to work properly, right? I think the right call is to remove from callback-stub.c, add empty impls to the apps that won't have a real impl, with TODO comments pointing to issues we file to track implementing those bits.

Of course, I was just curious that's why I asked. So I followed your suggestions and I implemented empty callbacks for all platforms supporting lighting/lock apps. Hope that I did what you mean, but please verify it.

bzbarsky-apple · 2020-10-08T14:38:08Z

examples/lighting-app/nrfconnect/main/AppTask.cpp

@@ -360,6 +366,8 @@ void AppTask::ActionCompleted(LightingManager::Action_t aAction)
    {
        LOG_INF("Turn Off Action has been completed");
    }
+


This code will be reached even if the action is coming from the data model itself, right? Do we really want to re-enter the data model at that point with the attribute write?

Good point, I forgot about checking if action was triggered by pushing button manually. I fixed it.

bzbarsky-apple · 2020-10-13T02:29:24Z

examples/lighting-app/nrfconnect/main/AppTask.cpp

@@ -360,6 +363,8 @@ void AppTask::ActionInitiated(LightingManager::Action_t aAction)
    {
        LOG_INF("Turn Off Action has been initiated");
    }
+
+    sAppTask.mButtonTriggeredActon = (aActor == AppEvent::kEventType_Button) ? true : false;


"Action" instead of "Acton"? Also what's aActor?

But most importantly, do we want to store this in a global variable? What happens if another action is initiated before the previous one completes?

First of all I think that nothing wrong would happen, because LightManager and BoltLockManager don't allow to start new action before previous one is completed. However I agree that storing global variable there was not the best design, so I proposed two different solutions for lighting and lock app, as they are calling ActionCompleted event in different way. Lighting app is calling it directly from the LightingManager InitiateAction method, so I think that there is no need to store actor value anywhere and it could be just passed as a ActionCompleted method argument. In the case of lock-app situation is different, because ActionCompleted is called from the Timer event handler. I thought that maybe it would be better to store actor value as class field not in the AppTask class, but in the BoltLockManager, which can pass current value to the ActionCompleted method.

bzbarsky-apple · 2020-10-13T02:31:58Z

examples/lock-app/nrfconnect/main/AppTask.cpp

@@ -385,6 +390,12 @@ void AppTask::ActionCompleted(BoltLockManager::Action_t aAction)
        LOG_INF("Unlock Action has been completed");
        sLockLED.Set(false);
    }
+
+    if (sAppTask.mButtonTriggeredActon)


Is there a sane way to have the caller pass the actor here, like they did for ActionInitiated?

I answered on this remark in the comment above.

…tual light/lock state. Initial value of ZCL ON/OFF cluster is not defined, while the lock/lighting is initially turned on in nRF Connect examples. * Added UpdateClusterState method called on the cluster post init callback and each completed on/off action. * Removed emberAfPluginOnOffClusterServerPostInitCallback method implementation from callback-stub.c files for lighting and lock apps. * Added emberAfPluginOnOffClusterServerPostInitCallback method empty implementations for all platforms supporting lighting/lock app.

github-actions · 2020-10-13T15:54:55Z

Size increase report for "nrfconnect-example-build"

File	Section	File	VM
chip-lock.elf	text	94	94
chip-lock.elf	rodata	24	24
chip-lock.elf	bss	0	4
chip-lock.elf	[LOAD #1 [RWX]]	2	2
chip-lock.elf	[LOAD #3 [RW]]	0	-4
chip-lighting.elf	text	108	108
chip-lighting.elf	noinit	0	40
chip-lighting.elf	rodata	24	24
chip-lighting.elf	[LOAD #1 [RWX]]	-12	-12

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,3233
.debug_line,0,512
.debug_loc,0,231
text,94,94
.symtab,0,80
.debug_abbrev,0,78
.debug_str,0,66
.strtab,0,31
.debug_frame,0,24
rodata,24,24
.debug_aranges,0,16
bss,4,0
[LOAD #1 [RWX]],2,2
[Unmapped],0,1
[LOAD #3 [RW]],-4,0
.debug_ranges,0,-8

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,3867
.debug_line,0,523
.debug_loc,0,447
.debug_abbrev,0,211
text,108,108
.symtab,0,80
.debug_str,0,66
.debug_frame,0,44
noinit,40,0
.strtab,0,38
.debug_ranges,0,24
rodata,24,24
.debug_aranges,0,16
[Unmapped],0,-8
[LOAD #1 [RWX]],-12,-12

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

bzbarsky-apple

Looks great, thank you!

bzbarsky-apple · 2020-10-16T05:05:04Z

@mspang @andy31415 @hawk248 @jelderton

boring-cyborg bot added the examples label Oct 8, 2020

restyled-io bot mentioned this pull request Oct 8, 2020

Restyle [samples/nrfconnect] Added updating ZCL cluster state to fit actual light/lock state. #3133

Closed

pullapprove bot requested review from andy31415, BroderickCarlin, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, mspang, rwalker-apple and saurabhst October 8, 2020 13:27

bzbarsky-apple reviewed Oct 8, 2020

View reviewed changes

kkasperczyk-no force-pushed the zcl_init_pr_public branch from 668669a to aa08af9 Compare October 12, 2020 10:24

restyled-io bot mentioned this pull request Oct 12, 2020

Restyle [samples/nrfconnect] Added updating ZCL cluster state to fit actual light/lock state. #3194

Closed

bzbarsky-apple reviewed Oct 13, 2020

View reviewed changes

kkasperczyk-no force-pushed the zcl_init_pr_public branch from 8d72a73 to dc4b68b Compare October 13, 2020 15:16

Restyled by clang-format

d1241a5

restyled-io bot mentioned this pull request Oct 13, 2020

Restyle [samples/nrfconnect] Added updating ZCL cluster state to fit actual light/lock state. #3218

Closed

bzbarsky-apple approved these changes Oct 13, 2020

View reviewed changes

rwalker-apple approved these changes Oct 13, 2020

View reviewed changes

BroderickCarlin approved these changes Oct 16, 2020

View reviewed changes

mspang approved these changes Oct 21, 2020

View reviewed changes

mspang merged commit 5dfde56 into project-chip:master Oct 21, 2020

kkasperczyk-no deleted the zcl_init_pr_public branch April 16, 2021 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[samples/nrfconnect] Added updating ZCL cluster state to fit actual light/lock state. #3132

[samples/nrfconnect] Added updating ZCL cluster state to fit actual light/lock state. #3132

kkasperczyk-no commented Oct 8, 2020

bzbarsky-apple left a comment

bzbarsky-apple Oct 8, 2020

kkasperczyk-no Oct 9, 2020

bzbarsky-apple Oct 9, 2020

kkasperczyk-no Oct 12, 2020

rwalker-apple Oct 12, 2020

bzbarsky-apple Oct 12, 2020

kkasperczyk-no Oct 13, 2020 •

edited

Loading

bzbarsky-apple Oct 8, 2020

kkasperczyk-no Oct 12, 2020

bzbarsky-apple Oct 13, 2020

kkasperczyk-no Oct 13, 2020 •

edited

Loading

bzbarsky-apple Oct 13, 2020

kkasperczyk-no Oct 13, 2020

github-actions bot commented Oct 13, 2020

bzbarsky-apple left a comment

bzbarsky-apple commented Oct 16, 2020

[samples/nrfconnect] Added updating ZCL cluster state to fit actual light/lock state. #3132

[samples/nrfconnect] Added updating ZCL cluster state to fit actual light/lock state. #3132

Conversation

kkasperczyk-no commented Oct 8, 2020

Problem

Summary of Changes

bzbarsky-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkasperczyk-no Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkasperczyk-no Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 13, 2020

bzbarsky-apple left a comment

Choose a reason for hiding this comment

bzbarsky-apple commented Oct 16, 2020

kkasperczyk-no Oct 13, 2020 •

edited

Loading

kkasperczyk-no Oct 13, 2020 •

edited

Loading