Camera spec updates #37229
Open
Camera spec updates #37229
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
gmarcosb
force-pushed
the
spec-updates
branch
from
806f163 to
4e9ec45
Compare
|
@andy31415 how do I circumvent the |
gmarcosb
added
the
skip-protocol-compatibility
gmarcosb
force-pushed
the
spec-updates
branch
from
4e9ec45 to
99fe4cc
Compare
andy31415
reviewed
Jan 28, 2025
| kMedium = 1; | ||
| kHigh = 2; | ||
| kAutomatic = 3; | ||
| kAuto = 0; |
There was a problem hiding this comment.
This seems like a backwards-incompatible change
Any ideas what is going on here? Why does a camera change affect air purifier app?
There was a problem hiding this comment.
ThreeLevelAutoEnum is a global enum, usable by any cluster, but only currently used by the camera clusters
There was a problem hiding this comment.
We were asked to rename the field and put Auto=0 by the DataModel team. Cameras is the first to use it and defined the new global type.
andy31415
reviewed
Jan 28, 2025
| <item name="Medium" value="0x01"/> | ||
| <item name="High" value="0x02"/> | ||
| <item name="Automatic" value="0x03"/> | ||
| <item name="Auto" value="0x00"/> |
There was a problem hiding this comment.
Is this provisional? Globals make it hard to figure out where things originale from. Maybe we can ask alchemy to somehow hightligh section/usage for easier review
There was a problem hiding this comment.
Camera is the first cluster to use ThreeLevelAutoEnum - it's a global to be used by any cluster, but currently only used by cameras (& introduced with cameras)
There was a problem hiding this comment.
andy31415
reviewed
Jan 28, 2025
| @@ -36,7 +36,7 @@ Git: 1.4-534-g3214b3502 | |||
| </struct> | |||
|
|
|||
| <cluster apiMaturity="provisional"> | |||
| <domain name="General"/> | |||
| <domain name="General">General</domain> | |||
There was a problem hiding this comment.
webrtc provider does not have this duplicate name and content for the domain. Why did we get this change?
There was a problem hiding this comment.
Something changed with alchemy 😬 It's a benign change, because the value is read from both, but I was going to submit a PR to only use value or content, not both
gmarcosb
force-pushed
the
spec-updates
branch
from
ebc1438 to
afeefa5
Compare
|
PR #37229: Size comparison from a3561e1 to afeefa5 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| <item fieldId="2" name="VendorID" type="vendor_id"/> | ||
| <item fieldId="3" name="FabricID" type="fabric_id"/> | ||
| <item fieldId="4" name="NodeID" type="node_id"/> | ||
| <item fieldId="5" name="Label" type="char_string" length="32"/> | ||
| <item fieldId="6" name="VidVerificationStatement" type="octet_string" optional="true" length="85" minLength="85"/> |
There was a problem hiding this comment.
This has nothing to do with cameras....
Furthermore, shouldn't there be provisional annotations on the VID verification bits being added here and elsewhere in this file?
| <name>Zone Management</name> | ||
| <code>0x0550</code> | ||
| <define>ZONE_MANAGEMENT_CLUSTER</define> | ||
| <description>This cluster provides an interface to manage regions of interest, or Zones, which can be either manufacturer or user defined.</description> | ||
| <client init="false" tick="false">true</client> | ||
| <features> | ||
| <feature bit="0" code="TWODCART" name="TwoDimensionalCartesianZone" summary="Support Two Dimensional Cartesian Zones"> | ||
| <optionalConform/> | ||
| <mandatoryConform/> |
There was a problem hiding this comment.
Why do we have a feature bit which is always required to be true? This is spec feedback, not here...
| @@ -38,7 +38,7 @@ Git: 0.9-fall2024-301-g4e2f0c1c4 | |||
| <server init="false" tick="false">true</server> | |||
| <globalAttribute code="0xFFFD" side="either" value="1"/> | |||
| <attribute code="0x0000" side="server" define="INSTALLED_CHIME_SOUNDS" type="array" entryType="ChimeSoundStruct" length="255" minLength="1">InstalledChimeSounds</attribute> | |||
| <attribute code="0x0001" side="server" define="ACTIVE_CHIME_SOUND_ID" type="int8u" min="0" max="255" default="0" writable="true">ActiveChimeID</attribute> | |||
| <attribute code="0x0001" side="server" define="ACTIVE_CHIME_SOUND_ID" type="int8u" min="0" max="255" default="0" writable="true">SelectedChime</attribute> | |||
There was a problem hiding this comment.
Doesn't this need changes to chime-server, which now won't compile? Why do we have dead code that nothing compiles in our tree (which is how this is passing CI)?
There was a problem hiding this comment.
I would've thought so, but none of the CI failed (I was getting the PR going to find out which CI failed so I could run the commands locally)
Does that mean the cluster server impls don't compile in CI, or at least, the chime server? 😬😬
I'll try to put together a command locally to run it... I am confused as well
There was a problem hiding this comment.
Does that mean the cluster server impls don't compile in CI, or at least, the chime server? 😬😬
It means the chime server is not compiled in CI. Probably because no example app that's compiled in CI actually enables it.
There was a problem hiding this comment.
That is what I observed too. I had to add the camera-av-stream-mgmt cluster to an example app's zap definition for that server file to compile. So, without a representation of the cluster server impl in any example app, it won't be built. Not just CI, even locally.
gmarcosb
force-pushed
the
spec-updates
branch
from
eb9c997 to
8dc4cbd
Compare
|
PR #37229: Size comparison from c56eb2b to 8dc4cbd Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
gmarcosb
force-pushed
the
spec-updates
branch
from
8dc4cbd to
2e1e541
Compare
gmarcosb
force-pushed
the
spec-updates
branch
from
2e1e541 to
2490a24
Compare
gmarcosb
force-pushed
the
spec-updates
branch
from
2490a24 to
2a52b87
Compare
|
PR #37229: Size comparison from 95ba8a4 to 2a52b87 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
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.
Spec updates from https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/10794
Generated with Alchemy & zap_regen_all.py
skip-protocol-compatibility: All the structs & enums modified here are part of the camera spec, which is provisionalTesting
Verified by CI