Add Camera clusters XML #35773
Draft
Add Camera clusters XML #35773
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Exclude self-symlinks
This reverts commit fb361b0.
…ments referenced in python_testing
Generated using Alchemy: https://github.com/project-chip/alchemy
…fabric_idx constant make it difficult to have this in XSD
This reverts commit 01c4f87.
# Conflicts: # src/app/zap-templates/zcl/data-model/all.xml # src/app/zap_cluster_list.json
|
Review changes with SemanticDiff. Analyzed 12 of 152 files. Overall, the semantic diff is 2% smaller than the GitHub diff. File Information
|
github-actions
bot
added
documentation
|
@aronrosenberg FYI as this is the zap generation for the spec pull you have |
There was a problem hiding this comment.
Please do split these out. Finding several small chunks of time to review separate PRs is much easier than finding a big chunk of time to review a single large PR. Splitting things out also allows the review work to be parallelized across more people, in general...
Note that there are merge issues here with ZLL_COMMISSIONING_CLUSTER being incorrectly re-introduced and whatnot. Those would be easier to catch in a smaller PR.
In terms of shared concepts... those are not really relevant to reviewing whether the spec was correctly translated into the XML.
By the way, whatever hand-edits you had to make should probably be turned into alchemy issues, if they were edits to alchemy output?
There was a problem hiding this comment.
Sounds good, will split it out!
Had some general questions here though before doing that:
- I put these in
/draft- is that correct, or should I just put them in/chip? - Hand edits: the camera spec is dependent on ~typedefs, e.g.
VideoStreamID: This data type is derived from uint16- but there doesn't seem to be a way to specify that in the zap xml schema, so I think it'd actually need zap generation changes - structs used across clusters: should I put these in
global-structs.xmlas I have here, have them defined in one of the clusters, or something else altogether? - If I don't add a
<cluster code="0x0551"/>to those shared structures, zap generation ends up generating it in every single.matterfile (even non-camera ones); if I add that restriction, though, it seems it causes duplicate definition (see build errors:multiple definition of ‘enum class chip::app::Clusters::detail::StreamTypeEnum’)- what's the right way of dealing with these shared structures?
Thanks Boris!
There was a problem hiding this comment.
- Good question. I think the "draft" bits got removed in Remove all un-specified draft clusters from SDK #35583 so putting them in
draftlikely does not do the right thing. - Ah, yes, I discussed this with @andyg-apple just yesterday. Please talk to @hasty about Alchemy handling this by desugaring the typedefs for now?
- global-structs should be for actually global structs, which are not tied to any cluster or list of clusters at all. There are some open questions about which, if any, of the camera structs will end up that way. For now, I'd put them in one of the clusters, or a new camera-structs.xml or something.
- For global things (not associated with any clusters), listing them in all .matter files is correct. Things that are associated with clusters should work; please drop me a link on Slack to a branch or SHA that has the duplicate definition error for you, and I will take a look.
There was a problem hiding this comment.
- As far as I can tell, as long as the
/draftpath is specified inxmlRootofzcl.json, it seems to work & generate the files - but if we're moving away from the/draftpath (which maybe seems to be the case?) then I'll just put these in/chip(let me know otherwise) - Sounds good, I'll make the changes to alchemy, shouldn't be a problem (the files it will generate should look like my hand-edited ones, but I'll make the changes & verify in parallel)
- Will do 👍
- Will do 👍 (UPDATED: welp I can't get it to repro so I must be missing something, ignore this one)
There was a problem hiding this comment.
- Right, but it's not specified there anymore, right? I figure putting these in
chipmakes sense.
There was a problem hiding this comment.
Alchemy support for "typedefs" so that we automatically get the ID types to be output as their raw simple types @ project-chip/alchemy#5
This was referenced Sep 25, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add camera clusters from https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/10004
Generated with (with some hand-edits to work with ZAP compile):
Note to reviewer: if you prefer, I'm happy to split this into 1-pull-per-cluster, I just thought it may be easier to review them together as they share structures & concepts
Note that this pull also includes some necessary dependent changes (which, I'm also happy to split out to separate pull if preferred)
ThreeLevelAutoEnum, which [although defined in the spec] did not seem to have a type speczcl.xsd