connectedhomeip
TC-VALCC-3.1: Allow immediate open of valve
#35851
Merged
Overview
Commits
16
Changes
View On GitHub

TC-VALCC-3.1: Allow immediate open of valve #35851

mergify merged 16 commits into project-chip:master from cecille:VALCC_3_1_allow_immediate_open
cecille
cecille214 days ago (edited 207 days ago)

If the valve can be opened before the command is complete or before the next read is done, the test will fail because it expects the transitioning phase to happen AND it expects to be able to read the attribute in the intermediate state.

In the spec:
When the movement is complete, the device SHALL set the CurrentState attribute to the Open value.

^ this can happen before the end of the command.

The current implementation does actually set the current state to open immediately, but the test doesn't catch this because the current implementation also doesn't set the target back to NULL when the change to CurrentState happens.

Changes to the test:

  • start with an close command so the device is in a known state before test start
  • change the reads to a subscription

Changes to the current cluster implementation:

  • add logic to set target values correctly for level and state
  • push attribute changes to the current level through the update functions so they can set the targets accordingly
  • remove direct set calls from the delegate

Note that these changes are being done as part of an effort to refactor this cluster entirely, so changes to the implementation will be short-lived and are implemented only to allow landing the test changes before the refactor.

cecille TC-VALCC-3.1: Allow immediate open of valve
fa83b47b
semanticdiff-com
semanticdiff-com214 days ago (edited 144 days ago)

Review changes with  SemanticDiff

Changed Files
File Status
  src/python_testing/TC_VALCC_3_3.py  27% smaller
  src/python_testing/TC_VALCC_3_1.py  25% smaller
  src/python_testing/TC_VALCC_3_2.py  21% smaller
  examples/all-clusters-app/linux/ValveControlDelegate.cpp Unsupported file format
  src/app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp Unsupported file format
github-actions github-actions added tests
github-actions github-actions added matter-1.4-te2-script-change
pullapprove pullapprove added review - pending
github-actions
github-actions214 days ago (edited 214 days ago)

PR #35851: Size comparison from 9c6c365 to fa83b47

Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
platform target config section 9c6c365 fa83b47 change % change
bl602 lighting-app bl602 FLASH 1280924 1280924 0 0.0
RAM 95848 95848 0 0.0
bl602+mfd FLASH 1295046 1295046 0 0.0
RAM 95992 95992 0 0.0
bl602+rpc FLASH 1320874 1320874 0 0.0
RAM 104272 104272 0 0.0
bl702 lighting-app bl702 FLASH 945028 945028 0 0.0
RAM 15145 15145 0 0.0
bl702+mfd FLASH 947428 947428 0 0.0
RAM 15305 15305 0 0.0
bl702+rpc FLASH 1040592 1040592 0 0.0
RAM 24173 24173 0 0.0
bl706-eth FLASH 647194 647194 0 0.0
RAM 25233 25233 0 0.0
bl706-wifi FLASH 895066 895066 0 0.0
RAM 14477 14477 0 0.0
bl702l lighting-app bl702l FLASH 961688 961688 0 0.0
RAM 16804 16804 0 0.0
bl702l+mfd FLASH 964310 964310 0 0.0
RAM 16964 16964 0 0.0
cc13x4_26x4 lighting-app LP_EM_CC1354P10_6 FLASH 829452 829452 0 0.0
RAM 123300 123300 0 0.0
lock-ftd LP_EM_CC1354P10_6 FLASH 814480 814480 0 0.0
RAM 125172 125172 0 0.0
pump-app LP_EM_CC1354P10_6 FLASH 760964 760964 0 0.0
RAM 113664 113664 0 0.0
pump-controller-app LP_EM_CC1354P10_6 FLASH 745200 745200 0 0.0
RAM 113856 113856 0 0.0
cc32xx air-purifier CC3235SF_LAUNCHXL FLASH 616682 616682 0 0.0
RAM 205396 205396 0 0.0
lock CC3235SF_LAUNCHXL FLASH 656730 656730 0 0.0
RAM 205548 205548 0 0.0
cyw30739 light CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 678293 678293 0 0.0
RAM 78516 78516 0 0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 698145 698145 0 0.0
RAM 81148 81148 0 0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 698145 698145 0 0.0
RAM 81148 81148 0 0.0
CYW930739M2EVB-02 unknown 2040 2040 0 0.0
FLASH 655081 655081 0 0.0
RAM 73584 73584 0 0.0
light-switch CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 614365 614365 0 0.0
RAM 71468 71468 0 0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 634001 634001 0 0.0
RAM 74020 74020 0 0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 634001 634001 0 0.0
RAM 74020 74020 0 0.0
lock CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 633637 633637 0 0.0
RAM 74516 74516 0 0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 653353 653353 0 0.0
RAM 77068 77068 0 0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 653353 653353 0 0.0
RAM 77068 77068 0 0.0
thermostat CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 608989 608989 0 0.0
RAM 68604 68604 0 0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 628849 628849 0 0.0
RAM 71236 71236 0 0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 628849 628849 0 0.0
RAM 71236 71236 0 0.0
efr32 lighting-app BRD4187C FLASH 933948 933948 0 0.0
RAM 135884 135884 0 0.0
lock-app BRD2605a FLASH 741640 741632 -8 -0.0
RAM 230240 230240 0 0.0
BRD4338a FLASH 742288 742280 -8 -0.0
RAM 230272 230272 0 0.0
window-app BRD4187C FLASH 1016992 1016984 -8 -0.0
RAM 127804 127804 0 0.0
esp32 all-clusters-app c3devkit DRAM 94080 94080 0 0.0
FLASH 1538186 1538186 0 0.0
IRAM 82538 82538 0 0.0
m5stack DRAM 115032 115032 0 0.0
FLASH 1548646 1548646 0 0.0
IRAM 117039 117039 0 0.0
linux air-purifier-app debug unknown 4688 4688 0 0.0
FLASH 2775457 2775457 0 0.0
RAM 129424 129424 0 0.0
all-clusters-app debug unknown 5528 5528 0 0.0
FLASH 6078886 6078886 0 0.0
RAM 507392 507392 0 0.0
all-clusters-minimal-app debug unknown 5424 5424 0 0.0
FLASH 5412510 5412510 0 0.0
RAM 241136 241136 0 0.0
bridge-app debug unknown 5408 5408 0 0.0
FLASH 4746200 4746200 0 0.0
RAM 218304 218304 0 0.0
chip-tool debug unknown 5960 5960 0 0.0
FLASH 12557500 12557500 0 0.0
RAM 555250 555250 0 0.0
chip-tool-ipv6only arm64 unknown 20520 20520 0 0.0
FLASH 11214128 11214128 0 0.0
RAM 605504 605504 0 0.0
fabric-admin debug unknown 5792 5792 0 0.0
FLASH 10918655 10918655 0 0.0
RAM 554266 554266 0 0.0
fabric-bridge-app debug unknown 4640 4640 0 0.0
FLASH 4569182 4569182 0 0.0
RAM 204888 204888 0 0.0
lighting-app debug+rpc+ui unknown 6056 6056 0 0.0
FLASH 5687137 5687137 0 0.0
RAM 228552 228552 0 0.0
lock-app debug unknown 5344 5344 0 0.0
FLASH 4795350 4795350 0 0.0
RAM 204376 204376 0 0.0
ota-provider-app debug unknown 4720 4720 0 0.0
FLASH 4425122 4425122 0 0.0
RAM 198112 198112 0 0.0
ota-requestor-app debug unknown 4656 4656 0 0.0
FLASH 4563966 4563966 0 0.0
RAM 202680 202680 0 0.0
shell debug unknown 4216 4216 0 0.0
FLASH 3105517 3105517 0 0.0
RAM 159184 159184 0 0.0
thermostat-no-ble arm64 unknown 9448 9448 0 0.0
FLASH 4314240 4314240 0 0.0
RAM 242808 242808 0 0.0
tv-app debug unknown 5624 5624 0 0.0
FLASH 6023701 6023701 0 0.0
RAM 580928 580928 0 0.0
tv-casting-app debug unknown 5208 5208 0 0.0
FLASH 10750941 10750941 0 0.0
RAM 643008 643008 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 FLASH 913956 913956 0 0.0
RAM 142199 142199 0 0.0
nrf7002dk_nrf5340_cpuapp FLASH 884988 884988 0 0.0
RAM 140338 140338 0 0.0
all-clusters-minimal-app nrf52840dk_nrf52840 FLASH 847468 847468 0 0.0
RAM 141093 141093 0 0.0
nxp contact k32w0+release FLASH 581864 581864 0 0.0
RAM 70784 70784 0 0.0
k32w1+release FLASH 596144 596144 0 0.0
RAM 62992 62992 0 0.0
mcxw71+release FLASH 595904 595904 0 0.0
RAM 62992 62992 0 0.0
light k32w0+release FLASH 618372 618372 0 0.0
RAM 70256 70256 0 0.0
k32w1+release FLASH 682160 682160 0 0.0
RAM 48632 48632 0 0.0
mcxw71+release FLASH 682176 682176 0 0.0
RAM 48632 48632 0 0.0
lock k32w1+release FLASH 704488 704488 0 0.0
RAM 67132 67132 0 0.0
mcxw71+release FLASH 704512 704512 0 0.0
RAM 67132 67132 0 0.0
psoc6 all-clusters cy8ckit_062s2_43012 FLASH 1645876 1645876 0 0.0
RAM 210888 210888 0 0.0
all-clusters-minimal cy8ckit_062s2_43012 FLASH 1551868 1551868 0 0.0
RAM 207688 207688 0 0.0
light cy8ckit_062s2_43012 FLASH 1467684 1467684 0 0.0
RAM 200696 200696 0 0.0
lock cy8ckit_062s2_43012 FLASH 1464428 1464428 0 0.0
RAM 225048 225048 0 0.0
qpg lighting-app qpg6105+debug FLASH 660048 660048 0 0.0
RAM 105236 105236 0 0.0
lock-app qpg6105+debug FLASH 618020 618020 0 0.0
RAM 99704 99704 0 0.0
stm32 light STM32WB5MM-DK FLASH 481608 481608 0 0.0
RAM 144692 144692 0 0.0
telink air-quality-sensor-app tlsr9528a_retention FLASH 620512 620512 0 0.0
RAM 50988 50988 0 0.0
all-clusters-app tlsr9118bdk40d FLASH 687712 687712 0 0.0
RAM 148328 148328 0 0.0
all-clusters-minimal-app tlsr9528a FLASH 780678 780678 0 0.0
RAM 110780 110780 0 0.0
bridge-app tlsr9258a FLASH 681514 681514 0 0.0
RAM 91644 91644 0 0.0
contact-sensor-app tlsr9528a_retention FLASH 620274 620274 0 0.0
RAM 50940 50940 0 0.0
light-switch-app-ota-shell-factory-data tlsr9528a FLASH 708216 708216 0 0.0
RAM 74280 74280 0 0.0
lighting-app-ota-factory-data tlsr9118bdk40d FLASH 625204 625204 0 0.0
RAM 144316 144316 0 0.0
lighting-app-ota-rpc-factory-data-4mb tlsr9518adk80d FLASH 810888 810888 0 0.0
RAM 99448 99448 0 0.0
lock-app-dfu tlsr9528a FLASH 656120 656120 0 0.0
RAM 67000 67000 0 0.0
ota-requestor-app tlsr9258a FLASH 697490 697490 0 0.0
RAM 91236 91236 0 0.0
pump-app-usb tlsr9518adk80d FLASH 633898 633898 0 0.0
RAM 55816 55816 0 0.0
pump-controller-app tlsr9518adk80d FLASH 611088 611088 0 0.0
RAM 53060 53060 0 0.0
shell tlsr9518adk80d FLASH 467798 467798 0 0.0
RAM 68668 68668 0 0.0
smoke_co_alarm-app tlsr9528a_retention FLASH 627404 627404 0 0.0
RAM 52660 52660 0 0.0
temperature-measurement-app-mars-ota tlsr9518adk80d FLASH 653224 653224 0 0.0
RAM 56608 56608 0 0.0
thermostat tlsr9518adk80d FLASH 637818 637818 0 0.0
RAM 53452 53452 0 0.0
window-covering tlsr9118bdk40d FLASH 523972 523972 0 0.0
RAM 97284 97284 0 0.0
tizen all-clusters-app arm unknown 4904 4904 0 0.0
FLASH 1725176 1725176 0 0.0
RAM 89076 89076 0 0.0
chip-tool-ubsan arm unknown 10348 10348 0 0.0
FLASH 17362010 17362010 0 0.0
RAM 7564628 7564628 0 0.0
andy31415
andy31415 commented on 2024-10-03
Conversation is marked as resolved
Show resolved
src/python_testing/TC_VALCC_3_1.py
91 target_state_dut = await self.read_valcc_attribute_expect_success(endpoint=endpoint, attribute=attributes.TargetState)
9192 current_state_dut = await self.read_valcc_attribute_expect_success(endpoint=endpoint, attribute=attributes.CurrentState)
9293 asserts.assert_true(current_state_dut is not NullValue, "CurrentState is null")
94
 asserts.assert_true(target_state_dut is not NullValue, "TargetState is null")
andy31415211 days ago

should these nullability asserts also apply outside of the while? It seems like conditions should always be consistent except we have a retry while current state is transitioning.

andy31415211 days ago

I believe the checks are also done in step 4, it just means we read it twice to get there. Will add a checkmark, up to you if this really needs update or not.

cecille207 days ago

I ended up having to refactor this thing a bunch more, and this code is now gone. But the code is quite different from when you last looked. Would you mind taking another peek when you get a chance?

andy31415
andy31415 approved these changes on 2024-10-03
cecille fixup for 3.1 test
18c06133
cecille another fixup for 3.1
7d45e458
cecille Add close at start of test
109f4002
github-actions github-actions added examples
github-actions github-actions added app
cecille linter
c735c6a2
fessehaeve
fessehaeve206 days ago

This seems out of sync with the test plan. Is that being updated as well?
Also, what changes are happening to the cluster? Can you please point me to where the changes are happening?

cecille
cecille205 days ago

@fessehaeve - yes, the test plan will need to be updated, I can update it from this script once this is approved. The changes to the cluster are happening in a separate branch. I should have the changes up in a PR by the SDK tomorrow, but they're orthogonal to this change - just an explanation for why I'm only doing a partial clean of the current code.

restyled-commits Restyled by clang-format
7009dd36
restyled-commits Restyled by isort
3ab061b3
ReneJosefsen
ReneJosefsen commented on 2024-10-09
Conversation is marked as resolved
Show resolved
src/python_testing/TC_VALCC_3_1.py
4543 steps = [
4644 TestStep(1, "Commissioning, already done", is_commissioning=True),
47 TestStep(2, "Send Open command"),
48 TestStep(3, "Read TargetState attribute"),
49 TestStep(4, "Read CurrentState attribute"),
50 TestStep(5, "Send Close command"),
51 TestStep(6, "Read TargetState attribute"),
52 TestStep(7, "Read CurrentState attribute"),
45 TestStep(2, "Set up a subscription to all attributes on the DUT"),
46
 TestStep(3, "Send a close command to the DUT and wait until the CurrentState is closed", "DUT returns SUCCESS"),
ReneJosefsen205 days ago

This step is used to get the valve into a well-known state before doing the test?

cecille205 days ago

yup

ReneJosefsen
ReneJosefsen approved these changes on 2024-10-09
pullapprove pullapprove removed review - pending
pullapprove pullapprove added review - approved
fessehaeve
fessehaeve commented on 2024-10-10
Conversation is marked as resolved
Show resolved
src/app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp
361361 {
362362 DataModel::Nullable<Percent> cLevel = delegate->HandleOpenValve(level);
363 if (HasFeature(ep, ValveConfigurationAndControl::Feature::kLevel))
363
 if (!cLevel.IsNull())
fessehaeve204 days ago (edited 204 days ago)

It is not guaranteed that the delegate will do the right thing and return null if it doesn't support the Level feature. So keeping the feature map check seems alright to me.

cecille193 days ago

Added back in the feature check - should be checking both.

fessehaeve
fessehaeve commented on 2024-10-10
Conversation is marked as resolved
Show resolved
src/app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp
381 TargetLevel::Get(ep, targetLevel);
382 if (!targetLevel.IsNull() && currentLevel == targetLevel.Value())
383 {
384
 targetLevel = DataModel::NullNullable;
fessehaeve204 days ago

Spec currently doesn't say TargetLevel should be nulled once opening or closing is complete. It might be important not to explicitly null it, since it allows to verify the target and current level are similar for troubleshooting purposes. It could also be used, in addition to the CurrentState, to verify state of change.

fessehaeve204 days ago

This change also causes VALCC_3_3 to fail.

ReneJosefsen204 days ago

The description of the TargetState attribute does IMO indicate that it is expected to be nulled when the transition is done, this might now be when Open and Close are "done" but instead when the transition is done.

cecille197 days ago

In my reading, it does:
This attribute SHALL indicate the target state, while changing the state, of the valve.

A value of null SHALL indicate that no target position is set, since the change in state is either done or failed.

ie, this applies while the state is changing and it shall be null when the change in state is done or failed.

fessehaeve
fessehaeve commented on 2024-10-10
Conversation is marked as resolved
Show resolved
src/app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp
395 TargetState::Get(ep, targetState);
396 if (currentState == targetState.ValueOr(ValveStateEnum::kUnknownEnumValue))
397 {
398
 targetState = DataModel::NullNullable;
fessehaeve204 days ago

Same comment as above.

mergify mergify added conflict
mergify mergify removed conflict
mergify mergify added conflict
cecille cecille removed matter-1.4-te2-script-change
cecille Add back the feature check
fa25f334
cecille TC-VALCC-3.3: Allow immediate open
b858bfee
github-actions github-actions added matter-1.4-te2-script-change
fessehaeve
fessehaeve approved these changes on 2024-10-23
cecille Merge remote-tracking branch 'upstream/master' into VALCC_3_1_allow_i…
63c7cef2
mergify mergify removed conflict
cecille Linter
9b664524
cecille Fix 3.2
d48e1472
cecille Merge remote-tracking branch 'upstream/master' into VALCC_3_1_allow_i…
00deb598
cecille couple little fixes
801e054a
restyled-commits Restyled by isort
0793cebf
github-actions
github-actions144 days ago (edited 144 days ago)

PR #35851: Size comparison from 84fb78f to 0793ceb

Full report (10 builds for cc32xx, nrfconnect, qpg, stm32, tizen)
platform target config section 84fb78f 0793ceb change % change
cc32xx air-purifier CC3235SF_LAUNCHXL FLASH 631026 631026 0 0.0
RAM 205824 205824 0 0.0
lock CC3235SF_LAUNCHXL FLASH 669622 669622 0 0.0
RAM 205968 205968 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 FLASH 917576 917844 268 0.0
RAM 143292 143292 0 0.0
nrf7002dk_nrf5340_cpuapp FLASH 890080 890352 272 0.0
RAM 141487 141487 0 0.0
all-clusters-minimal-app nrf52840dk_nrf52840 FLASH 851720 851732 12 0.0
RAM 142200 142200 0 0.0
qpg lighting-app qpg6105+debug FLASH 663976 663976 0 0.0
RAM 105424 105424 0 0.0
lock-app qpg6105+debug FLASH 621772 621772 0 0.0
RAM 99868 99868 0 0.0
stm32 light STM32WB5MM-DK FLASH 484696 484696 0 0.0
RAM 144880 144880 0 0.0
tizen all-clusters-app arm unknown 4988 4988 0 0.0
FLASH 1732480 1733968 1488 0.1
RAM 90744 90744 0 0.0
chip-tool-ubsan arm unknown 10804 10804 0 0.0
FLASH 17969454 17969454 0 0.0
RAM 7840748 7840748 0 0.0
cecille linter
1f3a1ea7
mergify mergify merged a43ce0eb into master 140 days ago

Login to write a write a comment.

Login via GitHub

Reviewers
Assignees
No one assigned
Labels
examples app review - approved tests matter-1.4-te2-script-change
Milestone
No milestone
Terms Of Use Privacy Policy Refund Policy Impressum