From 4234e246283bc57b86753c23b7648082d5ad58b7 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 2 Feb 2023 09:45:57 -0500 Subject: [PATCH 1/6] Fix missing cases in some door lock switch statements. (#24797) Some credential types were missing from switch statements in door lock cluster server code, so we did not handle those credentials correctly. --- .../door-lock-server-callback.cpp | 36 +++++++++ .../door-lock-server/door-lock-server.cpp | 23 ++++++ .../door-lock-server/door-lock-server.h | 80 +++++++++++++++++++ 3 files changed, 139 insertions(+) diff --git a/src/app/clusters/door-lock-server/door-lock-server-callback.cpp b/src/app/clusters/door-lock-server/door-lock-server-callback.cpp index 0259f6100516b4..3a319ab5d494f2 100644 --- a/src/app/clusters/door-lock-server/door-lock-server-callback.cpp +++ b/src/app/clusters/door-lock-server/door-lock-server-callback.cpp @@ -195,3 +195,39 @@ emberAfPluginDoorLockSetSchedule(chip::EndpointId endpointId, uint8_t holidayInd void __attribute__((weak)) emberAfPluginDoorLockLockoutStarted(chip::EndpointId endpointId, chip::System::Clock::Timestamp lockoutEndTime) {} + +bool __attribute__((weak)) +emberAfPluginDoorLockGetNumberOfFingerprintCredentialsSupported(chip::EndpointId endpointId, uint16_t & maxNumberOfCredentials) +{ + return false; +} + +bool __attribute__((weak)) +emberAfPluginDoorLockGetNumberOfFingerVeinCredentialsSupported(chip::EndpointId endpointId, uint16_t & maxNumberOfCredentials) +{ + return false; +} + +bool __attribute__((weak)) +emberAfPluginDoorLockGetNumberOfFaceCredentialsSupported(chip::EndpointId endpointId, uint16_t & maxNumberOfCredentials) +{ + return false; +} + +bool __attribute__((weak)) +emberAfPluginDoorLockGetFingerprintCredentialLengthConstraints(chip::EndpointId endpointId, uint8_t & minLen, uint8_t & maxLen) +{ + return false; +} + +bool __attribute__((weak)) +emberAfPluginDoorLockGetFingerVeinCredentialLengthConstraints(chip::EndpointId endpointId, uint8_t & minLen, uint8_t & maxLen) +{ + return false; +} + +bool __attribute__((weak)) +emberAfPluginDoorLockGetFaceCredentialLengthConstraints(chip::EndpointId endpointId, uint8_t & minLen, uint8_t & maxLen) +{ + return false; +} diff --git a/src/app/clusters/door-lock-server/door-lock-server.cpp b/src/app/clusters/door-lock-server/door-lock-server.cpp index 35fc3c954dc40d..a08abd8ac3f20f 100644 --- a/src/app/clusters/door-lock-server/door-lock-server.cpp +++ b/src/app/clusters/door-lock-server/door-lock-server.cpp @@ -1457,6 +1457,15 @@ DlStatus DoorLockServer::credentialLengthWithinRange(chip::EndpointId endpointId statusMin = GetAttribute(endpointId, Attributes::MinRFIDCodeLength::Id, Attributes::MinRFIDCodeLength::Get, minLen); statusMax = GetAttribute(endpointId, Attributes::MaxRFIDCodeLength::Id, Attributes::MaxRFIDCodeLength::Get, maxLen); break; + case CredentialTypeEnum::kFingerprint: + statusMin = statusMax = emberAfPluginDoorLockGetFingerprintCredentialLengthConstraints(endpointId, minLen, maxLen); + break; + case CredentialTypeEnum::kFingerVein: + statusMin = statusMax = emberAfPluginDoorLockGetFingerVeinCredentialLengthConstraints(endpointId, minLen, maxLen); + break; + case CredentialTypeEnum::kFace: + statusMin = statusMax = emberAfPluginDoorLockGetFaceCredentialLengthConstraints(endpointId, minLen, maxLen); + break; default: return DlStatus::kFailure; } @@ -1496,6 +1505,15 @@ bool DoorLockServer::getMaxNumberOfCredentials(chip::EndpointId endpointId, Cred case CredentialTypeEnum::kRfid: status = GetNumberOfRFIDCredentialsSupported(endpointId, maxNumberOfCredentials); break; + case CredentialTypeEnum::kFingerprint: + status = emberAfPluginDoorLockGetNumberOfFingerprintCredentialsSupported(endpointId, maxNumberOfCredentials); + break; + case CredentialTypeEnum::kFingerVein: + status = emberAfPluginDoorLockGetNumberOfFingerVeinCredentialsSupported(endpointId, maxNumberOfCredentials); + break; + case CredentialTypeEnum::kFace: + status = emberAfPluginDoorLockGetNumberOfFaceCredentialsSupported(endpointId, maxNumberOfCredentials); + break; default: return false; } @@ -2391,6 +2409,11 @@ bool DoorLockServer::credentialTypeSupported(chip::EndpointId endpointId, Creden return SupportsPIN(endpointId); case CredentialTypeEnum::kRfid: return SupportsRFID(endpointId); + case CredentialTypeEnum::kFingerprint: + case CredentialTypeEnum::kFingerVein: + return SupportsFingers(endpointId); + case CredentialTypeEnum::kFace: + return SupportsFace(endpointId); default: return false; } diff --git a/src/app/clusters/door-lock-server/door-lock-server.h b/src/app/clusters/door-lock-server/door-lock-server.h index edf6a439e7042a..3f8fab53e8cd62 100644 --- a/src/app/clusters/door-lock-server/door-lock-server.h +++ b/src/app/clusters/door-lock-server/door-lock-server.h @@ -992,3 +992,83 @@ bool emberAfPluginDoorLockSetCredential(chip::EndpointId endpointId, uint16_t cr * @param lockoutEndTime Monotonic time of when lockout ends. */ void emberAfPluginDoorLockLockoutStarted(chip::EndpointId endpointId, chip::System::Clock::Timestamp lockoutEndTime); + +/** + * @brief This callback is called when the Door Lock server needs to find out + * the number of Fingerprint credentials supported, since there is no attribute + * that represents that value. + * + * @param[in] endpointId ID of the endpoint that contains the door lock. + * @param[out] maxNumberOfCredentials the number of Fingerprint credentials supported by the lock. + * + * @return false on failure, true on success. On failure, the cluster + * implementation will assume that 0 Fingerprint credentials are supported. + */ +bool emberAfPluginDoorLockGetNumberOfFingerprintCredentialsSupported(chip::EndpointId endpointId, + uint16_t & maxNumberOfCredentials); + +/** + * @brief This callback is called when the Door Lock server needs to find out + * the number of FingerVein credentials supported, since there is no attribute + * that represents that value. + * + * @param[in] endpointId ID of the endpoint that contains the door lock. + * @param[out] maxNumberOfCredentials the number of FingerVein credentials supported by the lock. + * + * @return false on failure, true on success. On failure, the cluster + * implementation will assume that 0 FingerVein credentials are supported. + */ +bool emberAfPluginDoorLockGetNumberOfFingerVeinCredentialsSupported(chip::EndpointId endpointId, uint16_t & maxNumberOfCredentials); + +/** + * @brief This callback is called when the Door Lock server needs to find out + * the number of Face credentials supported, since there is no attribute + * that represents that value. + * + * @param[in] endpointId ID of the endpoint that contains the door lock. + * @param[out] maxNumberOfCredentials the number of Face credentials supported by the lock. + * + * @return false on failure, true on success. On failure, the cluster + * implementation will assume that 0 Face credentials are supported. + */ +bool emberAfPluginDoorLockGetNumberOfFaceCredentialsSupported(chip::EndpointId endpointId, uint16_t & maxNumberOfCredentials); + +/** + * @brief This callback is called when the Door Lock server needs to find out + * the min and max lengths of Fingerprint credentials supported, since there are no + * attributes that represents those values. + * + * @param[in] endpointId ID of the endpoint that contains the door lock. + * @param[out] minLen the minimal length, in bytes, of a Fingerprint credential supported by the lock. + * @param[out] maxLen the minimal length, in bytes, of a Fingerprint credential supported by the lock. + * + * @return false on failure, true on success. + */ +bool emberAfPluginDoorLockGetFingerprintCredentialLengthConstraints(chip::EndpointId endpointId, uint8_t & minLen, + uint8_t & maxLen); + +/** + * @brief This callback is called when the Door Lock server needs to find out + * the min and max lengths of FingerVein credentials supported, since there are no + * attributes that represents those values. + * + * @param[in] endpointId ID of the endpoint that contains the door lock. + * @param[out] minLen the minimal length, in bytes, of a FingerVein credential supported by the lock. + * @param[out] maxLen the minimal length, in bytes, of a FingerVein credential supported by the lock. + * + * @return false on failure, true on success. + */ +bool emberAfPluginDoorLockGetFingerVeinCredentialLengthConstraints(chip::EndpointId endpointId, uint8_t & minLen, uint8_t & maxLen); + +/** + * @brief This callback is called when the Door Lock server needs to find out + * the min and max lengths of Face credentials supported, since there are no + * attributes that represents those values. + * + * @param[in] endpointId ID of the endpoint that contains the door lock. + * @param[out] minLen the minimal length, in bytes, of a Face credential supported by the lock. + * @param[out] maxLen the minimal length, in bytes, of a Face credential supported by the lock. + * + * @return false on failure, true on success. + */ +bool emberAfPluginDoorLockGetFaceCredentialLengthConstraints(chip::EndpointId endpointId, uint8_t & minLen, uint8_t & maxLen); From 5bf0b8d53131bc4ba06250253de37e14271efa78 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 2 Feb 2023 09:46:08 -0500 Subject: [PATCH 2/6] Fix crash if chip-tool gets an error status response. (#24799) We are trying to do JSON logging without a delegate in that case. --- examples/chip-tool/commands/common/RemoteDataModelLogger.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/chip-tool/commands/common/RemoteDataModelLogger.cpp b/examples/chip-tool/commands/common/RemoteDataModelLogger.cpp index ccde7acd9781e4..1b397dfe8989d0 100644 --- a/examples/chip-tool/commands/common/RemoteDataModelLogger.cpp +++ b/examples/chip-tool/commands/common/RemoteDataModelLogger.cpp @@ -144,6 +144,8 @@ CHIP_ERROR LogErrorAsJSON(const chip::app::EventHeader & header, const chip::app CHIP_ERROR LogErrorAsJSON(const CHIP_ERROR & error) { + VerifyOrReturnError(gDelegate != nullptr, CHIP_NO_ERROR); + Json::Value value; chip::app::StatusIB status; status.InitFromChipError(error); From 2fba6d65f66a4406ac2bb21df48bfc87fd3aeaa6 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 2 Feb 2023 09:46:26 -0500 Subject: [PATCH 3/6] Only update placeholder if pics is enabled (#24795) --- .../py_matter_yamltests/matter_yamltests/parser.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/scripts/py_matter_yamltests/matter_yamltests/parser.py b/scripts/py_matter_yamltests/matter_yamltests/parser.py index 258693b0ec20a7..2c9a14a4e92b43 100644 --- a/scripts/py_matter_yamltests/matter_yamltests/parser.py +++ b/scripts/py_matter_yamltests/matter_yamltests/parser.py @@ -401,12 +401,13 @@ def __init__(self, test: _TestStepWithPlaceholders, runtime_config_variable_stor self._runtime_config_variable_storage = runtime_config_variable_storage self.arguments = copy.deepcopy(test.arguments_with_placeholders) self.response = copy.deepcopy(test.response_with_placeholders) - self._update_placeholder_values(self.arguments) - self._update_placeholder_values(self.response) - self._test.node_id = self._config_variable_substitution( - self._test.node_id) - test.update_arguments(self.arguments) - test.update_response(self.response) + if test.is_pics_enabled: + self._update_placeholder_values(self.arguments) + self._update_placeholder_values(self.response) + self._test.node_id = self._config_variable_substitution( + self._test.node_id) + test.update_arguments(self.arguments) + test.update_response(self.response) @property def is_enabled(self): From 94c7f68adef9c1341ccc851575d333e6f32fc4e9 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Thu, 2 Feb 2023 06:47:12 -0800 Subject: [PATCH 4/6] Fix program crash when try to show command helper info (#24809) --- .../java/src/com/matter/controller/Main.java | 3 --- .../controller/commands/common/CommandManager.java | 10 ++++++++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/examples/java-matter-controller/java/src/com/matter/controller/Main.java b/examples/java-matter-controller/java/src/com/matter/controller/Main.java index ffeefb43699a2e..0e24c6dc1c212d 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/Main.java +++ b/examples/java-matter-controller/java/src/com/matter/controller/Main.java @@ -112,9 +112,6 @@ public static void main(String[] args) { try { commandManager.run(args); - } catch (IllegalArgumentException e) { - logger.log(Level.INFO, "Arguments init failed with exception: " + e.getMessage()); - System.exit(1); } catch (Exception e) { logger.log(Level.INFO, "Run command failed with exception: " + e.getMessage()); System.exit(1); diff --git a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CommandManager.java b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CommandManager.java index 2e1b373873478a..47e184ec3ae4d7 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CommandManager.java +++ b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CommandManager.java @@ -96,8 +96,14 @@ public final void run(String[] args) throws Exception { // need skip over binary and command name and only get arguments String[] temp = Arrays.copyOfRange(args, 2, args.length); - command.initArguments(temp.length, temp); - showCommand(args[0], command); + try { + command.initArguments(temp.length, temp); + } catch (IllegalArgumentException e) { + logger.log(Level.INFO, "Arguments init failed with exception: " + e.getMessage()); + showCommand(args[0], command); + System.exit(1); + } + command.run(); } From 64d8ae1ec8f9d913abaafaaa0cc12bc64f536bd4 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 2 Feb 2023 10:08:51 -0500 Subject: [PATCH 5/6] Add "DiscoveryCommands" to yaml repl tests, make `TestDiscovery.yaml` pass (#24763) * Make discoverCommissionable work for now. More items to follow * TestDiscovery now passes * Minor update because short discriminator was wrong before * Stop trying to auto-cast short discriminator in discovery commands * Undo minmdns high verbosity * Restyle * Disable Test_TC_OO_2_4 * Set filter for MC to none (not needed) * Update after review comments * Update double-disable of flaky test... separating flaky from todo * Undo restyle only change * Make Test flaky instead of failing ... since it seems sometimes it may pass --------- Co-authored-by: Andrei Litvin --- scripts/tests/chiptest/__init__.py | 10 +- .../yamltest_with_chip_repl_tester.py | 8 +- src/app/tests/suites/TestDiscovery.yaml | 5 +- .../commands/discovery/DiscoveryCommands.cpp | 3 +- src/controller/python/chip/yaml/runner.py | 147 ++++++++++++++++-- .../chip-tool/zap-generated/test/Commands.h | 4 +- 6 files changed, 152 insertions(+), 25 deletions(-) diff --git a/scripts/tests/chiptest/__init__.py b/scripts/tests/chiptest/__init__.py index d8698e7a110a34..08153207f1420e 100644 --- a/scripts/tests/chiptest/__init__.py +++ b/scripts/tests/chiptest/__init__.py @@ -88,7 +88,6 @@ def _GetManualTests() -> Set[ManualTest]: manualtests.add(ManualTest(yaml="Test_TC_MEDIAPLAYBACK_6_2.yaml", reason="TODO")) manualtests.add(ManualTest(yaml="Test_TC_MEDIAPLAYBACK_6_3.yaml", reason="TODO")) manualtests.add(ManualTest(yaml="Test_TC_MEDIAPLAYBACK_6_4.yaml", reason="TODO")) - manualtests.add(ManualTest(yaml="Test_TC_OO_2_4.yaml", reason="Flaky")) manualtests.add(ManualTest(yaml="Test_TC_PCC_2_1.yaml", reason="TODO")) manualtests.add(ManualTest(yaml="Test_TC_PS_2_1.yaml", reason="TODO")) manualtests.add(ManualTest(yaml="Test_TC_SC_5_1.yaml", reason="TODO")) @@ -101,11 +100,15 @@ def _GetManualTests() -> Set[ManualTest]: manualtests.add(ManualTest(yaml="Test_TC_WNCV_2_5.yaml", reason="TODO")) manualtests.add(ManualTest(yaml="TestClusterMultiFabric.yaml", reason="TODO")) manualtests.add(ManualTest(yaml="TestCommissionerNodeId.yaml", reason="TODO")) - manualtests.add(ManualTest(yaml="TestDiscovery.yaml", reason="TODO")) manualtests.add(ManualTest(yaml="TestEvents.yaml", reason="TODO")) manualtests.add(ManualTest(yaml="TestGroupMessaging.yaml", reason="TODO")) manualtests.add(ManualTest(yaml="TestMultiAdmin.yaml", reason="TODO")) + # Failing, unclear why. Likely repl specific, used to pass however first + # failure point seems unrelated. Historically this seems (very?) flaky + # in repl. + manualtests.add(ManualTest(yaml="Test_TC_OO_2_4.yaml", reason="Flaky")) + # Examples: # # Currently these are not in ciTests.json, however yaml logic currently @@ -114,8 +117,7 @@ def _GetManualTests() -> Set[ManualTest]: # This is on purpose for now to make it harder to orphan files, however # we can reconsider as things evolve. manualtests.add(ManualTest(yaml="Config_Example.yaml", reason="Example")) - manualtests.add(ManualTest( - yaml="Config_Variables_Example.yaml", reason="Example")) + manualtests.add(ManualTest(yaml="Config_Variables_Example.yaml", reason="Example")) manualtests.add(ManualTest(yaml="PICS_Example.yaml", reason="Example")) manualtests.add(ManualTest(yaml="Response_Example.yaml", reason="Example")) manualtests.add(ManualTest(yaml="Test_Example.yaml", reason="Example")) diff --git a/scripts/tests/chiptest/yamltest_with_chip_repl_tester.py b/scripts/tests/chiptest/yamltest_with_chip_repl_tester.py index f852d30c9b4a53..f1d5583feb865c 100644 --- a/scripts/tests/chiptest/yamltest_with_chip_repl_tester.py +++ b/scripts/tests/chiptest/yamltest_with_chip_repl_tester.py @@ -15,6 +15,7 @@ # limitations under the License. import atexit +import logging import os import tempfile import traceback @@ -32,7 +33,7 @@ from chip.ChipStack import * from chip.yaml.runner import ReplTestRunner from matter_yamltests.definitions import SpecDefinitionsFromPaths -from matter_yamltests.parser import TestParser +from matter_yamltests.parser import PostProcessCheckStatus, TestParser _DEFAULT_CHIP_ROOT = os.path.abspath( os.path.join(os.path.dirname(__file__), "..", "..", "..")) @@ -118,6 +119,11 @@ def _StackShutDown(): post_processing_result = test_step.post_process_response( decoded_response) if not post_processing_result.is_success(): + logging.warning(f"Test step failure in 'test_step.label'") + for entry in post_processing_result.entries: + if entry.state == PostProcessCheckStatus.SUCCESS: + continue + logging.warning("%s: %s", entry.state, entry.message) raise Exception(f'Test step failed {test_step.label}') except Exception: print(traceback.format_exc()) diff --git a/src/app/tests/suites/TestDiscovery.yaml b/src/app/tests/suites/TestDiscovery.yaml index fc01f5c1cfef8b..474bfb02945884 100644 --- a/src/app/tests/suites/TestDiscovery.yaml +++ b/src/app/tests/suites/TestDiscovery.yaml @@ -20,6 +20,9 @@ config: discriminator: type: int16u defaultValue: 3840 + shortDiscriminator: + type: int16u + defaultValue: 15 vendorId: type: int16u defaultValue: 65521 @@ -146,7 +149,7 @@ tests: arguments: values: - name: "value" - value: discriminator + value: shortDiscriminator - label: "Check Commissioning Mode (_CM)" cluster: "DiscoveryCommands" diff --git a/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp b/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp index 7cf0dde53f525e..c3ba7b4ca0e65e 100644 --- a/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp +++ b/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp @@ -37,8 +37,7 @@ CHIP_ERROR DiscoveryCommands::FindCommissionableByShortDiscriminator( { ReturnErrorOnFailure(SetupDiscoveryCommands()); - uint64_t shortDiscriminator = static_cast((value.value >> 8) & 0x0F); - chip::Dnssd::DiscoveryFilter filter(chip::Dnssd::DiscoveryFilterType::kShortDiscriminator, shortDiscriminator); + chip::Dnssd::DiscoveryFilter filter(chip::Dnssd::DiscoveryFilterType::kShortDiscriminator, value.value); return mDNSResolver.DiscoverCommissionableNodes(filter); } diff --git a/src/controller/python/chip/yaml/runner.py b/src/controller/python/chip/yaml/runner.py index 0318d5262b9b79..8d8f6c970d8b7a 100644 --- a/src/controller/python/chip/yaml/runner.py +++ b/src/controller/python/chip/yaml/runner.py @@ -25,7 +25,7 @@ import chip.interaction_model import chip.yaml.format_converter as Converter import stringcase -from chip import ChipDeviceCtrl +from chip.ChipDeviceCtrl import ChipDeviceController, discovery from chip.clusters.Attribute import AttributeStatus, SubscriptionTransaction, TypedAttributePath, ValueDecodeFailure from chip.exceptions import ChipStackError from chip.yaml.errors import ParsingError, UnexpectedParsingError @@ -98,7 +98,7 @@ def pics_enabled(self): return self._pics_enabled @abstractmethod - def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult: + def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult: pass @@ -109,7 +109,7 @@ def __init__(self, test_step): if not _PSEUDO_CLUSTERS.supports(test_step): raise ParsingError(f'Default cluster {test_step.cluster} {test_step.command}, not supported') - def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult: + def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult: resp = asyncio.run(_PSEUDO_CLUSTERS.execute(self._test_step)) return _ActionResult(status=_ActionStatus.SUCCESS, response=None) @@ -118,7 +118,7 @@ class InvokeAction(BaseAction): '''Single invoke action to be executed.''' def __init__(self, test_step, cluster: str, context: _ExecutionContext): - '''Converts 'test_step' to invoke command action that can execute with ChipDeviceCtrl. + '''Converts 'test_step' to invoke command action that can execute with ChipDeviceController. Args: 'test_step': Step containing information required to run invoke command action. @@ -162,7 +162,7 @@ def __init__(self, test_step, cluster: str, context: _ExecutionContext): else: self._request_object = command_object - def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult: + def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult: try: resp = asyncio.run(dev_ctrl.SendCommand( self._node_id, self._endpoint, self._request_object, @@ -179,7 +179,7 @@ class ReadAttributeAction(BaseAction): '''Single read attribute action to be executed.''' def __init__(self, test_step, cluster: str, context: _ExecutionContext): - '''Converts 'test_step' to read attribute action that can execute with ChipDeviceCtrl. + '''Converts 'test_step' to read attribute action that can execute with ChipDeviceController. Args: 'test_step': Step containing information required to run read attribute action. @@ -224,7 +224,7 @@ def __init__(self, test_step, cluster: str, context: _ExecutionContext): raise UnexpectedParsingError( f'ReadAttribute doesnt have valid attribute_type. {self.label}') - def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult: + def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult: try: raw_resp = asyncio.run(dev_ctrl.ReadAttribute(self._node_id, [(self._endpoint, self._request_object)], @@ -289,7 +289,7 @@ def __init__(self, test_step): # Timeout is provided in seconds we need to conver to milliseconds. self._timeout_ms = request_data_as_dict['timeout'] * 1000 - def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult: + def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult: try: if self._expire_existing_session: dev_ctrl.ExpireSessions(self._node_id) @@ -326,7 +326,7 @@ class SubscribeAttributeAction(ReadAttributeAction): '''Single subscribe attribute action to be executed.''' def __init__(self, test_step, cluster: str, context: _ExecutionContext): - '''Converts 'test_step' to subscribe attribute action that can execute with ChipDeviceCtrl. + '''Converts 'test_step' to subscribe attribute action that can execute with ChipDeviceController. Args: 'test_step': Step containing information required to run write attribute action. @@ -349,7 +349,7 @@ def __init__(self, test_step, cluster: str, context: _ExecutionContext): f'SubscribeAttribute action does not have max_interval {self.label}') self._max_interval = test_step.max_interval - def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult: + def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult: try: subscription = asyncio.run( dev_ctrl.ReadAttribute(self._node_id, [(self._endpoint, self._request_object)], @@ -381,7 +381,7 @@ class WriteAttributeAction(BaseAction): '''Single write attribute action to be executed.''' def __init__(self, test_step, cluster: str, context: _ExecutionContext): - '''Converts 'test_step' to write attribute action that can execute with ChipDeviceCtrl. + '''Converts 'test_step' to write attribute action that can execute with ChipDeviceController. Args: 'test_step': Step containing information required to run write attribute action. @@ -425,7 +425,7 @@ def __init__(self, test_step, cluster: str, context: _ExecutionContext): # Create a cluster object for the request from the provided YAML data. self._request_object = attribute(request_data) - def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult: + def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult: try: resp = asyncio.run( dev_ctrl.WriteAttribute(self._node_id, [(self._endpoint, self._request_object)], @@ -463,7 +463,7 @@ def __init__(self, test_step, context: _ExecutionContext): if self._output_queue is None: raise UnexpectedParsingError(f'Could not find output queue') - def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult: + def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult: try: # While there should be a timeout here provided by the test, the current codegen version # of YAML tests doesn't have a per test step timeout, only a global timeout for the @@ -495,7 +495,7 @@ def __init__(self, test_step): self._setup_payload = request_data_as_dict['payload'] self._node_id = request_data_as_dict['nodeId'] - def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult: + def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult: resp = dev_ctrl.CommissionWithCode(self._setup_payload, self._node_id) if resp: @@ -504,10 +504,78 @@ def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult: return _ActionResult(status=_ActionStatus.ERROR, response=None) +class DiscoveryCommandAction(BaseAction): + """DiscoveryCommand implementation (FindCommissionable* methods).""" + + @staticmethod + def _filter_for_step(test_step) -> (discovery.FilterType, any): + """Given a test step, figure out the correct filters to give to + DiscoverCommissionableNodes. + """ + + if test_step.command == 'FindCommissionable': + return discovery.FilterType.NONE, None + + if test_step.command == 'FindCommissionableByCommissioningMode': + # this is just a "_CM" subtype + return discovery.FilterType.COMMISSIONING_MODE, None + + # all the items below require a "value" to use for filtering + args = test_step.arguments['values'] + request_data_as_dict = Converter.convert_list_of_name_value_pair_to_dict(args) + + filter = request_data_as_dict['value'] + + if test_step.command == 'FindCommissionableByDeviceType': + return discovery.FilterType.DEVICE_TYPE, filter + + if test_step.command == 'FindCommissionableByLongDiscriminator': + return discovery.FilterType.LONG_DISCRIMINATOR, filter + + if test_step.command == 'FindCommissionableByShortDiscriminator': + return discovery.FilterType.SHORT_DISCRIMINATOR, filter + + if test_step.command == 'FindCommissionableByVendorId': + return discovery.FilterType.VENDOR_ID, filter + + raise UnexpectedParsingError(f'Invalid command: {test_step.command}') + + def __init__(self, test_step): + super().__init__(test_step) + self.filterType, self.filter = DiscoveryCommandAction._filter_for_step(test_step) + + def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult: + devices = dev_ctrl.DiscoverCommissionableNodes( + filterType=self.filterType, filter=self.filter, stopOnFirst=True, timeoutSecond=5) + + # Devices will be a list: [CommissionableNode(), ...] + logging.info("Discovered devices: %r" % devices) + + if not devices: + logging.error("No devices found") + return _ActionResult(status=_ActionStatus.ERROR, response="NO DEVICES FOUND") + elif len(devices) > 1: + logging.warning("Commissionable discovery found multiple results!") + + return _ActionResult(status=_ActionStatus.SUCCESS, response=devices[0]) + + +class NotImplementedAction(BaseAction): + """Raises a "NOT YET IMPLEMENTED" exception when run.""" + + def __init__(self, test_step, cluster, command): + super().__init__(test_step) + self.cluster = cluster + self.command = command + + def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult: + raise Exception(f"NOT YET IMPLEMENTED: {self.cluster}::{self.command}") + + class ReplTestRunner: '''Test runner to encode/decode values from YAML test Parser for executing the TestStep. - Uses ChipDeviceCtrl from chip-repl to execute parsed YAML TestSteps. + Uses ChipDeviceController from chip-repl to execute parsed YAML TestSteps. ''' def __init__(self, test_spec_definition, certificate_authority_manager, alpha_dev_ctrl): @@ -624,7 +692,9 @@ def encode(self, request) -> BaseAction: # Some of the tests contain 'cluster over-rides' that refer to a different # cluster than that specified in 'config'. - if cluster == 'DelayCommands' and command == 'WaitForCommissionee': + elif cluster == 'DiscoveryCommands': + return DiscoveryCommandAction(request) + elif cluster == 'DelayCommands' and command == 'WaitForCommissionee': action = self._wait_for_commissionee_action_factory(request) elif command == 'writeAttribute': action = self._attribute_write_action_factory(request, cluster) @@ -668,6 +738,51 @@ def decode(self, result: _ActionResult): decoded_response['error'] = stringcase.snakecase(response.name).upper() return decoded_response + if isinstance(response, chip.discovery.CommissionableNode): + # CommissionableNode( + # instanceName='04DD55352DD2AC53', + # hostName='E6A32C6DBA8D0000', + # port=5540, + # longDiscriminator=3840, + # vendorId=65521, + # productId=32769, + # commissioningMode=1, + # deviceType=0, + # deviceName='', + # pairingInstruction='', + # pairingHint=36, + # mrpRetryIntervalIdle=None, + # mrpRetryIntervalActive=None, + # supportsTcp=True, + # addresses=['fd00:0:1:1::3', '10.10.10.1'] + # ), ... + decoded_response['value'] = { + 'instanceName': response.instanceName, + 'hostName': response.hostName, + 'port': response.port, + 'longDiscriminator': response.longDiscriminator, + 'vendorId': response.vendorId, + 'productId': response.productId, + 'commissioningMode': response.commissioningMode, + 'deviceType': response.deviceType, + 'deviceName': response.deviceName, + 'pairingInstruction': response.pairingInstruction, + 'pairingHint': response.pairingHint, + 'mrpRetryIntervalIdle': response.mrpRetryIntervalIdle, + 'mrpRetryIntervalActive': response.mrpRetryIntervalActive, + 'supportsTcp': response.supportsTcp, + 'addresses': response.addresses, + + # TODO: NOT AVAILABLE + 'rotatingIdLen': 0, + + # derived values + 'numIPs': len(response.addresses), + + } + + return decoded_response + if isinstance(response, ChipStackError): decoded_response['error'] = 'FAILURE' return decoded_response diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index c69097fde2ab1f..995f4a5e66bdff 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -65274,6 +65274,7 @@ class TestDiscoverySuite : public TestCommand AddArgument("nodeId", 0, UINT64_MAX, &mNodeId); AddArgument("endpoint", 0, UINT16_MAX, &mEndpoint); AddArgument("discriminator", 0, UINT16_MAX, &mDiscriminator); + AddArgument("shortDiscriminator", 0, UINT16_MAX, &mShortDiscriminator); AddArgument("vendorId", 0, UINT16_MAX, &mVendorId); AddArgument("productId", 0, UINT16_MAX, &mProductId); AddArgument("deviceType", 0, UINT16_MAX, &mDeviceType); @@ -65298,6 +65299,7 @@ class TestDiscoverySuite : public TestCommand chip::Optional mNodeId; chip::Optional mEndpoint; chip::Optional mDiscriminator; + chip::Optional mShortDiscriminator; chip::Optional mVendorId; chip::Optional mProductId; chip::Optional mDeviceType; @@ -65619,7 +65621,7 @@ class TestDiscoverySuite : public TestCommand LogStep(8, "Check Short Discriminator (_S)"); ListFreer listFreer; chip::app::Clusters::DiscoveryCommands::Commands::FindCommissionableByShortDiscriminator::Type value; - value.value = mDiscriminator.HasValue() ? mDiscriminator.Value() : 3840ULL; + value.value = mShortDiscriminator.HasValue() ? mShortDiscriminator.Value() : 15ULL; return FindCommissionableByShortDiscriminator(kIdentityAlpha, value); } case 9: { From 6b8e32eea27e0d44caed5b5499cb401241a3540f Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 2 Feb 2023 10:09:15 -0500 Subject: [PATCH 6/6] Allow checking constraint type by struct name (#24802) --- .../matter_yamltests/constraints.py | 44 ++++++++++--------- .../matter_yamltests/parser.py | 9 +++- 2 files changed, 31 insertions(+), 22 deletions(-) diff --git a/scripts/py_matter_yamltests/matter_yamltests/constraints.py b/scripts/py_matter_yamltests/matter_yamltests/constraints.py index 22d461c960dd43..b186fb1bef6282 100644 --- a/scripts/py_matter_yamltests/matter_yamltests/constraints.py +++ b/scripts/py_matter_yamltests/matter_yamltests/constraints.py @@ -32,7 +32,7 @@ def __init__(self, types: list, is_null_allowed: bool = False): self._types = types self._is_null_allowed = is_null_allowed - def is_met(self, value): + def is_met(self, value, value_type_name): if value is None: return self._is_null_allowed @@ -43,10 +43,10 @@ def is_met(self, value): if not found_type_match: return False - return self.check_response(value) + return self.check_response(value, value_type_name) @abstractmethod - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: pass @@ -55,13 +55,13 @@ def __init__(self, has_value): super().__init__(types=[]) self._has_value = has_value - def is_met(self, value): + def is_met(self, value, value_type_name): # We are overriding the BaseConstraint of is_met since has value is a special case where # we might not be expecting a value at all, but the basic null check in BaseConstraint # is not what we want. - return self.check_response(value) + return self.check_response(value, value_type_name) - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: has_value = value is not None return self._has_value == has_value @@ -71,7 +71,7 @@ def __init__(self, type): super().__init__(types=[], is_null_allowed=True) self._type = type - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: success = False if self._type == 'boolean' and type(value) is bool: success = True @@ -195,6 +195,8 @@ def check_response(self, value) -> bool: success = value >= -36028797018963967 and value <= 36028797018963967 elif self._type == 'nullable_int64s' and type(value) is int: success = value >= -9223372036854775807 and value <= 9223372036854775807 + else: + success = self._type == value_type_name return success @@ -203,7 +205,7 @@ def __init__(self, min_length): super().__init__(types=[str, bytes, list]) self._min_length = min_length - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return len(value) >= self._min_length @@ -212,7 +214,7 @@ def __init__(self, max_length): super().__init__(types=[str, bytes, list]) self._max_length = max_length - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return len(value) <= self._max_length @@ -221,7 +223,7 @@ def __init__(self, is_hex_string: bool): super().__init__(types=[str]) self._is_hex_string = is_hex_string - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return all(c in string.hexdigits for c in value) == self._is_hex_string @@ -230,7 +232,7 @@ def __init__(self, starts_with): super().__init__(types=[str]) self._starts_with = starts_with - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return value.startswith(self._starts_with) @@ -239,7 +241,7 @@ def __init__(self, ends_with): super().__init__(types=[str]) self._ends_with = ends_with - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return value.endswith(self._ends_with) @@ -248,7 +250,7 @@ def __init__(self, is_upper_case): super().__init__(types=[str]) self._is_upper_case = is_upper_case - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return value.isupper() == self._is_upper_case @@ -257,7 +259,7 @@ def __init__(self, is_lower_case): super().__init__(types=[str]) self._is_lower_case = is_lower_case - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return value.islower() == self._is_lower_case @@ -266,7 +268,7 @@ def __init__(self, min_value): super().__init__(types=[int, float], is_null_allowed=True) self._min_value = min_value - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return value >= self._min_value @@ -275,7 +277,7 @@ def __init__(self, max_value): super().__init__(types=[int, float], is_null_allowed=True) self._max_value = max_value - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return value <= self._max_value @@ -284,7 +286,7 @@ def __init__(self, contains): super().__init__(types=[list]) self._contains = contains - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return set(self._contains).issubset(value) @@ -293,7 +295,7 @@ def __init__(self, excludes): super().__init__(types=[list]) self._excludes = excludes - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return set(self._excludes).isdisjoint(value) @@ -302,7 +304,7 @@ def __init__(self, has_masks_set): super().__init__(types=[int]) self._has_masks_set = has_masks_set - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return all([(value & mask) == mask for mask in self._has_masks_set]) @@ -311,7 +313,7 @@ def __init__(self, has_masks_clear): super().__init__(types=[int]) self._has_masks_clear = has_masks_clear - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return all([(value & mask) == 0 for mask in self._has_masks_clear]) @@ -320,7 +322,7 @@ def __init__(self, not_value): super().__init__(types=[], is_null_allowed=True) self._not_value = not_value - def check_response(self, value) -> bool: + def check_response(self, value, value_type_name) -> bool: return value != self._not_value diff --git a/scripts/py_matter_yamltests/matter_yamltests/parser.py b/scripts/py_matter_yamltests/matter_yamltests/parser.py index 2c9a14a4e92b43..cb17d7a35846f8 100644 --- a/scripts/py_matter_yamltests/matter_yamltests/parser.py +++ b/scripts/py_matter_yamltests/matter_yamltests/parser.py @@ -233,6 +233,7 @@ def __init__(self, test: dict, config: dict, definitions: SpecDefinitions, pics_ argument_mapping = None response_mapping = None + response_mapping_name = None if self.is_attribute: attribute = definitions.get_attribute_by_name( @@ -242,6 +243,7 @@ def __init__(self, test: dict, config: dict, definitions: SpecDefinitions, pics_ attribute.definition.data_type.name) argument_mapping = attribute_mapping response_mapping = attribute_mapping + response_mapping_name = attribute.definition.data_type.name else: command = definitions.get_command_by_name( self.cluster, self.command) @@ -250,9 +252,11 @@ def __init__(self, test: dict, config: dict, definitions: SpecDefinitions, pics_ definitions, self.cluster, command.input_param) response_mapping = self._as_mapping( definitions, self.cluster, command.output_param) + response_mapping_name = command.output_param self.argument_mapping = argument_mapping self.response_mapping = response_mapping + self.response_mapping_name = response_mapping_name self.update_arguments(self.arguments_with_placeholders) self.update_response(self.response_with_placeholders) @@ -678,6 +682,7 @@ def _response_constraints_validation(self, response, result): error_success = 'Constraints check passed' error_failure = 'Constraints check failed' + response_type_name = self._test.response_mapping_name for value in self.response['values']: if 'constraints' not in value: continue @@ -685,6 +690,8 @@ def _response_constraints_validation(self, response, result): received_value = response.get('value') if not self.is_attribute: expected_name = value.get('name') + response_type_name = self._test.response_mapping.get( + expected_name) if received_value is None or expected_name not in received_value: received_value = None else: @@ -693,7 +700,7 @@ def _response_constraints_validation(self, response, result): constraints = get_constraints(value['constraints']) - if all([constraint.is_met(received_value) for constraint in constraints]): + if all([constraint.is_met(received_value, response_type_name) for constraint in constraints]): result.success(check_type, error_success) else: # TODO would be helpful to be more verbose here