Skip to content

Add hivi speaker integration#165307

Open
swansmart wants to merge 143 commits intohome-assistant:devfrom
swansmart:add-hivi-speaker-integration
Open

Add hivi speaker integration#165307
swansmart wants to merge 143 commits intohome-assistant:devfrom
swansmart:add-hivi-speaker-integration

Conversation

@swansmart
Copy link
Copy Markdown

Breaking change

Proposed change

Add HiVi Speaker integration into Core

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • I understand the code I am submitting and can explain how it works.
  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.
  • Any generated code has been carefully reviewed for correctness and compliance with project standards.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies a diff between library versions and ideally a link to the changelog/release notes is added to the PR description.

To help with the load of incoming pull requests:

Copilot AI review requested due to automatic review settings March 11, 2026 06:41
Copy link
Copy Markdown

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @swansmart

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link
Copy Markdown

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

Copy link
Copy Markdown

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new hivi_speaker integration to Home Assistant Core, including discovery, media player support, master/slave sync control, services, translations, and initial config/option flows.

Changes:

  • Introduces the hivi_speaker integration (config flow, discovery scheduler, device manager/registry, media player, switch entities, group coordinator).
  • Adds services (refresh_discovery, postpone_discovery, remove_device) and English/Chinese translations.
  • Adds config flow test coverage and fixtures for the new integration.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 18 comments.

Show a summary per file
File Description
homeassistant/components/hivi_speaker/__init__.py Sets up/unloads the integration and handles device removal behavior.
homeassistant/components/hivi_speaker/config_flow.py Implements single-instance config flow and an options flow to trigger discovery refresh.
homeassistant/components/hivi_speaker/const.py Defines domain and discovery-related constants.
homeassistant/components/hivi_speaker/device.py Defines the device and slave-device models and capability helpers.
homeassistant/components/hivi_speaker/device_data_registry.py Persists extra device data via Store and reacts to device registry changes.
homeassistant/components/hivi_speaker/device_manager.py Core orchestration for discovery, entity creation/removal, and status updates.
homeassistant/components/hivi_speaker/discovery_scheduler.py Implements periodic SSDP-based discovery and adaptive scheduling.
homeassistant/components/hivi_speaker/group_coordinator.py Coordinates master/slave grouping operations and verification.
homeassistant/components/hivi_speaker/media_player.py Implements MediaPlayerEntity with DLNA browsing/playback and local media browsing.
homeassistant/components/hivi_speaker/switch.py Implements switch entities for controlling master/slave sync relationships.
homeassistant/components/hivi_speaker/services.py Registers integration services and dispatches to the device manager.
homeassistant/components/hivi_speaker/services.yaml Documents the services and their fields for the UI.
homeassistant/components/hivi_speaker/manifest.json Declares metadata/dependencies for the integration.
homeassistant/components/hivi_speaker/translations/en.json English strings for config/options flows.
homeassistant/components/hivi_speaker/translations/zh-Hans.json Simplified Chinese strings for config/options flows.
tests/components/hivi_speaker/conftest.py Test fixtures for config entries and setup patching.
tests/components/hivi_speaker/test_config_flow.py Tests for config flow and options flow behavior.
tests/components/hivi_speaker/__init__.py Marks the test package for the integration.

Comment on lines +70 to +92
async def device_registry_updated(event):
_LOGGER.debug("Device registry updated event: %s", event.data)
ha_device_id = event.data["device_id"]
action = event.data["action"]
if action == "remove":
# Device deleted - delete current switch if slave device is removed
slave_device_dict = self._device_manager.device_data_registry.get_device_dict_by_ha_device_id(
ha_device_id=ha_device_id
)
if slave_device_dict is not None:
speaker_device_id = slave_device_dict.get("speaker_device_id")
if speaker_device_id == self._slave_speaker_device_id:
_LOGGER.debug(
"Removing switch entity for deleted slave device %s",
speaker_device_id,
)
# Remove from hub
self._hub.switches.pop(self._attr_unique_id, None)
# Remove entity from Home Assistant
if self.hass and hasattr(self, "async_remove"):
await self.async_remove()

self.hass.bus.async_listen("device_registry_updated", device_registry_updated)
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The device_registry_updated listener looks up the removed device in device_data_registry using the HA device id. However DeviceDataRegistry also listens to the same event and pops that data on remove, so this lookup can race and return None, preventing switch cleanup. Consider using the device registry identifiers directly (or ensure the registry mapping is still available when handling the event) and store/unsubscribe the listener in async_will_remove_from_hass to avoid leaks.

Copilot uses AI. Check for mistakes.
Comment on lines +6 to +11
from homeassistant.exceptions import HomeAssistantError
from homeassistant.helpers.dispatcher import async_dispatcher_send
from homeassistant.helpers.entity import EntityCategory

from .const import DOMAIN
from .device import ConnectionStatus, HIVIDevice, SlaveDeviceInfo
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are unused imports here (HomeAssistantError, SlaveDeviceInfo) which will fail Ruff/flake checks (F401). Please remove them or use them.

Suggested change
from homeassistant.exceptions import HomeAssistantError
from homeassistant.helpers.dispatcher import async_dispatcher_send
from homeassistant.helpers.entity import EntityCategory
from .const import DOMAIN
from .device import ConnectionStatus, HIVIDevice, SlaveDeviceInfo
from homeassistant.helpers.dispatcher import async_dispatcher_send
from homeassistant.helpers.entity import EntityCategory
from .const import DOMAIN
from .device import ConnectionStatus, HIVIDevice

Copilot uses AI. Check for mistakes.
Comment on lines +211 to +218
"""Get master device object"""
return self._master

@property
def slave_device(self) -> HIVIDevice:
"""Get slave device object"""
return self._slave

Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

master_device/slave_device properties return self._master and self._slave, but those attributes are never set in __init__. Accessing these properties will raise AttributeError; either initialize and maintain these attributes or remove the unused properties.

Suggested change
"""Get master device object"""
return self._master
@property
def slave_device(self) -> HIVIDevice:
"""Get slave device object"""
return self._slave
"""Get master device object."""
return self.get_master_device()
@property
def slave_device(self) -> HIVIDevice | None:
"""Get slave device object.
Returns None if the slave device cannot be found in the registry.
"""
slave_device_dict = (
self._device_manager.device_data_registry.get_device_dict_by_speaker_device_id(
self._slave_speaker_device_id
)
)
if slave_device_dict is None:
_LOGGER.error(
"Cannot find information for slave device %s",
self._slave_speaker_device_id,
)
return None
return HIVIDevice(**slave_device_dict)

Copilot uses AI. Check for mistakes.
Comment on lines +993 to +1064
async def _handle_operation_success(self, operation_id: str):
"""Handle operation success"""
operation = self._pending_operations.get(operation_id)
if not operation:
return

# Update operation status
operation["status"] = "success"
operation["end_time"] = datetime.now()

duration = (operation["end_time"] - operation["start_time"]).total_seconds()

_LOGGER.debug(
"operation: %s (time: %.1f seconds, try num: %d)",
operation_id,
duration,
operation["retry_count"],
)

# Send operation success event
self.hass.bus.async_fire(
f"{DOMAIN}_group_operation_succeeded",
{
"operation_id": operation_id,
"master": operation["master"],
"slave": operation["slave"],
"action": operation["type"],
"duration": duration,
"retry_count": operation["retry_count"],
"timestamp": datetime.now().isoformat(),
},
)

# Trigger immediate discovery to ensure status synchronization
_LOGGER.debug("Trigger immediate discovery to synchronize status")
# self.discovery_scheduler.schedule_immediate_discovery(force=False)

# Clean up operation
await self._cleanup_operation(operation_id)

async def _handle_operation_timeout(self, operation_id: str):
"""Handle operation timeout"""
operation = self._pending_operations.get(operation_id)
if not operation:
return

# Update operation status
operation["status"] = "timeout"
operation["end_time"] = datetime.now()

_LOGGER.warning("Operation timeout: %s", operation_id)

# Send operation timeout event
self.hass.bus.async_fire(
f"{DOMAIN}_group_operation_timeout",
{
"operation_id": operation_id,
"master": operation["master"],
"slave": operation["slave"],
"action": operation["type"],
"duration": self._operation_timeout,
"timestamp": datetime.now().isoformat(),
},
)

# Even if timeout, trigger discovery to get current status
_LOGGER.debug("Operation timeout, trigger discovery to get current status")
# await self.discovery_scheduler.schedule_immediate_discovery(force=False)

# Clean up operation
await self._cleanup_operation(operation_id)

Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_handle_operation_success, _handle_operation_timeout, and _cleanup_operation are defined twice in this class. The later definitions overwrite the earlier ones, which is error-prone and makes it unclear which logic is intended (e.g., callback cleanup differs). Please remove the duplicates and keep a single canonical implementation.

Copilot uses AI. Check for mistakes.
Comment on lines +645 to +653
params = operation_data.get("params", {})
slave_ip = params.get("slave_ip", "172.18.8.207")
ssid = params.get("ssid", "5357414E204C532D3130305F30353139")
wifi_channel = params.get("wifi_channel", 1)
auth = params.get("auth", "WPAPSKWPA2PSK")
encry = params.get("encry", "AES")
psk = params.get("psk", "12345678")
master_ip = params.get("master_ip", "172.18.8.205")
uuid = params.get("uuid", "FF31F0121338FA6FEED60519")
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_execute_operation falls back to hard-coded IPs/SSID/PSK when required params are missing. If a param is accidentally omitted, this can target the wrong host/network credentials. Prefer failing fast (reject the operation) when required params are missing, rather than using defaults that represent real-looking values.

Copilot uses AI. Check for mistakes.
"integration_type": "hub",
"iot_class": "local_polling",
"issue_tracker": "https://github.com/home-assistant/core/issues",
"requirements": ["hivico>=0.1.0"],
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Home Assistant integrations pin PyPI dependencies to an exact version in manifest.json (e.g., async-upnp-client==...). Using a range (hivico>=0.1.0) can lead to untested versions being installed. Please pin to a specific tested version.

Suggested change
"requirements": ["hivico>=0.1.0"],
"requirements": ["hivico==0.1.0"],

Copilot uses AI. Check for mistakes.
Comment on lines +49 to +51
success = await domain_data["device_manager"].remove_device(
speaker_device_id
)
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async_handle_remove_device calls domain_data["device_manager"].remove_device(...), but HIVIDeviceManager does not implement a remove_device method (only async_remove_device_with_entities). This will raise AttributeError when the service is called; either implement remove_device on the manager or update the service handler to call the existing removal API.

Suggested change
success = await domain_data["device_manager"].remove_device(
speaker_device_id
)
success = await domain_data[
"device_manager"
].async_remove_device_with_entities(speaker_device_id)

Copilot uses AI. Check for mistakes.
Comment on lines +2 to +4
from dataclasses import dataclass
from typing import Dict, List, Optional, Set
import asyncio
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module has several unused imports (dataclass, Dict, List, Set, asyncio) which will fail Ruff/flake checks (F401) in Home Assistant. Please remove the unused imports.

Suggested change
from dataclasses import dataclass
from typing import Dict, List, Optional, Set
import asyncio
from typing import Optional

Copilot uses AI. Check for mistakes.
Comment on lines +93 to +97
def __post_init__(self):
"""Post-initialization processing"""
if not self.unique_id:
self.unique_id = f"hivi_{self.mac_address.replace(':', '')}"

Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HIVIDevice inherits from Pydantic BaseModel, so __post_init__ will not be called. If you need to auto-generate unique_id when missing, use Pydantic validation (e.g., model_post_init / validators) or set the default when constructing the model.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 10 comments.


You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +55 to +61
# Initially add buttons for existing devices
entities = []
for device in device_manager.device_data_registry._device_data.values():
if device.is_available_for_media:
entity = HIVIMediaPlayerEntity(device, device_manager, hass)
entities.append(entity)

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async_setup_entry iterates over device_manager.device_data_registry._device_data.values(), but those values are registry dicts (not HIVIDevice objects), so device.is_available_for_media will raise. It also instantiates HIVIMediaPlayerEntity with the wrong argument order/arity vs its __init__ signature. This setup path will fail as soon as the platform loads.

Suggested change
# Initially add buttons for existing devices
entities = []
for device in device_manager.device_data_registry._device_data.values():
if device.is_available_for_media:
entity = HIVIMediaPlayerEntity(device, device_manager, hass)
entities.append(entity)
# Initially add media player entities for existing devices, if available
entities = []
for device_entry in device_manager.device_data_registry._device_data.values():
# Registry entries may be dicts; extract the underlying device if present
device = (
device_entry.get("device")
if isinstance(device_entry, dict)
else device_entry
)
if not isinstance(device, HIVIDevice):
continue
if not getattr(device, "is_available_for_media", False):
continue
# Use keyword arguments to match HIVIMediaPlayerEntity's __init__ signature
entity = HIVIMediaPlayerEntity(
hass=hass,
device_manager=device_manager,
device=device,
)
entities.append(entity)

Copilot uses AI. Check for mistakes.
Comment on lines +218 to +231
return self._master

@property
def slave_device(self) -> HIVIDevice:
"""Get slave device object"""
return self._slave

async def async_turn_on(self, **kwargs):
"""Turn on the switch - set master-slave relationship"""
master_device = self.get_master_device()
_LOGGER.debug(
"Setting device %s as slave of device %s",
self._slave_device_friendly_name,
master_device.friendly_name,
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

master_device / slave_device properties return self._master and self._slave, but these attributes are never set in __init__, so accessing the properties will raise AttributeError. Also async_turn_on logs master_device.friendly_name without checking if get_master_device() returned None, which can crash when the master device is missing from the registry.

Suggested change
return self._master
@property
def slave_device(self) -> HIVIDevice:
"""Get slave device object"""
return self._slave
async def async_turn_on(self, **kwargs):
"""Turn on the switch - set master-slave relationship"""
master_device = self.get_master_device()
_LOGGER.debug(
"Setting device %s as slave of device %s",
self._slave_device_friendly_name,
master_device.friendly_name,
# Use helper to safely retrieve the current master device
return self.get_master_device()
@property
def slave_device(self) -> HIVIDevice:
"""Get slave device object"""
# Use helper to safely retrieve the current slave device
return self.get_slave_device()
async def async_turn_on(self, **kwargs):
"""Turn on the switch - set master-slave relationship"""
master_device = self.get_master_device()
if master_device is None:
_LOGGER.warning(
"Unable to set slave relationship for %s: master device not found",
self._slave_device_friendly_name,
)
return
master_name = getattr(master_device, "friendly_name", str(master_device))
_LOGGER.debug(
"Setting device %s as slave of device %s",
self._slave_device_friendly_name,
master_name,

Copilot uses AI. Check for mistakes.
try:
dlna_info = await parse_ssdp_response(response_text, addr)
location = dlna_info.get("location", "")
device_info = await parse_local_url(location)
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse_local_url() can return None on errors, but _parse_one unconditionally does device_info.get("UDN"). If the description fetch/parsing fails for a device, discovery will crash with AttributeError. Guard against None before calling .get().

Suggested change
device_info = await parse_local_url(location)
device_info = await parse_local_url(location)
if not device_info:
return

Copilot uses AI. Check for mistakes.
Comment on lines +125 to +134
if data:
device_dict = data.get("device_dict")
if device_dict and isinstance(device_dict, dict):
if device_dict.get("speaker_device_id") == speaker_device_id:
return device_dict
else:
continue
else:
return default
return None
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_device_dict_by_speaker_device_id returns default immediately when it encounters the first ha_device_id whose stored data is falsy. That prematurely stops the search and can cause false negatives whenever the first entry is empty/partially initialized. This should continue the loop (or just skip falsy entries) instead of returning early.

Suggested change
if data:
device_dict = data.get("device_dict")
if device_dict and isinstance(device_dict, dict):
if device_dict.get("speaker_device_id") == speaker_device_id:
return device_dict
else:
continue
else:
return default
return None
if not data:
# Skip entries without any stored data
continue
device_dict = data.get("device_dict")
if device_dict and isinstance(device_dict, dict):
if device_dict.get("speaker_device_id") == speaker_device_id:
return device_dict
return default

Copilot uses AI. Check for mistakes.
Comment on lines +61 to +69
# Status information
connection_status: ConnectionStatus = ConnectionStatus.ONLINE
last_seen: datetime

# Master-slave relationship
master_speaker_device_id: Optional[str] = None # Master speaker ID
slave_device_num: int = 0
slave_device_list: list[SlaveDeviceInfo]

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HIVIDevice has required fields without defaults (last_seen and slave_device_list). Since device dicts are loaded from storage and may not contain these keys (e.g., after schema changes or partial writes), constructing HIVIDevice(**device_dict) can raise validation errors and break startup. Consider providing defaults (e.g., last_seen default factory, slave_device_list default factory) or handling missing fields when loading.

Copilot uses AI. Check for mistakes.
finally:
self._handle_discovery_worker = None

await self.device_data_registry.async_clear_all_data()
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async_cleanup() calls device_data_registry.async_clear_all_data(), which clears the persistent storage file. Since async_cleanup() is invoked from async_unload_entry, this will erase cached device data on every unload/reload (and potentially during shutdown), which is unexpected and can cause devices to be re-created/lose state. Persistent storage cleanup should usually only happen on config entry removal (async_remove_entry), not unload.

Suggested change
await self.device_data_registry.async_clear_all_data()

Copilot uses AI. Check for mistakes.
Comment on lines +97 to +103
# Mock service call so flow does not depend on integration being fully set up
with patch.object(
hass.services,
"async_call",
new_callable=AsyncMock,
return_value=None,
):
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test patches hass.services.async_call but never asserts it was called with the expected domain/service when confirm_refresh is True. As written, the test would still pass even if the options flow stopped triggering the refresh service. Capture the patched mock and assert the expected call (and consider asserting it’s not called in the confirm_refresh=False test too).

Copilot uses AI. Check for mistakes.
Comment on lines +363 to +367
# device_obj = HIVIDevice(**device_dict)
# if device_obj.speaker_device_id in slave_device_uuid_set:
_LOGGER.debug(
"device %s is recognized as a slave device, will delete its device and entities",
device_obj.friendly_name,
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the “Delete slave devices” loop, the log message references device_obj.friendly_name, but device_obj is not defined in that scope (it’s only defined in the earlier loop). If no devices were processed above, this will raise NameError; even if it doesn’t, it will log the wrong device name. Fetch the device dict for ha_device_id in this loop (or log using ha_device.name).

Suggested change
# device_obj = HIVIDevice(**device_dict)
# if device_obj.speaker_device_id in slave_device_uuid_set:
_LOGGER.debug(
"device %s is recognized as a slave device, will delete its device and entities",
device_obj.friendly_name,
device_name = ha_device.name or ha_device_id
_LOGGER.debug(
"device %s is recognized as a slave device, will delete its device and entities",
device_name,

Copilot uses AI. Check for mistakes.
Comment on lines +187 to +200
# HIVI speaker specific SOAP format
args_xml = ""
for key, value in kwargs.items():
args_xml += f"<{key}>{value}</{key}>"

# Use correct namespace
soap_envelope = f'''<?xml version="1.0" encoding="utf-8"?>
<s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/" s:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">
<s:Body>
<u:{action} xmlns:u="{service_type}">
{args_xml}
</u:{action}>
</s:Body>
</s:Envelope>'''
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_soap_request builds args_xml by string-concatenating raw values into XML (f"<{key}>{value}</{key}>"). If any value contains XML special characters (e.g., &, <, >), the SOAP envelope becomes invalid and requests will fail. Escape/encode values (or use an XML builder) before embedding them into the SOAP body.

Copilot uses AI. Check for mistakes.
Comment on lines +212 to +218
# Use shorter timeout to avoid long blocking
timeout = aiohttp.ClientTimeout(total=8, connect=3, sock_read=5)

# Create new temporary session for each request to avoid connection pool issues
async with aiohttp.ClientSession(timeout=timeout) as temp_session:
async with temp_session.post(
url, data=soap_envelope.encode("utf-8"), headers=headers
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_soap_request creates a brand-new aiohttp.ClientSession for every SOAP call. This is expensive and can leak sockets/FDs under load; it also defeats connection pooling. Reuse the existing session (self._session) or use homeassistant.helpers.aiohttp_client.async_get_clientsession with per-request timeouts instead.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 19 out of 21 changed files in this pull request and generated 1 comment.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 21 changed files in this pull request and generated 2 comments.



@pytest.fixture
def mock_setup_entry() -> Generator[AsyncMock]:
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type annotation for this fixture is incomplete. Generator requires three type parameters: Generator[YieldType, SendType, ReturnType]. This should be Generator[AsyncMock, None, None] to be properly type-hinted according to the guidelines which state "Ensure all test function parameters have type annotations. Prefer concrete types over Any."

Suggested change
def mock_setup_entry() -> Generator[AsyncMock]:
def mock_setup_entry() -> Generator[AsyncMock, None, None]:

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 21 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 21 changed files in this pull request and generated 1 comment.



@pytest.fixture
def mock_setup_entry() -> Generator[AsyncMock]:
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Generator type hint should include all three type parameters: Generator[YieldType, SendType, ReturnType]. For a generator that yields an AsyncMock and returns None, use Generator[AsyncMock, None, None] instead of Generator[AsyncMock].

Suggested change
def mock_setup_entry() -> Generator[AsyncMock]:
def mock_setup_entry() -> Generator[AsyncMock, None, None]:

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 21 out of 23 changed files in this pull request and generated 1 comment.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated no new comments.

@swansmart
Copy link
Copy Markdown
Author

Hi @joostlek,

This PR has been quiet for a while. CI is green and I’m happy to address any further feedback whenever you have time.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated 1 comment.

Comment on lines +302 to +323
ssid = master_device.ssid
wifi_channel = master_device.wifi_channel
auth = master_device.auth_mode
encry = master_device.encryption_mode
psk = master_device.psk
master_ip = master_device.ip_addr
uuid = master_device.uuid
operation_data = {
"type": "set_slave",
"master": self._master_speaker_device_id,
"slave": self._slave_speaker_device_id,
"expected_state": "slave",
"params": {
"slave_ip": slave_ip,
"ssid": ssid,
"wifi_channel": wifi_channel,
"auth": auth,
"encry": encry,
"psk": psk,
"master_ip": master_ip,
"uuid": uuid,
},
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code accesses nullable device properties (ssid, auth_mode, encryption_mode, psk, uuid) without null checks. According to the HIVIDevice model, these fields can be None, but they are passed directly in the operation_data dict at lines 302-323. This could cause a type mismatch or runtime error when the operation is executed. Either add validation/null checks before using these values, or ensure the device state guarantees these fields are non-null by the time async_turn_on is called.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added verification checks.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants