-
-
Notifications
You must be signed in to change notification settings - Fork 28.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add husqvarna automower ble integration #108326
base: dev
Are you sure you want to change the base?
Add husqvarna automower ble integration #108326
Conversation
2f8f5c6
to
1a7f62f
Compare
1a7f62f
to
45de1bc
Compare
45de1bc
to
b6a7da4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a merge conflict.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
b6a7da4
to
07c766e
Compare
07c766e
to
3cd6a8e
Compare
@joostlek and @Thomas55555 maybe you can help get some traction on this |
Never really worked a lot with bluetooth so I can't help you on that part |
I'm also not, and I'm self starting to learn here. I just gave you some general review. |
bb611ce
to
6b1c752
Compare
6b1c752
to
a54d106
Compare
a54d106
to
c3699fc
Compare
034267c
to
311df40
Compare
fe9ca25
to
5a18fe5
Compare
I made quite a few changes to a local version trying to clean up the flow and startup. It's still not perfect, and I'm not sure i like how I've set the devices up with "current" values rather than "stored" values, but I think you may find it helpful: |
homeassistant/components/husqvarna_automower_ble/coordinator.py
Outdated
Show resolved
Hide resolved
7b1e851
to
3ef9a6d
Compare
Signed-off-by: Alistair Francis <alistair@alistair23.me>
7c7ebf7
to
e2a9817
Compare
Tests are passing and comments are addressed |
Happy to see this PR and important feature to be added. Thx @alistair23 for the quite huge effort. @emontnemery |
What about this comment though: #108326 (comment), users should not have to reload the integration to make it work. What happens if the automower is out of bluetooth range? |
I don't think it's possible to configure the integration if the automower is out of range (it won't / can't be discovered). |
assert result == snapshot | ||
|
||
result = await hass.config_entries.flow.async_configure( | ||
result["flow_id"], | ||
user_input={"address": "00000000-0000-0000-0000-000000000001"}, | ||
) | ||
assert result == snapshot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike the use of snapshots here as you "hide" what you are actually testing
await hass.async_block_till_done(wait_background_tasks=True) | ||
|
||
result = await hass.config_entries.flow.async_init( | ||
DOMAIN, context={"source": config_entries.SOURCE_USER} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOMAIN, context={"source": config_entries.SOURCE_USER} | |
DOMAIN, context={"source": SOURCE_USER} |
Personal ick
LOGGER.debug("connected and paired") | ||
|
||
model = await mower.get_model() | ||
LOGGER.info("Connected to Automower: %s", model) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't log on info level
def __init__( | ||
self, | ||
hass: HomeAssistant, | ||
logger: logging.Logger, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you passing in a logger if you made one on line 24?
If you like, you can also create an integration level logger with LOGGER = logging.getLogger(__package__)
in const.py
return data | ||
|
||
|
||
class HusqvarnaAutomowerBleEntity(CoordinatorEntity[HusqvarnaCoordinator]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to entity.py
unique_id: str, | ||
name: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we just pass in what we need and let the entity decide
) -> None: | ||
"""Initialize the lawn mower.""" | ||
super().__init__(coordinator) | ||
self._attr_name = name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lawn mower is the main feature of the device, so the name of the lawn mower should be set to _attr_name = None
. This way, the name of this entity is decided by the name of the device, which is consistent
[ | ||
AutomowerLawnMower( | ||
coordinator, | ||
"automower" + str(model) + "_" + str(address), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, for unique_id, address should be purely unique I assume? Reminder that unique_id is unique per platform per integration, so there's no need to prepend it with automower (if that's what you were trying to avoid)
if state is None: | ||
return None | ||
|
||
if activity is None: | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be combined
return | ||
|
||
await self.coordinator.mower.mower_resume() | ||
if self._attr_activity == LawnMowerActivity.DOCKED: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if self._attr_activity == LawnMowerActivity.DOCKED: | |
if self._attr_activity is LawnMowerActivity.DOCKED: |
channel_id = random.randint(1, 0xFFFFFFFF) | ||
|
||
try: | ||
(manufacture, device_type, model) = await Mower( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call appears to return a bool
File "/config/custom_components/husqvarna_automower_ble/config_flow.py", line 117, in async_step_user
return await self.async_step_confirm()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/config/custom_components/husqvarna_automower_ble/config_flow.py", line 81, in async_step_confirm
(manufacture, device_type, model) = await Mower(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: cannot unpack non-iterable bool object
I have been following this integration for a long time. Is it possible to have it in the release HA 2024.7? |
@al31c0 |
Proposed change
This PR adds support for the Husqvarna Automower BLE integration.
The Husqvarna Automower BLE integration provides connectivity with Husqvarna Automowers lawn mowers via a local Bluetooth connection. This allows connecting and controlling an Automower without any accounts, cloud or network connection.
The integration is based on AutoMower-BLE, a unofficial reverse engineered Husqvarna Automower Connect BLE library.
The integration has been tested against a 305 Automower using a ESPHome Bluetooth proxy.
This is somewhat similar to #100569, except #100569 uses the cloud API while this integration uses the local BLE. Not all mowers support BLE and not all mowers support the cloud, so although there is overlap both integrations support different mowers.
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: