Refactor: Move Door Sensor logic to HardwareManager and implement debouncing#225
Refactor: Move Door Sensor logic to HardwareManager and implement debouncing#225foylaou wants to merge 12 commits intorednblkx:mainfrom
Conversation
2
3 實作內容：
4 - 後端 (C++)：
5 - 在 `misc_config_t` 配置結構中新增 `doorSensorPin` 欄位（預設為 GPIO 32）。
6 - 在 `LockMechanismService` 中加入 `CurrentDoorState` 特徵 (Characteristic)。
7 - 重寫 `LockMechanismService::loop()` 以輪詢 GPIO 狀態，支援即時同步門位至 HomeKit。
8 - 邏輯實作：GPIO LOW 為關門 (Secured)，HIGH 為開門 (Unsecured)。
9 - 確保 `doorSensorPin` 可透過 `ConfigManager` 持久化儲存於 NVS。
10
11 - 前端 (Web UI)：
12 - 在 `api.ts` 中更新 `MiscConfig` 的類型定義。
13 - 在「系統設定 (System Settings)」區塊新增「Door Sensor GPIO Pin」輸入欄位。
14 - 重新編譯前端靜態資源並執行檔案壓縮優化（預壓縮以節省 Flash 空間）。
15
16 - 硬體配置：
17 - 預設使用 GPIO 32 作為感應器輸入，並啟用內部上拉電阻 (INPUT_PULLUP)。
2
3 實作內容：
4 - 後端 (C++)：
5 - 在 `misc_config_t` 配置結構中新增 `doorSensorPin` 欄位（預設為 GPIO 32）。
6 - 在 `LockMechanismService` 中加入 `CurrentDoorState` 特徵 (Characteristic)。
7 - 重寫 `LockMechanismService::loop()` 以輪詢 GPIO 狀態，支援即時同步門位至 HomeKit。
8 - 邏輯實作：GPIO LOW 為關門 (Secured)，HIGH 為開門 (Unsecured)。
9 - 確保 `doorSensorPin` 可透過 `ConfigManager` 持久化儲存於 NVS。
10
11 - 前端 (Web UI)：
12 - 在 `api.ts` 中更新 `MiscConfig` 的類型定義。
13 - 在「系統設定 (System Settings)」區塊新增「Door Sensor GPIO Pin」輸入欄位。
14 - 重新編譯前端靜態資源並執行檔案壓縮優化（預壓縮以節省 Flash 空間）。
15
16 - 硬體配置：
17 - 預設使用 GPIO 32 作為感應器輸入，並啟用內部上拉電阻 (INPUT_PULLUP)。
…nvert is missing. This means the invert setting won't be persisted to NVS or properly serialized/deserialized, causing the setting to be lost on device reboot.
- Relocate door sensor GPIO interaction to HardwareManager as requested. - Implement 50ms debouncing for door sensor state changes. - Remove unrelated documentation files added in previous commits. - Refactor LockMechanismService to use HardwareManager for state updates. - Organize Door Sensor settings within the System Settings UI section.
# Conflicts: # dependencies.lock # main/CMakeLists.txt
- Relocate door sensor GPIO interaction to HardwareManager as requested. - Implement 50ms debouncing for door sensor state changes. - Remove unrelated documentation files added in previous commits. - Refactor LockMechanismService to use HardwareManager for state updates. - Organize Door Sensor settings within the System Settings UI section.
# Conflicts: # dependencies.lock
WalkthroughAdds door-sensor support end-to-end: new misc config fields and UI, HardwareManager GPIO setup and debounce checkDoorSensor(), wiring HardwareManager into HomeKitLock/LockMechanismService, and publishing door state via a new characteristic polled from LockMechanismService. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ConfigMgr as ConfigManager
participant HW as HardwareManager
participant LockSvc as LockMechanismService
participant HomeKit as HomeKit Accessory
Note over ConfigMgr,HW: Startup wiring
ConfigMgr->>HW: new HardwareManager(misc_config_t, actions_config_t)
HW->>HW: begin() — configure doorSensorPin GPIO (INPUT_PULLUP) if set
Note over LockSvc,HW: Runtime polling loop
loop periodic
LockSvc->>HW: checkDoorSensor()
alt valid state change
HW-->>LockSvc: 1 (Closed) or 0 (Open)
LockSvc->>LockSvc: update m_doorState characteristic
LockSvc->>HomeKit: publish characteristic update
else no change / invalid
HW-->>LockSvc: -1
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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 (1)
main/include/HardwareManager.hpp (1)
101-102: Consider renaming for clarity.The naming is potentially confusing:
m_sysConfigholdsmisc_config_twhilem_miscConfigholdsactions_config_t. Consider renaming tom_miscConfigandm_actionsConfigrespectively for consistency with the type names.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
data/src/lib/components/AppMisc.sveltedata/src/lib/types/api.tsmain/ConfigManager.cppmain/HKServices.cppmain/HardwareManager.cppmain/HomeKitLock.cppmain/NfcManager.cppmain/WebServerManager.cppmain/include/HardwareManager.hppmain/include/HomeKitLock.hppmain/include/config.hppmain/main.cppsdkconfig.defaults
💤 Files with no reviewable changes (1)
- main/WebServerManager.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
main/ConfigManager.cpp (1)
main/include/ConfigManager.hpp (2)
key(76-76)key(77-77)
main/HomeKitLock.cpp (1)
main/include/HomeKitLock.hpp (1)
HomeKitLock(24-24)
🔇 Additional comments (26)
sdkconfig.defaults (1)
94-94: LGTM!The increased header length (4096 bytes) is reasonable for handling the expanded configuration payloads in this PR.
main/include/HardwareManager.hpp (3)
10-10: LGTM!The expanded forward declarations and constructor signature correctly support the new door sensor configuration via
misc_config_t.Also applies to: 26-26
39-43: LGTM!The return value semantics (-1 for no change/invalid) are clear and appropriate for debounced state polling.
106-109: Debounce state variables and logic are sound.The 50ms debounce delay is appropriate for mechanical door sensors. The implementation in
HardwareManager.cppusesesp_timer_get_time()(uint64_t) for time arithmetic rather thanmillis(), which safely avoids wrap-around issues. The code is correct as-is.data/src/lib/types/api.ts (1)
100-103: LGTM!The new
doorSensorPinanddoorSensorInvertfields are correctly typed and well-documented, consistent with the existing interface patterns.main/NfcManager.cpp (1)
108-108: LGTM!The format specifier change from
%dto%uwith explicit casts is correct for unsigned firmware version values.main/include/config.hpp (3)
7-7: LGTM!Using
esp_log.his the standard include for ESP-IDF logging.
128-129: LGTM!GPIO32 is a sensible default (ADC1 channel, widely available on ESP32 boards), and the default inversion logic aligns with typical reed switch behavior.
137-137: LGTM!Changing the default log level to
ESP_LOG_INFOprovides better visibility for debugging while still being configurable.data/src/lib/components/AppMisc.svelte (2)
44-45: LGTM!Default values for
doorSensorPinanddoorSensorInvertare consistent with the C++ configuration defaults.
626-647: LGTM!The invert logic toggle with clear explanatory text provides good UX for configuring door sensor polarity.
main/ConfigManager.cpp (4)
113-114: LGTM!The new config map entries correctly enable persistence and JSON handling for the door sensor configuration.
199-200: LGTM!Using
%zuforsize_tvalues is the correct portable format specifier.
577-577: LGTM!The updated format specifiers correctly match the types being logged.
1082-1082: LGTM!Logging the actual missing key name improves debuggability.
main/HardwareManager.cpp (3)
30-36: LGTM!The constructor correctly accepts both configuration structs and initializes member variables in the initializer list. The dependency injection pattern is clean.
141-143: LGTM!The door sensor GPIO initialization is correctly guarded and uses
INPUT_PULLUP, which is appropriate for typical magnetic door sensors.
245-264: The member variablesm_lastDoorState,m_lastRawDoorState, andm_lastDoorChangeTimeare properly initialized in the header file. Bothm_lastDoorStateandm_lastRawDoorStateare initialized to-1, which is the optimal approach for this use case. This ensures the first stable reading is always reported correctly, as the initial state will differ from the -1 sentinel value. The debounce implementation is sound with no initialization concerns.main/main.cpp (2)
5-6: Verify that disabling stdio locking is safe for this application.These empty stubs for
flockfile/funlockfiledisable thread-safety for stdio operations. This is a common workaround when libraries expect POSIX locking that ESP-IDF doesn't provide. However, if multiple tasks write to the sameFILE*(e.g.,stdoutviaprintf), output could interleave.Given this is an embedded application where logging typically goes through ESP-IDF's
ESP_LOG*macros (which have their own synchronization), this is likely acceptable.
66-69: LGTM!The dependency injection is correctly wired.
HardwareManageris constructed with both configuration structs before being passed toHomeKitLock.main/HomeKitLock.cpp (2)
33-38: LGTM!The constructor correctly accepts and initializes the
HardwareManagerreference in the member initializer list.
232-232: LGTM!
LockMechanismServiceis correctly constructed with theHardwareManagerreference, enabling door sensor integration.main/HKServices.cpp (2)
99-116: LGTM!The constructor correctly:
- Stores the
HardwareManagerreference- Creates the
CurrentDoorStatecharacteristic with a sensible default (1 = Closed)- Maintains the existing lock state initialization and event publishing
140-145: LGTM!The
loop()implementation correctly polls the door sensor throughHardwareManagerand only updates the HomeKit characteristic when a valid (debounced) state change is detected. This prevents notification storms as intended by the PR.main/include/HomeKitLock.hpp (2)
19-19: LGTM!Forward declaration correctly avoids including the full
HardwareManager.hppheader, keeping compilation dependencies minimal.
59-69: LGTM!The
LockMechanismServicestruct is properly extended with:
HardwareManager&reference member for hardware accessm_doorStatecharacteristic pointer for HomeKit door stateloop()override for periodic pollingThe declarations are consistent with the implementation in
HKServices.cpp.
…esent on other GPIO pin inputs in this file. This could allow invalid GPIO values.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @main/HKServices.cpp:
- Around line 104-106: The m_doorState characteristic is created but not exposed
to the bridge; update the creation to assign it to both the service member and
the bridge (use the same pattern as m_lockCurrentState/m_lockTargetState), i.e.
set m_doorState and bridge.m_doorState to the new
Characteristic::CurrentDoorState(1) so the door state is registered with HomeKit
clients.
🧹 Nitpick comments (1)
main/main.cpp (1)
2-2: Include appears appropriate but may be redundant.The
<cstdio>include is appropriate for thesprintfcall on line 33. However, verify whethersprintfis already available transitively through other headers to avoid redundant includes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
data/src/lib/components/AppMisc.sveltedata/src/lib/types/api.tsmain/ConfigManager.cppmain/HKServices.cppmain/HomeKitLock.cppmain/NfcManager.cppmain/WebServerManager.cppmain/include/config.hppmain/main.cpp
💤 Files with no reviewable changes (1)
- main/WebServerManager.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- data/src/lib/types/api.ts
🧰 Additional context used
🧬 Code graph analysis (3)
main/ConfigManager.cpp (1)
main/include/ConfigManager.hpp (2)
key(76-76)key(77-77)
main/main.cpp (1)
main/include/WebServerManager.hpp (2)
mqttManager(54-54)mqttManager(54-54)
main/HomeKitLock.cpp (1)
main/include/HomeKitLock.hpp (1)
HomeKitLock(24-24)
🔇 Additional comments (15)
main/NfcManager.cpp (1)
291-291: LGTM: Improved logging format for unsigned values.The change from
%dto%uwith explicit casts tounsigned intcorrectly aligns the format specifiers with the unsigned nature of the firmware version components extracted via bit shifts.data/src/lib/components/AppMisc.svelte (2)
45-46: LGTM: Door sensor configuration properly initialized.The default values for
doorSensorPin(32) anddoorSensorInvert(false) align with the backend configuration inmain/include/config.hpp.
622-665: LGTM: Door Sensor UI section properly implemented.The new Door Sensor configuration section follows the established UI patterns and includes:
- GPIO pin input with proper validation (min="0", max="255") as confirmed by past review
- Clear invert logic toggle with explanatory text
- Consistent styling and layout with other sections
main/ConfigManager.cpp (4)
114-115: LGTM: Door sensor configuration keys properly registered.The
doorSensorPinanddoorSensorInvertkeys are correctly mapped to their correspondingm_miscConfigfields, enabling serialization and deserialization through the existing config infrastructure.
200-200: LGTM: Correct format specifier for size_t.Changing from
%luto%zuis the proper format specifier forsize_ttypes used by NVS statistics, ensuring portability across different architectures.
578-578: LGTM: Improved format specifier for uint64_t.Using
%lluforv.val.via.u64correctly handles the 64-bit unsigned integer, replacing the potentially incorrect%d.
1083-1083: LGTM: Enhanced error logging with actual key name.Now logs the actual key string that couldn't be found, significantly improving debuggability compared to the previous empty placeholder.
main/HKServices.cpp (3)
6-6: LGTM: HardwareManager dependency properly included.The include is necessary for the new door sensor integration in
LockMechanismService.
100-100: LGTM: Constructor properly accepts HardwareManager reference.The
HardwareManager¶meter is correctly added to the constructor signature and stored inm_hardwareManager, enabling door sensor access in the service.
141-146: LGTM: Door sensor polling properly implemented with error handling.The
loop()method correctly:
- Polls the hardware manager for door state
- Only updates the characteristic when a valid reading is obtained (doorState != -1)
- Avoids unnecessary HomeKit notifications for invalid/debouncing states
main/include/config.hpp (3)
7-7: LGTM: Updated to use the main ESP logging header.The change from
esp_log_level.htoesp_log.his appropriate, asesp_log.his the main ESP-IDF logging header that includes all necessary logging functionality.
129-130: LGTM: Door sensor configuration fields properly defined.The new fields have sensible defaults:
doorSensorPin = 32: A valid, commonly available GPIO pin on ESP32doorSensorInvert = false: Standard logic (LOW = Closed, HIGH = Open)These align with the UI defaults in
AppMisc.svelteand are properly integrated into the configuration infrastructure.
138-138: The default log level change from ERROR to INFO is fully configurable via the web UI and already documented.The change from
ESP_LOG_ERRORtoESP_LOG_INFOis confirmed and part of an intentional logging infrastructure overhaul. This is not a breaking change because:
- Fully configurable at runtime: Users can change the log level to any level (ERROR, WARN, INFO, DEBUG, VERBOSE) through the web interface's Logs section without restarting
- Already documented: Configuration documentation (section 5.8 "Logs") explicitly lists all available log levels and how to configure them
- Intentional design decision: The change reflects an explicit effort to improve logging visibility for better debugging (as evidenced by related commits "Increase log verbosity and adjust log timestamp generation" and "set boot log level")
The implementation already addresses all original concerns about performance impact, user experience, and backward compatibility through full runtime configurability.
main/HomeKitLock.cpp (1)
33-33: LGTM! Clean dependency injection.The changes correctly thread the
HardwareManagerdependency throughHomeKitLockintoLockMechanismService:
- Constructor signature properly extended with
HardwareManager¶meter- Member initialization follows the correct pattern
- Service construction passes the reference downstream
This aligns well with the PR's architectural goal of centralizing hardware logic in
HardwareManager.Also applies to: 37-37, 232-232
main/main.cpp (1)
63-63: No initialization order issue: GPIO is initialized before door sensor access occurs.The concern is based on a misunderstanding of when door sensor access happens. LockMechanismService constructor (line 105) only sets a default door state value—it does NOT read the GPIO. Door sensor reading only occurs in
LockMechanismService::loop()(line 142 in HKServices.cpp), which is called by the HomeSpan framework during the main application loop, not duringsetup().Since
setup()completes before the main loop begins,hardwareManager->begin()(line 82) initializes GPIO before anyloop()methods execute. Therefore, GPIO is properly initialized whencheckDoorSensor()(line 248 in HardwareManager.cpp) performs itsdigitalRead()operation.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @main/HKServices.cpp:
- Around line 104-106: There is a typo using "brodge" instead of the correct
"bridge" when assigning m_doorState; update the assignment so it reads
m_doorState = bridge.m_doorState = new Characteristic::CurrentDoorState(1)
(using the existing bridge variable and the Characteristic::CurrentDoorState
constructor) to match surrounding code and allow compilation.
- Around line 141-146: The loop() method
(HomeKitLock::LockMechanismService::loop()) calls m_doorState->setVal() without
HomeSpan synchronization and can race with HomeSpan event processing; wrap the
setVal() call in HomeSpan mutex protection (either call homeSpanPAUSE() before
and homeSpanRESUME() after, or acquire/release the HomeSpan mutex via
getMutex()) to serialize access; apply the same protection pattern to other
characteristic writes such as updateLockState() and updateBatteryStatus() where
setVal() is called.
🧹 Nitpick comments (1)
main/HKServices.cpp (1)
141-146: Consider adding logging for door state changes.The loop() method silently updates the door state without logging. Adding debug logs when the state changes would improve observability and help diagnose issues with the door sensor or debouncing logic.
💡 Suggested enhancement
void HomeKitLock::LockMechanismService::loop() { int doorState = m_hardwareManager.checkDoorSensor(); if (doorState != -1) { + int currentState = m_doorState->getVal(); + if (currentState != doorState) { + ESP_LOGD(HomeKitLock::TAG, "Door state changed: %d -> %d", currentState, doorState); + } m_doorState->setVal(doorState); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main/HKServices.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
main/HKServices.cpp (1)
main/include/HomeKitLock.hpp (1)
LockMechanismService(66-66)
🔇 Additional comments (2)
main/HKServices.cpp (2)
6-6: LGTM!The include is necessary for the HardwareManager dependency injection and is properly placed.
100-100: LGTM!The dependency injection pattern correctly integrates HardwareManager into the LockMechanismService, improving architectural separation as intended.
- Wrap setVal() calls with homeSpanPAUSE()/homeSpanRESUME() to prevent race conditions with HomeSpan event processing - Fix incorrect variable naming in HKServices.cpp 105
|
Hi! sorry for the delay. i've a couple key points i want to touch upon:
That being said, this needs some more thought, i'll see if i have some time for it next week to clean it up a bit unless you manage to make the changes before that. Let me know if i can help with anything. |
2 participants
Overview
This PR addresses the feedback received regarding the Door Sensor implementation. The core hardware logic has been moved to the HardwareManager to ensure better architectural separation, and debouncing has been implemented to prevent notification storms. Additionally, unrelated documentation files have
been removed to keep the repository clean.
Key Changes
Verification Results
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.