add the DeviceLocation attribute to the Basic Information cluster XMLs#43342
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds the provisional DeviceLocation attribute to the Basic Information cluster. The changes correctly update the data models in XML and .matter files, as well as the corresponding generated Java and Kotlin code.
My review is focused on the generated code, as per the repository's style guide. I've found a potential null pointer issue in the newly generated Java struct for LocationDescriptorStruct where a mandatory field is not being validated before use. I've left a specific comment with a suggested fix.
src/controller/java/generated/java/chip/devicecontroller/ChipStructs.java
Show resolved
Hide resolved
|
PR #43342: Size comparison from 8376e55 to ba62cb6 Full report (6 builds for cc32xx, nrfconnect, realtek, stm32)
|
|
PR #43342: Size comparison from 8376e55 to c8cb79c Full report (4 builds for nrfconnect, realtek, stm32)
|
src/controller/java/generated/java/chip/devicecontroller/ChipStructs.java
Show resolved
Hide resolved
|
PR #43342: Size comparison from 8376e55 to 9aeab5c Full report (6 builds for cc32xx, nrfconnect, realtek, stm32)
|
src/app/zap-templates/zcl/data-model/chip/basic-information-cluster.xml
Outdated
Show resolved
Hide resolved
andy31415
left a comment
There was a problem hiding this comment.
Request changes: Updates to XMLs that use alchemy should keep using alchemy. Given that the git text did not change, I think maybe alchemy was not used?
|
@plauric should we also add this for bridged device basic info? Or would that be a separate PR? Asking because I am working on a code driven conversion and we did not implement the location yet because it was not defined in the XML. Yet https://github.com/project-chip/connectedhomeip/blob/master/examples/bridge-app/linux/Device.cpp#L88 somewhat has it (except it is not exposed in https://github.com/project-chip/connectedhomeip/blob/master/examples/bridge-app/linux/main.cpp#L142 because we have no constants for it ... |
Yes, that's the plan. I'll make the change in the next commit. |
@andy31415 I think that's a different location :) Specifically, that's an attribute named Location, which has a string type value, and which indicates the country code where the device is located. That's meant to help with things like helping the device set the wifi regulatory domain, for example. The attribute I'm trying to add is called DeviceLocation, and it has a complex type (a struct). |
Ah, that explains things better. Thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43342 +/- ##
==========================================
+ Coverage 54.05% 54.07% +0.02%
==========================================
Files 1548 1548
Lines 106692 106701 +9
Branches 13314 13309 -5
==========================================
+ Hits 57670 57699 +29
+ Misses 49022 49002 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #43342: Size comparison from 7a5d279 to dd543a1 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
src/app/zap-templates/zcl/data-model/chip/bridged-device-basic-information-cluster.xml
Show resolved
Hide resolved
…c_info_device_location_xml
…nts from the zap-template XMLs updated to include DeviceLocation
|
PR #43342: Size comparison from 719a930 to 4b06018 Full report (29 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #43342: Size comparison from 719a930 to e9e7004 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
src/app/zap-templates/zcl/data-model/chip/basic-information-cluster.xml
Outdated
Show resolved
Hide resolved
|
PR #43342: Size comparison from 719a930 to 4fc826f Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Add the DeviceLocation attribute to the Basic Information cluster XMLs. This is a provisional attribute in the current spec text.
Testing
I successfully ran the zap regen script and built chip-tool using these changes.