[1.0] TC-LVL-5.1 Fail on step 2c for Bridged Device

[latch-danielkneip]

Reproduction steps

Run automated test for TC-LVL-5.1 with a Bridged Dimmer Device.

Bug prevalence

Whenever I do this

GitHub hash of the SDK that was being used

e61a8d077861ce8960125eea03d274100170c134

Platform

core

Platform Version(s)

No response

Type

Common Cluster Logic

Anything else?

We were able to trace the issue to here:

• https://github.com/project-chip/connectedhomeip/blob/e61a8d077861ce8960125eea03d274100170c134/src/app/clusters/level-control/level-control.cpp#L884 where max level is 0

[bzbarsky-apple]

where max level is 0

Why is it 0 @latch-danielkneip ?

[latch-danielkneip]

Unsure, when we run everything manually, we are able to do ./chip-tool levelcontrol read max-level 1 4 on the endpoint and get back [1662752589.533495][146309:146314] CHIP:TOO: MaxLevel: 254 as expected, which is why we think something is wrong in the SDK.

[bzbarsky-apple]

It's really hard to say anything here without access to code or even logs. Can you try debugging on the device to see where it's getting that value?

[latch-danielkneip]

@bzbarsky-apple The following patch seems to resolve this issue for us:

diff --git a/src/app/clusters/level-control/level-control.cpp b/src/app/clusters/level-control/level-control.cpp
index bcf52cd16..2ba741f40 100644
--- a/src/app/clusters/level-control/level-control.cpp
+++ b/src/app/clusters/level-control/level-control.cpp
@@ -875,6 +875,9 @@ static void stepHandler(EndpointId endpoint, CommandId commandId, uint8_t stepMo
 
     state->commandId = commandId;
 
+    Attributes::MinLevel::Get(endpoint, &state->minLevel);
+    Attributes::MaxLevel::Get(endpoint, &state->maxLevel);
+
     // Step commands cause the device to move from its current level to a new
     // level over the specified transition time.
     switch (stepMode)

[latch-danielkneip]

@bzbarsky-apple Can you review and let me know if you think that is a reasonable patch?

[bzbarsky-apple]

@latch-danielkneip Why did the corresponding lines in emberAfLevelControlClusterServerInitCallback not produce the right values? These are fixed values; they are not supposed to change on every step!

I'll leave it to @jmartinez-silabs to decide whether the workaround of getting them at every step is reasonable, but the real bug is they did not have the right values when the endpoint was set up, so the gets in emberAfLevelControlClusterServerInitCallback did not get the right values.

[jmartinez-silabs]

@latch-danielkneip @bzbarsky-apple Boris is right, Your min and max level should be read from the data model config and set in the global struct stateTable in the emberAfLevelControlClusterServerInitCallback. The values should not be changed after that.

Your patch might make you pass the test case but it is hiding an issue.

I would try to debug and watch stateTable variables for your given endpoint. See what are the values read in emberAfLevelControlClusterServerInitCallback and if anything is changing the stateTable minLevel or maxLevel for your endpoint.

Also, just to make sure, is your level control cluster set on endpoint 1? Because the automated test, tests on endpoint 1 only if I am not mistaken.

from the yaml test

name: 24.5.1. [TC-LVL-5.1] Step Verification (DUT as Server)

PICS:
    - LVL.S

config:
    nodeId: 0x12344321
    cluster: "Level Control"
    endpoint: 1

Unsure, when we run everything manually, we are able to do ./chip-tool levelcontrol read max-level 1 4

From your previous comments, you seem to manually read for endpoint 4

[latch-danielkneip]

@jmartinez-silabs Our DUT is a Bridge, so this is a Bridged Device. We've been manually running this test case for debugging purposes. From our debugging, state->maxLevel is correct after emberAfLevelControlClusterServerInitCallback() is called but is 0 when performing the moveTo command.

[latch-danielkneip]

@jmartinez-silabs We just found that the emberAfLevelControlClusterServerInitCallback() is receiving endpoint 2 when it should be endpoint 4. The moveTo command correctly uses endpoint 4, which is why the patch was needed.

[jmartinez-silabs]

Which leads me to say the patch only hides the real issue.

If it is indeed what happens with the init callback. we need to focus on why it is receiving endpoint 2 in the init callback and fix that

[latch-danielkneip]

@jmartinez-silabs It seems that emberAfLevelControlClusterServerInitCallback is only called at startup. Endpoint 2 is a dummy endpoint defined in the ZAP file that includes all the cluster definitions needed for our dynamic endpoints. So this is why we are seeing 2 for the initial endpoint. I see that when a device is added that emberAfLevelControlClusterInitCallback (a different but similarly named function) is called, but I see no substantive definition for this function, only a stub.

[jmartinez-silabs]

@bzbarsky-apple any idea here why the callback wouldn't be call for the dynamic endpoints? I am not familiar with that path

[latch-danielkneip]

@jmartinez-silabs Please find attached a patch that fixes our issue and addresses some earlier concerns. Can you have a look? level-control.cpp.patch.zip

[jmartinez-silabs]

At first glance the change seems ok with me. If and only if,

emberAfLevelControlClusterInitCallback and emberAfLevelControlClusterServerInitCallback are not both called at boot up for a zap configurated endpoint with level control.

Then you would do twice the startup logic because they both call your new levelControlClusterServerInit function where you moved the logic.

I believe in your case of dynamic endpoint only emberAfLevelControlClusterInitCallback is called. But not for all cases.

I can test it on my side.

[bzbarsky-apple]

emberAfLevelControlClusterInitCallback is a callback into the application to let it know a cluster is being initialized, afaict. It's not expected to be implemented by the SDK itself.

The init function for a cluster will be called based on the cluster's metadata. If the cluster is on a zap-generated endpoint, this is generated automatically to call emberAfLevelControlClusterServerInitCallback. If the cluster is on a dynamic endpoint, the consumer is providing the cluster metadata, no?

Concretely, it should have ZAP_CLUSTER_MASK(INIT_FUNCTION) in the cluster mask and the cluster's functions should point to the right thing (that being an array of EmberAfGenericClusterFunction containing a single entry, which is emberAfLevelControlClusterServerInitCallback).

DECLARE_DYNAMIC_CLUSTER does not pass in the right things here.

So in the short run, instead of using DECLARE_DYNAMIC_CLUSTER you would declare the cluster metadata with the above bits set correctly and it should work.

In the long run, we really need to fix cluster initialization bits.

[s-stark]

Came across the same issue and was expecting it to init those values from exposed min / max attributes which can already be defined for dynamic level control clusters. But manually adding the init callback (or even calling it directly after adding the endpoint) also seems to work.

Should this be done for all dynamic clusters or are init functions in general not needed for dynamic endpoints?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale[bot]]

This stale issue has been automatically closed. Thank you for your contributions.