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 all 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
7 changes: 7 additions & 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 @@ -30,4 +30,11 @@ struct BindingCommandData
chip::CommandId commandId;
chip::ClusterId clusterId;
bool isGroup = false;
union
{
struct
{
uint8_t level;
} LevelData;
};
};
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
58 changes: 52 additions & 6 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 @@ -35,11 +35,11 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa
Messaging::ExchangeManager * exchangeMgr, const SessionHandle & sessionHandle)
{
auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) {
ChipLogProgress(NotSpecified, "OnOff command succeeds");
ChipLogProgress(AppServer, "OnOff command succeeds");
};

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

switch (commandId)
Expand All @@ -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(AppServer, "LevelControl command succeeds");
};

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

switch (commandId)
{
case Clusters::LevelControl::Commands::Move::Id:
Clusters::LevelControl::Commands::MoveToLevelWithOnOff::Type moveCommand;
moveCommand.level = data->LevelData.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,9 +107,23 @@ 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->LevelData.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"));
VerifyOrReturn(context != nullptr, ChipLogError(AppServer, "OnDeviceConnectedFn: context is null"));
BindingCommandData * data = static_cast<BindingCommandData *>(context);

if (binding.type == MATTER_MULTICAST_BINDING && data->isGroup)
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,13 +147,18 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation
ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device->GetExchangeManager(),
peer_device->GetSecureSession().Value());
break;
case Clusters::LevelControl::Id:
VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady());
ProcessLevelControlUnicastBindingCommand(data->commandId, binding, peer_device->GetExchangeManager(),
arun-silabs marked this conversation as resolved.
Show resolved Hide resolved
peer_device->GetSecureSession().Value(), data);
break;
}
}
}

void LightSwitchContextReleaseHandler(void * context)
{
VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "LightSwitchContextReleaseHandler: context is null"));
VerifyOrReturn(context != nullptr, ChipLogError(AppServer, "LightSwitchContextReleaseHandler: context is null"));
Platform::Delete(static_cast<BindingCommandData *>(context));
}

Expand All @@ -135,15 +179,17 @@ void InitBindingHandlerInternal(intptr_t arg)

void SwitchWorkerFunction(intptr_t context)
{
VerifyOrReturn(context != 0, ChipLogError(NotSpecified, "SwitchWorkerFunction - Invalid work data"));
VerifyOrReturn(context != 0, ChipLogError(AppServer, "SwitchWorkerFunction - Invalid work data"));

BindingCommandData * data = reinterpret_cast<BindingCommandData *>(context);
BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast<void *>(data));

Platform::Delete(data);
}

void BindingWorkerFunction(intptr_t context)
{
VerifyOrReturn(context != 0, ChipLogError(NotSpecified, "BindingWorkerFunction - Invalid work data"));
VerifyOrReturn(context != 0, ChipLogError(AppServer, "BindingWorkerFunction - Invalid work data"));

EmberBindingTableEntry * entry = reinterpret_cast<EmberBindingTableEntry *>(context);
AddBindingEntry(*entry);
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->LevelData.level = level;

ChipLogProgress(DeviceLayer, "Level is - %d", data->LevelData.level);
DeviceLayer::PlatformMgr().ScheduleWork(SwitchWorkerFunction, reinterpret_cast<intptr_t>(data));
arun-silabs marked this conversation as resolved.
Show resolved Hide resolved
}

void LightSwitchMgr::GenericSwitchWorkerFunction(intptr_t context)
{

Expand Down
Loading
Loading