📝 Add docstrings to Add-fan-speed-and-oscillation-feature#55
📝 Add docstrings to Add-fan-speed-and-oscillation-feature#55coderabbitai[bot] wants to merge 1 commit into
Add-fan-speed-and-oscillation-feature#55Conversation
Docstrings generation was requested by @mikailofficial. * #54 (comment) The following files were modified: * `components/homekit/fan.hpp`
WalkthroughThe change enhances HomeKit fan integration by introducing batched write handling for On, Rotation Speed, and Swing Mode states, expanding state reflection from ESPHome to HomeKit with detailed logging, exposing additional characteristics during setup, and making the FanEntity constructor public for external instantiation. Changes
Sequence Diagram(s)sequenceDiagram
participant HK as HomeKit
participant CB as Batched<br/>Write Handler
participant Fan as ESPHome Fan
participant State as HomeKit State
HK->>CB: Write On/Speed/Swing
activate CB
CB->>CB: Queue write<br/>Update persisted status
CB->>State: Propagate state<br/>(immediate)
alt Any state changed?
CB->>Fan: Single fan() call
Fan->>Fan: Update fan state
end
deactivate CB
Fan->>CB: on_fanupdate event
activate CB
CB->>State: Update On, Speed,<br/>Swing Mode
CB->>CB: Log enhanced state<br/>with speed & oscillation
deactivate CB
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/homekit/fan.hpp(3 hunks)
🔇 Additional comments (6)
components/homekit/fan.hpp (6)
19-30: Well-documented function interface.The docstring comprehensively describes the batched write handling mechanism, parameters, and return value.
82-92: Clear and comprehensive documentation.The docstring accurately describes the state reflection mechanism and the characteristic mappings.
93-130: Robust state synchronization with proper null-safety.The function correctly reflects ESPHome fan state to HomeKit with appropriate null checks for the accessory, service, and characteristics. The bidirectional mappings for speed and oscillation are consistent.
131-143: Well-documented identify handler.The docstring clearly describes the identify functionality.
145-155: Good documentation for the now-public constructor.The docstring clearly explains the constructor's purpose and includes important notes about the lifetime requirements of the
fanPtrparameter.
156-225: Comprehensive setup documentation and correct implementation.The docstring thoroughly describes the accessory creation, characteristic registration, and callback setup. The implementation correctly exposes Rotation Speed and Swing Mode characteristics and properly wires up the write and state callbacks.
| if (!strcmp(hap_char_get_type_uuid(write->hc), HAP_CHAR_UUID_ON)) { | ||
| ESP_LOGD(TAG, "Received Write for fan '%s' -> %s", fanPtr->get_name().c_str(), write->val.b ? "On" : "Off"); | ||
| ESP_LOGD(TAG, "[STATE] CURRENT STATE: %d", fanPtr->state); | ||
| fanPtr->make_call().set_state(write->val.b).perform(); | ||
| call.set_state(write->val.b); | ||
| update_state = true; | ||
| hap_char_update_val(write->hc, &(write->val)); | ||
| *(write->status) = HAP_STATUS_SUCCESS; | ||
| } | ||
| else if (!strcmp(hap_char_get_type_uuid(write->hc), HAP_CHAR_UUID_ROTATION_SPEED)) { | ||
| float speed_percentage = write->val.f; | ||
| ESP_LOGD(TAG, "Received Write for fan '%s' speed -> %.1f%%", fanPtr->get_name().c_str(), speed_percentage); | ||
|
|
||
| // Direct mapping: HomeKit percentage (0-100) to ESPHome speed (0-100) | ||
| int speed_level = static_cast<int>(speed_percentage); | ||
|
|
||
| ESP_LOGD(TAG, "Setting fan speed to level: %d", speed_level); | ||
| call.set_speed(speed_level); | ||
| update_speed = true; | ||
| hap_char_update_val(write->hc, &(write->val)); | ||
| *(write->status) = HAP_STATUS_SUCCESS; | ||
| } | ||
| else if (!strcmp(hap_char_get_type_uuid(write->hc), HAP_CHAR_UUID_SWING_MODE)) { | ||
| bool swing_mode = write->val.i; | ||
| ESP_LOGD(TAG, "Received Write for fan '%s' oscillation -> %s", fanPtr->get_name().c_str(), swing_mode ? "On" : "Off"); | ||
|
|
||
| call.set_oscillating(swing_mode); | ||
| update_oscillating = true; | ||
| hap_char_update_val(write->hc, &(write->val)); | ||
| *(write->status) = HAP_STATUS_SUCCESS; | ||
| } | ||
| else { | ||
| *(write->status) = HAP_STATUS_RES_ABSENT; | ||
| } | ||
| } | ||
|
|
||
| if (update_state || update_speed || update_oscillating) { | ||
| call.perform(); | ||
| } |
There was a problem hiding this comment.
State consistency issue: HomeKit characteristics updated before fan call is performed.
The code updates HomeKit characteristic values immediately via hap_char_update_val (lines 47, 60, 69) before the actual fan call is performed at line 78. If call.perform() fails or throws an exception, HomeKit will reflect values that don't match the actual fan state, leading to user confusion and potential integration issues.
Additionally, there's no error handling for call.perform(), and the function always returns HAP_SUCCESS regardless of whether the fan call succeeded.
Consider this approach:
- Perform the fan call first
- Only update HomeKit characteristics if the call succeeds
- Return appropriate error status if the call fails
}
- if (update_state || update_speed || update_oscillating) {
- call.perform();
+ // Perform the fan call first, then update HomeKit characteristics only if successful
+ if (update_state || update_speed || update_oscillating) {
+ call.perform();
+
+ // Re-update characteristics after successful perform to ensure consistency
+ // (in case perform modifies values or fails partially)
+ for (i = 0; i < count; i++) {
+ write = &write_data[i];
+ if (!strcmp(hap_char_get_type_uuid(write->hc), HAP_CHAR_UUID_ON)) {
+ hap_val_t val;
+ val.b = fanPtr->state;
+ hap_char_update_val(write->hc, &val);
+ }
+ else if (!strcmp(hap_char_get_type_uuid(write->hc), HAP_CHAR_UUID_ROTATION_SPEED)) {
+ hap_val_t val;
+ val.f = static_cast<float>(fanPtr->speed);
+ hap_char_update_val(write->hc, &val);
+ }
+ else if (!strcmp(hap_char_get_type_uuid(write->hc), HAP_CHAR_UUID_SWING_MODE)) {
+ hap_val_t val;
+ val.i = fanPtr->oscillating ? 1 : 0;
+ hap_char_update_val(write->hc, &val);
+ }
+ }
}
return ret;Alternatively, if you prefer to keep the current structure, wrap call.perform() in a try-catch and revert the HomeKit characteristic values if it fails.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/homekit/fan.hpp around lines 43 to 79, the HomeKit characteristics
are updated (hap_char_update_val) before the fan action is performed which can
leave HomeKit out of sync if call.perform() fails; change the flow to perform
the fan call first, capture its success/failure (and catch exceptions if
call.perform() can throw), and only call hap_char_update_val and set
*(write->status) = HAP_STATUS_SUCCESS when the call succeeds; if the call fails,
set *(write->status) to an appropriate HAP error code, avoid updating the
characteristic (or revert any optimistic updates), and log the error so the
HomeKit state remains consistent with the device.
| else if (!strcmp(hap_char_get_type_uuid(write->hc), HAP_CHAR_UUID_ROTATION_SPEED)) { | ||
| float speed_percentage = write->val.f; | ||
| ESP_LOGD(TAG, "Received Write for fan '%s' speed -> %.1f%%", fanPtr->get_name().c_str(), speed_percentage); | ||
|
|
||
| // Direct mapping: HomeKit percentage (0-100) to ESPHome speed (0-100) | ||
| int speed_level = static_cast<int>(speed_percentage); | ||
|
|
||
| ESP_LOGD(TAG, "Setting fan speed to level: %d", speed_level); | ||
| call.set_speed(speed_level); | ||
| update_speed = true; | ||
| hap_char_update_val(write->hc, &(write->val)); | ||
| *(write->status) = HAP_STATUS_SUCCESS; | ||
| } |
There was a problem hiding this comment.
Clarify type conversion in the "Direct mapping" comment.
The comment at line 54 states "Direct mapping" but line 55 casts the float to int, which truncates any decimal values. While this is likely intentional, the comment should clarify this rounding/truncation behavior to avoid confusion.
Apply this diff to clarify the comment:
- // Direct mapping: HomeKit percentage (0-100) to ESPHome speed (0-100)
+ // Map HomeKit percentage (0-100 float) to ESPHome speed (0-100 int, truncated)
int speed_level = static_cast<int>(speed_percentage);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else if (!strcmp(hap_char_get_type_uuid(write->hc), HAP_CHAR_UUID_ROTATION_SPEED)) { | |
| float speed_percentage = write->val.f; | |
| ESP_LOGD(TAG, "Received Write for fan '%s' speed -> %.1f%%", fanPtr->get_name().c_str(), speed_percentage); | |
| // Direct mapping: HomeKit percentage (0-100) to ESPHome speed (0-100) | |
| int speed_level = static_cast<int>(speed_percentage); | |
| ESP_LOGD(TAG, "Setting fan speed to level: %d", speed_level); | |
| call.set_speed(speed_level); | |
| update_speed = true; | |
| hap_char_update_val(write->hc, &(write->val)); | |
| *(write->status) = HAP_STATUS_SUCCESS; | |
| } | |
| else if (!strcmp(hap_char_get_type_uuid(write->hc), HAP_CHAR_UUID_ROTATION_SPEED)) { | |
| float speed_percentage = write->val.f; | |
| ESP_LOGD(TAG, "Received Write for fan '%s' speed -> %.1f%%", fanPtr->get_name().c_str(), speed_percentage); | |
| // Map HomeKit percentage (0-100 float) to ESPHome speed (0-100 int, truncated) | |
| int speed_level = static_cast<int>(speed_percentage); | |
| ESP_LOGD(TAG, "Setting fan speed to level: %d", speed_level); | |
| call.set_speed(speed_level); | |
| update_speed = true; | |
| hap_char_update_val(write->hc, &(write->val)); | |
| *(write->status) = HAP_STATUS_SUCCESS; | |
| } |
🤖 Prompt for AI Agents
In components/homekit/fan.hpp around lines 50 to 62, update the inline comment
on the mapping at line ~54 to explicitly state that the float -> int conversion
truncates the fractional part (via static_cast<int>) rather than rounding;
replace the ambiguous "Direct mapping" comment with one that notes the 0-100
range mapping and that decimals are truncated, and optionally mention that if
rounding is desired the code should use a rounding function (e.g., std::round or
std::lround) before casting.
rednblkx
force-pushed
the
main
branch
6 times, most recently
from
4e3bba5 to
e37b956
Compare
1 participant
Docstrings generation was requested by @mikailofficial.
The following files were modified:
components/homekit/fan.hppℹ️ Note
Summary by CodeRabbit