-
Notifications
You must be signed in to change notification settings - Fork 2k
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
pressurement cluster again #7996
pressurement cluster again #7996
Conversation
@bzbarsky-apple here is the second attempt, lets see if we can get it merged before the repo runs away again ;) |
@janusheide This PR removes the pressure management cluster from various apps (e.g. all-clusters-app). Is that intended? |
@@ -0,0 +1,38 @@ | |||
<?xml version="1.0"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should presumably live in some directory where ZAP will find it, which will fix the "this is removing this cluster from everything" problem. Something like src/app/zap-templates/zcl/data-model/chip
perhaps?
@janusheide I tried to fix this up, but I don't have write permissions to git@github.com:Grundfos/connectedhomeip.git The fixup goes like this:
With that, you are done. You can run regeneration if you want, but there should be no changed to generated files (and ther are not for me), because the XML is just moving around with no other changes. |
de2b9cb
to
dbcd97a
Compare
… pressure-measurement-cluster-2
I opted to go back to the first commit, which did not contain any auto generated stuff, fix that and force push to the branch, simpler and requires no merging work. |
ZAP ci error seems real. The zap configuration file for all-cluster-app (and any that had the pressurment cluster enabled) need to be update also |
I am unable to fix this as the zap tool got broken here after a recent update... |
@jmartinez-silabs too me the zap ci error looks like an error caused by the zap tool, do you have an idea of what needs to be done to fix this? |
@janusheide The ZAP error shows that it fails to do the generatoion of files for all-clusters-app base on this .zap configuration. From what I see from the diff files you moved some definitions from one xml file to another. the Pressure Measurement was in another domain and i don't see any change to the configuration files (.zap) Seems like the .zap file is still configured to enable the clusters Pressure Measurement in the Measurement & Sensing domain that doesn't exist anymore. Verify tha the zap files are correctly configured based on your changes |
…connectedhomeip into pressure-measurement-cluster-2
/rebase |
@jmartinez-silabs @Damian-Nordic @jepenven-silabs @msandstedt @saurabhst Please take a look? |
@janusheide Was the change of ScaledTolerance from int16s to int16u purposeful? |
@bzbarsky-apple according to the spec https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/master/src/app_clusters/PressureMeasurement.adoc scaled tolerance should be unsigned. It was unsigned in the zcl raw imported version spec. So it looks like the spec and impl diverted before and now they are the same. To me it makes sense that it is alway positive, but from the definition of the scaled tolerance it looks like a tolerance of +42 and -42 would give the same range, but in different orders. |
@janusheide OK, great. I just wanted to make sure that was a purposeful change, since the PR description just talked about moving to a separate XML; not saying we shouldn't have made it in general. |
* pressurement cluster again * moved xml file to proper folder * adding dropped ranges (again) * added default values * scaled tolerance unsigned * removed ranges not in spec * trying to remove this, to see if we can pass the ci * fix xml errors * Update src/app/zap-templates/zcl/data-model/silabs/ha.xml
that causes ZAP CI to fail (project-chip#8280)
Problem
What is being fixed? Examples:
Change overview
Testing
How was this tested? (at least one bullet point required)
superseeds #7228