Skip to content

Commit

Permalink
Fix OnOff state shadowing by the LevelControl cluster while OnOff tra…
Browse files Browse the repository at this point in the history 
…nsitions are in progress
  • Loading branch information
@lmapii
lmapii committed Dec 20, 2024
1 parent c1afc02 commit 61bf11c
Show file tree
Hide file tree
Showing 3 changed files with 149 additions and 31 deletions.
87 changes: 73 additions & 14 deletions src/app/clusters/level-control/level-control.cpp
View file
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)
{
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,17 +1495,32 @@ 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);
}
}
}

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

void emberAfLevelControlClusterServerInitCallback(EndpointId endpoint)
{
EmberAfLevelControlState * state = getState(endpoint);
Expand Down
90 changes: 73 additions & 17 deletions src/app/clusters/on-off-server/on-off-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,17 +373,67 @@ 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))
#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;
}
}

// we either got a toggle, or an on when off, or an off when on,
// so we need to swap the value
newValue = !currentValue;
ChipLogProgress(Zcl, "Toggle ep%x on/off from state %x to %x", endpoint, currentValue, newValue);
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 effectActive = false;
bool targetValueOn = false;
emberAfOnOffClusterLevelControlIsEffectActiveCallback(endpoint, &effectActive, &targetValueOn);
if (effectActive)
{
newValue = !targetValueOn;
}
}
#endif
break;

case Commands::OffWithEffect::Id:
case Commands::OnWithRecallGlobalScene::Id:
case Commands::OnWithTimedOff::Id:
default:
// this function is not supposed to be called with the given commands.
ChipLogProgress(Zcl, "ERR: unknown/unsupported command %x", command);
return Status::Failure;
break;
}

ChipLogProgress(Zcl, "Change endpoint %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
// be depend on whether we are turning on or off. If we are turning on the light, we
Expand Down Expand Up @@ -415,11 +465,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 +514,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 +573,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
3 changes: 3 additions & 0 deletions src/app/clusters/on-off-server/on-off-server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,9 @@ struct OnOffEffect
*/
void emberAfOnOffClusterLevelControlEffectCallback(chip::EndpointId endpoint, bool newValue);

void emberAfOnOffClusterLevelControlIsEffectActiveCallback(chip::EndpointId endpoint, bool * const isActive,
bool * const targetState);

/**********************************************************
* Callbacks
*********************************************************/
Expand Down

0 comments on commit 61bf11c

Please sign in to comment.