Thermostat Cluster Init Function Enabled #12863
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
pullapprove
bot
requested review from
anush-apple,
balducci-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost,
LuDuda,
mlepage-google,
msandstedt,
pan-apple,
robszewczyk,
sagar-apple and
saurabhst
pullapprove
bot
requested review from
tcarmelveilleux,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yunhanw-google
|
PR #12863: Size comparison from 55b7aba to 38d3567 Increases above 0.2%:
Increases (5 builds for esp32, linux, mbed, p6)
Decreases (2 builds for mbed, p6)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
chrisbeach59
force-pushed
the
Thermostat-FeatureMapToInit
branch
2 times, most recently
from
5a5aa51 to
d1f98d3
Compare
|
PR #12863: Size comparison from de0af9a to d1f98d3 Increases (4 builds for esp32, linux, p6)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
bzbarsky-apple
approved these changes
Dec 11, 2021
emberAfThermostatClusterServerInitCallback via helper.js Eliminated #ifdefs FeatureMap new accessors Feature map in functions to allow for more than one thermostat endpoint within a device.
chrisbeach59
force-pushed
the
Thermostat-FeatureMapToInit
branch
from
d1f98d3 to
3188509
Compare
|
PR #12863: Size comparison from d3322e3 to 3188509 Increases (5 builds for esp32, linux, mbed, p6)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
Fast tracking given this has been in review for > 3 days. |
andy31415
reviewed
Dec 13, 2021
|
|
||
| if (FeatureMap & 1 << 5) // Bit 5 is Auto Mode supported | ||
| { | ||
| if (OurFeatureMap & 1 << 5) // Bit 5 is Auto Mode supported |
There was a problem hiding this comment.
we should have constants for these. Would be much easier to read:
constexpr uint16_t kFeatureMap_AutoModeBit = 1 << 5;
// ....
const bool AutoSupported = (OurFeatureMap & kFeatureMap_AutoModeBit) != 0;
andy31415
reviewed
Dec 13, 2021
| CoolSupported = true; | ||
|
|
||
| if (FeatureMap & 1 << 2) | ||
| OccupancySupported = true; | ||
| if (OurFeatureMap & 1 << 2) |
There was a problem hiding this comment.
Previous code I could read "OccupancySupported = true;" to figure out what this bit 2 is.
new code is totally opaque. At least a comment should be here or ideally constants that are readable.
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:
emberAfThermostatClusterServerInitCallback was not enabled in the helper.js file for code gen.
FeatureMap usage involves Conditional compiles and repeated code
Changes:
Enabled Init for thermostat
Eliminated #ifndefs
Used Feature Map accessor function now available in SDK, in place of deprecated read.
Tested:
Manual testing with thermostat-app (Linux) and chip-tool