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.

Sign up for GitHub

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

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e79282f
Level Control cluster implementation for light-switch
arun-silabs Nov 25, 2024
68201f8
Added group support for Level control and code-cleanup
arun-silabs Nov 26, 2024
f68295c
Updated Device type to dimmablelight in zap
arun-silabs Nov 26, 2024
6085111
Merge branch 'release_2.5-1.4' into feature/dimmer_switch
arun-silabs Nov 26, 2024
a4b0928
Addressed review comments
arun-silabs Nov 27, 2024
56fc42c
Made structural changes
arun-silabs Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 29 additions & 11 deletions examples/light-switch-app/light-switch-common/light-switch-app.zap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4400,29 +4400,29 @@
},
{
"id": 2,
"name": "MA-onofflightswitch",
"name": "MA-dimmablelight",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

"deviceTypeRef": {
"code": 259,
"code": 257,
"profileId": 259,
"label": "MA-onofflightswitch",
"name": "MA-onofflightswitch"
"label": "MA-dimmablelight",
"name": "MA-dimmablelight"
},
"deviceTypes": [
{
"code": 259,
"code": 257,
"profileId": 259,
"label": "MA-onofflightswitch",
"name": "MA-onofflightswitch"
"label": "MA-dimmablelight",
"name": "MA-dimmablelight"
}
],
"deviceVersions": [
1
],
"deviceIdentifiers": [
259
257
],
"deviceTypeName": "MA-onofflightswitch",
"deviceTypeCode": 259,
"deviceTypeName": "MA-dimmablelight",
"deviceTypeCode": 257,
"deviceTypeProfileId": 259,
"clusters": [
{
Expand Down Expand Up @@ -4733,6 +4733,24 @@
}
]
},
{
"name": "Level Control",
"code": 8,
"mfgCode": null,
"define": "LEVEL_CONTROL_CLUSTER",
"side": "client",
"enabled": 1,
"commands": [
{
"name": "MoveToLevelWithOnOff",
"code": 0,
"mfgCode": null,
"source": "client",
"isIncoming": 0,
"isEnabled": 1
}
]
},
{
"name": "Descriptor",
"code": 29,
Expand Down Expand Up @@ -5633,7 +5651,7 @@
"parentEndpointIdentifier": null
},
{
"endpointTypeName": "MA-onofflightswitch",
"endpointTypeName": "MA-dimmablelight",
"endpointTypeIndex": 1,
"profileId": 259,
"endpointId": 1,
Expand Down
1 change: 1 addition & 0 deletions examples/light-switch-app/silabs/include/AppEvent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ struct AppEvent
{
struct
{
uint8_t ButtonId;
uint8_t Action;
} ButtonEvent;
struct
Expand Down
2 changes: 2 additions & 0 deletions examples/light-switch-app/silabs/include/AppTask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,6 @@ class AppTask : public BaseApplication
* @param aEvent button event being processed
*/
static void SwitchActionEventHandler(AppEvent * aEvent);
static void LevelUpEventHandler(AppEvent * aEvent);
static void LevelDownEventHandler(AppEvent * aEvent);
};
1 change: 1 addition & 0 deletions examples/light-switch-app/silabs/include/BindingHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,6 @@ struct BindingCommandData
chip::EndpointId localEndpointId = 1;
chip::CommandId commandId;
chip::ClusterId clusterId;
uint8_t level;
bool isGroup = false;
};
5 changes: 5 additions & 0 deletions examples/light-switch-app/silabs/include/LightSwitchMgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
#include <lib/core/CHIPError.h>
#include <lib/support/CodeUtils.h>

#define MIN_LEVEL 0
#define MAX_LEVEL 254
Comment on lines +26 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


class LightSwitchMgr
{
public:
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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


static LightSwitchMgr & GetInstance() { return sSwitch; }

Expand Down
2 changes: 2 additions & 0 deletions examples/light-switch-app/silabs/src/AppTask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@ void AppTask::SwitchActionEventHandler(AppEvent * aEvent)
mCurrentButtonState ? LightSwitchMgr::LightSwitchAction::On : LightSwitchMgr::LightSwitchAction::Off;

LightSwitchMgr::GetInstance().TriggerLightSwitchAction(action);
LightSwitchMgr::GetInstance().currentLevel = mCurrentButtonState ? MAX_LEVEL : MIN_LEVEL;
LightSwitchMgr::GetInstance().TriggerLevelControlAction(LightSwitchMgr::GetInstance().currentLevel);
Comment on lines +146 to +147
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

LightSwitchMgr::GetInstance().GenericSwitchOnInitialPress();

#ifdef DISPLAY_ENABLED
Expand Down
43 changes: 43 additions & 0 deletions examples/light-switch-app/silabs/src/BindingHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,28 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa
}
}

void ProcessLevelControlUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding,
Messaging::ExchangeManager * exchangeMgr, const SessionHandle & sessionHandle,
BindingCommandData * data)
{
auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) {
ChipLogProgress(NotSpecified, "MoveToLevel command succeeds");
};

auto onFailure = [](CHIP_ERROR error) {
ChipLogError(NotSpecified, "MoveToLevel command failed: %" CHIP_ERROR_FORMAT, error.Format());
};

switch (commandId)
{
case Clusters::LevelControl::Commands::Move::Id:
Clusters::LevelControl::Commands::MoveToLevelWithOnOff::Type moveCommand;
moveCommand.level = data->level;
Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, moveCommand, onSuccess, onFailure);
break;
}
}

void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding)
{
Messaging::ExchangeManager & exchangeMgr = Server::GetInstance().GetExchangeManager();
Expand All @@ -85,6 +107,20 @@ void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTabl
}
}

void ProcessLevelControlGroupBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, BindingCommandData * data)
{
Messaging::ExchangeManager & exchangeMgr = Server::GetInstance().GetExchangeManager();

switch (commandId)
{
case Clusters::LevelControl::Commands::MoveToLevelWithOnOff::Id:
Clusters::LevelControl::Commands::MoveToLevelWithOnOff::Type moveCommand;
moveCommand.level = data->level;
Controller::InvokeGroupCommandRequest(&exchangeMgr, binding.fabricIndex, binding.groupId, moveCommand);
break;
}
}

void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device, void * context)
{
VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null"));
Expand All @@ -97,6 +133,9 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation
case Clusters::OnOff::Id:
ProcessOnOffGroupBindingCommand(data->commandId, binding);
break;
case Clusters::LevelControl::Id:
ProcessLevelControlGroupBindingCommand(data->commandId, binding, data);
Copy link
Contributor

Choose a reason for hiding this comment

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

See previous comment - mixing the application data to the binding data isn't a good way to go when we have multiple commands that require extra information.

break;
}
}
else if (binding.type == MATTER_UNICAST_BINDING && !data->isGroup)
Expand All @@ -108,6 +147,10 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation
ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device->GetExchangeManager(),
peer_device->GetSecureSession().Value());
break;
case Clusters::LevelControl::Id:
ProcessLevelControlUnicastBindingCommand(data->commandId, binding, peer_device->GetExchangeManager(),
peer_device->GetSecureSession().Value(), data);
break;
}
}
}
Expand Down
14 changes: 14 additions & 0 deletions examples/light-switch-app/silabs/src/LightSwitchMgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
BindingCommandData * data = Platform::New<BindingCommandData>();

data->clusterId = chip::app::Clusters::LevelControl::Id;
data->isGroup = isGroupCommand;

data->commandId = chip::app::Clusters::LevelControl::Commands::MoveToLevelWithOnOff::Id;
data->level = level;

ChipLogProgress(DeviceLayer, "Level is - %d", data->level);
DeviceLayer::PlatformMgr().ScheduleWork(SwitchWorkerFunction, reinterpret_cast<intptr_t>(data));
}

void LightSwitchMgr::GenericSwitchWorkerFunction(intptr_t context)
{

Expand Down
96 changes: 96 additions & 0 deletions examples/light-switch-app/silabs/src/ShellCommands.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include "ShellCommands.h"
#include "BindingHandler.h"
#include "LightSwitchMgr.h"

#include <app/clusters/bindings/bindings.h>
#include <lib/shell/Engine.h>
Expand All @@ -38,9 +39,11 @@ using Shell::streamer_printf;

Engine sShellSwitchSubCommands;
Engine sShellSwitchOnOffSubCommands;
Engine sShellSwitchLevelControlSubCommands;

Engine sShellSwitchGroupsSubCommands;
Engine sShellSwitchGroupsOnOffSubCommands;
Engine sShellSwitchGroupsLevelControlSubCommands;

Engine sShellSwitchBindingSubCommands;

Expand Down Expand Up @@ -114,6 +117,45 @@ CHIP_ERROR ToggleSwitchCommandHandler(int argc, char ** argv)
return CHIP_NO_ERROR;
}

/********************************************************
* LevelControl switch shell functions
*********************************************************/

CHIP_ERROR LevelControlHelpHandler(int argc, char ** argv)
{
sShellSwitchLevelControlSubCommands.ForEachCommand(Shell::PrintCommandHelp, nullptr);
return CHIP_NO_ERROR;
}

CHIP_ERROR LevelControlSwitchCommandHandler(int argc, char ** argv)
{
if (argc == 0)
{
return LevelControlHelpHandler(argc, argv);
}

return sShellSwitchLevelControlSubCommands.ExecCommand(argc, argv);
}

CHIP_ERROR MoveToLevelCommandHandler(int argc, char ** argv)
{
BindingCommandData * data = Platform::New<BindingCommandData>();
data->commandId = Clusters::LevelControl::Commands::MoveToLevelWithOnOff::Id;
data->clusterId = Clusters::LevelControl::Id;
data->level = atoi(argv[0]);
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check the Level Control implementation.

LightSwitchMgr::GetInstance().currentLevel = data->level;
DeviceLayer::PlatformMgr().ScheduleWork(SwitchWorkerFunction, reinterpret_cast<intptr_t>(data));
return CHIP_NO_ERROR;
}

/********************************************************
* bind switch shell functions
*********************************************************/
Expand Down Expand Up @@ -238,6 +280,47 @@ CHIP_ERROR GroupToggleSwitchCommandHandler(int argc, char ** argv)
return CHIP_NO_ERROR;
}

/********************************************************
* Groups LevelControl switch shell functions
*********************************************************/

CHIP_ERROR GroupsLevelControlHelpHandler(int argc, char ** argv)
{
sShellSwitchGroupsLevelControlSubCommands.ForEachCommand(Shell::PrintCommandHelp, nullptr);
return CHIP_NO_ERROR;
}

CHIP_ERROR GroupsLevelControlSwitchCommandHandler(int argc, char ** argv)
{
if (argc == 0)
{
return GroupsLevelControlHelpHandler(argc, argv);
}

return sShellSwitchGroupsLevelControlSubCommands.ExecCommand(argc, argv);
}

CHIP_ERROR GroupLevelControlSwitchCommandHandler(int argc, char ** argv)
{
BindingCommandData * data = Platform::New<BindingCommandData>();
data->commandId = Clusters::LevelControl::Commands::MoveToLevelWithOnOff::Id;
data->clusterId = Clusters::LevelControl::Id;
data->level = atoi(argv[0]);
data->isGroup = true;

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);
}
LightSwitchMgr::GetInstance().currentLevel = data->level;
DeviceLayer::PlatformMgr().ScheduleWork(SwitchWorkerFunction, reinterpret_cast<intptr_t>(data));
return CHIP_NO_ERROR;
}

/**
* @brief configures switch matter shell
*/
Expand All @@ -246,6 +329,7 @@ void RegisterSwitchCommands()
static const shell_command_t sSwitchSubCommands[] = {
{ &SwitchHelpHandler, "help", "Usage: switch <subcommand>" },
{ &OnOffSwitchCommandHandler, "onoff", " Usage: switch onoff <subcommand>" },
{ &LevelControlSwitchCommandHandler, "levelcontrol", " Usage: switch levelcontrol <subcommand>" },
{ &GroupsSwitchCommandHandler, "groups", "Usage: switch groups <subcommand>" },
{ &BindingSwitchCommandHandler, "binding", "Usage: switch binding <subcommand>" }
};
Expand All @@ -257,6 +341,11 @@ void RegisterSwitchCommands()
{ &ToggleSwitchCommandHandler, "toggle", "Sends toggle command to bound lighting app" }
};

static const shell_command_t sSwitchLevelControlSubCommands[] = {
{ &LevelControlHelpHandler, "help", "Usage : switch levelcontrol <subcommand>" },
{ &MoveToLevelCommandHandler, "move-to-level-with-on-off", "Sends move-to-level command to bound lighting app" },
};

static const shell_command_t sSwitchGroupsSubCommands[] = { { &GroupsHelpHandler, "help", "Usage: switch groups <subcommand>" },
{ &GroupsOnOffSwitchCommandHandler, "onoff",
"Usage: switch groups onoff <subcommand>" } };
Expand All @@ -268,6 +357,11 @@ void RegisterSwitchCommands()
{ &GroupToggleSwitchCommandHandler, "toggle", "Sends toggle command to group" }
};

static const shell_command_t sSwitchGroupsLevelControlSubCommands[] = {
{ &GroupsLevelControlHelpHandler, "help", "Usage: switch groups levelcontrol <subcommand>" },
{ &GroupLevelControlSwitchCommandHandler, "move-to-level-with-on-off", "Sends move-to-level command to bound group" },
};

static const shell_command_t sSwitchBindingSubCommands[] = {
{ &BindingHelpHandler, "help", "Usage: switch binding <subcommand>" },
{ &BindingGroupBindCommandHandler, "group", "Usage: switch binding group <fabric index> <group id>" },
Expand All @@ -278,7 +372,9 @@ void RegisterSwitchCommands()
"Light-switch commands. Usage: switch <subcommand>" };

sShellSwitchGroupsOnOffSubCommands.RegisterCommands(sSwitchGroupsOnOffSubCommands, ArraySize(sSwitchGroupsOnOffSubCommands));
sShellSwitchGroupsLevelControlSubCommands.RegisterCommands(sSwitchGroupsLevelControlSubCommands, ArraySize(sSwitchGroupsLevelControlSubCommands));
sShellSwitchOnOffSubCommands.RegisterCommands(sSwitchOnOffSubCommands, ArraySize(sSwitchOnOffSubCommands));
sShellSwitchLevelControlSubCommands.RegisterCommands(sSwitchLevelControlSubCommands, ArraySize(sSwitchLevelControlSubCommands));
sShellSwitchGroupsSubCommands.RegisterCommands(sSwitchGroupsSubCommands, ArraySize(sSwitchGroupsSubCommands));
sShellSwitchBindingSubCommands.RegisterCommands(sSwitchBindingSubCommands, ArraySize(sSwitchBindingSubCommands));
sShellSwitchSubCommands.RegisterCommands(sSwitchSubCommands, ArraySize(sSwitchSubCommands));
Expand Down
Loading