Add Doorbell device type to all-devices app. - #74027
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. (8 skipped: 8 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tersal
changed the title
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
🟡 Changes recommended
Doorbell endpoint registration currently lacks the standard rollback/transaction pattern used by other device types, which can leave partially-registered clusters/endpoints on failure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new Doorbell device type to the all-devices-app and wires it into the device-factory/build target lists so it can be enabled and instantiated like other device types.
Changes:
- Add
doorbellto the build target/device enablement registries (GN + CMake + build scripts). - Introduce a new
DoorbellSingleEndpoint device implementation (Identify, Switch, Binding; Chime as a client cluster). - Register the new device type in
DeviceFactoryso it can be created by name.
File summaries
| File | Description |
|---|---|
| scripts/build/build/targets.py | Adds doorbell to the selectable build targets list. |
| examples/all-devices-app/all-devices-common/device/types/doorbell/Doorbell.h | Declares the new Doorbell device type (SingleEndpoint) and its clusters. |
| examples/all-devices-app/all-devices-common/device/types/doorbell/Doorbell.cpp | Implements endpoint/cluster registration and client cluster list for Doorbell. |
| examples/all-devices-app/all-devices-common/device/types/doorbell/BUILD.gn | Introduces GN target for building the new Doorbell device type sources. |
| examples/all-devices-app/all-devices-common/device-factory/enabled_devices.gni | Adds doorbell to the GN-enabled device registry. |
| examples/all-devices-app/all-devices-common/device-factory/enabled_devices.cmake | Adds doorbell to the CMake-enabled device registry. |
| examples/all-devices-app/all-devices-common/device-factory/enabled_devices_config.h.in | Adds ALL_DEVICES_ENABLE_DOORBELL config define. |
| examples/all-devices-app/all-devices-common/device-factory/DeviceFactory.h | Registers a creator for the doorbell device string key. |
| examples/all-devices-app/all-devices-common/device-factory/BUILD.gn | Ensures device-factory depends on the new Doorbell GN target. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Doorbell registration currently lacks the standard rollback guard (and has an ambiguous Switch startup config), which can leave partially-registered state on early errors.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
examples/all-devices-app/all-devices-common/device/types/doorbell/Doorbell.cpp:58
- Doorbell::Register() does not use DeviceRegistrationTransaction, so any early-return error (e.g. AddCluster/AddEndpoint failure) will leave partially-registered clusters/descriptor state in the provider instead of rolling back via Unregister(). Also, SwitchCluster::StartupConfiguration is initialized positionally as
{ 2, 2 }, which implicitly setsmultiPressMaxeven though the feature map does not enable multi-press; use named initialization to avoid accidental field-order/feature mismatches.
examples/all-devices-app/all-devices-common/device/types/doorbell/Doorbell.cpp:4
- The copyright header year in this new source file is 2025, but the matching header (Doorbell.h) and the rest of the new Doorbell device type changes are 2026. Please make the year consistent.
/*
*
* Copyright (c) 2025 Project CHIP Authors
*
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new Doorbell device type unnecessarily depends on the Chime server cluster via includes/deps (rather than including the generated Chime IDs directly), which increases build/link surface and is avoidable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
examples/all-devices-app/all-devices-common/device/types/doorbell/Doorbell.h:23
Doorbell.hincludesapp/clusters/chime-server/ChimeCluster.h, but this header does not reference any Chime server symbols (it only needsChime::Idin the .cpp). Keeping this include pulls in unnecessary server-cluster dependencies and makesDoorbell.cpprely on a transitive include for the cluster ID.
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
🟡 Changes recommended
The new Doorbell header introduces an unnecessary chime-server include that can break strict include/dependency checking and should be removed or properly depended on.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The CMake-enabled device list adds doorbell but does not add Doorbell.cpp to the compiled device sources list, which can break ESP32/Telink all-devices builds when all devices are enabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
examples/all-devices-app/all-devices-common/device/types/doorbell/Doorbell.h:23
Doorbell.hincludes the Chime server cluster header, but the doorbell device only uses the Chime cluster ID as a client (and does not referenceChimeClustertypes). Keeping this include adds an unnecessary dependency surface and can break strict include/deps checking.
#include <app/clusters/bindings/BindingCluster.h>
#include <app/clusters/bindings/BindingManager.h>
#include <app/clusters/bindings/binding-table.h>
#include <app/clusters/chime-server/ChimeCluster.h>
#include <app/clusters/identify-server/IdentifyCluster.h>
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The new Doorbell device type is consistently integrated across the all-devices build/enablement pipeline and follows existing SingleEndpoint registration/unregistration patterns.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #74027 +/- ##
==========================================
- Coverage 57.42% 57.40% -0.03%
==========================================
Files 1940 1942 +2
Lines 127709 127758 +49
Branches 14502 14518 +16
==========================================
- Hits 73336 73335 -1
- Misses 54373 54423 +50 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
Adding the Doorbell device type to all-devices app, this cluster includes the
Identify,SwitchandBindingscluster, this last one because this device has aChimecluster as a client.Related issues
NA
Testing
Manually tested to confirm that the clusters are added and the client list contains Chime, actual communication between clusters is pending.