[DataModel] Initial proximity ranging cluster definition#43525
[DataModel] Initial proximity ranging cluster definition#43525s-mcclain wants to merge 2 commits intoproject-chip:masterfrom
Conversation
github-actions
bot
added
documentation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
s-mcclain
force-pushed
the
initial-proximity-ranging-cluster-definition
branch
from
d6a666d to
68e4369
Compare
|
PR #43525: Size comparison from a94849f to 68e4369 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43525 +/- ##
=======================================
Coverage 54.05% 54.05%
=======================================
Files 1549 1549
Lines 106697 106697
Branches 13318 13318
=======================================
Hits 57675 57675
Misses 49022 49022 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| kWiFiUSDProximityDetection = 0x1 [spec_name = "Wi-Fi USD Proximity Detection"]; | ||
| kBluetoothChannelSounding = 0x2; | ||
| kBLEBeaconRSSI = 0x4 [spec_name = "BLE Beacon RSSI"]; | ||
| kUWBRanging = 0x8 [spec_name = "UWB Ranging"]; |
There was a problem hiding this comment.
This does not seem to be part of https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/12685
| int8u sessionID = 0; | ||
| } | ||
|
|
||
| response struct RangingResult = 3 { |
There was a problem hiding this comment.
Was this vetted that this is implementable/works in the SDK? I wonder if there are any precedents of responses that are not sent as part of a request.
Otherwise was it considered to use something else like events for this kind of processing?
There was a problem hiding this comment.
I did a grep in the spec for all server to client responses and all seem to be a response to some form of request. So you may have extra hard time implementing this (it is not prevented by spec, however listening for unprompted command packets may not be exercised/tested currently).
There was a problem hiding this comment.
You're right, I do remember at some point looking into that and finding out that it is much harder to support, although I can't remember exactly what would need to be done to support this. The team had indicated that they do not want to utilize Attributes/Events for triggering this reporting to ensure that the report only is sent to the Configurator.. Wonder if the effort for supporting this is worth it or not
There was a problem hiding this comment.
I would suggest to try to look for alternatives if possible. I believe this will both be hard to implement in the SDK and will add extra work to all client APIs needing to support something new. Not impossible, but a solid slowdown.
There was a problem hiding this comment.
As specific feedback, I also asked around on our integration side and was told that this will at worst not be supported and at best it would take a lot of effort (i.e. support of this design in controllers would be delayed). What is happening is that this would imply some sort of a "session" to know who should receive the ranging data, however from our implementation the controllers keep active sessions on behalf of clients/applications and adding extra session-like information that is cluster specific would be rough.
So: hard to make work in the SDK, hard to get support in ecosystem integrations (at least in one example, would not be surprised if this is the case in others as well)
There was a problem hiding this comment.
Thanks really appreciate the feedback.
We are discussing this within the proximity ranging team and think we've reached decision to move away from using this async command from server=>client into using an Event instead.
Wonder if this should have some kind of statement in the cluster writing documentation for future groups.
There was a problem hiding this comment.
+1 to figuring out a way to document that going this path is likely not recommended. That may need a broader spec discussion as well (maybe this should be in spec text too somehow).
3 participants
Summary
Adds the Proximity Ranging cluster XML with Alchemy and regen ZAP files to include Proximity Ranging cluster.
Related issues
Relates to specification definition PR: https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/12685
Testing
Manually tested building chip-tool on Linux and Darwin platforms. Verified 'proximityranging' is available and
includes all fields for the commands.
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines