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

Jump to bottom

[Documentation] XML vs Specs differences Group Key management cluster #25642

Closed
Apollon77 opened this issue Mar 11, 2023 · 6 comments · Fixed by #28413
Closed

[Documentation] XML vs Specs differences Group Key management cluster #25642

Apollon77 opened this issue Mar 11, 2023 · 6 comments · Fixed by #28413
Labels
cert blocker Spec XML align SDK XML does not match the spec (including naming, etc)

Comments

@Apollon77
Copy link
Contributor

Documentation issues

  • The command "KeySetReadAllIndices" has no request parameters according to specs, but XML defines an ID as input. I would assume XML to be wrong when checking the meaning of the command
  • The Feature defined in the specs is not contained in the XML
  • The GroupKeySetStruct misses the GroupKey­Multicast­Policy enum as defined in the XML (ok this is a provisional feature, no ideal how to handle that?

Platform

No response

Anything else?

No response
@Apollon77 Apollon77 mentioned this issue Mar 11, 2023
Merged
@bzbarsky-apple
Copy link
Contributor

I would assume XML to be wrong when checking the meaning of the command

The XML is wrong; the question is how we fix it without breaking compat at this point... Need to think about this carefully.

The Feature defined in the specs is not contained in the XML

Yep, not least because that feature is just not implemented in the SDK, so there's been no need for it....

The GroupKeySetStruct misses the GroupKey­Multicast­Policy enum as defined in the XML (ok this is a provisional feature, no ideal how to handle that?

If it's provisional, it should generally not be included in the XML.

@bzbarsky-apple bzbarsky-apple added the Spec XML align SDK XML does not match the spec (including naming, etc) label Mar 13, 2023
@bzbarsky-apple
Copy link
Contributor

I filed https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/6344 to track reserving this field id on the spec side.

@Apollon77
Copy link
Contributor Author

Apollon77 commented Mar 14, 2023 •
edited
Loading

So we will add this field to the specs even that it makes no real sense? Ahh you just make sure that if ever fields get added they do not use this field ID?)

@bzbarsky-apple
Copy link
Contributor

Ahh you just make sure that if ever fields get added they do not use this field ID?

Yes, exactly.

@github-project-automation github-project-automation bot moved this to Open Cert Blockers in [Certification] Blockers May 23, 2023
@ReneJosefsen
Copy link
Contributor

The GroupKeySetStruct misses the GroupKey­Multicast­Policy enum as defined in the XML (ok this is a provisional feature, no ideal how to handle that?

Can you elaborate on this, looking at the spec, the defined GroupKeySetStruct type does include GroupKeyMulticastPolicy (field 8) and the type of that field is GroupKeyMulticastPolicyEnum?

@bzbarsky-apple
Copy link
Contributor

@ReneJosefsen The spec has that, but the XML in the SDK does not.

bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 2, 2023
@bzbarsky-apple
Remove the incorrect GroupKeySetIDs field from the KeySetReadAllIndic…
193d750 
…es command.

This field does not exist in the spec.

This is safe to do in terms of compat because the generated command parsing code
only parses the fields that are actually present and does not error out if
mandatory fields are missing; instead if uses type-default values for them.  So
commands updated clients send will still be parse-able by servers that had this
field in their XML.

Fixes part of project-chip#25642

For the Darwin codegen, I just special-cased this command in the templates for
now, but if we end up with more commands going from having required fields to
not having them we can figure out a more automated solution.
@bzbarsky-apple bzbarsky-apple mentioned this issue Jun 2, 2023
Merged
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 6, 2023
@bzbarsky-apple
Remove the incorrect GroupKeySetIDs field from the KeySetReadAllIndic…
df6e357 
…es command.

This field does not exist in the spec.

This is safe to do in terms of compat because the generated command parsing code
only parses the fields that are actually present and does not error out if
mandatory fields are missing; instead if uses type-default values for them.  So
commands updated clients send will still be parse-able by servers that had this
field in their XML.

Fixes part of project-chip#25642

For the Darwin codegen, I just special-cased this command in the templates for
now, but if we end up with more commands going from having required fields to
not having them we can figure out a more automated solution.
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 6, 2023
@bzbarsky-apple
Remove the incorrect GroupKeySetIDs field from the KeySetReadAllIndic…
1326922 
…es command.

This field does not exist in the spec.

This is safe to do in terms of compat because the generated command parsing code
only parses the fields that are actually present and does not error out if
mandatory fields are missing; instead if uses type-default values for them.  So
commands updated clients send will still be parse-able by servers that had this
field in their XML.

Fixes part of project-chip#25642

For the Darwin codegen, I just special-cased this command in the templates for
now, but if we end up with more commands going from having required fields to
not having them we can figure out a more automated solution.
bzbarsky-apple added a commit that referenced this issue Jun 6, 2023
@bzbarsky-apple
Remove the incorrect GroupKeySetIDs field from the KeySetReadAllIndic…
3d44787 
…es command. (#27044)

* Remove the incorrect GroupKeySetIDs field from the KeySetReadAllIndices command.

This field does not exist in the spec.

This is safe to do in terms of compat because the generated command parsing code
only parses the fields that are actually present and does not error out if
mandatory fields are missing; instead if uses type-default values for them.  So
commands updated clients send will still be parse-able by servers that had this
field in their XML.

Fixes part of #25642

For the Darwin codegen, I just special-cased this command in the templates for
now, but if we end up with more commands going from having required fields to
not having them we can figure out a more automated solution.

* Regenerate generated code.

* Address review comments.
@andy31415 andy31415 mentioned this issue Jul 28, 2023
Closed
@tcarmelveilleux tcarmelveilleux mentioned this issue Jul 31, 2023
Merged
@mergify mergify bot closed this as completed in #28413 Jul 31, 2023
@github-project-automation github-project-automation bot moved this from Open Cert Blockers to Complete in [Certification] Blockers Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cert blocker Spec XML align SDK XML does not match the spec (including naming, etc)
Projects
[Certification] Blockers
Archived in project
[Enhancements] Spec/XML Alignment
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add missing enums in Group Key Management cluster tcarmelveilleux/connectedhomeip
4 participants
@Apollon77 @woody-apple @bzbarsky-apple @ReneJosefsen