Add base station functions #645
Conversation
WalkthroughThe changes in this pull request involve the introduction of new classes and modifications to existing classes within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (4)
deebot_client/commands/json/station_action.py (3)
5-5: Remove unused importAnyfromtypingThe
Anytype is imported but not used in the code. Removing it will clean up the codebase by eliminating unnecessary imports.Apply this diff to remove the unused import:
-from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING🧰 Tools
🪛 Ruff (0.8.0)
5-5:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
11-11: Remove unused importCODEfrom.constThe
CODEconstant is imported but not utilized in the file. It's best practice to remove unused imports to maintain code cleanliness.Apply this diff to remove the unused import:
-from .const import CODE🧰 Tools
🪛 Ruff (0.8.0)
11-11:
.const.CODEimported but unusedRemove unused import:
.const.CODE(F401)
14-14: Remove unused importEventBusfromdeebot_client.event_busThe
EventBusclass is imported for type checking but isn't used in this file. Eliminating unused imports reduces clutter.Apply this diff to remove the unused import:
if TYPE_CHECKING: - from deebot_client.event_bus import EventBus🧰 Tools
🪛 Ruff (0.8.0)
14-14:
deebot_client.event_bus.EventBusimported but unusedRemove unused import:
deebot_client.event_bus.EventBus(F401)
deebot_client/capabilities.py (1)
152-157: Consider adding validation for base station capabilitiesThe
CapabilityBaseStationclass might benefit from validation to ensure that the action capability is properly configured when events are enabled.@dataclass(frozen=True, kw_only=True) class CapabilityBaseStation: """Capabilities for base station.""" action: CapabilityBaseStationAction event: CapabilityEvent[BaseStationEvent] | None = None + + def __post_init__(self) -> None: + """Validate base station capabilities configuration.""" + if self.event and not isinstance(self.action, CapabilityBaseStationAction): + raise ValueError("Base station event requires action capability")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
deebot_client/capabilities.py(3 hunks)deebot_client/commands/json/__init__.py(3 hunks)deebot_client/commands/json/station_action.py(1 hunks)deebot_client/events/__init__.py(2 hunks)deebot_client/hardware/deebot/buom7k.py(1 hunks)deebot_client/messages/json/__init__.py(3 hunks)deebot_client/messages/json/base_station.py(1 hunks)tests/messages/test_get_messages.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
deebot_client/commands/json/station_action.py
5-5: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
11-11: .const.CODE imported but unused
Remove unused import: .const.CODE
(F401)
14-14: deebot_client.event_bus.EventBus imported but unused
Remove unused import: deebot_client.event_bus.EventBus
(F401)
🔇 Additional comments (12)
deebot_client/hardware/deebot/buom7k.py (2)
161-162: Verify compatibility of MultimapStateEvent with SetMultimapState
In the multi_state capability of the map section, ensure that the SetMultimapState command is compatible with the MultimapStateEvent. Verify that enabling or disabling multimap functionality works as intended on the device.
171-175: Confirm device support for CarpetAutoFanBoost settings
The carpet_auto_fan_boost setting is implemented using CapabilitySetEnable. Please confirm that the Deebot N20 Plus supports this feature and that the commands GetCarpetAutoFanBoost and SetCarpetAutoFanBoost function correctly.
deebot_client/messages/json/__init__.py (1)
10-10: Addition of OnStationState is consistent and correct
Including OnStationState in the imports, __all__, and _MESSAGES lists properly integrates the new message type into the message handling system, ensuring it will be recognized and processed appropriately.
Also applies to: 19-19, 28-29
tests/messages/test_get_messages.py (1)
10-10: Ensure the test case for onStationState is functioning correctly
Adding ("onStationState", DataType.JSON, OnStationState) to the test parameters extends coverage to the new message type. Verify that the test properly checks message retrieval for OnStationState.
Also applies to: 22-22
deebot_client/events/__init__.py (4)
144-144: LGTM: LifeSpan enum extension
The new BASE_STATION_FILTER value follows the existing naming pattern and uses a consistent string value format.
152-158: LGTM: BaseStationStatus enum
The status values are logically structured with UNKNOWN as -1, IDLE as 0, and EMPTYING as 1, following standard enum patterns.
160-164: LGTM: BaseStationEvent class
The event class follows the established pattern of other event classes in the module.
146-151: Consider adding more base station actions
The BaseStationAction enum currently only supports EMPTY_DUSTBIN. Consider if other actions like CLEAN_FILTER or SELF_CLEAN might be needed in the future.
deebot_client/capabilities.py (3)
14-15: LGTM: New imports
The imports are correctly ordered alphabetically and specifically import only the needed types.
146-151: LGTM: CapabilityBaseStationAction class
The class follows the established pattern of other capability classes and provides a clean interface for executing base station actions.
228-228: LGTM: Optional base station capability
The base_station field is correctly marked as optional with None as default, allowing for devices without base station support.
deebot_client/commands/json/__init__.py (1)
45-45: LGTM: Command registration
The StationAction command is properly registered in all required locations:
- Import statement
- all list
- _COMMANDS list (maintaining alphabetical order)
Also applies to: 126-126, 231-233
| LifeSpan.BASE_STATION_FILTER | ||
| ), |
There was a problem hiding this comment.
Ensure BASE_STATION_FILTER is defined in LifeSpan enumeration
The LifeSpan enumeration includes LifeSpan.BASE_STATION_FILTER, but it's crucial to confirm that BASE_STATION_FILTER is a valid member of the LifeSpan enum. If it's not defined, this will lead to an AttributeError at runtime.
Also applies to: 147-148
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
deebot_client/events/__init__.py (3)
34-34: Export new base station enumsWhile
BaseStationEventis correctly exported, consider addingBaseStationActionandBaseStationStatusto__all__as they might be needed by external code working with base station functionality.__all__ = [ + "BaseStationAction", "BaseStationEvent", + "BaseStationStatus", "BatteryEvent",
144-144: Consider aligning enum value with naming conventionThe value
"spHeap"forBASE_STATION_FILTERdoesn't follow the snake_case pattern used by other enum values (e.g.,"dust_bag","cleaning_fluid"). Consider using"sp_heap"for consistency.
146-147: Remove extra blank lineThe file consistently uses one blank line between definitions, but here there are two blank lines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
deebot_client/commands/json/station_action.py(1 hunks)deebot_client/events/__init__.py(2 hunks)deebot_client/messages/json/__init__.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deebot_client/messages/json/init.py
- deebot_client/commands/json/station_action.py
🔇 Additional comments (3)
deebot_client/events/__init__.py (3)
147-151: LGTM!
The enum is well-structured with proper documentation and value assignment.
154-160: LGTM!
The enum provides a comprehensive set of states with logical value assignments.
163-167: LGTM!
The event class follows the established patterns and is properly documented.
|
N20 Pro Plus was added recently - this should probably point here too, instead of to the N20. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/hardware/test_init.py (1)
Line range hint
92-190: Add test cases forbuom7kintest_capabilities_event_extractionThe
test_capabilities_event_extractionfunction does not currently include the new devicebuom7k. To ensure comprehensive test coverage, consider adding test cases forbuom7kto validate its event-command mappings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
deebot_client/capabilities.py(3 hunks)deebot_client/hardware/deebot/buom7k.py(1 hunks)deebot_client/messages/json/base_station.py(1 hunks)tests/hardware/test_init.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deebot_client/messages/json/base_station.py
🔇 Additional comments (7)
deebot_client/hardware/deebot/buom7k.py (2)
137-138: Ensure BASE_STATION_FILTER is defined in LifeSpan enumeration
The LifeSpan enumeration includes LifeSpan.BASE_STATION_FILTER, but it's crucial to confirm that BASE_STATION_FILTER is a valid member of the LifeSpan enum. If it's not defined, this will lead to an AttributeError at runtime.
Also applies to: 147-148
92-198: Capabilities for Deebot N20 Plus are correctly implemented
The device capabilities for the Deebot N20 Plus are well-defined and integrated into the DEVICES dictionary. The use of appropriate commands and events aligns with the existing architecture.
deebot_client/capabilities.py (4)
14-15: Imports for base station capabilities are correctly added
The new imports BaseStationAction and BaseStationEvent are properly included, ensuring that the base station functionalities can be utilized without naming conflicts.
147-153: Definition of CapabilityBaseStationAction is appropriate
The CapabilityBaseStationAction class is well-defined, encapsulating the command necessary for base station actions. The use of Callable[[BaseStationAction], Command] accurately represents the expected function signature.
154-160: Definition of CapabilityBaseStation is appropriate
The CapabilityBaseStation class correctly includes the action and optional event attributes to represent base station capabilities. This aligns with the structure of other capability classes.
231-231: Integration of base_station into Capabilities class
Adding base_station: CapabilityBaseStation | None = None to the Capabilities class appropriately extends the capabilities framework to include base station functionalities.
tests/hardware/test_init.py (1)
269-269: Device buom7k correctly added to model tests
The addition of "buom7k" to the list of device models ensures that the Deebot N20 Plus is included in the test suite, which is important for validating device initialization.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## dev #645 +/- ##
==========================================
+ Coverage 86.87% 87.02% +0.15%
==========================================
Files 101 105 +4
Lines 3679 3737 +58
Branches 318 320 +2
==========================================
+ Hits 3196 3252 +56
- Misses 426 427 +1
- Partials 57 58 +1 ☔ View full report in Codecov by Sentry. |
edenhaus
left a comment
edenhaus
left a comment
There was a problem hiding this comment.
Thanks @MarkGodwin 👍
I refactored the code a little bit and added tests for the new functionality
edenhaus
changed the title
|
I tried out your new changes with my N20 Plus, it all looks good. Thanks! |
2 participants
The N20 Plus has a cyclone vacuum base station for emptying the deebot.
This comes with some an additional resettable counter for an internal filter inside the base station, which I added as a new resettable
Lifespantype.I also found a way to invoke an emptying of the Deebot, as well as an event that gets sent out during the automated vacuum process, which would maybe be useful to have if creating an automation to warn when the bin has been sucked out 20 times or something.
I couldn't see any existing Deebots that had any base-station functionality implemented, so I tried adding some of the configuration to allow this.
I'm a bit unsure as to what the implications of adding to the Capability classes really is - I'm sort of coding a bit blind, but it worked in a test script. Happy to change it further if this wasn't right.
Summary by CodeRabbit
Release Notes
New Features
StationActionfor base station operations.Bug Fixes
Tests
These updates enhance the device's operational capabilities, allowing for more efficient management and interaction with base station features.