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.

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

[SVE2 feedback] TestEventTrigger uses non-spec-compliant status code #22232

Closed
tcarmelveilleux opened this issue Aug 29, 2022 · 2 comments · Fixed by #22695
Closed

[SVE2 feedback] TestEventTrigger uses non-spec-compliant status code #22232

tcarmelveilleux opened this issue Aug 29, 2022 · 2 comments · Fixed by #22695
Assignees
Labels
spec Mismatch between spec and implementation sve V1.0

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

Found during SVE2

In General Commissioning Cluster TestEventTrigger command, when EnableKey is given incorrectly, the spec says EnableKeyMismatch status code should be given.

Spec:

11.11.8.1. TestEventTrigger Command

In order to prevent unwittingly actuating a particular trigger, this command SHALL respond with the cluster-specific error status code EnableKeyMismatch if the EnableKey field does not match the a-priori value configured on the device

However, the SDK currently uses UNSUPPORTED_ACCESS:

if (triggerDelegate == nullptr || !triggerDelegate->DoesEnableKeyMatch(commandData.enableKey))
{
    commandObj->AddStatus(commandPath, Status::UnsupportedAccess);
    return true;
}

Also, the SDK never got the EnableKeyMismatch enum, and this would probably break wire compatibility.

Proposed Solution

Either:

  • Update spec and SDK to use INVALID_COMMAND instead (which would be logical and match test plan), and would likely be less problematic for controllers in terms of wire format
  • Add the correct cluster-specific status
@tcarmelveilleux
Copy link
Contributor Author

After discussions with @robszewczyk regarding the state of the spec, we will make the SDK match the test plans, and will amend the spec post-1.0. Since the only differences are in error paths, and error paths can always be detected differently than success paths, and nobody actually used TestEvenTrigger in test plasn in 1.0, we can fix it like that.

@tcarmelveilleux
Copy link
Contributor Author

This impacts TC-DGEN-2.3

tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Sep 16, 2022
- There was a 3-way mismatch between spec, SDK and SVE2 test plans
  in the TestEventTrigger command.
- Since this command is meant for cert testing and 1.0 spec is loced
  down, it was determined that making behavior match TC-DGEN-2.3
  is the best outcome.

Fixes project-chip#22232

Changes:

- Make all EnableKey errors are ConstraintError
- Add test event trigger support to all Linux examples by default
  with a command that always succeeds in the reserved range
- Add integration test for feature
- Fix Python IM status codes list that had an old name
- Add "commission-only" mode to matter_testing_support.py done while
  testing this feature

Testing done:
- All unit tests still pass
- Integration tests pass, including new TC_TestEventTrigger.py
tcarmelveilleux added a commit that referenced this issue Sep 16, 2022
* Update TestEventTrigger to match SVE2 test plan

- There was a 3-way mismatch between spec, SDK and SVE2 test plans
  in the TestEventTrigger command.
- Since this command is meant for cert testing and 1.0 spec is loced
  down, it was determined that making behavior match TC-DGEN-2.3
  is the best outcome.

Fixes #22232

Changes:

- Make all EnableKey errors are ConstraintError
- Add test event trigger support to all Linux examples by default
  with a command that always succeeds in the reserved range
- Add integration test for feature
- Fix Python IM status codes list that had an old name
- Add "commission-only" mode to matter_testing_support.py done while
  testing this feature

Testing done:
- All unit tests still pass
- Integration tests pass, including new TC_TestEventTrigger.py

* Update some comments

* Fix CI YAML config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Mismatch between spec and implementation sve V1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants