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

[Scenes] Level control handler bugfix #29076

Merged
merged 2 commits into from
Sep 7, 2023
Merged

[Scenes] Level control handler bugfix #29076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63bd2cb
Used NumericAttributesTraits for storage null values, removed un-nece…
lpbeliveau-silabs Sep 5, 2023
e32c08f
Update src/app/clusters/on-off-server/on-off-server.cpp
lpbeliveau-silabs Sep 6, 2023
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
19 changes: 13 additions & 6 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 @@ -171,9 +171,16 @@ class DefaultLevelControlSceneHandler : public scenes::DefaultSceneHandlerImpl
uint8_t maxLevel;
VerifyOrReturnError(EMBER_ZCL_STATUS_SUCCESS == Attributes::MaxLevel::Get(endpoint, &maxLevel), CHIP_ERROR_READ_FAILED);

pairs[0].attributeID = Attributes::CurrentLevel::Id;
pairs[0].attributeValue = (!level.IsNull()) ? level.Value() : maxLevel + 1;
size_t attributeCount = 1;
pairs[0].attributeID = Attributes::CurrentLevel::Id;
if (!level.IsNull())
{
pairs[0].attributeValue = level.Value();
}
else
{
chip::app::NumericAttributeTraits<uint32_t>::SetNull(pairs[0].attributeValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

But ApplyScene was not fixed, so this is broken.... How was this tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, If null value (255) for a uint8_t is stored, this will cause the ApplyScene to fail as we are asking to move to a value outside of the boundaries.

This will return failure to the Apply Scene. Is the expected behavior here to silently fail instead of explicitly letting the user know the scene wasn't applied?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think the expected behavior is to move to the values that were stored in the scene, including a null level if that's what was stored.

Copy link
Contributor Author

@lpbeliveau-silabs lpbeliveau-silabs Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see, so we do not want that recall scene to end up in a failure.

How do we expect the level to behave to a null level? Value capped at maximum or lowest possible value, remaining at current level?

So basically, we either ignore the level command silently, ignore the whole scene returning an error (current implementation), or move to an unexpected level.

Say we explicitly add a scene with a null value, we could say this behaves as expected, but what if null is added from a save scene. This means the user just saved the current scene while the level value was null. This probably means the user was not expecting the level to have an impact when restoring the scene.

By that reasoning, should we decide that silently ignoring the move to null seems like the expected behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we expect the level to behave to a null level?

Well, how was it behaving when the scene was saved?

The questions you are asking are spec questions, not implementation questions, no? If the spec does not define this, it needs to...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MoveToLevel Command states that the command SHALL have the Level data field, constrained between 0 to 254. We do not respect that, therefore this should result in an error.

However the RecallScene states that the status of RecallScene at step 3 (applying EFS) SHALL be SUCCESS, so my initial interpretation seems wrong, silent failure seems to be the way here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the spec say that recalling a scene involves a MoveToLevel command? Seems like if that's the intent the spec needs to say it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

set the attributes and corresponding state of the cluster accordingly.
The "corresponding state of the cluster" bits.
Either way, setting the attributes to null without taking an action on the actual level would mean read level won't reflect the actual state of the cluster if no action is taken.

Now does it have to be the actual moveToLevel command, the spec doesn't precise it. So we could technically create a separate function that allows us to move the level to "null" but that is still not defined right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lpbeliveau-silabs Indeed.

}
size_t attributeCount = 1;
if (LevelControlHasFeature(endpoint, LevelControl::Feature::kFrequency))
{
uint16_t frequency;
Expand Down Expand Up @@ -238,9 +245,9 @@ class DefaultLevelControlSceneHandler : public scenes::DefaultSceneHandlerImpl
CommandId command = LevelControlHasFeature(endpoint, LevelControl::Feature::kOnOff) ? Commands::MoveToLevelWithOnOff::Id
: Commands::MoveToLevel::Id;

status = moveToLevelHandler(
endpoint, command, level, app::DataModel::MakeNullable<uint16_t>(static_cast<uint16_t>(timeMs / 100)),
chip::Optional<BitMask<LevelControlOptions>>(), chip::Optional<BitMask<LevelControlOptions>>(), INVALID_STORED_LEVEL);
status = moveToLevelHandler(endpoint, command, level, app::DataModel::MakeNullable(static_cast<uint16_t>(timeMs / 100)),
chip::Optional<BitMask<LevelControlOptions>>(), chip::Optional<BitMask<LevelControlOptions>>(),
INVALID_STORED_LEVEL);

if (status != Status::Success)
{
Expand Down
5 changes: 5 additions & 0 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 @@ -289,6 +289,11 @@ class DefaultOnOffSceneHandler : public scenes::DefaultSceneHandlerImpl
return err;
}

// This handler assumes it is being used with the default handler for the level control. Therefore if the level control
// cluster with on off feature is present on the endpoint and the level control handler is registered, it assumes this
// handler will take action on the on-off state. This assumes the level control attributes were also saved in the scene.
// This is to prevent a behavior where the on off state is set by this handler, and then the level control handler or vice
// versa.
#ifdef EMBER_AF_PLUGIN_LEVEL_CONTROL
if (!(LevelControlWithOnOffFeaturePresent(endpoint) &&
Scenes::ScenesServer::Instance().IsHandlerRegistered(endpoint, LevelControlServer::GetSceneHandler())))
Expand Down