Bugfix termostat user interface value validation #11058
Merged
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
|
PR #11058: Size comparison from 9a618bb to 30fc7fe Increases above 0.2%:
Increases (1 build for linux)
Decreases (1 build for esp32)
Full report (36 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
bzbarsky-apple
requested changes
Oct 28, 2021
| Protocols::InteractionModel::Status status = Protocols::InteractionModel::Status::Success; | ||
| if (attributePath.mClusterId == app::Clusters::Thermostat::Id) | ||
| { | ||
| status = MatterThermostatClusterServerPreAttributeChangedCallback(attributePath, type, size, value); |
There was a problem hiding this comment.
So this is relying on all apps that use the thermostat cluster to make those calls?
It seems to me like we should be internally calling cluster-specific pre-change callbacks to handle things like this instead of forcing everyone using the SDK to know about needing to call these things. The app callback should be for app-level validation that can't be handled internally.
There was a problem hiding this comment.
Yes, I would also prefer that. I mean the XML definition should have the limits. So why would one need to type that stuff out.
There was a problem hiding this comment.
It'd still need to be typed out for now, but only once (in the shared cluster impl)...
There was a problem hiding this comment.
As far as I know the cluster specific callbacks were not yet in place.
There was a problem hiding this comment.
I see the cluster-specific pre-change callbacks present in the tv-casting app:
const EmberAfGenericClusterFunction chipFuncArrayIasZoneServer[] = { \
(EmberAfGenericClusterFunction) emberAfIasZoneClusterServerInitCallback, \
(EmberAfGenericClusterFunction) emberAfIasZoneClusterServerMessageSentCallback, \
(EmberAfGenericClusterFunction) MatterIasZoneClusterServerPreAttributeChangedCallback, \
}; \
Not sure if it is working though. But if it isn't, we should endeavor to fix it.
There was a problem hiding this comment.
It should work, assuming the cluster has been added to: https://github.com/project-chip/connectedhomeip/blob/master/src/app/zap-templates/templates/app/helper.js#L109
This is something I'm trying to get rid off though but it will takes time, so it sounds fair to reuse the linked mechanism.
There was a problem hiding this comment.
@bzbarsky-apple removed the hook up through the MatterPreAttributeChangeCallback.
...ostat-user-interface-configuration-server/thermostat-user-interface-configuration-server.cpp
Outdated
Show resolved
Hide resolved
...ostat-user-interface-configuration-server/thermostat-user-interface-configuration-server.cpp
Outdated
Show resolved
Hide resolved
pullapprove
bot
requested review from
anush-apple,
austinh0,
balducci-apple,
carol-apple,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
franck-apple,
harimau-qirex,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost,
LuDuda,
mlepage-google,
msandstedt,
mspang,
pan-apple,
robszewczyk and
sagar-apple
|
PR #11058: Size comparison from 4947759 to d3aeaa2 Increases above 0.2%:
Increases (1 build for linux)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
/rebase |
added 6 commits
November 8, 2021 11:47
…on in all-clsuter-app
|
PR #11058: Size comparison from 3f82de1 to 909c591 Increases (2 builds for linux, p6)
Full report (21 builds for efr32, k32w, linux, p6, qpg, telink)
|
|
PR #11058: Size comparison from 3f82de1 to d47da28 Increases (5 builds for esp32, linux, mbed, p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
bzbarsky-apple
approved these changes
Nov 8, 2021
woody-apple
approved these changes
Nov 9, 2021
woody-apple
pushed a commit
that referenced
this pull request
Nov 9, 2021
* add support for boundry checks for thermostat-user-interface-configuration * add impl for boundry checks for thermostat-user-interface-configuration in all-clsuter-app * add check for size and remove static cast to unit8 * add support for ProAttribute hook for thermostat user interface configuration * remove preattribute hooks in all-cluster-app/linux * regen all * fix esp32 and mbed builds
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this pull request
Nov 9, 2021
These got added along with code in initial drafts of project-chip#11058 but then the code was removed without removing the now-unnecessary headers.
|
#11587 to address my review comment. |
andy31415
pushed a commit
that referenced
this pull request
Nov 9, 2021
These got added along with code in initial drafts of #11058 but then the code was removed without removing the now-unnecessary headers.
PSONALl
pushed a commit
to PSONALl/connectedhomeip
that referenced
this pull request
Dec 3, 2021
* add support for boundry checks for thermostat-user-interface-configuration * add impl for boundry checks for thermostat-user-interface-configuration in all-clsuter-app * add check for size and remove static cast to unit8 * add support for ProAttribute hook for thermostat user interface configuration * remove preattribute hooks in all-cluster-app/linux * regen all * fix esp32 and mbed builds
PSONALl
pushed a commit
to PSONALl/connectedhomeip
that referenced
this pull request
Dec 3, 2021
…11587) These got added along with code in initial drafts of project-chip#11058 but then the code was removed without removing the now-unnecessary headers.
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.
Problem
Change overview
Testing
all-cluster-app and chip-device-ctrl.