Add XML and generated files of the Energy Calendar cluster #33945
+12,018
−2
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
|
|
github-actions
bot
added
scripts
controller
app
github
workflows
documentation
|
PR #33945: Size comparison from 4d5e2ee to ce3dac7 Increases above 0.2%:
Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Jun 21, 2024
| @@ -36,6 +36,7 @@ CommandHandlerInterfaceOnlyClusters: | |||
| - Microwave Oven Control | |||
| - Energy EVSE | |||
| - Energy EVSE Mode | |||
| - Energy Calendar | |||
There was a problem hiding this comment.
This is a claim about implementation. It does not belong in this PR.
| <configurator> | ||
| <domain name="Energy Management"/> | ||
|
|
||
| <enum name="PeakPeriodSeverityEnum" type="enum8"> |
There was a problem hiding this comment.
For future reference: putting the XML in the same order as the spec helps review a lot.
| <field name="Saturday" mask="0x40"/> | ||
| </bitmap> | ||
|
|
||
| <struct name="Date"> |
There was a problem hiding this comment.
This struct definition isn't in the spec for this cluster.
The spec seems to be using the "date" Data Model type, which is currently not really supported in the ZAP XML, I suspect. chip-types.xml has it, but whether that actually leads to any sane behavior, who knows.
What needs to happen here is figuring out how "date" should actually be implemented. That can happen as a followup if we just need to unblock some work here, but it's a must-fix before this can ship.
There was a problem hiding this comment.
Fixed by using alchemy tool to generate XML.
| <item fieldId="3" name="DayOfWeek" type="int8u" min="1" max="7" isNullable="true" optional="true"/> | ||
| </struct> | ||
|
|
||
| <struct name="CalendarPeriod"> |
There was a problem hiding this comment.
This name does not match the spec.
There was a problem hiding this comment.
Fixed by using alchemy tool to generate XML.
Comment on lines
74
to
75
| <item fieldId="2" name="StartTime" type="epoch_s"/> | ||
| <item fieldId="3" name="EndTime" type="epoch_s"/> |
There was a problem hiding this comment.
These are both nullable in the spec.
I am stopping here; this XML clearly does not match the spec, and the different ordering makes it very hard to review. Please fix the ordering and make the XML actually match the spec.
There was a problem hiding this comment.
Fixed by using alchemy tool to generate XML.
There was a problem hiding this comment.
The ordering still does not match the spec.... any idea why not? I'll try to find time to review this again, but with the mis-ordering it's an O(N^2) process as I try to compare two differently-ordered lists, not an O(N) one, so much more time-consuming.....
There was a problem hiding this comment.
Perhaps the specification (*.adoc) is changing faster than I can recreate the source and its derivative files.
Can you give me access to the connectedhomeip-spec repository?
In addition, verification tasks are actively changing on github itself.
For example, from the last one: now in *.kt files in the description of functions there is a comma after the last parameter.
Was:
class GeneralDiagnosticsClusterNetworkFaultChangeEvent(
val current: List<UInt>,
val previous: List<UInt>
) {
Became:
class GeneralDiagnosticsClusterNetworkFaultChangeEvent(
val current: List<UInt>,
val previous: List<UInt>,
) {
At the initial stage, this was due to the fact that I created XML manually, not knowing about the alchemy tool, but since a week this is no longer relevant.
There was a problem hiding this comment.
What are you using as a reference if you don't have access to the spec repo?
There was a problem hiding this comment.
I'd like to echo @cecille's question.
I don't have a way to give you access to the spec repo, but if you need that you should talk to Chris LaPre at the CSA.
In addition, verification tasks are actively changing on github itself.
Yes, various other changes to the codegen can be happening... That's normal.
To make this converge faster, please order the XML in the same order as the spec. Alchemy will try to preserve your existing XML as much as it can if you run it with some XML already in place, so it will not fix issues like the ordering one for you unless you run it from a blank slate....
Michail-Antropov
force-pushed
the
energy_calendar-XML
branch
from
9b65af5
to
2043c10
Compare
|
PR #33945: Size comparison from 7c04979 to 2043c10 Increases above 0.2%:
Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #33945: Size comparison from 7c04979 to 6e453cd Increases above 0.2%:
Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Michail-Antropov
force-pushed
the
energy_calendar-XML
branch
from
6e453cd
to
b526b46
Compare
|
PR #33945: Size comparison from 4345e2d to 16d6046 Increases above 0.2%:
Full report (49 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, tizen)
|
|
PR #33945: Size comparison from 4345e2d to a2b2aec Increases above 0.2%:
Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Michail-Antropov
force-pushed
the
energy_calendar-XML
branch
from
a2b2aec
to
1e7629b
Compare
|
PR #33945: Size comparison from f62d3bf to 1e7629b Increases above 0.2%:
Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, 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.
Please check
data_model/clusters/EnergyCalendar.xmlandsrc/app/zap-templates/zcl/data-model/chip/energy-calendar-cluster.xmlonly.Other - generated files