Momentary state improvement #146
Conversation
|
Warning Rate limit exceeded@rednblkx has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve modifications to the HTML form for configuring hardware actions, updates to enumerations and macros in the configuration header, and restructuring of lock management logic in the main application code. The HTML form now includes new fields for Home Key controlled state and momentary states, while the configuration file introduces new enumerations for lock states and GPIO momentary states. The main application code refines the handling of GPIO actions and MQTT message processing, improving overall clarity and functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HomeApp
participant GPIOController
participant LockManager
User->>HomeApp: Trigger action
HomeApp->>GPIOController: Send action command
GPIOController->>LockManager: Process GPIO action
LockManager->>GPIOController: Confirm action
GPIOController->>HomeApp: Acknowledge action
HomeApp->>User: Notify action completion
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
src/config.h (1)
35-41: LGTM! Consider adding documentation.The implementation is well-structured:
- Uses enum class for type safety
- Properly implements bit flags for combinable states
- Explicit uint8_t type ensures consistent size
Consider adding documentation to explain:
- The purpose of each state
- When to use combined states
- The relationship with HomeKit Triggers
+// Enum controlling GPIO momentary state behavior: +// - M_DISABLED: Momentary state is disabled +// - M_HOME: Momentary state controlled by Home +// - M_HK: Momentary state controlled by HomeKit +// - M_HOME_HK: Momentary state controlled by both Home and HomeKit enum class gpioMomentaryStateStatus : uint8_tdata/actions.html (2)
128-131: Consider revising the section header and descriptions for clarity.The current header "HomeKit Triggers" might be misleading since this section handles both HomeKit and HomeKey triggers. Consider:
- Keeping "HomeKit/HomeKey Triggers" for accuracy
- Making the momentary state limitation more prominent, perhaps using bold text or a warning style
- <h3 style="margin-top: 0;text-align: center;">HomeKit Triggers</h3> + <h3 style="margin-top: 0;text-align: center;">HomeKit/HomeKey Triggers</h3> <h4 style="margin-top: 0;margin-bottom: .5rem;">Executed upon interaction in the Home app and in addition also on successful HomeKey Authentication</h4> <h4 style="margin-top: 0;">Follows "Always Lock/Unlock on HomeKey" option</h4> - <h4 style="margin-top: 0;">Momentary state is only available with LOCKED as initial state</h4> + <h4 style="margin-top: 0;color: #ff9800;"><strong>⚠️ Important:</strong> Momentary state is only available with LOCKED as initial state</h4>
159-167: Consider adding tooltips to explain momentary state options.While the options are clear, adding tooltips would help users understand the implications of each choice:
- Home App Only: Momentary state only triggered by Home app actions
- Home Key Only: Momentary state only triggered by HomeKey authentication
- Home App + Home Key: Momentary state triggered by both
<select name="gpio-a-momentary" id="gpio-a-momentary"> - <option value="0">Disabled</option> - <option value="1">Home App Only</option> - <option value="2">Home Key Only</option> - <option value="3">Home App + Home Key</option> + <option value="0" title="Momentary state will not be used">Disabled</option> + <option value="1" title="Momentary state only triggered by Home app actions">Home App Only</option> + <option value="2" title="Momentary state only triggered by HomeKey authentication">Home Key Only</option> + <option value="3" title="Momentary state triggered by both Home app and HomeKey">Home App + Home Key</option> </select>src/main.cpp (2)
Line range hint
1270-1274: Improve error handling for NFC initialization failureWhen initializing the NFC module:
if (!versiondata) { ESP_LOGE("NFC_SETUP", "Didn't find PN53x board"); } else { // Initialization code... }Consider adding proper error handling or retry mechanisms when the NFC module is not found. Failing silently or only logging an error without handling it might lead to unexpected behavior later in the code.
Line range hint
1321-1363: Refactor thenfc_thread_entryfunction to improve maintainabilityThe
nfc_thread_entryfunction is lengthy and handles multiple responsibilities, including NFC communication, authentication, GPIO control, and MQTT publishing. This makes the code hard to read and maintain.Consider refactoring this function by extracting specific functionalities into separate functions or classes. This will make the codebase more modular, easier to understand, and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
data/actions.html(3 hunks)src/config.h(3 hunks)src/main.cpp(24 hunks)
🔇 Additional comments (7)
src/config.h (2)
93-93: LGTM! Verify related GPIO configurations.
The macro change from boolean to enum improves flexibility in controlling momentary states, aligning with the PR objectives.
Let's verify the related GPIO configurations:
#!/bin/bash
# Check for GPIO pin configurations and their relationships
echo "=== GPIO Action Pin Configuration ==="
rg "GPIO_ACTION_" -A 1
# Check for any hardcoded values that should use the new enum
echo "=== Potential Hardcoded Momentary States ==="
rg "momentary.*state.*=.*[0-3]" --type cpp9-17: Consider consolidating or aligning lock state enumerations.
There are now two similar enumerations (lockStates and customLockStates) defining lock states. This could lead to confusion and potential bugs if the code assumes specific numeric values. Consider either:
- Consolidating these enums into a single enumeration
- Explicitly aligning the values between both enums
- Adding clear documentation about when to use each enum
Let's check for potential inconsistencies in usage:
data/actions.html (2)
152-158: LGTM! Clear and well-structured Home Key control toggle.
The new Home Key controlled state toggle provides a clear way to enable/disable HomeKey control of the GPIO state.
186-187: LGTM! Clean JavaScript initialization.
The initialization of the Home Key GPIO state follows the established pattern and is well-integrated with the existing code.
src/main.cpp (3)
210-210: Verify the logic of bitwise operations on status.source
In the conditions:
if (static_cast<uint8_t>(espConfig::miscConfig.gpioActionMomentaryEnabled) & status.source)and
if ((static_cast<uint8_t>(espConfig::miscConfig.gpioActionMomentaryEnabled) & status.source) && currentState == lockStates::LOCKED)a bitwise AND operation is performed between gpioActionMomentaryEnabled and status.source. Ensure that both variables are intended to be used as bitmasks and that their values correctly represent flags. If they are not meant to be bitmasks, this operation might not yield the expected results.
Also applies to: 241-241
1322-1323: Initialize gpioLockAction correctly to prevent undefined behavior
In the nfc_thread_entry function, you initialize gpioLockAction as follows:
const gpioLockAction action{ .source = gpioLockAction::HOMEKEY, .action = 0 };Ensure that the gpioLockAction struct is properly constructed and that the fields source and action are correctly set according to the intended logic. This prevents undefined behavior when the struct is used later.
Line range hint 1419-1424: Check for successful creation of queues
When creating queues:
gpio_led_handle = xQueueCreate(2, sizeof(uint8_t));
neopixel_handle = xQueueCreate(2, sizeof(uint8_t));
gpio_lock_handle = xQueueCreate(2, sizeof(gpioLockAction));Ensure that you check if the queues are successfully created by verifying that the handles are not nullptr. Failure to create a queue might lead to crashes when the handles are used later.
You can add checks like:
if (gpio_led_handle == nullptr) {
// Handle error
}
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Simple GPIOunder HomeKit TriggersSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor