chef: Fix FanControl mode/speed interdependence behavior #36256
+241
−96
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
This PR implements the FanControl logic to ensure the interdependent behavior between Mode/Speed/Percent is coherent. This is based on what was done in the AirPurifier examples. Changes: * Removed Custom Write/Read * Updated Zap file `SpeedMax` default to 10 instead of 100 * Forward `MatterPostAttributeChangeCallback` FanControl-related changes to `ChefFanControlManager` * Implements Mode/Speed/Percent interdependency logic based on the AirPurifier example Test: * Verified that Mode, Speed and SpeedPercent change accordingly when any of those attributes change and cross a boundary.
|
Review changes with |
|
PR #36256: Size comparison from e2ffa2d to 51788c2 Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Oct 26, 2024
| // Ensure new speed is in bounds | ||
| case FanControl::Attributes::PercentSetting::Id: { | ||
| ChipLogProgress(NotSpecified, "ChefFanControlManager::HandleFanControlAttributeChange PercentSetting"); | ||
| DataModel::Nullable<Percent> percentSetting = static_cast<DataModel::Nullable<uint8_t>>(*value); |
There was a problem hiding this comment.
This cast is wrong. That is not the binary layout of something that would get passed to MatterPostAttributeChangeCallback. Was this tested at all?
This sort of type-confusion thing is how one gets exploitable security bugs.
There was a problem hiding this comment.
I'm sorry, I'm new to the codebase and not familiar with all of the types yet. I followed what's currently done in the air-purifier-app, which is likely wrong as well: https://github.com/project-chip/connectedhomeip/blob/master/examples/air-purifier-app/air-purifier-common/src/air-purifier-manager.cpp#L157
Was this tested at all?
Yes, it was tested and it seems to have coherent values.
I would appreciate if you could point me to a good example and I can update both of the incorrect apps. Thanks!
There was a problem hiding this comment.
Yeah, that air purifier code is definitely wrong too....
When this was tested, was it tested with an actual null value?
What I would is that the actual type at the callsite of value here is uint8_t, with null represented as 0xFF.
There was a problem hiding this comment.
I updated the PR, let me know if it looks better now. I needed a little lesson from Andrei on how ember encodes null. Thanks for catching this, I'll fix the other app in a separate PR.
| case FanControl::Attributes::SpeedSetting::Id: { | ||
| ChipLogProgress(NotSpecified, "ChefFanControlManager::HandleFanControlAttributeChange SpeedSetting"); | ||
|
|
||
| DataModel::Nullable<uint8_t> speedSetting = static_cast<DataModel::Nullable<uint8_t>>(*value); |
There was a problem hiding this comment.
Again, this is wrong.
There was a problem hiding this comment.
Should be fixed now.
| } | ||
| } | ||
|
|
||
| void ChefFanControlManager::PercentSettingWriteCallback(uint8_t aNewPercentSetting) |
There was a problem hiding this comment.
So why was null turned into 0, exactly?
Looking at the spec, none of this code should be reached if null is written (cluster should ignore the write and return with an error per spec), no?
There was a problem hiding this comment.
Looking at the spec, none of this code should be reached if null is written (cluster should ignore the write and return with an error per spec), no?
It seems this is only true when the client is trying to write to null.
The cluster code can still set this to null and we use the post attribute change callback to detect the change in PercentSetting so we can update PercentCurrent.
There was a problem hiding this comment.
Ah, true. But then why are we turning null into 0? And why are we touching any other state if the application itself is writing null here? We should just start returning null on reads, I would think, and not change anything else.
It does not help that the spec does not define what null means for this field, of course.
There was a problem hiding this comment.
Yeah, the spec doesn't specify what to do with PercentCurrent when PercentSetting goes to null (a.k.a when the FanMode goes to kAuto).
We're deciding to set this to 0 in the app (as PercentCurrent can't be null as well).
Comment on lines
+257
to
+258
| case FanControl::FanModeEnum::kUnknownEnumValue: { | ||
| ChipLogProgress(NotSpecified, "ChefFanControlManager::FanModeWriteCallback: Unknown"); |
There was a problem hiding this comment.
Shouldn't this be handled by the cluster in a pre-write callback?
There was a problem hiding this comment.
I'm new to this code but it looks like the cluster code only handles kSmart and kOn on pre-callback. Everything else it just passes down: https://github.com/project-chip/connectedhomeip/blob/master/src/app/clusters/fan-control-server/fan-control-server.cpp#L183
Are you suggesting I go ahead and update the cluster code to check if it's kUnknownValue and return Status::ConstraintError instead of passing down?
I could do that. But searching the codebase, I don't see other clusters handling this on pre-callback, so I'm not sure what's the rationale here. Was the intention to just let the app/products handle this, maybe?
There was a problem hiding this comment.
I'm new to this code but it looks like the cluster code only handles kSmart and kOn on pre-callback.
That sounds like a bug in the cluster code; it should deal with invalid input in the way the spec specifies, so that applications do not have to.
I don't see other clusters handling this on pre-callback
Some clusters handle this in other ways. Others are being bad. ;)
|
PR #36256: Size comparison from e2ffa2d to f119c9a Increases above 0.2%:
Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36256: Size comparison from 928efd7 to 5ac4256 Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36256: Size comparison from 928efd7 to e15ae5a Full report (68 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.
This PR implements the FanControl logic to ensure the interdependent behavior between Mode/Speed/Percent is coherent.
This is based on what was done in the AirPurifier examples.
Changes:
SpeedMaxdefault to 10 instead of 100MatterPostAttributeChangeCallbackFanControl-related changes toChefFanControlManagerTest: