Add fan speed and oscillation feature#54
Conversation
Add fan speed and oscillation feature Portions of this code generated with assistance from DeepSeek AI.
WalkthroughAdds HomeKit Rotation Speed and Swing Mode characteristics to FanEntity, implements a batched write handler for On/Rotation Speed/Swing Mode, an identify handler, and an update callback to reflect device state into HomeKit; setup now conditionally creates and subscribes those characteristics. Changes
Sequence Diagram(s)sequenceDiagram
participant HKClient as HomeKit Client
participant HKService as HomeKit Service
participant FanEntity as FanEntity / Device
HKClient->>HKService: Write {On?, RotationSpeed?, SwingMode?}
activate HKService
HKService->>FanEntity: fanwrite(batch writes)
Note right of FanEntity `#E8F4FF`: accumulate changes into single call object\n(set flags: update_state/update_speed/update_oscillating)
alt any update flagged
FanEntity->>FanEntity: perform single batched call to device
end
deactivate HKService
Note over FanEntity,HKService: device state changes -> on_fanupdate
FanEntity->>HKService: update On (if characteristic exists)
FanEntity->>HKService: update Rotation Speed (if exists)
FanEntity->>HKService: update Swing Mode (if exists)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/homekit/fan.hpp(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/homekit/fan.hpp (3)
components/homekit/climate.hpp (6)
write_data(86-122)write_data(86-86)obj(21-47)obj(21-21)ha(123-126)ha(123-123)components/homekit/switch.hpp (6)
write_data(19-38)write_data(19-19)obj(39-49)obj(39-39)ha(50-53)ha(50-50)components/homekit/light.hpp (6)
write_data(19-88)write_data(19-19)obj(89-131)obj(89-89)ha(132-135)ha(132-132)
🔇 Additional comments (5)
components/homekit/fan.hpp (5)
25-28: Good batching pattern.Creating a single call object with flags to batch multiple characteristic updates follows the established pattern in this codebase (similar to climate.hpp).
32-38: LGTM!The On characteristic handler correctly batches the state change instead of immediately calling turn_on/turn_off, allowing multiple characteristics to be updated atomically.
53-61: LGTM!The Swing Mode characteristic handler correctly maps the integer value to a boolean and batches the oscillating update.
67-69: LGTM!The conditional perform call ensures all batched updates are applied atomically only when needed.
154-160: LGTM!The new characteristics are properly initialized with the current fan state and correctly added to the service.
fixed issues Portions of this code generated with assistance from DeepSeek AI.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @mikailofficial. * #54 (comment) The following files were modified: * `components/homekit/fan.hpp`
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/homekit/fan.hpp (1)
50-62: Speed conversion logic fixed correctly.The previous issues with no-op multiplication and minimum clamping have been resolved. The direct mapping
static_cast<int>(speed_percentage)now correctly handles the full range 0–100, allowing 0% to turn off the fan as required by the ESPHome API.Optional refinement: Consider using
std::roundorstatic_cast<int>(speed_percentage + 0.5f)instead of truncation to handle fractional percentages more accurately (e.g., 99.9% → 100 instead of 99). However, since HomeKit typically sends integer percentages, the practical impact is minimal.Optional: Apply rounding instead of truncation
- int speed_level = static_cast<int>(speed_percentage); + int speed_level = static_cast<int>(std::round(speed_percentage));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/homekit/fan.hpp(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/homekit/fan.hpp (3)
components/homekit/climate.hpp (6)
write_data(86-122)write_data(86-86)obj(21-47)obj(21-21)ha(123-126)ha(123-123)components/homekit/light.hpp (7)
write_data(19-88)write_data(19-19)obj(89-131)obj(89-89)ha(132-135)ha(132-132)acc_cfg(138-196)components/homekit/switch.hpp (7)
write_data(19-38)write_data(19-19)obj(39-49)obj(39-39)ha(50-53)ha(50-50)acc_cfg(56-100)
🔇 Additional comments (6)
components/homekit/fan.hpp (6)
20-30: LGTM! Excellent documentation.The docstrings are comprehensive, well-structured, and clearly explain the purpose, parameters, and behavior of each method. They follow Doxygen conventions and will be helpful for future maintainers.
Also applies to: 83-92, 132-139, 146-154, 157-166
36-39: LGTM! Efficient batched update pattern.The batched update approach correctly consolidates multiple HomeKit characteristic writes into a single
call.perform()invocation, avoiding redundant state changes. This follows the established pattern in the codebase and is the recommended approach for handling simultaneous property updates.Also applies to: 77-79
103-128: LGTM! Proper defensive null checks.The nested null checks for the service and each characteristic prevent crashes if any HomeKit components are missing. This defensive approach is particularly important if capability checks are added (per previous comment) to conditionally create characteristics.
221-222: LGTM! Callback registration is correct.The state callback registration correctly subscribes to fan state updates and propagates them to HomeKit. The check for
!fanPtr->is_internal()appropriately prevents callback registration for internal entities.
50-62: LGTM! Bidirectional mapping is consistent.The characteristic value conversions are symmetric between the write path (fanwrite) and read path (on_fanupdate):
- Speed:
float↔intconversions match- Swing Mode:
bool↔0/1 intconversions match- On state: Boolean handling is consistent
This ensures HomeKit reflects the same values that were written.
Also applies to: 113-119
31-81: Overall implementation is solid and follows established patterns.The implementation correctly:
- Batches multiple HomeKit writes into efficient single calls
- Reflects ESPHome state changes back to HomeKit
- Uses defensive null checks to prevent crashes
- Follows the architectural patterns established in light.hpp, switch.hpp, and climate.hpp
- Provides comprehensive documentation
The main recommended improvement is adding capability checks before creating optional characteristics (as noted in the earlier comment).
Also applies to: 93-130, 167-225
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/homekit/fan.hpp (1)
31-91: Excellent batched write implementation!The write handler correctly:
- Batches all writes into a single fan call
- Checks capabilities before processing speed and oscillation writes
- Returns
HAP_STATUS_RES_ABSENTfor unsupported features- Only performs the call when updates are queued
This addresses the critical issues from the previous review.
Optional precision improvement:
Line 57 truncates the speed percentage when casting from float to int (e.g., 50.9% becomes 50). For better accuracy, consider rounding:
- int speed_level = static_cast<int>(speed_percentage); + int speed_level = static_cast<int>(std::round(speed_percentage));This is a minor refinement since HomeKit typically sends whole numbers, but it ensures more accurate handling of edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/homekit/fan.hpp(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/homekit/fan.hpp (3)
components/homekit/climate.hpp (6)
write_data(86-122)write_data(86-86)obj(21-47)obj(21-21)ha(123-126)ha(123-123)components/homekit/switch.hpp (7)
write_data(19-38)write_data(19-19)obj(39-49)obj(39-39)ha(50-53)ha(50-50)acc_cfg(56-100)components/homekit/light.hpp (7)
write_data(19-88)write_data(19-19)obj(89-131)obj(89-89)ha(132-135)ha(132-132)acc_cfg(138-196)
🔇 Additional comments (3)
components/homekit/fan.hpp (3)
103-144: LGTM - State synchronization is well-implemented!The callback correctly:
- Provides detailed logging of fan state changes
- Uses defensive null checks for accessory, service, and characteristics
- Conditionally updates speed and oscillation characteristics only when the fan supports those features
- Performs direct 0-100 mapping between ESPHome speed and HomeKit percentage, which is now consistent with the write path
This properly addresses the previous review comments about conversion inconsistencies.
154-157: LGTM!Standard identify handler implementation, consistent with other HomeKit entity types in the codebase.
181-248: Excellent work addressing the previous review feedback!The setup method now properly:
- Checks
fanPtr->get_traits().supports_speed()before adding the Rotation Speed characteristic (lines 215-220)- Checks
fanPtr->get_traits().supports_oscillation()before adding the Swing Mode characteristic (lines 222-227)- Provides detailed logging showing which features are enabled (lines 244-247)
This critical fix ensures HomeKit only exposes controls that the underlying ESPHome fan actually supports, preventing users from seeing non-functional controls. The implementation now follows the same defensive pattern used in
light.hppfor conditional characteristic creation.Based on learnings from the previous review.
rednblkx
force-pushed
the
main
branch
6 times, most recently
from
4e3bba5 to
e37b956
Compare
1 participant
Add fan speed and oscillation feature
Portions of this code generated with assistance from DeepSeek AI.
Summary by CodeRabbit
New Features
Enhancements