Proximity Ranging server and example app implementation#43768
Proximity Ranging server and example app implementation#43768s-mcclain wants to merge 9 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}
There was a problem hiding this comment.
Code Review
This pull request implements the Proximity Ranging cluster server, including core logic, a driver interface, and a Linux example application. The review identified several critical issues in session management where synchronous callbacks could lead to leaked or "zombie" sessions. Key recommendations include registering sessions before invoking external drivers, implementing collision detection for session ID generation, and ensuring event emission is guarded by successful session removal to prevent re-entrancy issues.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Proximity Ranging cluster (0x0433), including the server implementation, driver interfaces, and integration into the Linux example application. The implementation follows the code-driven cluster pattern and supports features like Wi-Fi USD, Bluetooth Channel Sounding, and BLE Beacon RSSI. Feedback is provided regarding the use of a more appropriate error code when registering duplicate technology adapters.
|
PR #43768: Size comparison from a27c039 to 27df0d4 Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
s-mcclain
force-pushed
the
proximity-ranging-server-impl
branch
from
27df0d4 to
18f3bd1
Compare
|
PR #43768: Size comparison from 5d5eb0c to 18f3bd1 Full report (6 builds for cc32xx, nrfconnect, realtek, stm32)
|
|
PR #43768: Size comparison from e09871c to 5fcfde1 Full report (9 builds for cc13x4_26x4, cc32xx, realtek, stm32)
|
s-mcclain
force-pushed
the
proximity-ranging-server-impl
branch
from
5fcfde1 to
cf073ba
Compare
s-mcclain
force-pushed
the
proximity-ranging-server-impl
branch
from
f79017a to
4045bf0
Compare
|
PR #43768: Size comparison from 61afff7 to 4045bf0 Full report (16 builds for cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
|
Successful in running tests with linux and ESP32 sample application using chip-tool and running the commands: |
|
PR #43768: Size comparison from 462fcc2 to c14c126 Full report (16 builds for cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
s-mcclain
force-pushed
the
proximity-ranging-server-impl
branch
from
c14c126 to
b552616
Compare
Additionally fixed pyspelling in README.md
…mity-ranger to txt validation file
s-mcclain
force-pushed
the
proximity-ranging-server-impl
branch
from
b552616 to
cced2de
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43768 +/- ##
==========================================
- Coverage 54.52% 54.52% -0.01%
==========================================
Files 1592 1592
Lines 108733 108732 -1
Branches 13388 13388
==========================================
- Hits 59291 59290 -1
Misses 49442 49442 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #43768: Size comparison from fc27df9 to cced2de Full report (33 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #43768: Size comparison from 7e589aa to 899feef Full report (30 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
s-mcclain
force-pushed
the
proximity-ranging-server-impl
branch
from
899feef to
ddc6ae1
Compare
|
PR #43768: Size comparison from dcfc4ce to ddc6ae1 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
| - Operational State | ||
| - Oven Cavity Operational State | ||
| - Oven Mode | ||
| - Proximity Ranging |
There was a problem hiding this comment.
If I recall correctly, it is actually not needed to add the cluster in this list if it was already added in the CodeDrivenClusters one since CHI and SCI are mutually exclusive.
|
|
||
| #define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DEVICE_TYPE 1 | ||
|
|
||
| #define CHIP_DEVICE_CONFIG_DEVICE_TYPE 0x0152 // Proximity Ranger |
There was a problem hiding this comment.
We used to hardcode these this way, but generally wondering if we should use generated constants instead of manual ones. This ID shows up as chip::app::Device::kProximityRangerDeviceTypeId in
| class AppDeviceCallbacks : public CommonDeviceCallbacks | ||
| { | ||
| public: | ||
| virtual void PostAttributeChangeCallback(chip::EndpointId endpointId, chip::ClusterId clusterId, chip::AttributeId attributeId, |
There was a problem hiding this comment.
Is this callbacks class required for this example? All we seem to do is logging...
There was a problem hiding this comment.
Is there a readme on why we have this example app?
Looking at it I wonder:
- why did we create separate apps (this adds extra CI or we miss compilation checks) instead of adding it to https://github.com/project-chip/connectedhomeip/tree/master/examples/all-devices-app
- why is the enabling RSSI BLE ranging an option .... that seems to be the only possible ranging right now and not implemented at all. What is the value of not selecting it?
- how is this app to be built/tested?
There was a problem hiding this comment.
I'll go ahead and update this to be included within the all-devices-app instead.
One question is: If we do not have an example directory specific for the proximity ranger (e.g. proximity-ranger-common), what would be the recommendation for where to keep the "common" application files for this cluster / device type?
| * | ||
| * @code | ||
| * static MyProximityRangingDriver sDriver; | ||
| * static ProximityRanging::Instance sInstance(endpointId, sDriver, features); |
There was a problem hiding this comment.
If we do this, this means the cluster is fully code driven and zap configuration is not taken into account at all.
Does this mean that we do not have much codegen integration at all (i.e. I cannot select something from zap and have it work)?
Other choices that other instances have:
- they use zap to initialize/shutdown
- they provide a
FindClusterByEndpointIdand then main would do something like either:SetDriver(endpointid, driver)before app init (so we can save a pointer and use it in a constructorFindClusterByEndpointId(id)->SetDriver()after app init (so that cluster is created)
Neither are amazing because SetDriver requires separate pointer and FindClusterByEndpoint makes the driver mutable ... however at least it makes a cluster appear as soon as the zap UI selects something in the ZAP UI.
If we explicitly chose not to support zap UI (essentially, no features are forwarded, no other options) we should probably state in the docs here.
| switch (request.path.mAttributeId) | ||
| { | ||
| case Attributes::RangingCapabilities::Id: | ||
| return mDriver.GetRangingCapabilities(encoder); |
There was a problem hiding this comment.
Ranging capabilities (and it seems like ALL attributes) are non-fixed. This means whenever they change the driver has to "NotifyAttributeChanged". Was this considered? I see we did pass "self" to the init of the driver, however we do not expose a notification mechanism.
There was a problem hiding this comment.
Fair point, currently only the SessionIdList change notifications get published. We'll add notification mechanisms for all the attributes.
| if (config.featureFlags.Has(Feature::kBluetoothChannelSounding)) | ||
| { | ||
| attrs.ForceSet<Attributes::BLTDevIK::Id>(); | ||
| attrs.ForceSet<Attributes::BLTCSSecurityLevel::Id>(); |
There was a problem hiding this comment.
This one is marked O in the spec, so seems like fully optional. Maybe a spec issue? It should be [BLTCS] maybe? but even then it would be "optional for this feature" and the config here enforces it for the feature. So we cannot fully implement all spec variations.
This can be fine (if we chose our impl to support a specific subset) but should be called out very explicitly.
| DefaultServerCluster({ config.endpointId, ProximityRanging::Id }), mDriver(config.driver), | ||
| mFeatureFlags(config.featureFlags), mOptionalAttributes([&config]() -> OptionalAttributes { | ||
| AttributeSet attrs; | ||
| if (config.featureFlags.Has(Feature::kWiFiUsdProximityDetection)) |
There was a problem hiding this comment.
nit: please add {} everywhere for ifs.
| return mDriver.GetRangingCapabilities(encoder); | ||
|
|
||
| case Attributes::WiFiDevIK::Id: { | ||
| VerifyOrReturnError(mFeatureFlags.Has(Feature::kWiFiUsdProximityDetection), CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute)); |
There was a problem hiding this comment.
This is not needed - please remove from all instances:
- API requirement is to guarantee that Read/Write/Invoke is done for existent paths only. This means that if you implement
Attributes()correctly you do not need to verify (clutters the code less, uses less flash)
| MutableByteSpan span(buf); | ||
| CHIP_ERROR err = mDriver.GetWiFiDevIK(span); | ||
| VerifyOrReturnError(err != CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE, CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute)); | ||
| VerifyOrReturnError(err == CHIP_NO_ERROR, Protocols::InteractionModel::Status::Failure); |
There was a problem hiding this comment.
return the error directly: there are slightly better conversions (it allows drivers to return things like global IM errors that get converted) and I am unsure if we should allow Attributes() to say attribute exist and then return unsupported attribute.
I would ReturnErrorOnFailure(mDriver.GetWiFiDevIK(span)). Allowing UNSUPPORTED_CHIP_FEATURE seems like an antipattern. Driver should tell you what it supports (feature map) and if it supports something, it should be implemented.
Thinking on this, we should not pass in the feature map to the cluster then: the cluster should get the feature map from the driver then.
| } | ||
|
|
||
| case Attributes::FeatureMap::Id: | ||
| return encoder.Encode(mFeatureFlags.Raw()); |
There was a problem hiding this comment.
| return encoder.Encode(mFeatureFlags.Raw()); | |
| return encoder.Encode(mFeatureFlags); |
I think raw is not needed
| return encoder.Encode(ProximityRanging::kRevision); | ||
|
|
||
| default: | ||
| return CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute); |
There was a problem hiding this comment.
maybe Status::UnsupportedAttribute is clearer and the same for commands, so we do not wrap in a chip-error.
the ActionRestult has casts from chiperror and statuses.
| CommandHandler * handler) | ||
| { | ||
| Commands::StartRangingRequest::DecodableType commandData; | ||
| VerifyOrReturnValue(commandData.Decode(reader) == CHIP_NO_ERROR, CHIP_IM_GLOBAL_STATUS(InvalidCommand)); |
There was a problem hiding this comment.
return the error directly
| VerifyOrReturnValue(commandData.Decode(reader) == CHIP_NO_ERROR, CHIP_IM_GLOBAL_STATUS(InvalidCommand)); | |
| ReturnErrorOnFailure(commandData.Decode(reader)); |
The idea would be that if you check, you will lose the actual error information when you convert.
| VerifyOrReturnValue(commandData.Decode(reader) == CHIP_NO_ERROR, CHIP_IM_GLOBAL_STATUS(InvalidCommand)); | ||
|
|
||
| Commands::StartRangingResponse::Type response; | ||
| ResultCodeEnum resultCode; |
There was a problem hiding this comment.
why don't you use response.resultCode directly all the time? why have a separate resultCode variable?
| { | ||
| MarkSessionIdListModified(); | ||
|
|
||
| if (mContext != nullptr) |
There was a problem hiding this comment.
to make it consistent, should we VerifyOrReturn(mContext != nullptr); like the above measurement data handler? that would also decrease indent a bit.
| bool collision = false; | ||
| for (size_t i = 0; i < activeSessions.size(); i++) | ||
| { | ||
| if (activeSessions.data()[i] == candidate) | ||
| { | ||
| collision = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!collision) | ||
| { | ||
| return candidate; | ||
| } |
There was a problem hiding this comment.
Is this where we could use std::none_of?
I imagine code like:
if (std::none_of(activeSessions.begin(), activeSessions.end(), [candidate](uint8_t value) {
return value == candidate;
}))
{
return candidate;
}
| } | ||
|
|
||
| std::optional<DataModel::ActionReturnStatus> | ||
| ProximityRangingCluster::HandleStopRangingRequest(const DataModel::InvokeRequest & request, TLV::TLVReader & reader, |
There was a problem hiding this comment.
you do not seem to need the handler as an argument (no response) and then you do not need the std::optional as a response either.
andy31415
left a comment
andy31415
left a comment
There was a problem hiding this comment.
Request changes:
-
please split cluster implementation from "add example app". I would very much prefer to not create yet another example app as they are harder to maintain (either we overload CI or never test it, both being bad) and we already have a lot of unmaintained apps in https://project-chip.github.io/connectedhomeip-doc/issue_triage.html#example-maintenance . If we do add another app, we need maintainers.
-
we need unit tests for any code driven cluster. This PR does not add any unit test, not even a skeleton. I think the cluster is testable today because the driver can be mocked, so we should have tests.
4 participants
Summary
Adds the code-driven Proximity Ranging cluster server implementation and a Linux example application (proximity-ranger-app).
The cluster follows the combined DefaultServerCluster pattern with an application-owned Instance for registration.
Commands (StartRangingRequest, StopRangingRequest) are handled synchronously by the driver. Async events (measurement data, unsolicited session stops) flow back through a narrow ProximityRangingDriver::Callback interface,
keeping the driver decoupled from the cluster class.
The example app demonstrates the controller/adapter architecture: DefaultProximityRangingDriver ->
RangingTechnologyController -> per-technology RangingTechnologyAdapter implementations (platform adapters not yet
included).
This PR
is stacked on #43525 (cluster XML definition and ZAP generation).
Related issues
N/A
Testing
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
[descriptive](https://project-chip.github.io/connectedhomeip-doc/contributing/pull_request_guidelines.html#
title-formatting)
"When in Rome…"
rule (coding style)
See: [Pull Request Guidelines](https://project-chip.github.io/connectedhomeip-doc/contributing/pull_request_
guidelines.html)