Add Mammotion integration - #143429
Open
mikey0000 wants to merge 69 commits into
Open
Conversation
mikey0000
marked this pull request as draft
April 21, 2025 23:52
joostlek
requested changes
Apr 22, 2025
NoRi2909
reviewed
Apr 24, 2025
NoRi2909
left a comment
NoRi2909
left a comment
Contributor
There was a problem hiding this comment.
A few quick comments on the strings from me as a translator.
NoRi2909
reviewed
Apr 29, 2025
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
mikey0000
force-pushed
the
mammotion-lawnmower
branch
from
August 27, 2025 10:35
42727d1 to
b35d922
Compare
Contributor
|
@mikey0000 CI is failing |
Contributor
Author
Yup I know. I recently updated this branch with more items fixed, tests also need more work. I'm onto it now. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Contributor
Author
|
Keep open |
mikey0000
marked this pull request as ready for review
November 21, 2025 09:13
NoRi2909
reviewed
Nov 21, 2025
NoRi2909
left a comment
NoRi2909
left a comment
Contributor
There was a problem hiding this comment.
As a translator only a few suggestions on the user-facing strings below.
mikey0000
force-pushed
the
mammotion-lawnmower
branch
from
December 2, 2025 22:37
ba67ae1 to
e7c544c
Compare
NoRi2909
reviewed
Dec 4, 2025
Contributor
Author
|
All checks have passed WOOP WOOP! |
8 tasks
12 tasks
Contributor
Author
|
Going to pop this back to draft as I will do another sweep over the code. |
mikey0000
marked this pull request as draft
January 9, 2026 04:05
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 24 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
homeassistant/components/mammotion/config.py:55
- The docstring says this cancels the delayed write, but the implementation doesn’t cancel any scheduled
async_delay_save. If the intent is to prevent a later redundant write (and avoid writes after unload), call the store’s delayed-save cancel mechanism (e.g.,async_cancel_delayed_save()if available) before saving, and/or adjust_save_pendinghandling accordingly.
async def async_flush_mower_data(self) -> None:
"""Write queued mower data to disk, cancelling the delayed write."""
if not self._save_pending:
return
await self.async_save(self._data_to_save())
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
homeassistant/components/mammotion/config.py:50
_data_to_saveis documented (and inStore.async_delay_saveis typically executed) in an executor thread, but it mutates_save_pendingwhich is also read/written from the event loop thread. This introduces a cross-thread race. Prefer keeping_data_to_saveside-effect-free (just return the snapshot) and clear_save_pendingon the event loop thread (e.g., via an event-loop scheduled callback tied to the delayed save firing), so all state mutation happens on the same thread.
# A pending write keeps its own deadline: async_delay_save would push the
# write back on every call and never fire while polling continues.
if self._save_pending:
return
self._save_pending = True
self.async_delay_save(self._data_to_save, SAVE_DELAY)
def _data_to_save(self) -> dict[str, Any]:
"""Return a snapshot to persist; runs in the executor thread."""
self._save_pending = False
return dict(self.mower_data)
homeassistant/components/mammotion/init.py:171
- If
device_entry.identifiersdoes not contain a(DOMAIN, ...)identifier,next(...)will raiseStopIterationand error the device-removal flow. Usenext(..., None)and handle theNonecase (typically by returningTrueto allow removal, orFalseif you want to block removal when the device cannot be matched).
mower_names = (
next(
identifier[1]
for identifier in device_entry.identifiers
if identifier[0] == DOMAIN
),
)
| except ClientError, TimeoutError, OSError: | ||
| errors["base"] = "cannot_connect" | ||
| return errors, None | ||
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
homeassistant/components/mammotion/config.py:50
Store.async_delay_save()executes the provided callable in an executor thread._data_to_save()mutates_save_pendingand iterates/copiesself.mower_datafrom that executor thread whileasync_update_mower_data()may be mutating it in the event loop thread. This can lead to race conditions (includingRuntimeError: dictionary changed size during iteration) and incorrect_save_pendingstate. A safer approach is to ensure the executor callable does not touch shared mutable state: create an immutable snapshot in the event loop thread and have the executor callable only return that snapshot, or replace the delayed-save mechanism with an event-loop scheduled task/handle that callsasync_save()afterSAVE_DELAY.
@callback
def async_update_mower_data(self, device_name: str, data: dict[str, Any]) -> None:
"""Update a mower in memory, writing to disk at most once per SAVE_DELAY."""
if self.mower_data.get(device_name) == data:
return
self.mower_data[device_name] = data
# A pending write keeps its own deadline: async_delay_save would push the
# write back on every call and never fire while polling continues.
if self._save_pending:
return
self._save_pending = True
self.async_delay_save(self._data_to_save, SAVE_DELAY)
def _data_to_save(self) -> dict[str, Any]:
"""Return a snapshot to persist; runs in the executor thread."""
self._save_pending = False
return dict(self.mower_data)
homeassistant/components/mammotion/coordinator.py:11
NoTransportAvailableErroris imported frompymammotion.transporthere, whilehomeassistant/components/mammotion/const.pyimports it frompymammotion.transport.base. If these are different symbols (or if one import path is not available in the pinned version), exception handling can silently break (e.g., not being caught byCOMMAND_EXCEPTIONSor the update loop handler). Standardize the import path across the integration to a single canonical location.
from pymammotion.aliyun.exceptions import DeviceOfflineException
from pymammotion.aliyun.model.dev_by_account_response import Device
from pymammotion.data.model.device import MowingDevice
from pymammotion.homeassistant import HomeAssistantMowerApi
from pymammotion.transport import NoTransportAvailableError
homeassistant/components/mammotion/config_flow.py:37
- This integration is opted into strict typing (
.strict-typing+mypy.ini). Usingself._config: dict = {}introduces an untypeddict[Any, Any]that tends to propagateAnythrough the flow logic. Prefer a fully typed annotation (e.g.,dict[str, Any]) to keep mypy effective under strict settings.
def __init__(self) -> None:
"""Initialize the config flow."""
self._config: dict = {}
self._discovered_devices: dict[str, str] = {}
self._discovered_device: BLEDevice | None = None
| """Get data from the device.""" | ||
| try: | ||
| data = await self.api.update(self.device_name) | ||
| except DeviceOfflineException, NoTransportAvailableError: |
Contributor
Author
There was a problem hiding this comment.
ruff formatted it that way
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
homeassistant/components/mammotion/config.py:50
_data_to_saveis documented to run in the executor thread, but it mutates_save_pending, which is also read/written from the event loop thread inasync_update_mower_data. That cross-thread mutation can race and cause missed scheduling or duplicate saves. Move the_save_pending = Falsetransition back onto the event loop thread (e.g., clear the flag in a callback/task scheduled when the delayed save fires, and keep_data_to_saveas a pure snapshot function).
def _data_to_save(self) -> dict[str, Any]:
"""Return a snapshot to persist; runs in the executor thread."""
self._save_pending = False
return dict(self.mower_data)
|
|
||
| try: | ||
| await mammotion_http.login_v2(account, password) | ||
| except ClientError, TimeoutError, OSError: |
| """Get data from the device.""" | ||
| try: | ||
| data = await self.api.update(self.device_name) | ||
| except DeviceOfflineException, NoTransportAvailableError: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
homeassistant/components/mammotion/config.py:46
self._dirtyis cleared before the save completes. Ifasync_savefails/raises (I/O error, shutdown race, etc.), pending changes will be dropped and never retried. Set_dirty = Falseonly after a successful save (or restore it in an exception handler).
async def async_flush_mower_data(self) -> None:
"""Write the in-memory mower data to disk if it changed."""
if not self._dirty:
return
self._dirty = False
await self.async_save(dict(self.mower_data))
homeassistant/components/mammotion/init.py:47
- The
ha_versionvalue is hardcoded, which can break library compatibility/telemetry if the upstream expects the actual Home Assistant version. Prefer using the runtime version fromhass.config.version(or the project’s standard version source) to keep it accurate.
api = HomeAssistantMowerApi(
ha_version="1.0.0", session=async_get_clientsession(hass)
)
homeassistant/components/mammotion/init.py:192
- This introduces
async_remove_config_entry_devicebehavior but the added test suite doesn’t appear to cover it. Add a test that exercises device removal decisions for a Mammotion device entry vs. a non-Mammotion device entry to lock in expected UI behavior.
async def async_remove_config_entry_device(
hass: HomeAssistant, config_entry: MammotionConfigEntry, device_entry: DeviceEntry
) -> bool:
"""Remove a config entry from a device."""
mower_names = (
next(
identifier[1]
for identifier in device_entry.identifiers
if identifier[0] == DOMAIN
),
)
mower = next(
(
mower
for mower in config_entry.runtime_data.mowers
if mower.name in mower_names
),
None,
)
return not bool(mower)
| """Get data from the device.""" | ||
| try: | ||
| data = await self.api.update(self.device_name) | ||
| except DeviceOfflineException, NoTransportAvailableError: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
homeassistant/components/mammotion/init.py:127
- Shutdown and storage flush are each scheduled in two ways: (1) via event listeners (
STOP/FINAL_WRITE) and (2) viaentry.async_on_unload(...). This can lead to duplicatemammotion.stop()calls and duplicate store writes during shutdown/unload, which is avoidable operational noise and can complicate debugging. Consider using only the event listeners (for HA stop) and only the unload callbacks (for entry unload), or gating the scheduled tasks to ensure they run once.
entry.async_on_unload(
hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, shutdown_mammotion)
)
entry.async_on_unload(
hass.bus.async_listen_once(
EVENT_HOMEASSISTANT_FINAL_WRITE, schedule_flush_mower_data
)
)
entry.async_on_unload(schedule_shutdown_mammotion)
entry.async_on_unload(schedule_flush_mower_data)
homeassistant/components/mammotion/init.py:48
- The
ha_versionis hard-coded to\"1.0.0\", which is likely incorrect and may affect upstream behavior (e.g., user-agent/version-based logic). Prefer using Home Assistant's actual runtime version (or a constant provided by HA) so the library receives accurate version info.
api = HomeAssistantMowerApi(
ha_version="1.0.0", session=async_get_clientsession(hass)
)
|
|
||
| try: | ||
| await mammotion_http.login_v2(account, password) | ||
| except ClientError, TimeoutError, OSError: |
| """Get data from the device.""" | ||
| try: | ||
| data = await self.api.update(self.device_name) | ||
| except DeviceOfflineException, NoTransportAvailableError: |
mikey0000
marked this pull request as ready for review
August 16, 2026 07:14
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 4 comments.
Suppressed comments (3)
homeassistant/components/mammotion/coordinator.py:11
- The import path for
NoTransportAvailableErrordiffers fromhomeassistant/components/mammotion/const.py(which imports it frompymammotion.transport.base). Standardize on a single import location across the integration to reduce the risk of catching/raising different exception classes and to keep exception handling consistent.
from pymammotion.transport import NoTransportAvailableError
homeassistant/components/mammotion/coordinator.py:106
- Calling
from_dictvia an instance (MowingDevice().from_dict(...)) is unconventional and makes it harder to tell whether it’s a classmethod or mutating instance method. Prefer the explicit form (MowingDevice.from_dict(mower_data)) to clarify intent and avoid confusion during future refactors.
mower_state = MowingDevice()
if mower_data := self.store.mower_data.get(self.device_name):
try:
mower_state = MowingDevice().from_dict(mower_data)
except InvalidFieldValue:
mower_state = MowingDevice()
homeassistant/components/mammotion/coordinator.py:76
CommandFailedErroris aHomeAssistantError, but this raise uses a raw, non-translated message even though the integration defines exception translations instrings.json. Consider raising withtranslation_domain/translation_key(or only raising translatedHomeAssistantErrorat the service layer) so user-facing failures remain localizable and consistent.
if not await self.api.async_send_command(self.device_name, command, **kwargs):
raise CommandFailedError(f"Command {command} failed for {self.device_name}")
|
|
||
| return errors, login_info.userInformation.userAccount | ||
|
|
||
| async def async_step_wifi( |
| """Get data from the device.""" | ||
| try: | ||
| data = await self.api.update(self.device_name) | ||
| except DeviceOfflineException, NoTransportAvailableError: |
Comment on lines
+221
to
+233
| async_fire_time_changed(hass) | ||
| await hass.async_block_till_done() | ||
| assert hass.states.get(ENTITY_ID).state == STATE_UNAVAILABLE | ||
|
|
||
| mock_mower_api.update.return_value = mock_mowing_device | ||
| freezer.tick(timedelta(minutes=1)) | ||
| async_fire_time_changed(hass) | ||
| await hass.async_block_till_done() | ||
| assert hass.states.get(ENTITY_ID).state != STATE_UNAVAILABLE | ||
|
|
||
| mock_mower_api.is_online.return_value = False | ||
| freezer.tick(timedelta(minutes=1)) | ||
| async_fire_time_changed(hass) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
homeassistant/components/mammotion/config_flow.py:236
user_accountis typed asstr | Nonefrom_async_validate_login, but is passed toasync_set_unique_idwithout narrowing. Even if logically non-None whenerrorsis empty, this will fail strict typing and is fragile if_async_validate_loginchanges. Add an explicit narrowing step (e.g.,assert user_account is not Noneor restructure_async_validate_loginto returnstron success) before callingasync_set_unique_id.
account = user_input[CONF_ACCOUNTNAME]
password = user_input[CONF_PASSWORD]
errors, user_account = await self._async_validate_login(account, password)
if not errors:
await self.async_set_unique_id(user_account, raise_on_progress=False)
self._abort_if_unique_id_configured()
return self.async_create_entry(
Comment on lines
+117
to
+123
| try: | ||
| data = await self.api.update(self.device_name) | ||
| except DeviceOfflineException, NoTransportAvailableError: | ||
| return self.data | ||
|
|
||
| if data is None: | ||
| raise UpdateFailed(f"No data returned for {self.device_name}") |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
Suppressed comments (5)
homeassistant/components/mammotion/config_flow.py:141
- The dict merge order here prevents updating an existing entry’s stored BLE address for the same mower name:
**entry.data[...]will overwrite the newly-discoveredname: ...value. Swap the merge order so the freshly-discovered address wins. Consider adding a regression test that starts with an entry havingCONF_BLE_DEVICESfornameand confirms a new advertisement updates it.
if entry := await self.check_and_update_bluetooth_device(device):
existing_devices = {
name: format_mac(device.address),
**entry.data.get(CONF_BLE_DEVICES, {}),
}
self._abort_if_unique_id_configured(
updates={CONF_BLE_DEVICES: existing_devices}, reload_on_update=False
)
homeassistant/components/mammotion/config.py:45
- The dirty flag is cleared before the save completes. If
async_save(...)raises (I/O error, disk full, etc.), pending state is silently dropped and won’t be retried. Set_dirty = Falseonly after a successful save (or restore_dirty = Truein an exception handler).
async def async_flush_mower_data(self, _event: Event | None = None) -> None:
"""Write the in-memory mower data to disk if it changed."""
if not self._dirty:
return
self._dirty = False
await self.async_save(dict(self.mower_data))
homeassistant/components/mammotion/lawn_mower.py:95
activitytreats bothWorkMode.MODE_PAUSEandWorkMode.MODE_CHARGING_PAUSEas paused, butasync_start_mowingonly resumes whenMODE_PAUSE. If the device reportsMODE_CHARGING_PAUSE,SERVICE_START_MOWINGwill currently sendstart_jobinstead ofresume_execute_task. IncludeMODE_CHARGING_PAUSEin the resume condition to match the activity mapping.
if mode == WorkMode.MODE_PAUSE:
trans_key = "resume_failed"
try:
await self.coordinator.async_send_command("resume_execute_task")
homeassistant/components/mammotion/init.py:44
- Hard-coding
ha_version=\"1.0.0\"will become stale and may affect library behavior (e.g., user-agent/version gating). Prefer passing Home Assistant’s actual version (e.g., via the core version constant) or omit it if the library can derive a sensible default.
api = HomeAssistantMowerApi(
ha_version="1.0.0", session=async_get_clientsession(hass)
)
homeassistant/components/mammotion/coordinator.py:11
- Within this PR,
NoTransportAvailableErroris imported from different modules (pymammotion.transporthere vspymammotion.transport.baseinconst.py). Align these imports to a single canonical location to avoid confusion and reduce the chance of runtime import issues if the library reorganizes symbols.
from pymammotion.homeassistant import HomeAssistantMowerApi
from pymammotion.transport import NoTransportAvailableError
| """Get data from the device.""" | ||
| try: | ||
| data = await self.api.update(self.device_name) | ||
| except DeviceOfflineException, NoTransportAvailableError: |
|
|
||
| try: | ||
| await mammotion_http.login_v2(account, password) | ||
| except ClientError, TimeoutError, OSError: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
homeassistant/components/mammotion/init.py:164
- There is duplicated credential-persistence logic here and in
MammotionBaseUpdateCoordinator.store_cloud_credentials(). Consolidating into a single helper (or using one implementation from the other) reduces drift risk and makes future credential schema changes safer.
def store_cloud_credentials(
hass: HomeAssistant,
config_entry: MammotionConfigEntry,
mammotion: MammotionClient,
) -> None:
"""Store cloud credentials in config entry."""
cache = mammotion.to_cache()
if not cache:
return
hass.config_entries.async_update_entry(
config_entry, data={**config_entry.data, **cache}
)
homeassistant/components/mammotion/init.py:188
- If the entry is loaded but
entry.runtime_data.mowersis empty (e.g., account has no supported devices), unload will skipmammotion.stop()and state flush. This can leak an active client/session across unload/reload cycles. Consider always stopping the client and flushing the store whenunload_okis true (you may need to storeapi/storeinruntime_dataso it’s available even with zero mowers).
async def async_unload_entry(hass: HomeAssistant, entry: MammotionConfigEntry) -> bool:
"""Unload a config entry."""
if unload_ok := await hass.config_entries.async_unload_platforms(entry, PLATFORMS):
for mower in entry.runtime_data.mowers:
mower.coordinator.store_cloud_credentials()
with contextlib.suppress(TimeoutError):
await mower.api.mammotion.remove_device(mower.name)
if mowers := entry.runtime_data.mowers:
await mowers[0].coordinator.store.async_flush_mower_data()
await mowers[0].api.mammotion.stop()
return unload_ok
| data = await self.api.update(self.device_name) | ||
| except DeviceOfflineException, NoTransportAvailableError: | ||
| return self.data | ||
|
|
|
|
||
| try: | ||
| await mammotion_http.login_v2(account, password) | ||
| except ClientError, TimeoutError, OSError: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
homeassistant/components/mammotion/config_flow.py:141
- The dict merge order here prevents the newly discovered address from updating an existing
CONF_BLE_DEVICESentry when the mower name key already exists (the existing entry value overrides the new one). Swap the merge order so the discovered{name: format_mac(device.address)}wins.
existing_devices = {
name: format_mac(device.address),
**entry.data.get(CONF_BLE_DEVICES, {}),
}
self._abort_if_unique_id_configured(
updates={CONF_BLE_DEVICES: existing_devices}, reload_on_update=False
)
homeassistant/components/mammotion/init.py:53
- Hard-coding
ha_version=\"1.0.0\"is likely incorrect and can lead to misleading client identification (and harder debugging). Prefer using Home Assistant’s actual runtime version (e.g.,homeassistant.const.__version__) or a value derived fromhass.
api = HomeAssistantMowerApi(
ha_version="1.0.0", session=async_get_clientsession(hass)
)
homeassistant/components/mammotion/lawn_mower.py:118
- This duplicates the same error wrapping and IoT sync logic across both branches. Consider consolidating the try/except/finally so only the command + translation key selection differs; this reduces the chance of future drift (e.g., one branch forgetting
async_request_iot_sync).
if mode == WorkMode.MODE_PAUSE:
trans_key = "resume_failed"
try:
await self.coordinator.async_send_command("resume_execute_task")
except COMMAND_EXCEPTIONS as exc:
raise HomeAssistantError(
translation_domain=DOMAIN, translation_key=trans_key
) from exc
finally:
await self.coordinator.api.async_request_iot_sync(
self.coordinator.device_name
)
else:
try:
await self.coordinator.async_send_command("start_job")
except COMMAND_EXCEPTIONS as exc:
raise HomeAssistantError(
translation_domain=DOMAIN, translation_key=trans_key
) from exc
finally:
await self.coordinator.api.async_request_iot_sync(
self.coordinator.device_name
)
| """Get data from the device.""" | ||
| try: | ||
| data = await self.api.update(self.device_name) | ||
| except DeviceOfflineException, NoTransportAvailableError: |
|
|
||
| try: | ||
| await mammotion_http.login_v2(account, password) | ||
| except ClientError, TimeoutError, OSError: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Breaking change
Proposed change
Type of change
Add Mammotion lawn mower Integration
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.To help with the load of incoming pull requests: