HomeKit plugin extension with a choice between two integrations#527
HomeKit plugin extension with a choice between two integrations#527modsmthng wants to merge 23 commits intojniebuhr:masterfrom
Conversation
Thermostat Mode: Emulates a thermostat for direct temperature control (as before). Bridge Mode: Creates multiple devices: Sensor that reflects the heating status (red / heating = closed, green / ready = open) Switch for Standby - Brew Switch for Brew - Steam An automatic update logic to transfer existing Homekit plugin settings did not work (see commented-out code in Settings.cpp), but the plugin can simply be reactivated and by switching the integration worked without resetting the display. A reset function can be worked on next, but is not really necessary.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
📝 WalkthroughWalkthroughSwitch HomeKit from a boolean to a mode enum (DISABLED, THERMOSTAT, BRIDGE); migrate legacy "hk" → "hkm"; always register mDNS and register the appropriate HomeKit plugin by mode; add HomeKit Bridge plugin, rename thermostat plugin, update web settings UI, and emit heating‑stability events only on change. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant PluginManager
participant HomeSpan as HomeSpan/HomeKit
participant HomekitBridge as HomekitBridgePlugin
participant HomekitTherm as HomekitThermostatPlugin
Controller->>PluginManager: register plugins (always mDNS + mode-based)
alt HOMEKIT_MODE_BRIDGE
PluginManager->>HomekitBridge: setup(controller, pluginManager)
HomekitBridge->>HomeSpan: init bridge & accessories
HomeSpan-->>HomekitBridge: accessory callbacks (switch toggles)
HomekitBridge->>Controller: schedule/apply actions via atomic flags in loop
Controller-->>PluginManager: emit events/state updates
PluginManager-->>HomekitBridge: handlers update accessory states
else HOMEKIT_MODE_THERMOSTAT
PluginManager->>HomekitTherm: setup(controller, pluginManager)
HomekitTherm->>HomeSpan: init thermostat accessory & callbacks
HomekitTherm->>Controller: apply thermostat actions in loop
Controller-->>PluginManager: emit events/state updates
PluginManager-->>HomekitTherm: handlers update accessory
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
src/display/core/Controller.cpp (1)
55-65: Consider extracting common mDNS registration to reduce duplication.The HomeKit mode selection logic is correct. However,
mDNSPluginis registered identically in all three branches. You could simplify by registering it unconditionally after the HomeKit block.// HomeKit Mode Selection if (settings.getHomekitMode() == HOMEKIT_MODE_THERMOSTAT) { pluginManager->registerPlugin(new HomekitThermostatPlugin(settings.getWifiSsid(), settings.getWifiPassword())); - pluginManager->registerPlugin(new mDNSPlugin()); } else if (settings.getHomekitMode() == HOMEKIT_MODE_BRIDGE) { - pluginManager->registerPlugin(new HomekitBridgePlugin(settings.getWifiSsid(), settings.getWifiPassword())); - pluginManager->registerPlugin(new mDNSPlugin()); - } else { - pluginManager->registerPlugin(new mDNSPlugin()); + pluginManager->registerPlugin(new HomekitBridgePlugin(settings.getWifiSsid(), settings.getWifiPassword())); } + pluginManager->registerPlugin(new mDNSPlugin());web/src/pages/Settings/index.jsx (1)
165-170: Fix inconsistent indentation inupdateHomekitModefunction.The function body is not properly indented relative to the function declaration.
- const updateHomekitMode = (mode) => { - setFormData({ - ...formData, - homekitMode: parseInt(mode, 10), - }); -}; + const updateHomekitMode = (mode) => { + setFormData({ + ...formData, + homekitMode: parseInt(mode, 10), + }); + };src/display/plugins/HomekitThermostatPlugin.h (1)
1-3: Redundant include guard with#pragma once.Using both
#pragma once(Line 1) and traditional include guards (Lines 2-3, 50) is redundant. Most modern compilers support#pragma once, so the traditional guards can be removed for cleaner code. However, keeping both provides compatibility with older compilers.src/display/core/Settings.h (1)
85-85: Consider using enum type for stronger type safety.
getHomekitMode()returnsintinstead ofHomeKitMode. While this works, using the enum type would provide compile-time type checking and make the API more self-documenting.- int getHomekitMode() const { return homekitMode; } + HomeKitMode getHomekitMode() const { return static_cast<HomeKitMode>(homekitMode); }Note: This would require updating call sites to compare against enum values, which they should already be doing.
src/display/core/Settings.cpp (2)
5-5: Unused include when migration code is commented out.The
esp32-hal-log.hinclude is only used by the commented-out migration logic. If this migration code is permanently disabled, consider removing this include to avoid unnecessary dependencies.
525-525: Commented-out save for smartGrindToggle.This appears to be intentional deprecation (replaced by
smartGrindModeon line 526), but consider removing the commented line if the migration is complete.src/display/plugins/HomekitBridgePlugin.h (3)
1-3: Redundant include guards.
#pragma onceis sufficient on its own. The traditional#ifndefguards are redundant and can be removed.#pragma once -#ifndef HOMEKITBRIDGEPLUGIN_H -#define HOMEKITBRIDGEPLUGIN_HAnd remove
#endifat line 97.
27-48: Near-identical switch classes could share a base or template.
GaggiMatePowerSwitchandGaggiMateSteamSwitchhave identical structure - they only differ in theHomekitActionconstant passed to the callback. Consider a single parameterized class:class GaggiMateSwitch : public Service::Switch { public: GaggiMateSwitch(bridge_callback_t callback, HomekitAction actionType); boolean update() override; void setState(bool active); private: bridge_callback_t callback; HomekitAction actionType; SpanCharacteristic *power; };This would reduce code duplication while maintaining the same functionality.
12-13: Extract duplicate Homekit configuration macros to shared header.
HOMESPAN_PORTandDEVICE_NAMEare defined identically in bothHomekitBridgePlugin.handHomekitThermostatPlugin.h. Since both headers are included inController.cpp, these duplicates should be moved to a shared header (e.g.,HomekitCommon.h) to maintain single source of truth and simplify future updates.src/display/plugins/HomekitBridgePlugin.cpp (2)
7-43: Duplicate switch implementations.As noted in the header review,
GaggiMatePowerSwitchandGaggiMateSteamSwitchhave nearly identical implementations. The only difference is theHomekitActionenum value. A parameterized class would reduce this duplication.
200-210: Redundant sync withcontroller:mode:changehandler.Both
handleBoilerStatus(lines 200-210) and thecontroller:mode:changeevent handler (lines 89-103) updatepowerSwitchandsteamSwitchstates. This could result in duplicate state updates. Ifboiler:status:changefires in response to mode changes, consider whether one handler is sufficient.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
src/display/core/Controller.cpp(2 hunks)src/display/core/Settings.cpp(7 hunks)src/display/core/Settings.h(4 hunks)src/display/plugins/HomekitBridgePlugin.cpp(1 hunks)src/display/plugins/HomekitBridgePlugin.h(1 hunks)src/display/plugins/HomekitThermostatPlugin.cpp(3 hunks)src/display/plugins/HomekitThermostatPlugin.h(3 hunks)src/display/plugins/WebUIPlugin.cpp(2 hunks)src/display/ui/default/DefaultUI.cpp(2 hunks)src/display/ui/default/DefaultUI.h(1 hunks)web/src/pages/Settings/PluginCard.jsx(2 hunks)web/src/pages/Settings/index.jsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
web/src/pages/Settings/PluginCard.jsx (2)
web/src/pages/Settings/index.jsx (2)
formData(23-23)updateHomekitMode(165-170)web/src/pages/OTA/index.jsx (1)
formData(25-25)
src/display/core/Settings.h (2)
web/src/pages/Settings/PluginCard.jsx (1)
homekitMode(17-17)src/display/core/Settings.cpp (2)
setHomekitMode(264-271)setHomekitMode(264-264)
src/display/plugins/HomekitThermostatPlugin.h (2)
src/display/plugins/HomekitThermostatPlugin.cpp (1)
HomekitThermostatPlugin(45-49)src/display/core/Plugin.h (1)
Plugin(6-12)
src/display/plugins/WebUIPlugin.cpp (1)
web/src/pages/Home/ProcessControls.jsx (1)
settings(177-187)
src/display/core/Controller.cpp (1)
src/display/core/Controller.h (1)
pluginManager(134-134)
src/display/core/Settings.cpp (1)
web/src/pages/Settings/PluginCard.jsx (1)
homekitMode(17-17)
src/display/plugins/HomekitBridgePlugin.cpp (2)
src/display/plugins/HomekitBridgePlugin.h (3)
update(42-58)HomekitBridgePlugin(61-95)loop(65-73)src/display/core/Controller.h (1)
pluginManager(134-134)
src/display/ui/default/DefaultUI.cpp (1)
src/display/core/Controller.h (1)
pluginManager(134-134)
web/src/pages/Settings/index.jsx (1)
web/src/pages/OTA/index.jsx (1)
formData(25-25)
src/display/plugins/HomekitBridgePlugin.h (3)
src/display/plugins/HomekitBridgePlugin.cpp (32)
GaggiMatePowerSwitch(8-10)update(12-19)update(12-12)update(32-39)update(32-32)setState(21-23)setState(21-21)setState(41-43)setState(41-41)GaggiMateSteamSwitch(28-30)GaggiMateHeatingSensor(48-50)setHeatingState(52-60)setHeatingState(52-52)HomekitBridgePlugin(64-67)setup(77-170)setup(77-77)loop(172-196)loop(172-172)getAction(69-69)getAction(69-69)getSwitch1State(70-70)getSwitch1State(70-70)getSwitch2State(71-71)getSwitch2State(71-71)hasAction(73-73)hasAction(73-73)clearAction(75-75)clearAction(75-75)handleHeatingStatus(213-220)handleHeatingStatus(213-213)handleBoilerStatus(200-210)handleBoilerStatus(200-200)src/display/core/Plugin.h (1)
Plugin(6-12)src/display/core/Controller.h (1)
pluginManager(134-134)
🔇 Additional comments (20)
src/display/ui/default/DefaultUI.h (1)
72-73: LGTM - follows existing pattern for stability tracking.The new
prevIsTemperatureStablemember correctly mirrors the existingisTemperatureStablepattern (Line 69) and enables detection of stability status changes for plugin event broadcasting.Note: Both variables use
intwith boolean semantics andfalseinitialization. This is an existing pattern in this file, thoughboolwould be more type-safe. Not blocking since it matches the established convention.src/display/ui/default/DefaultUI.cpp (1)
43-71: LGTM - Clean implementation of stability change detection.The logic correctly:
- Captures the previous stability state before recalculation
- Triggers the event only when stability status actually changes
- Passes the current stability state to subscribers
This enables the HomekitBridgePlugin to react to heating status changes.
web/src/pages/Settings/index.jsx (1)
220-221: LGTM - correct handling of homekitMode in form submission.The default value of
0correctly maps toHOMEKIT_MODE_DISABLEDwhen the value is undefined.src/display/plugins/HomekitThermostatPlugin.h (1)
30-32: LGTM - class renamed consistently.The rename from
HomekitPlugintoHomekitThermostatPlugincorrectly reflects the mode-based architecture and aligns with the newHomekitBridgePlugin.src/display/core/Settings.h (2)
13-17: LGTM - well-defined enum for HomeKit modes.The explicit values (0, 1, 2) are appropriate for serialization/persistence. The enum names are clear and self-documenting.
148-149: LGTM - convenient helper method.The
isHomekitEnabled()helper provides a clean abstraction for code that only needs to check if HomeKit is active, regardless of mode.src/display/plugins/WebUIPlugin.cpp (2)
456-457: LGTM - correctly uses new Settings API.The handler now properly calls
setHomekitMode(int)to update the HomeKit mode setting.
574-574: Breaking API change:homekitboolean replaced withhomekitModeinteger.The response now returns
homekitMode(integer: 0, 1, or 2) instead of the previoushomekitboolean. This is a breaking change for any external clients or automations consuming this API.Please confirm this breaking change is acceptable and document it in release notes. External integrations reading the settings API will need to update their parsing logic.
web/src/pages/Settings/PluginCard.jsx (2)
16-37: LGTM - correct state synchronization for HomeKit mode.The implementation correctly:
- Derives initial state from
homekitMode- Syncs panel open state when mode changes externally
- Defaults to Thermostat mode (1) when toggling on from disabled
The toggle-on default to mode 1 is reasonable. If you want to preserve the user's last-selected mode, you'd need additional state management.
161-231: LGTM - well-structured HomeKit mode selection UI.The expanded panel provides:
- Clear mode selection buttons with visual feedback
- Mode-specific descriptions
- Troubleshooting guidance
- Pairing instructions with visual
The UI is accessible with semantic HTML and appropriate labels.
src/display/core/Settings.cpp (3)
27-46: Migration logic is correctly documented but permanently disabled.The commented block clearly explains the read-only mode limitation. Line 27 already unconditionally reads from
"hkm", so users with the old"hk"key will get the defaultHOMEKIT_MODE_DISABLEDrather than a migrated value.If automatic migration is important, you could implement it once at the start of
doSave()(which opens preferences in write mode), or accept that users upgrading must reconfigure HomeKit.
263-271: LGTM - Good use of clamping and conditional save.The setter correctly clamps input to valid range and avoids unnecessary writes when the value hasn't changed.
513-513: LGTM - Persistence key updated consistently.The save logic correctly uses
"hkm"for the integer mode value, matching the load logic.src/display/plugins/HomekitThermostatPlugin.cpp (2)
1-1: LGTM - Include updated for renamed header.
45-102: LGTM - Clean rename with unchanged behavior.The class rename from
HomekitPlugintoHomekitThermostatPluginaligns with the new mode-based architecture while preserving the existing thermostat functionality.src/display/plugins/HomekitBridgePlugin.h (1)
61-95: LGTM - Well-structured plugin class.The class interface is clean, with proper separation between action query methods and internal state. The
getName()method helps with debugging/logging.src/display/plugins/HomekitBridgePlugin.cpp (4)
52-60: LGTM - Clear mapping of heating state to HomeKit contact sensor.The logic correctly maps heating (unstable) to "open/detected" and ready (stable) to "closed/not detected", with helpful comments explaining the color semantics.
133-157: LGTM - HomeSpan bridge setup with multiple accessories.The accessory chain is correctly structured with a bridge accessory followed by named device accessories. The callback is properly shared across switches.
213-219: LGTM - Heating status event handler.Correctly maps stability flag to heating sensor state.
185-193: Thedeactivate()call is necessary and correctly placed.The
deactivate()method cleans up any active process and triggers proper end events (brew:end, grind:end, process:end). ThesetMode(MODE_BREW)method only changes the mode state and triggers a mode change event—it does not handle process cleanup. Withoutdeactivate(), an active process would remain in memory. This pattern is consistent with the existingdeactivateStandby()helper method, which also callsdeactivate()beforesetMode(MODE_BREW). The difference between SWITCH_1_TOGGLE and SWITCH_2_TOGGLE is the order of operations (setMode→deactivate vs deactivate→setMode), not whetherdeactivate()is needed.
Reinserted commented-out line that has nothing to do with this pull request
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
comment has been added back
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/display/core/Settings.cpp (2)
5-5: Remove unused include.The
esp32-hal-log.hheader is only used in the commented-out migration logic. Since the migration code is inactive, this include is unnecessary.Apply this diff:
-#include <esp32-hal-log.h>
263-271: LGTM: Setter correctly implements mode validation and conditional save.The implementation properly clamps the mode to valid range and only triggers a save when the value actually changes, avoiding unnecessary writes.
Optional: The comment on line 266 could be more descriptive:
- // check save + // Only save if the value changed
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/display/core/Settings.cpp(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/display/core/Settings.cpp (1)
web/src/pages/Settings/PluginCard.jsx (1)
homekitMode(17-17)
🔇 Additional comments (2)
src/display/core/Settings.cpp (2)
27-27: LGTM: Loading logic correctly migrated.The change from boolean to integer mode is implemented correctly with an appropriate default value.
513-513: LGTM: Save logic correctly migrated.The change from boolean to integer persistence is implemented correctly and consistent with the loading logic.
Fixed nitpick "The function body is not properly indented relative to the function declaration."
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/display/plugins/HomekitBridgePlugin.cpp (2)
85-87: Document or make configurable the hardcoded HomeKit pairing code.The pairing code
"46637726"is hardcoded. If this is a shared default for all GaggiMate devices, consider documenting the security implications or making it configurable per device for better security.
125-129: Inconsistent event naming convention.Event names use different styles:
"BOILER_STATUS"(uppercase) versus"boiler:heating:stable"(lowercase with colons). Consider standardizing to a single convention for maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/display/plugins/HomekitBridgePlugin.cpp(1 hunks)src/display/plugins/HomekitBridgePlugin.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/display/plugins/HomekitBridgePlugin.cpp (2)
src/display/plugins/HomekitBridgePlugin.h (2)
update(43-60)loop(68-73)src/display/core/Controller.cpp (4)
setup(34-99)setup(34-34)loop(266-349)loop(266-266)
src/display/plugins/HomekitBridgePlugin.h (4)
src/display/plugins/HomekitBridgePlugin.cpp (24)
GaggiMatePowerSwitch(9-11)update(13-20)update(13-13)update(32-39)update(32-32)setState(22-24)setState(22-22)setState(41-43)setState(41-41)GaggiMateSteamSwitch(28-30)GaggiMateHeatingSensor(47-50)setStability(52-60)setStability(52-52)HomekitBridgePlugin(65-67)setup(69-131)setup(69-69)loop(133-157)loop(133-133)clearAction(159-162)clearAction(159-159)handleHeatingStatus(176-182)handleHeatingStatus(176-176)handleBoilerStatus(166-174)handleBoilerStatus(166-166)src/display/core/Plugin.h (1)
Plugin(6-12)src/display/core/Controller.cpp (4)
setup(34-99)setup(34-34)loop(266-349)loop(266-266)src/display/core/Controller.h (1)
pluginManager(134-134)
🔇 Additional comments (9)
src/display/plugins/HomekitBridgePlugin.cpp (5)
72-83: LGTM! Thread safety improved with atomic operations.The callback now uses atomic
.store()operations for all shared state variables, properly addressing the race condition flagged in the previous review.
91-120: LGTM! Proper HomeSpan accessory setup.The bridge and accessory initialization follows HomeSpan's declarative API pattern correctly, with custom accessories properly stored for later reference.
166-182: LGTM! Event handlers are well-structured.Both event handlers properly check for null pointers and correctly map event data to accessory states.
147-154: The code asymmetry is intentional and follows established patterns in the codebase.The
deactivate()call beforesetMode(MODE_BREW)is not an oversight. Throughout the codebase, all transitions to MODE_BREW consistently calldeactivate()first (see Controller.cpp:645, 743-744, 776-777 and ui_events.cpp:35-36, 53-54, 68-69), while MODE_STEAM transitions usesetMode(MODE_STEAM)directly. This is an intentional design:deactivate()stops the current process and triggers cleanup events ("controller:brew:end", "controller:process:end"), which is necessary before starting a new brew but not required for steam mode.
8-11: No destructors needed. HomeSpan automatically registers instantiated Characteristics and adds them to the HomeSpan HAP Database, and creating Characteristics withnewis the documented pattern for HomeSpan. The framework manages the lifecycle of these objects. This code follows HomeSpan best practices.Likely an incorrect or invalid review comment.
src/display/plugins/HomekitBridgePlugin.h (4)
1-14: LGTM! Proper header setup with necessary includes.The header guards and includes are correctly structured, including
<atomic>for the atomic member variables.
16-23: LGTM! Clean enum and typedef declarations.Using
enum classprovides type safety, and thestd::functiontypedef makes the callback signature clear and maintainable.
85-88: LGTM! Proper atomic variable declarations for thread safety.The shared state variables are correctly declared as
std::atomictypes with appropriate initializers, fully resolving the race condition concerns from the previous review.
28-60: Add virtual destructors to HomeSpan-derived Service classes.These derived classes (
GaggiMatePowerSwitch,GaggiMateSteamSwitch,GaggiMateHeatingSensor) inherit from HomeSpan base classes that have virtual methods. Although HomeSpan appears to manage object lifecycle through its framework, declaring virtual destructors follows C++ best practices for polymorphic base classes and prevents potential undefined behavior if these objects are ever deleted through base class pointers.
Fixed comment: Thread-Safe variables (Atomic), fixing race condition
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/display/plugins/HomekitBridgePlugin.h (1)
27-49: Consider consolidating duplicate switch classes.
GaggiMatePowerSwitchandGaggiMateSteamSwitchshare identical structure and differ only in theHomekitActionpassed to the callback. Consider refactoring into a single parameterized class to reduce duplication and improve maintainability.Example approach:
class GaggiMateSwitch : public Service::Switch { public: GaggiMateSwitch(bridge_callback_t callback, HomekitAction action); boolean update() override; void setState(bool active); private: bridge_callback_t callback; HomekitAction action; SpanCharacteristic *power; };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/display/plugins/HomekitBridgePlugin.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/display/plugins/HomekitBridgePlugin.h (1)
src/display/plugins/HomekitBridgePlugin.cpp (24)
GaggiMatePowerSwitch(9-11)update(13-20)update(13-13)update(32-39)update(32-32)setState(22-24)setState(22-22)setState(41-43)setState(41-41)GaggiMateSteamSwitch(28-30)GaggiMateHeatingSensor(47-50)setStability(52-60)setStability(52-52)HomekitBridgePlugin(65-67)setup(69-131)setup(69-69)loop(133-157)loop(133-133)clearAction(159-162)clearAction(159-159)handleHeatingStatus(176-182)handleHeatingStatus(176-176)handleBoilerStatus(166-174)handleBoilerStatus(166-166)
🔇 Additional comments (5)
src/display/plugins/HomekitBridgePlugin.h (5)
1-14: LGTM! Clean header structure.The includes are appropriate for the HomeKit bridge functionality, and the header guards follow standard conventions.
16-23: LGTM! Clear action enum and callback type.The enum class provides type safety, and the callback signature is well-defined for the bridge pattern.
52-60: LGTM! Clear sensor interface with good documentation.The
setStabilitymethod is well-documented with its boolean mapping, making the interface intuitive.
84-88: LGTM! Proper thread-safety with atomics.The use of
std::atomicforlastAction, state flags, andactionRequiredcorrectly addresses race conditions between HomeSpan callbacks (which run on HomeSpan's thread) and the mainloop()execution.
64-95: LGTM! Well-structured plugin class.The plugin follows the Plugin interface correctly, uses appropriate event handlers, and maintains proper separation of concerns. The null-initialized pointers align with HomeSpan's lifecycle management patterns.
Synchronization problem with the heating status caused by the use of atomic has been fixed. Atomic is now only used for the switches, but not for the heating status, because Atomics was introduced to prevent race conditions when HomeKit (a separate process/task) attempts to control the GaggiMate controller. However, the data flow is reversed for the heating status: - Source: The controller triggers an event (boiler:heating:stable). - Target: The HomeKit plugin receives the event and writes the value to the HomeSpan sensor (heatingSensor->setStability). Since HomeSpan is programmed to be thread-safe internally and the setVal(value, true) method ensures that the value is copied safely to the HomeKit memory, no additional atomic is necessary here. There is no “decision loop” that could be interrupted. In addition, homespan classes have been cleaned up again so that Apple Home accepts the devices without any problems.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/display/plugins/HomekitBridgePlugin.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/display/plugins/HomekitBridgePlugin.cpp (1)
src/display/plugins/HomekitBridgePlugin.h (2)
update(43-60)loop(68-73)
🔇 Additional comments (7)
src/display/plugins/HomekitBridgePlugin.cpp (7)
8-25: LGTM: Power switch implementation is clean.The callback pattern and state management logic are correct. The null check in
setState()is a good defensive practice.
27-44: LGTM: Steam switch follows the same clean pattern.Consistent implementation with the power switch. No concerns.
46-56: LGTM: Heating sensor mapping is logical.The stable-to-closed mapping (1 = ready, 0 = heating) is intuitive and correctly implemented. The value-check before updating avoids unnecessary HomeKit notifications.
61-67: LGTM: Constructor properly initializes atomic state.The use of
std::movefor the WiFi credentials and explicit atomic initialization are both correct.
142-145: LGTM: Action clearing is correctly scoped.Appropriately resets only the action flag and type, while preserving the switch state atomics which represent the current state rather than pending actions.
147-151: LGTM: Boiler status synchronization is correct.The logic properly maps the boiler state to HomeKit accessories:
- Power switch reflects the inverse of standby status
- Steam switch reflects steam mode status
Null checks add appropriate safety.
153-158: LGTM: Heating status handler is straightforward.The direct transfer of stability state to the sensor is clean. The comment on line 155 helpfully documents the thread-safety assumption.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
modsmthng
left a comment
modsmthng
left a comment
There was a problem hiding this comment.
An automatic migration logic for transferring existing Homekit plugin settings has been implemented.
The migration works on the controller side, but sometimes the controller needs to be restarted or sailed through the two integrations once for Apple Home to update the devices.
…e integration of the devices has also been simplified again to avoid connection problems with Homekit.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
…op-down menu, and an info text about the integration added, as with the other plugins.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/src/pages/Settings/PluginCard.jsx (1)
207-207: Consider improving clarity of heating sensor description.The description "red / heating = closed, green / ready = open" maps visual states (red/green) to contact sensor states (closed/open). While accurate, this mapping may confuse users unfamiliar with HomeKit's ContactSensor semantics. Consider adding a brief explanation or simplifying the language.
💡 Suggested clarification
- <li>Sensor that reflects the heating status (red / heating = closed, green / ready = open)</li> + <li>Sensor that reflects the heating status (shows red while heating, green when ready)</li>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/display/core/Settings.cpp(4 hunks)src/display/plugins/HomekitBridgePlugin.cpp(1 hunks)src/display/plugins/HomekitBridgePlugin.h(1 hunks)web/src/pages/Settings/PluginCard.jsx(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:44:54.672Z
Learning: In the GaggiMate codebase (src/display/plugins/HomekitBridgePlugin.cpp), HomeSpan acts as a secondary, non-critical service that operates independently of core machine control. HomeSpan initialization failures do not affect boiler safety or operation.
📚 Learning: 2025-12-18T12:44:54.672Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:44:54.672Z
Learning: In the GaggiMate codebase (src/display/plugins/HomekitBridgePlugin.cpp), HomeSpan acts as a secondary, non-critical service that operates independently of core machine control. HomeSpan initialization failures do not affect boiler safety or operation.
Applied to files:
src/display/plugins/HomekitBridgePlugin.cppweb/src/pages/Settings/PluginCard.jsxsrc/display/plugins/HomekitBridgePlugin.h
📚 Learning: 2025-12-18T12:44:54.672Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:44:54.672Z
Learning: The HomeSpan library handles connection retries internally, so explicit error handling for initialization failures in HomeKit plugins is not required in the GaggiMate codebase.
Applied to files:
src/display/plugins/HomekitBridgePlugin.cppsrc/display/plugins/HomekitBridgePlugin.h
📚 Learning: 2025-12-18T12:44:54.672Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:44:54.672Z
Learning: Do not add or rely on WiFi credential validation inside HomekitBridgePlugin.cpp. This project validates HomeKit plugin credentials in the WebUI before passing them to the plugin; reviews should assume upstream validation has occurred and avoid duplicating validation in the plugin unless there is a concrete, tested reason to revalidate at the plugin boundary.
Applied to files:
src/display/plugins/HomekitBridgePlugin.cpp
📚 Learning: 2025-12-18T12:44:54.672Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:44:54.672Z
Learning: In the GaggiMate codebase, WiFi credentials for HomeKit plugins are managed and validated by the WebUI before being passed to the HomeKit plugin, so additional validation in the plugin is unnecessary.
Applied to files:
web/src/pages/Settings/PluginCard.jsxsrc/display/plugins/HomekitBridgePlugin.hsrc/display/core/Settings.cpp
🧬 Code graph analysis (4)
src/display/plugins/HomekitBridgePlugin.cpp (1)
src/display/plugins/HomekitBridgePlugin.h (2)
update(43-60)loop(68-73)
web/src/pages/Settings/PluginCard.jsx (1)
web/src/pages/Settings/index.jsx (2)
formData(23-23)updateHomekitMode(165-170)
src/display/plugins/HomekitBridgePlugin.h (1)
src/display/plugins/HomekitBridgePlugin.cpp (20)
GaggiMatePowerSwitch(11-14)update(16-23)update(16-16)update(35-42)update(35-35)setState(25-27)setState(25-25)setState(44-46)setState(44-44)GaggiMateSteamSwitch(30-33)GaggiMateHeatingSensor(49-51)setStability(53-59)setStability(53-53)HomekitBridgePlugin(64-77)setup(79-141)setup(79-79)loop(143-186)loop(143-143)clearAction(188-191)clearAction(188-188)
src/display/core/Settings.cpp (1)
src/display/core/Settings.h (1)
Settings(55-271)
🔇 Additional comments (6)
src/display/plugins/HomekitBridgePlugin.cpp (2)
79-141: LGTM!The setup method correctly initializes HomeSpan, creates accessories, and registers event handlers. Thread safety is properly handled using atomic stores in callbacks, and memory management follows HomeSpan's ownership model. WiFi credential validation is handled upstream by the WebUI as per the established pattern.
Based on learnings, HomeSpan acts as a secondary, non-critical service, and initialization failures do not affect boiler safety.
143-186: LGTM!The loop correctly synchronizes state between HomeKit and the controller using atomic exchange operations. The SWITCH_2_TOGGLE asymmetry (entering steam mode without explicit deactivate) is intentional, as the controller's state machine handles the transition internally.
src/display/core/Settings.cpp (2)
8-31: LGTM! Migration logic properly implemented.The two-phase approach (read-write for migration, then read-only for initialization) correctly handles the upgrade path from boolean
hkto integerhkm. The migration preserves existing user settings by mappingtrue → HOMEKIT_MODE_THERMOSTATandfalse → HOMEKIT_MODE_DISABLED, and properly removes the obsolete key.This addresses the previous review concern about enabling migration for existing users.
270-278: LGTM!The setter correctly clamps the mode to the valid range and only triggers a save when the value actually changes, consistent with other setters in this file.
src/display/plugins/HomekitBridgePlugin.h (2)
1-23: LGTM!The header structure is well-organized with proper include guards, necessary dependencies, and type-safe enums. The callback typedef clearly documents the interface between HomeKit and the controller.
85-99: LGTM! Thread-safe design with atomic variables.The atomic state variables correctly address the race condition concerns identified in previous reviews. All shared state accessed by both event callbacks and the main loop is properly protected using
std::atomic.
Added: "My Homekit stopped working" as "Reset the HomeKit plugin via the console" from official GaggiMate troubleshooting web page
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/src/pages/Settings/PluginCard.jsx (2)
29-37: Consider preserving the last selected mode when re-enabling.Currently, when toggling HomeKit off and then back on, the handler always defaults to mode 1 (Thermostat) regardless of the previous selection. If a user had selected Bridge mode (2), toggled off, then toggled back on, they would need to reselect Bridge mode.
Consider storing the last active mode (1 or 2) before setting it to 0, so re-enabling restores the previous selection rather than always defaulting to Thermostat.
🔎 Potential improvement
+ const lastModeRef = useRef(homekitMode > 0 ? homekitMode : 1); + + useEffect(() => { + if (homekitMode > 0) { + lastModeRef.current = homekitMode; + } + }, [homekitMode]); + const handleHomekitToggle = () => { const nextState = !isHomekitOpen; setIsHomekitOpen(nextState); if (!nextState) { updateHomekitMode(0); - } else if (homekitMode === 0) { - updateHomekitMode(1); + } else if (homekitMode === 0) { + updateHomekitMode(lastModeRef.current); } };Note: This would require importing
useReffrompreact/hooks.
180-195: Consider adding accessibility attributes to mode selector buttons.The mode selector buttons lack
aria-pressedoraria-checkedattributes to communicate the active state to screen readers. While the visual styling conveys the active mode to sighted users, assistive technology users would benefit from explicit ARIA attributes.🔎 Suggested improvement
<button type='button' className={`btn btn-sm ${homekitMode === 1 ? 'btn-primary' : 'btn-outline'}`} onClick={() => updateHomekitMode(1)} + aria-pressed={homekitMode === 1} + aria-label='Thermostat mode integration' > Thermostat </button> <button type='button' className={`btn btn-sm ${homekitMode === 2 ? 'btn-primary' : 'btn-outline'}`} onClick={() => updateHomekitMode(2)} + aria-pressed={homekitMode === 2} + aria-label='Bridge mode integration with multiple accessories' > Bridge </button>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/pages/Settings/PluginCard.jsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:53-59
Timestamp: 2025-12-19T22:16:37.672Z
Learning: In the GaggiMate HomeKit bridge plugin (src/display/plugins/HomekitBridgePlugin.cpp), the GaggiMateHeatingSensor intentionally maps stable (ready) state to ContactSensorState value 1 (Open) so that Apple Home displays the sensor prominently in the overview when the machine is ready. This is the correct behavior because Apple Home automatically highlights "open" devices, making the ready status more visible to users.
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:44:54.672Z
Learning: In the GaggiMate codebase (src/display/plugins/HomekitBridgePlugin.cpp), HomeSpan acts as a secondary, non-critical service that operates independently of core machine control. HomeSpan initialization failures do not affect boiler safety or operation.
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:44:54.672Z
Learning: In the GaggiMate codebase, WiFi credentials for HomeKit plugins are managed and validated by the WebUI before being passed to the HomeKit plugin, so additional validation in the plugin is unnecessary.
📚 Learning: 2025-08-14T20:05:55.096Z
Learnt from: igadmile
Repo: jniebuhr/gaggimate PR: 360
File: web/src/pages/Home/ProcessControls.jsx:106-114
Timestamp: 2025-08-14T20:05:55.096Z
Learning: In React components that use WebSocket-based state updates, there can be race conditions when using useEffect to watch for state transitions. For example, when a flush is started, local state (`isFlushing`) is updated immediately, but WebSocket-driven state (`active`) is updated asynchronously after the message is received, creating timing issues if useEffect tries to act on both states simultaneously.
Applied to files:
web/src/pages/Settings/PluginCard.jsx
📚 Learning: 2025-12-19T22:16:37.672Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:53-59
Timestamp: 2025-12-19T22:16:37.672Z
Learning: In the GaggiMate HomeKit bridge plugin (src/display/plugins/HomekitBridgePlugin.cpp), the GaggiMateHeatingSensor intentionally maps stable (ready) state to ContactSensorState value 1 (Open) so that Apple Home displays the sensor prominently in the overview when the machine is ready. This is the correct behavior because Apple Home automatically highlights "open" devices, making the ready status more visible to users.
Applied to files:
web/src/pages/Settings/PluginCard.jsx
📚 Learning: 2025-12-18T12:44:54.672Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:44:54.672Z
Learning: In the GaggiMate codebase (src/display/plugins/HomekitBridgePlugin.cpp), HomeSpan acts as a secondary, non-critical service that operates independently of core machine control. HomeSpan initialization failures do not affect boiler safety or operation.
Applied to files:
web/src/pages/Settings/PluginCard.jsx
🧬 Code graph analysis (1)
web/src/pages/Settings/PluginCard.jsx (1)
web/src/pages/Settings/index.jsx (2)
formData(23-23)updateHomekitMode(165-170)
🔇 Additional comments (4)
web/src/pages/Settings/PluginCard.jsx (4)
4-4: LGTM!The new imports and
updateHomekitModeprop are correctly added to support the HomeKit mode selection functionality.Also applies to: 9-9
17-27: LGTM!The state initialization and sync logic are correct. The one-way sync in the
useEffect(only opening whenhomekitMode > 0, not closing when it becomes 0) was previously reviewed and confirmed as intentional for migration logic, with user-initiated toggles handled separately byhandleHomekitToggle.
168-176: Excellent migration guidance!The migration warning provides clear, proactive instructions to prevent common issues during the update. Highlighting not to remove the device and directing users to troubleshooting is great UX.
231-282: Excellent troubleshooting documentation!The collapsible troubleshooting section is comprehensive and well-structured with:
- Clear escalation path from simple restarts to full resets
- Proper formatting of console commands with
<code>tags- Appropriate severity styling for critical warnings
- Step-by-step instructions that are easy to follow
This will significantly reduce user frustration and support requests.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/src/pages/Settings/PluginCard.jsx (2)
180-196: Enhance accessibility for mode selector buttons.The mode selector buttons function correctly but could benefit from accessibility attributes to announce the active state to screen readers. Similar to the day-of-week buttons in the Auto Wakeup section (lines 76-111), consider adding
roleandaria-pressedattributes.🔎 Suggested accessibility enhancement
<div className='grid grid-cols-2 gap-2'> <button type='button' className={`btn btn-sm ${homekitMode === 1 ? 'btn-primary' : 'btn-outline'}`} onClick={() => updateHomekitMode(1)} + role='radio' + aria-pressed={homekitMode === 1} + aria-label='Thermostat mode' > Thermostat </button> <button type='button' className={`btn btn-sm ${homekitMode === 2 ? 'btn-primary' : 'btn-outline'}`} onClick={() => updateHomekitMode(2)} + role='radio' + aria-pressed={homekitMode === 2} + aria-label='Bridge mode' > Bridge </button> </div>
217-222: Consider Tailwind classes for style consistency.The inline
maxHeightstyle works but breaks the Tailwind-first pattern used throughout the codebase. For consistency, consider using Tailwind's arbitrary value syntax.🔎 Suggested refactor
<img src={homekitImage} - alt='Homekit Setup Code' + alt='HomeKit pairing QR code for GaggiMate setup' className='h-auto w-auto max-w-full object-contain' - style={{ maxHeight: '150px' }} + className='h-auto w-auto max-w-full object-contain max-h-[150px]' />Note: Also improves the alt text for better screen reader context.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/pages/Settings/PluginCard.jsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:53-59
Timestamp: 2025-12-19T22:16:37.672Z
Learning: In the GaggiMate HomeKit bridge plugin (src/display/plugins/HomekitBridgePlugin.cpp), the GaggiMateHeatingSensor intentionally maps stable (ready) state to ContactSensorState value 1 (Open) so that Apple Home displays the sensor prominently in the overview when the machine is ready. This is the correct behavior because Apple Home automatically highlights "open" devices, making the ready status more visible to users.
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:44:54.672Z
Learning: In the GaggiMate codebase (src/display/plugins/HomekitBridgePlugin.cpp), HomeSpan acts as a secondary, non-critical service that operates independently of core machine control. HomeSpan initialization failures do not affect boiler safety or operation.
📚 Learning: 2025-08-14T20:05:55.096Z
Learnt from: igadmile
Repo: jniebuhr/gaggimate PR: 360
File: web/src/pages/Home/ProcessControls.jsx:106-114
Timestamp: 2025-08-14T20:05:55.096Z
Learning: In React components that use WebSocket-based state updates, there can be race conditions when using useEffect to watch for state transitions. For example, when a flush is started, local state (`isFlushing`) is updated immediately, but WebSocket-driven state (`active`) is updated asynchronously after the message is received, creating timing issues if useEffect tries to act on both states simultaneously.
Applied to files:
web/src/pages/Settings/PluginCard.jsx
📚 Learning: 2025-12-19T22:16:37.672Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:53-59
Timestamp: 2025-12-19T22:16:37.672Z
Learning: In the GaggiMate HomeKit bridge plugin (src/display/plugins/HomekitBridgePlugin.cpp), the GaggiMateHeatingSensor intentionally maps stable (ready) state to ContactSensorState value 1 (Open) so that Apple Home displays the sensor prominently in the overview when the machine is ready. This is the correct behavior because Apple Home automatically highlights "open" devices, making the ready status more visible to users.
Applied to files:
web/src/pages/Settings/PluginCard.jsx
📚 Learning: 2025-12-18T12:44:54.672Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:44:54.672Z
Learning: In the GaggiMate codebase (src/display/plugins/HomekitBridgePlugin.cpp), HomeSpan acts as a secondary, non-critical service that operates independently of core machine control. HomeSpan initialization failures do not affect boiler safety or operation.
Applied to files:
web/src/pages/Settings/PluginCard.jsx
🧬 Code graph analysis (1)
web/src/pages/Settings/PluginCard.jsx (1)
web/src/pages/Settings/index.jsx (2)
formData(23-23)updateHomekitMode(165-170)
🔇 Additional comments (2)
web/src/pages/Settings/PluginCard.jsx (2)
17-37: LGTM! State management logic is sound.The local state and toggle handler work correctly together:
isHomekitOpentracks UI expansion statehandleHomekitToggleensures consistent bidirectional sync between UI and mode- The useEffect one-way sync (opening when
homekitMode > 0) aligns with the migration logic you described in previous commentsThe implementation correctly prevents mode conflicts and handles the disable/enable flow.
149-285: Excellent UX implementation for HomeKit integration modes.The expanded HomeKit UI is comprehensive and user-friendly:
✅ Migration warning alerts users upfront about potential connection issues
✅ Clear mode descriptions explain Thermostat vs Bridge functionality
✅ Progressive troubleshooting guides users from simple (restart) to advanced (console reset) to last-resort (re-flash) solutions
✅ Appropriate warnings for destructive actions (re-flash styled withtext-error)The implementation handles the migration path thoughtfully and provides users with the information they need to troubleshoot independently.
…d UI - Added settings to toggle Power, Steam, and Sensor in Bridge Mode independently. - Assigned static Accessory IDs to prevent HomeKit cache conflicts when toggling devices. - Updated WebUI with new toggles - WebUI descriptions shortened and adapted. - Fixed WebUI form handling for new boolean values. - Comments on the HomekitBridgePlugin have been slightly adjusted and corrected with the help of AI (and checked by me).
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/display/core/Settings.cpp (1)
284-297: Consider adding change-check to per-device setters.Unlike
setHomekitMode, thesetHk*Enabledsetters always callsave()even if the value hasn't changed. This could cause unnecessary flash writes when toggling settings repeatedly.🔎 Optional optimization
void Settings::setHkPowerEnabled(bool enabled) { + if (hkPowerEnabled == enabled) return; hkPowerEnabled = enabled; save(); } void Settings::setHkSteamEnabled(bool enabled) { + if (hkSteamEnabled == enabled) return; hkSteamEnabled = enabled; save(); } void Settings::setHkSensorEnabled(bool enabled) { + if (hkSensorEnabled == enabled) return; hkSensorEnabled = enabled; save(); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/display/core/Settings.cppsrc/display/core/Settings.hsrc/display/plugins/HomekitBridgePlugin.cppsrc/display/plugins/WebUIPlugin.cppweb/src/pages/Settings/PluginCard.jsxweb/src/pages/Settings/index.jsx
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:53-59
Timestamp: 2025-12-19T22:16:46.304Z
Learning: In the GaggiMate HomeKit bridge plugin (src/display/plugins/HomekitBridgePlugin.cpp), the GaggiMateHeatingSensor intentionally maps stable (ready) state to ContactSensorState value 1 (Open) so that Apple Home displays the sensor prominently in the overview when the machine is ready. This is the correct behavior because Apple Home automatically highlights "open" devices, making the ready status more visible to users.
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:45:27.712Z
Learning: In the GaggiMate codebase (src/display/plugins/HomekitBridgePlugin.cpp), HomeSpan acts as a secondary, non-critical service that operates independently of core machine control. HomeSpan initialization failures do not affect boiler safety or operation.
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:45:27.712Z
Learning: In the GaggiMate codebase, WiFi credentials for HomeKit plugins are managed and validated by the WebUI before being passed to the HomeKit plugin, so additional validation in the plugin is unnecessary.
📚 Learning: 2025-12-18T12:45:27.712Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:45:27.712Z
Learning: In the GaggiMate codebase, WiFi credentials for HomeKit plugins are managed and validated by the WebUI before being passed to the HomeKit plugin, so additional validation in the plugin is unnecessary.
Applied to files:
src/display/plugins/WebUIPlugin.cppsrc/display/core/Settings.cpp
📚 Learning: 2025-12-19T22:16:46.304Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:53-59
Timestamp: 2025-12-19T22:16:46.304Z
Learning: In the GaggiMate HomeKit bridge plugin (src/display/plugins/HomekitBridgePlugin.cpp), the GaggiMateHeatingSensor intentionally maps stable (ready) state to ContactSensorState value 1 (Open) so that Apple Home displays the sensor prominently in the overview when the machine is ready. This is the correct behavior because Apple Home automatically highlights "open" devices, making the ready status more visible to users.
Applied to files:
src/display/core/Settings.cppweb/src/pages/Settings/PluginCard.jsxsrc/display/core/Settings.h
📚 Learning: 2025-12-18T12:45:27.712Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:45:27.712Z
Learning: In the GaggiMate codebase (src/display/plugins/HomekitBridgePlugin.cpp), HomeSpan acts as a secondary, non-critical service that operates independently of core machine control. HomeSpan initialization failures do not affect boiler safety or operation.
Applied to files:
src/display/plugins/HomekitBridgePlugin.cppweb/src/pages/Settings/PluginCard.jsx
📚 Learning: 2025-12-19T22:16:37.672Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:53-59
Timestamp: 2025-12-19T22:16:37.672Z
Learning: In src/display/plugins/HomekitBridgePlugin.cpp, the GaggiMateHeatingSensor maps the stable (ready) state to ContactSensorState value 1 (Open) so Apple Home highlights the sensor in the overview. This is a design decision to improve visibility for users when the machine is ready. Reviewers should ensure this mapping is intentional, well-documented in code comments, and covered by tests that verify the correct HomeKit status is exposed for the ready state. If similar sensors exist, consider documenting a consistent policy for what HomeKit states correspond to readiness, and avoid duplicated logic that could drift over time.
Applied to files:
src/display/plugins/HomekitBridgePlugin.cpp
📚 Learning: 2025-12-18T12:44:54.672Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:44:54.672Z
Learning: Do not add or rely on WiFi credential validation inside HomekitBridgePlugin.cpp. This project validates HomeKit plugin credentials in the WebUI before passing them to the plugin; reviews should assume upstream validation has occurred and avoid duplicating validation in the plugin unless there is a concrete, tested reason to revalidate at the plugin boundary.
Applied to files:
src/display/plugins/HomekitBridgePlugin.cpp
📚 Learning: 2025-12-18T12:45:27.712Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:45:27.712Z
Learning: The HomeSpan library handles connection retries internally, so explicit error handling for initialization failures in HomeKit plugins is not required in the GaggiMate codebase.
Applied to files:
src/display/plugins/HomekitBridgePlugin.cpp
📚 Learning: 2025-08-14T20:05:55.096Z
Learnt from: igadmile
Repo: jniebuhr/gaggimate PR: 360
File: web/src/pages/Home/ProcessControls.jsx:106-114
Timestamp: 2025-08-14T20:05:55.096Z
Learning: In React components that use WebSocket-based state updates, there can be race conditions when using useEffect to watch for state transitions. For example, when a flush is started, local state (`isFlushing`) is updated immediately, but WebSocket-driven state (`active`) is updated asynchronously after the message is received, creating timing issues if useEffect tries to act on both states simultaneously.
Applied to files:
web/src/pages/Settings/PluginCard.jsx
🧬 Code graph analysis (5)
src/display/plugins/WebUIPlugin.cpp (1)
web/src/pages/Home/ProcessControls.jsx (1)
settings(177-187)
src/display/core/Settings.cpp (2)
src/display/core/Settings.h (1)
Settings(55-283)web/src/pages/Settings/index.jsx (1)
Settings(20-986)
src/display/plugins/HomekitBridgePlugin.cpp (1)
src/display/plugins/HomekitBridgePlugin.h (2)
update(43-60)loop(68-73)
web/src/pages/Settings/PluginCard.jsx (1)
web/src/pages/Settings/index.jsx (3)
formData(23-23)updateHomekitMode(172-177)onChange(105-160)
src/display/core/Settings.h (2)
web/src/pages/Settings/PluginCard.jsx (1)
homekitMode(17-17)src/display/core/Settings.cpp (8)
setHomekitMode(275-282)setHomekitMode(275-275)setHkPowerEnabled(285-288)setHkPowerEnabled(285-285)setHkSteamEnabled(290-293)setHkSteamEnabled(290-290)setHkSensorEnabled(295-298)setHkSensorEnabled(295-295)
🔇 Additional comments (19)
web/src/pages/Settings/PluginCard.jsx (3)
16-37: LGTM on HomeKit state management.The local state management with
isHomekitOpenand the sync effect is well-structured. The one-way sync (homekitMode > 0→ open) is intentional for migration, as the toggle handler ensures proper bidirectional updates during user interaction.
168-186: Mode selector implementation looks good.The two-button selector correctly maps mode values (1=Thermostat, 2=Bridge) and uses
updateHomekitModeto propagate changes upstream.
201-261: Accessory toggles correctly default to enabled.The
checked={formData.hkPowerEnabled !== false}pattern ensures new users get all accessories enabled by default, which aligns with the backend defaults (hkPowerEnabled = truein Settings.h).src/display/plugins/WebUIPlugin.cpp (2)
456-462: HomeKit settings handling is correct.The unconditional
setHk*Enabled(request->hasArg(...))pattern is appropriate for HTML checkboxes—when unchecked, the parameter is absent, sohasArgreturnsfalse, correctly disabling the feature.
579-582: API response includes all HomeKit fields.The GET response correctly exposes
homekitModeand the three per-device toggles, enabling the frontend to initialize state properly.src/display/core/Settings.h (3)
13-17: Well-structured HomeKit mode enum.The enum values are clear and well-documented. Using explicit integer values (0, 1, 2) ensures stable serialization and aligns with the mode selector in the UI.
85-89: Clean public API for HomeKit settings.The getter/setter pairs follow the existing codebase patterns, and the
isHomekitEnabled()convenience method simplifies conditional logic elsewhere.Also applies to: 152-157
226-230: Sensible default values for HomeKit state.Defaulting
homekitModetoHOMEKIT_MODE_DISABLEDand the per-device toggles totrueis appropriate—when users enable HomeKit, all accessories will be available by default.web/src/pages/Settings/index.jsx (3)
47-50: Consistent initialization of HomeKit toggles.Defaulting to
truewhen undefined matches the backend defaults inSettings.h, ensuring a smooth experience for new users enabling HomeKit.
118-123: Correct onChange handling for HomeKit fields.The
homekitModeis properly parsed as an integer, and the toggle fields use the standard boolean inversion pattern consistent with other checkboxes in this form.
172-177: HomeKit mode update and submission logic is correct.The
updateHomekitModefunction correctly updatesformData, and the submission fallback to0ensures disabled state is persisted whenhomekitModeis undefined.Also applies to: 227-227
src/display/core/Settings.cpp (3)
8-31: Solid migration implementation.The two-phase approach (read-write for migration, then read-only for standard init) is well-structured. The migration correctly maps the legacy boolean to the new integer mode and cleans up the obsolete key.
274-282: Safe mode setter with clamping.The
std::clampensures invalid values can't be persisted, and the change-check optimization avoids unnecessary flash writes.
540-545: Persistence includes all HomeKit state.The
doSavemethod correctly persistshomekitModeand all three per-device toggles with consistent key naming.src/display/plugins/HomekitBridgePlugin.cpp (5)
68-81: Proper atomic initialization.All shared state variables are correctly initialized as atomics in the constructor, ensuring thread-safe access between HomeSpan callbacks and the main loop.
86-89: Conditional accessory creation is well-implemented.Reading the enable flags from settings and conditionally creating accessories avoids HomeKit clutter. Using explicit static IDs (1-4) ensures stable accessory identification across restarts and configuration changes.
Also applies to: 118-146
55-61: Sensor state mapping is intentional per learnings.The mapping of
isStable ? 1 : 0(stable=Open) is intentional to leverage Apple Home's UX where "open" sensors appear prominently in the overview. The comment could be updated to clarify this design choice.Based on learnings, this is the correct behavior.
165-210: Thread-safe loop implementation.The
exchange(false)pattern for status/heating flags prevents missed updates and ensures atomic read-reset. Null checks before accessingpowerSwitch,steamSwitch, andheatingSensorcorrectly handle partially-enabled configurations.
92-100: Event name and payload key are consistent. The event"boiler:heating:stable"is correctly emitted from DefaultUI.cpp with the payload key"isStable", matching the subscription and key access in HomekitBridgePlugin.cpp at lines 156–159.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
fix text overflow in homekit bridge descriptions
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @web/src/pages/Settings/PluginCard.jsx:
- Around line 23-27: The useEffect currently only opens the UI when homekitMode
> 0 but never closes it when homekitMode becomes 0; update the effect that
references homekitMode, setIsHomekitOpen to set false when homekitMode === 0
(e.g., setIsHomekitOpen(homekitMode > 0)), ensuring the effect depends on
homekitMode so local state is kept in sync with external form/state changes.
🧹 Nitpick comments (1)
web/src/pages/Settings/PluginCard.jsx (1)
171-184: Consider addingaria-pressedfor accessibility parity.The mode selector buttons indicate active state visually but lack
aria-pressedattributes that the day-of-week buttons use (line 85). This would improve screen reader support.Suggested accessibility improvement
<button type='button' className={`btn btn-sm ${homekitMode === 1 ? 'btn-primary' : 'btn-outline'}`} onClick={() => updateHomekitMode(1)} + aria-pressed={homekitMode === 1} > Thermostat </button> <button type='button' className={`btn btn-sm ${homekitMode === 2 ? 'btn-primary' : 'btn-outline'}`} onClick={() => updateHomekitMode(2)} + aria-pressed={homekitMode === 2} > Bridge </button>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/pages/Settings/PluginCard.jsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:53-59
Timestamp: 2025-12-19T22:16:46.304Z
Learning: In the GaggiMate HomeKit bridge plugin (src/display/plugins/HomekitBridgePlugin.cpp), the GaggiMateHeatingSensor intentionally maps stable (ready) state to ContactSensorState value 1 (Open) so that Apple Home displays the sensor prominently in the overview when the machine is ready. This is the correct behavior because Apple Home automatically highlights "open" devices, making the ready status more visible to users.
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:45:27.712Z
Learning: In the GaggiMate codebase, WiFi credentials for HomeKit plugins are managed and validated by the WebUI before being passed to the HomeKit plugin, so additional validation in the plugin is unnecessary.
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:45:27.712Z
Learning: In the GaggiMate codebase (src/display/plugins/HomekitBridgePlugin.cpp), HomeSpan acts as a secondary, non-critical service that operates independently of core machine control. HomeSpan initialization failures do not affect boiler safety or operation.
📚 Learning: 2025-08-14T20:05:55.096Z
Learnt from: igadmile
Repo: jniebuhr/gaggimate PR: 360
File: web/src/pages/Home/ProcessControls.jsx:106-114
Timestamp: 2025-08-14T20:05:55.096Z
Learning: In React components that use WebSocket-based state updates, there can be race conditions when using useEffect to watch for state transitions. For example, when a flush is started, local state (`isFlushing`) is updated immediately, but WebSocket-driven state (`active`) is updated asynchronously after the message is received, creating timing issues if useEffect tries to act on both states simultaneously.
Applied to files:
web/src/pages/Settings/PluginCard.jsx
📚 Learning: 2025-12-19T22:16:46.304Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:53-59
Timestamp: 2025-12-19T22:16:46.304Z
Learning: In the GaggiMate HomeKit bridge plugin (src/display/plugins/HomekitBridgePlugin.cpp), the GaggiMateHeatingSensor intentionally maps stable (ready) state to ContactSensorState value 1 (Open) so that Apple Home displays the sensor prominently in the overview when the machine is ready. This is the correct behavior because Apple Home automatically highlights "open" devices, making the ready status more visible to users.
Applied to files:
web/src/pages/Settings/PluginCard.jsx
📚 Learning: 2025-12-18T12:45:27.712Z
Learnt from: modsmthng
Repo: jniebuhr/gaggimate PR: 527
File: src/display/plugins/HomekitBridgePlugin.cpp:69-120
Timestamp: 2025-12-18T12:45:27.712Z
Learning: In the GaggiMate codebase (src/display/plugins/HomekitBridgePlugin.cpp), HomeSpan acts as a secondary, non-critical service that operates independently of core machine control. HomeSpan initialization failures do not affect boiler safety or operation.
Applied to files:
web/src/pages/Settings/PluginCard.jsx
🧬 Code graph analysis (1)
web/src/pages/Settings/PluginCard.jsx (1)
web/src/pages/Settings/index.jsx (3)
formData(23-23)updateHomekitMode(172-177)onChange(105-160)
🔇 Additional comments (2)
web/src/pages/Settings/PluginCard.jsx (2)
201-261: LGTM!The accessory toggles are well-structured. The
!== falsepattern correctly defaults all accessories to enabled when entering Bridge mode, which is good UX for first-time setup. The labels and descriptions are clear.
268-335: Well-implemented troubleshooting section.The collapsible troubleshooting section using
<details>/<summary>is a clean approach that keeps advanced help available without cluttering the main UI. The image has appropriate alt text for accessibility.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/display/core/Controller.cpp (1)
45-62: Avoid double-creatingDefaultUI(leak + stale state).
DefaultUIis allocated at Line 47 and again at Lines 60-62. The first instance is leaked and any state/listeners it held are discarded. Keep a single initialization (prefer the later one if UI depends onProfileManagersetup).🛠️ Suggested fix (keep only the later init)
`#ifndef` GAGGIMATE_HEADLESS - ui = new DefaultUI(this, driver, pluginManager); if (driver->supportsSDCard() && driver->installSDCard()) { sdcard = true; ESP_LOGI(LOG_TAG, "SD Card detected and mounted"); ESP_LOGI(LOG_TAG, "Used: %lluMB, Capacity: %lluMB", SD_MMC.usedBytes() / 1024 / 1024, SD_MMC.cardSize() / 1024 / 1024); } `#endif`
🤖 Fix all issues with AI agents
In `@src/display/core/Controller.cpp`:
- Around line 64-72: The Bridge mode path currently omits registering the
mDNSPlugin, so when settings.getHomekitMode() == HOMEKIT_MODE_BRIDGE the device
won't be advertised via mDNS; update the control flow in Controller.cpp to
ensure pluginManager->registerPlugin(new mDNSPlugin()) is invoked for bridge
mode as well (either by registering mDNSPlugin unconditionally before/after the
HomekitBridgePlugin registration or by adding a registerPlugin call alongside
HomekitBridgePlugin), referencing the existing HomekitBridgePlugin and
mDNSPlugin instantiations to keep behavior consistent with the
thermostat/disabled branches.
|
1 participant
Thermostat Mode: Emulates a thermostat for direct temperature control (as before).
Bridge Mode: Creates multiple devices:
Sensor that reflects the heating status (red / heating = closed, green / ready = open) Switch for Standby - Brew
Switch for Brew - Steam
In Bridge mode, devices can be activated and deactivated individually.
An automatic migration logic for transferring existing Homekit plugin settings has been implemented.
When switching integrations, Apple Home automatically recognizes that the devices have been changed, removes the old ones, and adds the new ones. This allows you to switch between integrations without any problems and without devices appearing twice. Devices that are no longer initialized are automatically removed from Apple Home.
The UI has been adjusted accordingly and a fold-out troubleshooting section has been added:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.