[SL-UP] Add Dimmer-switch app #130

arun-silabs · 2024-11-25T18:36:14Z

This PR adds the dimmer-switch application by extending the existing light-switch application.

The zap file of light-switch app is modified by adding level-control-cluster client. The level of the bounded lighting app can be controlled by using buttons (btn0 and btn1) or by sending commands from shell.

arun-silabs · 2024-11-26T19:18:27Z

Currently, the level of the bounded light app can only be changed using shell commands, the button implementation is not yet done.
Raising this PR to make dimmer-switch available by feature complete, button implementation will be done asap!

mkardous-silabs

This PR isn't ready to be merged yet. The structure of added code prevents mutliple commands of being easily supported.

We should also add more than a single level control command.

mkardous-silabs · 2024-11-26T19:22:47Z

examples/light-switch-app/light-switch-common/light-switch-app.zap

@@ -4400,29 +4400,29 @@
    },
    {
      "id": 2,
-      "name": "MA-onofflightswitch",
+      "name": "MA-dimmablelight",


Are we use this is correct? it should be a dimmable light swithc iirc

Yes, it is suggested in the design document that we'll make the endpoint1 type as dimmable light

Is this sample app a dimmer switch or a dimmable light?

It is a dimmer-switch. I've named it as dimmable light as per the discussion in design page..
please let me know if it needs to be updated as dimmable light switch

Outside of the design page, is it logical for this device to be a dimmabe light?
Also, where in the design page does it say that the this device should be a dimmable light?

examples/light-switch-app/silabs/include/AppEvent.h

mkardous-silabs · 2024-11-26T19:29:16Z

examples/light-switch-app/silabs/include/LightSwitchMgr.h

+#define MIN_LEVEL 0
+#define MAX_LEVEL 254


This doesn't work within the cluster functions. Level Control has two attributes that defines this. We cannot redefine arbitrary values.

I did not add any attributes of Level Control cluster in the zap file. Since Level control cluster is added as a client(similar to On/Off cluster), i followed the same way to implement this.

What happens if the level is changed by another controller? I still fail to see the usefulness of storing this.

If the level is changed by another controller, the currentLevel attribute will store that value. I;ve declared these 2 variables just for the MIN and MAX values of level as i did not add any attributes in the level control cluster.
Let me know if i should remove this and add the cluster attributes and use them.

My point is that it doesn't make sense to hardcode these values since any light device can have a different value. You are assuming configuration and behaviors which will not work outside of a bulb with those configurations.

mkardous-silabs · 2024-11-26T19:30:14Z

examples/light-switch-app/silabs/include/LightSwitchMgr.h

@@ -45,6 +48,8 @@ class LightSwitchMgr
    void GenericSwitchOnShortRelease();

    void TriggerLightSwitchAction(LightSwitchAction action, bool isGroupCommand = false);
+    void TriggerLevelControlAction(uint8_t level, bool isGroupCommand = false);
+    uint8_t currentLevel;


Why do we need to store a copy of the current level instead of using the attribute that is already persisted?

I didn't add attributes to the Level Control cluster, should i include the current_level attribute of cluster and use that for storing level?

Again like state in other comments, it doesn't make any sense for the light-switch to store a current level this way.

mkardous-silabs · 2024-11-26T19:31:28Z

examples/light-switch-app/silabs/src/AppTask.cpp

+        LightSwitchMgr::GetInstance().currentLevel = mCurrentButtonState ? MAX_LEVEL : MIN_LEVEL;
+        LightSwitchMgr::GetInstance().TriggerLevelControlAction(LightSwitchMgr::GetInstance().currentLevel);


Why are we changing the level of the light when turning it on or off? This doesn't seem right...

When the light is Off, the level of the light is minimum.
When the light is On, the level of the light is maximum.
I'm changing the level here as it is mentioned in design doc that the level and on-off attributes will be adjusted.

This doesn't integrate correctly with the level control - please check the level control implementation.

mkardous-silabs · 2024-11-26T19:47:01Z

examples/light-switch-app/silabs/src/LightSwitchMgr.cpp

@@ -125,6 +125,20 @@ void LightSwitchMgr::TriggerLightSwitchAction(LightSwitchAction action, bool isG
    DeviceLayer::PlatformMgr().ScheduleWork(SwitchWorkerFunction, reinterpret_cast<intptr_t>(data));
 }

+void LightSwitchMgr::TriggerLevelControlAction(uint8_t level, bool isGroupCommand)


The structure and name of this function does not fit with the current implementation of the on off cluster commands.
The code needs to be structured in way that makes it coherent.

mkardous-silabs · 2024-11-26T19:47:18Z

examples/light-switch-app/silabs/src/LightSwitchMgr.cpp

@@ -125,6 +125,20 @@ void LightSwitchMgr::TriggerLightSwitchAction(LightSwitchAction action, bool isG
    DeviceLayer::PlatformMgr().ScheduleWork(SwitchWorkerFunction, reinterpret_cast<intptr_t>(data));
 }

+void LightSwitchMgr::TriggerLevelControlAction(uint8_t level, bool isGroupCommand)


The structure of this function doesn't make it easy and straight forward to add new commands.

This function is only used when button is pressed. This function will be modified with the button implementation

In that case, please remove this function from this PR and add it when it will be used.

examples/light-switch-app/silabs/src/ShellCommands.cpp

mkardous-silabs · 2024-11-26T19:53:45Z

examples/light-switch-app/silabs/src/ShellCommands.cpp

+    if (LightSwitchMgr::GetInstance().currentLevel > MIN_LEVEL && data->level == MIN_LEVEL)
+    {
+        OffSwitchCommandHandler(0,NULL);
+    }
+    if (LightSwitchMgr::GetInstance().currentLevel == MIN_LEVEL && data->level > MIN_LEVEL)
+    {
+        OnSwitchCommandHandler(1,NULL);
+    }


This doesn't make any sense. This is managed with the level control commands.

As per the spec, if level is moved to MIN_LEVEL, the light should be OFF> If level is MAX_LEVEL, light should be ON. So, i've added this while handling MoveToLevel Command

Please check the Level Control implementation.

examples/light-switch-app/silabs/src/ShellCommands.cpp

arun-silabs added 3 commits November 25, 2024 23:41

Level Control cluster implementation for light-switch

e79282f

Added group support for Level control and code-cleanup

68201f8

Updated Device type to dimmablelight in zap

f68295c

arun-silabs changed the title ~~Add Dimmer-switch app~~ [SL-UP] Add Dimmer-switch app Nov 26, 2024

Merge branch 'release_2.5-1.4' into feature/dimmer_switch

6085111

arun-silabs marked this pull request as ready for review November 26, 2024 19:15

arun-silabs requested a review from a team as a code owner November 26, 2024 19:15

mkardous-silabs requested changes Nov 26, 2024

View reviewed changes

arun-silabs added 2 commits November 27, 2024 14:42

Addressed review comments

a4b0928

Made structural changes

56fc42c

mkardous-silabs added the sl-up This TAG indicates that this commit needs to be upstreamed to CSA before its next release. label Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SL-UP] Add Dimmer-switch app #130

[SL-UP] Add Dimmer-switch app #130

arun-silabs commented Nov 25, 2024

arun-silabs commented Nov 26, 2024

mkardous-silabs left a comment

mkardous-silabs Nov 26, 2024

arun-silabs Nov 27, 2024

mkardous-silabs Nov 27, 2024

arun-silabs Nov 27, 2024

mkardous-silabs Nov 27, 2024

mkardous-silabs Nov 26, 2024

arun-silabs Nov 27, 2024

mkardous-silabs Nov 27, 2024

arun-silabs Nov 27, 2024

mkardous-silabs Nov 27, 2024

mkardous-silabs Nov 26, 2024

arun-silabs Nov 27, 2024

mkardous-silabs Nov 27, 2024

mkardous-silabs Nov 26, 2024

arun-silabs Nov 27, 2024

mkardous-silabs Nov 27, 2024

mkardous-silabs Nov 26, 2024

mkardous-silabs Nov 26, 2024

arun-silabs Nov 27, 2024

mkardous-silabs Nov 27, 2024

mkardous-silabs Nov 26, 2024

arun-silabs Nov 27, 2024

mkardous-silabs Nov 27, 2024

		LightSwitchMgr::GetInstance().currentLevel = mCurrentButtonState ? MAX_LEVEL : MIN_LEVEL;
		LightSwitchMgr::GetInstance().TriggerLevelControlAction(LightSwitchMgr::GetInstance().currentLevel);

[SL-UP] Add Dimmer-switch app #130

Are you sure you want to change the base?

[SL-UP] Add Dimmer-switch app #130

Conversation

arun-silabs commented Nov 25, 2024

arun-silabs commented Nov 26, 2024

mkardous-silabs 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment