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

Fix OnOff state shadowing by the LevelControl cluster while OnOff tra… #36922

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix OnOff state shadowing by the LevelControl cluster while OnOff tra… #36922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
89 changes: 75 additions & 14 deletions src/app/clusters/level-control/level-control.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ struct EmberAfLevelControlState
CallbackScheduleState callbackSchedule;
QuieterReportingAttribute<uint8_t> quietCurrentLevel{ DataModel::NullNullable };
QuieterReportingAttribute<uint16_t> quietRemainingTime{ DataModel::MakeNullable<uint16_t>(0) };
bool commandIsOnOff; // tracks transitions triggered by the on/off cluster
};

static EmberAfLevelControlState stateTable[kLevelControlStateTableSize];
Expand All @@ -110,7 +111,7 @@ static EmberAfLevelControlState * getState(EndpointId endpoint);

static Status moveToLevelHandler(EndpointId endpoint, CommandId commandId, uint8_t level,
DataModel::Nullable<uint16_t> transitionTimeDs, chip::Optional<BitMask<OptionsBitmap>> optionsMask,
chip::Optional<BitMask<OptionsBitmap>> optionsOverride, uint16_t storedLevel);
chip::Optional<BitMask<OptionsBitmap>> optionsOverride, uint16_t storedLevel, bool commandIsOnOff);
static void moveHandler(CommandHandler * commandObj, const ConcreteCommandPath & commandPath, MoveModeEnum moveMode,
DataModel::Nullable<uint8_t> rate, chip::Optional<BitMask<OptionsBitmap>> optionsMask,
chip::Optional<BitMask<OptionsBitmap>> optionsOverride);
Expand Down Expand Up @@ -258,7 +259,7 @@ class DefaultLevelControlSceneHandler : public scenes::DefaultSceneHandlerImpl
{
moveToLevelHandler(
endpoint, Commands::MoveToLevel::Id, level, DataModel::MakeNullable(static_cast<uint16_t>(timeMs / 100)),
chip::Optional<BitMask<OptionsBitmap>>(1), chip::Optional<BitMask<OptionsBitmap>>(1), INVALID_STORED_LEVEL);
chip::Optional<BitMask<OptionsBitmap>>(1), chip::Optional<BitMask<OptionsBitmap>>(1), INVALID_STORED_LEVEL, false);
}

return CHIP_NO_ERROR;
Expand Down Expand Up @@ -499,6 +500,11 @@ void emberAfLevelControlClusterServerTickCallback(EndpointId endpoint)
}
}

// "clear" the stored command.
state->commandId = Commands::Stop::Id;
// since the transition is complete, clear the tracking of commands from the OnOff cluster.
state->commandIsOnOff = false;

state->callbackSchedule.runTime = System::Clock::Milliseconds32(0);
writeRemainingTime(endpoint, 0);
}
Expand Down Expand Up @@ -695,7 +701,7 @@ Status MoveToLevel(EndpointId endpointId, const Commands::MoveToLevel::Decodable

return moveToLevelHandler(endpointId, Commands::MoveToLevel::Id, level, transitionTime,
Optional<BitMask<OptionsBitmap>>(optionsMask), Optional<BitMask<OptionsBitmap>>(optionsOverride),
INVALID_STORED_LEVEL); // Don't revert to the stored level
INVALID_STORED_LEVEL, false); // Don't revert to the stored level
}

#ifdef MATTER_DM_PLUGIN_SCENES_MANAGEMENT
Expand Down Expand Up @@ -734,7 +740,7 @@ bool emberAfLevelControlClusterMoveToLevelWithOnOffCallback(CommandHandler * com
Status status =
moveToLevelHandler(commandPath.mEndpointId, Commands::MoveToLevelWithOnOff::Id, level, transitionTime,
Optional<BitMask<OptionsBitmap>>(optionsMask), Optional<BitMask<OptionsBitmap>>(optionsOverride),
INVALID_STORED_LEVEL); // Don't revert to the stored level
INVALID_STORED_LEVEL, false); // Don't revert to the stored level

commandObj->AddStatus(commandPath, status);

Expand Down Expand Up @@ -870,7 +876,7 @@ bool emberAfLevelControlClusterStopWithOnOffCallback(CommandHandler * commandObj

static Status moveToLevelHandler(EndpointId endpoint, CommandId commandId, uint8_t level,
DataModel::Nullable<uint16_t> transitionTimeDs, chip::Optional<BitMask<OptionsBitmap>> optionsMask,
chip::Optional<BitMask<OptionsBitmap>> optionsOverride, uint16_t storedLevel)
chip::Optional<BitMask<OptionsBitmap>> optionsOverride, uint16_t storedLevel, bool commandIsOnOff)
{
EmberAfLevelControlState * state = getState(endpoint);
DataModel::Nullable<uint8_t> currentLevel;
Expand Down Expand Up @@ -907,7 +913,8 @@ static Status moveToLevelHandler(EndpointId endpoint, CommandId commandId, uint8
return Status::Failure;
}

state->commandId = commandId;
state->commandId = commandId;
state->commandIsOnOff = commandIsOnOff;

// Move To Level commands cause the device to move from its current level to
// the specified level at the specified rate.
Expand Down Expand Up @@ -1097,6 +1104,9 @@ static void moveHandler(CommandHandler * commandObj, const ConcreteCommandPath &

// Cancel any currently active command before fiddling with the state.
cancelEndpointTimerCallback(endpoint);
// the moveHandler is only called for commands that are triggered by this cluster and therefore
// the current command is _not_ coming from the OnOff cluster.
state->commandIsOnOff = false;

state->eventDurationMs = eventDuration;
status = Attributes::CurrentLevel::Get(endpoint, currentLevel);
Expand Down Expand Up @@ -1337,6 +1347,7 @@ static void stopHandler(CommandHandler * commandObj, const ConcreteCommandPath &
cancelEndpointTimerCallback(endpoint);
SetCurrentLevelQuietReport(endpoint, state, state->quietCurrentLevel.value(), true /*isEndOfTransition*/);
writeRemainingTime(endpoint, 0);
state->commandIsOnOff = false;

send_default_response:
commandObj->AddStatus(commandPath, status);
Expand Down Expand Up @@ -1425,22 +1436,55 @@ void emberAfOnOffClusterLevelControlEffectCallback(EndpointId endpoint, bool new
transitionTime.SetNull();
#endif // IGNORE_LEVEL_CONTROL_CLUSTER_ON_OFF_TRANSITION_TIME

// Check if we're already performing an on/off transition.
if (state->commandIsOnOff)
Copy link
Author

Choose a reason for hiding this comment

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

It may be possible to introduce some state instead of comparing the moveToLevel against the limits?

{
if (!newValue && (state->moveToLevel == minimumLevelAllowedForTheDevice))
{
// new state should be 'OFF' and we're already performing the 'OFF' transition
return;
}
if (newValue && (state->moveToLevel != minimumLevelAllowedForTheDevice))
{
// new state should be 'ON' and we're already performing the 'ON' transition
return;
}
if (!newValue && (state->moveToLevel != minimumLevelAllowedForTheDevice))
{
// new state should be 'OFF' but we're performing an 'ON' transition.
// prevent that the current dimming level (used by the transition effect)
// is being stored as "temporary new level"
temporaryCurrentLevelCache.SetNonNull(state->moveToLevel);
}
if ((newValue) && (state->moveToLevel == minimumLevelAllowedForTheDevice))
{
// new state should be 'ON' but we're performing an 'OFF' transition.
// in this case - without intercepting this sequence - the ON transition would
// restart at the minimum level.
resolvedLevel.SetNonNull(state->storedLevel);
}
}

if (newValue)
{
// If newValue is OnOff::Commands::On::Id...
// "Set CurrentLevel to minimum level allowed for the device."
status = SetCurrentLevelQuietReport(endpoint, state, minimumLevelAllowedForTheDevice, false /*isEndOfTransition*/);
if (status != Status::Success)
// ... except if an On/Off transition is already in progress, then don't manipulate the
// current level but instead just adjust the "moveTo" direction.
if (!state->commandIsOnOff)
{
ChipLogProgress(Zcl, "ERR: reading current level %x", to_underlying(status));
return;
status = SetCurrentLevelQuietReport(endpoint, state, minimumLevelAllowedForTheDevice, false /*isEndOfTransition*/);
if (status != Status::Success)
{
ChipLogProgress(Zcl, "ERR: reading current level %x", to_underlying(status));
return;
}
}

// "Move CurrentLevel to OnLevel, or to the stored level if OnLevel is not
// defined, over the time period OnOffTransitionTime."
moveToLevelHandler(endpoint, Commands::MoveToLevel::Id, resolvedLevel.Value(), transitionTime, chip::NullOptional,
chip::NullOptional,
INVALID_STORED_LEVEL); // Don't revert to stored level
chip::NullOptional, INVALID_STORED_LEVEL, true); // Don't revert to stored level
}
else
{
Expand All @@ -1451,13 +1495,13 @@ void emberAfOnOffClusterLevelControlEffectCallback(EndpointId endpoint, bool new
{
// If OnLevel is defined, don't revert to stored level.
moveToLevelHandler(endpoint, Commands::MoveToLevelWithOnOff::Id, minimumLevelAllowedForTheDevice, transitionTime,
chip::NullOptional, chip::NullOptional, INVALID_STORED_LEVEL);
chip::NullOptional, chip::NullOptional, INVALID_STORED_LEVEL, true);
}
else
{
// If OnLevel is not defined, set the CurrentLevel to the stored level.
moveToLevelHandler(endpoint, Commands::MoveToLevelWithOnOff::Id, minimumLevelAllowedForTheDevice, transitionTime,
chip::NullOptional, chip::NullOptional, temporaryCurrentLevelCache.Value());
chip::NullOptional, chip::NullOptional, temporaryCurrentLevelCache.Value(), true);
}
}
}
Expand Down Expand Up @@ -1592,4 +1636,21 @@ bool LevelControlHasFeature(EndpointId endpoint, Feature feature)
return success ? ((featureMap & to_underlying(feature)) != 0) : false;
}

bool LevelControlIsOnOffTransitionActive(chip::EndpointId endpoint, bool * const targetState)
{
bool isActive = false;
EmberAfLevelControlState * state = getState(endpoint);
if (state == nullptr)
{
ChipLogProgress(Zcl, "ERR: Level control cluster not available on ep%d", endpoint);
return isActive;
}
isActive = state->commandIsOnOff;
if (isActive)
{
*targetState = state->moveToLevel != state->minLevel;
}
return isActive;
}

void MatterLevelControlPluginServerInitCallback() {}
6 changes: 6 additions & 0 deletions src/app/clusters/level-control/level-control.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ void emberAfPluginLevelControlClusterServerPostInitCallback(chip::EndpointId end
*/
bool LevelControlHasFeature(chip::EndpointId endpoint, chip::app::Clusters::LevelControl::Feature feature);

/**
* Check whether the instance of the Level Control cluster on the given endpoint
* is currently performing an On/Off transition.
*/
bool LevelControlIsOnOffTransitionActive(chip::EndpointId endpoint, bool * const targetState);

namespace LevelControlServer {

chip::Protocols::InteractionModel::Status
Expand Down
100 changes: 84 additions & 16 deletions src/app/clusters/on-off-server/on-off-server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,16 +373,78 @@ Status OnOffServer::setOnOffValue(chip::EndpointId endpoint, chip::CommandId com
return status;
}

// if the value is already what we want to set it to then do nothing
if ((!currentValue && command == Commands::Off::Id) || (currentValue && command == Commands::On::Id))
#ifdef MATTER_DM_PLUGIN_LEVEL_CONTROL
// the level control cluster supports on/off transitions or effects, e.g., controlled via
// the OnOffTransitionTime attribute: in a transition to "off", the level control cluster
// changes the CurrentLevel attribute using a timer until it reaches the minimum supported value
// and only then changes the OnOff value in the OnOff cluster. therefore, comparing the current
// OnOff state with the required state is not sufficient for OnOff cluster values that support
// level control:
// - the current state might be "On"
// - even though the level control cluster changes the state towards "Off"
// - and thus any other "On" value should prevent the cluster from turning off.
//
// this restriction does not exist for the "Off" command since level movements by definition
// are only available while the OnOff state is "On".
if (!LevelControlWithOnOffFeaturePresent(endpoint) || (command == Commands::Off::Id))
Copy link
Author

Choose a reason for hiding this comment

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

It might be necessary to restrict this to lighting applications in the future. I don't, however, see how not having this behavior can be useful for other applications - but I might be missing the bigger picture here.

#endif
{
ChipLogProgress(Zcl, "Endpoint %x On/off already set to new value", endpoint);
return Status::Success;
// if the value is already what we want to set it to then do nothing
if ((!currentValue && command == Commands::Off::Id) || (currentValue && command == Commands::On::Id))
{
ChipLogProgress(Zcl, "Endpoint %x On/off already set to new value", endpoint);
return Status::Success;
}
}

switch (command)
{
case Commands::Off::Id:
newValue = false;
break;

case Commands::On::Id:
newValue = true;
break;

case Commands::Toggle::Id:
newValue = !currentValue;
#ifdef MATTER_DM_PLUGIN_LEVEL_CONTROL
if (LevelControlWithOnOffFeaturePresent(endpoint))
{
bool targetValueOn = false;
bool transitionActive = LevelControlIsOnOffTransitionActive(endpoint, &targetValueOn);
if (transitionActive)
{
newValue = !targetValueOn;
}
}
#endif
break;

case Commands::OffWithEffect::Id:
case Commands::OnWithRecallGlobalScene::Id:
case Commands::OnWithTimedOff::Id:
default:
// this function is only supposed to be called for the commands On, Off, and Toggle
ChipLogProgress(Zcl, "ERR: unknown/unsupported command " ChipLogFormatMEI, ChipLogValueMEI(command));
return Status::Failure;
break;
}

// we either got a toggle, or an on when off, or an off when on,
// so we need to swap the value
newValue = !currentValue;
#ifdef MATTER_DM_PLUGIN_LEVEL_CONTROL
if (LevelControlWithOnOffFeaturePresent(endpoint))
{
bool targetValueOn = false;
bool transitionActive = LevelControlIsOnOffTransitionActive(endpoint, &targetValueOn);
if (!transitionActive && (newValue == currentValue))
{
ChipLogProgress(Zcl, "Endpoint %x On/off already set to new value", endpoint);
return Status::Success;
}
}
#endif

ChipLogProgress(Zcl, "Toggle ep%x on/off from state %x to %x", endpoint, currentValue, newValue);

// the sequence of updating on/off attribute and kick off level change effect should
Expand Down Expand Up @@ -415,11 +477,14 @@ Status OnOffServer::setOnOffValue(chip::EndpointId endpoint, chip::CommandId com
}

// write the new on/off value
status = Attributes::OnOff::Set(endpoint, newValue);
if (status != Status::Success)
if (newValue != currentValue)
{
ChipLogProgress(Zcl, "ERR: writing on/off %x", to_underlying(status));
return status;
status = Attributes::OnOff::Set(endpoint, newValue);
if (status != Status::Success)
{
ChipLogProgress(Zcl, "ERR: writing on/off %x", to_underlying(status));
return status;
}
}

#ifdef MATTER_DM_PLUGIN_LEVEL_CONTROL
Expand Down Expand Up @@ -461,11 +526,14 @@ Status OnOffServer::setOnOffValue(chip::EndpointId endpoint, chip::CommandId com
#endif
{
// write the new on/off value
status = Attributes::OnOff::Set(endpoint, newValue);
if (status != Status::Success)
if (newValue != currentValue)
{
ChipLogProgress(Zcl, "ERR: writing on/off %x", to_underlying(status));
return status;
status = Attributes::OnOff::Set(endpoint, newValue);
if (status != Status::Success)
{
ChipLogProgress(Zcl, "ERR: writing on/off %x", to_underlying(status));
return status;
}
}

if (SupportsLightingApplications(endpoint))
Expand Down Expand Up @@ -517,7 +585,7 @@ void OnOffServer::initOnOffServer(chip::EndpointId endpoint)
Status status = getOnOffValueForStartUp(endpoint, onOffValueForStartUp);
if (status == Status::Success)
{
status = setOnOffValue(endpoint, onOffValueForStartUp, true);
status = setOnOffValue(endpoint, onOffValueForStartUp ? Commands::On::Id : Commands::Off::Id, true);
}

#if defined(MATTER_DM_PLUGIN_SCENES_MANAGEMENT) && CHIP_CONFIG_SCENES_USE_DEFAULT_HANDLERS
Expand Down
Loading