Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions homeassistant/components/huawei_lte/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
)
from url_normalize import url_normalize

from homeassistant.components.binary_sensor import DOMAIN as BINARY_SENSOR_DOMAIN
from homeassistant.components.device_tracker import DOMAIN as DEVICE_TRACKER_DOMAIN
from homeassistant.components.notify import DOMAIN as NOTIFY_DOMAIN
from homeassistant.components.sensor import DOMAIN as SENSOR_DOMAIN
Expand Down Expand Up @@ -50,6 +51,7 @@
KEY_DEVICE_INFORMATION,
KEY_DEVICE_SIGNAL,
KEY_DIALUP_MOBILE_DATASWITCH,
KEY_MONITORING_STATUS,
KEY_MONITORING_TRAFFIC_STATISTICS,
KEY_WLAN_HOST_LIST,
UPDATE_OPTIONS_SIGNAL,
Expand Down Expand Up @@ -97,6 +99,13 @@
extra=vol.ALLOW_EXTRA,
)

CONFIG_ENTRY_PLATFORMS = (
BINARY_SENSOR_DOMAIN,
DEVICE_TRACKER_DOMAIN,
SENSOR_DOMAIN,
SWITCH_DOMAIN,
)


@attr.s
class Router:
Expand Down Expand Up @@ -161,6 +170,7 @@ def get_data(key: str, func: Callable[[None], Any]) -> None:
get_data(KEY_DEVICE_BASIC_INFORMATION, self.client.device.basic_information)
get_data(KEY_DEVICE_SIGNAL, self.client.device.signal)
get_data(KEY_DIALUP_MOBILE_DATASWITCH, self.client.dial_up.mobile_dataswitch)
get_data(KEY_MONITORING_STATUS, self.client.monitoring.status)
get_data(
KEY_MONITORING_TRAFFIC_STATISTICS, self.client.monitoring.traffic_statistics
)
Expand Down Expand Up @@ -276,7 +286,7 @@ def signal_update() -> None:
router.subscriptions.clear()

# Forward config entry setup to platforms
for domain in (DEVICE_TRACKER_DOMAIN, SENSOR_DOMAIN, SWITCH_DOMAIN):
for domain in CONFIG_ENTRY_PLATFORMS:
hass.async_create_task(
hass.config_entries.async_forward_entry_setup(config_entry, domain)
)
Expand Down Expand Up @@ -319,7 +329,7 @@ async def async_unload_entry(
"""Unload config entry."""

# Forward config entry unload to platforms
for domain in (DEVICE_TRACKER_DOMAIN, SENSOR_DOMAIN, SWITCH_DOMAIN):
for domain in CONFIG_ENTRY_PLATFORMS:
await hass.config_entries.async_forward_entry_unload(config_entry, domain)

# Forget about the router and invoke its cleanup
Expand Down
122 changes: 122 additions & 0 deletions homeassistant/components/huawei_lte/binary_sensor.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
"""Support for Huawei LTE binary sensors."""

import logging
from typing import Optional

import attr
from huawei_lte_api.enums.cradle import ConnectionStatusEnum

from homeassistant.components.binary_sensor import (
DOMAIN as BINARY_SENSOR_DOMAIN,
BinarySensorDevice,
)
from homeassistant.const import CONF_URL
from . import HuaweiLteBaseEntity
from .const import DOMAIN, KEY_MONITORING_STATUS


_LOGGER = logging.getLogger(__name__)


async def async_setup_entry(hass, config_entry, async_add_entities):
"""Set up from config entry."""
router = hass.data[DOMAIN].routers[config_entry.data[CONF_URL]]
entities = []

if router.data.get(KEY_MONITORING_STATUS):
entities.append(HuaweiLteMobileConnectionBinarySensor(router))

async_add_entities(entities, True)


@attr.s
class HuaweiLteBaseBinarySensor(HuaweiLteBaseEntity, BinarySensorDevice):
"""Huawei LTE binary sensor device base class."""

key: str
item: str
_raw_state: Optional[str] = attr.ib(init=False, default=None)

async def async_added_to_hass(self):
"""Subscribe to needed data on add."""
await super().async_added_to_hass()
self.router.subscriptions[self.key].add(f"{BINARY_SENSOR_DOMAIN}/{self.item}")

async def async_will_remove_from_hass(self):
"""Unsubscribe from needed data on remove."""
await super().async_will_remove_from_hass()
self.router.subscriptions[self.key].remove(
f"{BINARY_SENSOR_DOMAIN}/{self.item}"
)

async def async_update(self):
"""Update state."""
try:
value = self.router.data[self.key][self.item]
except KeyError:
_LOGGER.debug("%s[%s] not in data", self.key, self.item)
self._available = False
return
self._available = True
self._raw_state = str(value)


CONNECTION_STATE_ATTRIBUTES = {
str(ConnectionStatusEnum.CONNECTING): "Connecting",
str(ConnectionStatusEnum.DISCONNECTING): "Disconnecting",
str(ConnectionStatusEnum.CONNECT_FAILED): "Connect failed",
str(ConnectionStatusEnum.CONNECT_STATUS_NULL): "Status not available",
str(ConnectionStatusEnum.CONNECT_STATUS_ERROR): "Status error",
}


@attr.s
class HuaweiLteMobileConnectionBinarySensor(HuaweiLteBaseBinarySensor):
"""Huawei LTE mobile connection binary sensor."""

def __attrs_post_init__(self):
"""Initialize identifiers."""
self.key = KEY_MONITORING_STATUS
self.item = "ConnectionStatus"

@property
def _entity_name(self) -> str:
return "Mobile connection"

@property
def _device_unique_id(self) -> str:
return f"{self.key}.{self.item}"

@property
def is_on(self) -> bool:
"""Return whether the binary sensor is on."""
return self._raw_state and int(self._raw_state) in (
ConnectionStatusEnum.CONNECTED,
ConnectionStatusEnum.DISCONNECTING,
)

@property
def assumed_state(self) -> bool:
"""Return True if real state is assumed, not known."""
return not self._raw_state or int(self._raw_state) not in (
ConnectionStatusEnum.CONNECT_FAILED,
ConnectionStatusEnum.CONNECTED,
ConnectionStatusEnum.DISCONNECTED,
)

@property
def icon(self):
"""Return mobile connectivity sensor icon."""
return "mdi:signal" if self.is_on else "mdi:signal-off"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Device class entity icons should not be overridden.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is already being done for pretty much all Huawei LTE sensors and switches.

Copy link
Copy Markdown
Member Author

@scop scop Oct 26, 2019

Choose a reason for hiding this comment

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

To elaborate:

If I leave this out, the default icon for a connectivity class binary sensor is something clearly related to wired connections, which is bad for a mobile connectivity sensor.

It would be also very different from the closely related mobile data switch, which would be bad too. Now it's the same, which is consistent and good.

If I remove it from mobile data switch as well, then its icon becomes the lightning one which isn't a good one for a mobile connectivity switch, and it would also be very different from any choices I have for the mobile connection sensor, so things would get even worse.

Note also that this icon is solely for the mobile connection sensor, it's not in the base binary sensor class. The method docstring could be improved to say it's the icon for mobile connection sensor, not binary sensor. Done now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll ask for a second opinion.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Any progress, can we move forward here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do we have some conclusion here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Device classes are not just icons, they also define what data is represented. It means it impacts how systems that built on top of Home Assistant represent it, like Google, Alexa or Almond.

So if the icon does not represent the device class properly, we should consider updating the device class icon

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For generic connectivity, I don't think there's anything wrong per se with the current icon. But for mobile connectivity, it's not a good one. And I don't want to get into the bikeshed whether one better describing mobile connectivity (e.g. the one I used here) would be acceptable for all connectivity.

I guess a kind of a sweet spot would be to be able to define a mobile connectivity device class that would inherit from the generic connectivity device class and have a different icon (possibly also some other differences/additional behavior or data sometime), although I suppose (without digging around) that that's not an option with the current device class implementation. And I think it's out of the scope for this pull request, and neither would I want to wait for it to come up before getting this in.

So at the moment my practical choices boil down to:

  1. use connectivity device class, override icon (this is what the PR does now)
  2. drop connectivity device class altogether, keep the icon I like
  3. use connectivity device class, remove icon override (this is what Martin suggested)
  4. wait for a new device class with the icon I like to be introduced

My favourite, by far, would be 1, then 4 sometime later when/if it's available. Going through 2 or 3 eventually to 4 when it's around would in my opinion in the meantime introduce either suboptimal user experience with the icon as elaborated earlier, or possible loss of some functionality caused by a missing device class. Just waiting for 4 before introducing this is an unnecessary delay.

If this can't go in its current form (i.e. 1 above) I'm currently torn between removing the icon and getting this approved, or just keeping the implementation to myself until 4 comes up. But I'd appreciate a clear decision/conclusion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer to go with 2 while we wait till 4 is available. The device classes are not extensively used. I think the only thing that I know that uses it now is Almond, for which you'll soon be able to ask if X is connected. Changing a device class is technically a breaking change, adding one is not.

We don't differentiate or inherit device classes, because the extra complexity will cause pain in other systems relying on it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Device class dropped.

At least in one sense inheriting a device class will cause less complexity to consumers than adding a new one. For example if "mobile connectivity" was extended from "connectivity", systems interesting in any kind of connectivity would just have to check for "connectivity" instead of both.


@property
def device_state_attributes(self):
"""Get additional attributes related to connection status."""
attributes = super().device_state_attributes
if self._raw_state in CONNECTION_STATE_ATTRIBUTES:
if attributes is None:
attributes = {}
attributes["additional_state"] = CONNECTION_STATE_ATTRIBUTES[
self._raw_state
]
return attributes
5 changes: 4 additions & 1 deletion homeassistant/components/huawei_lte/const.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@
KEY_DEVICE_INFORMATION = "device_information"
KEY_DEVICE_SIGNAL = "device_signal"
KEY_DIALUP_MOBILE_DATASWITCH = "dialup_mobile_dataswitch"
KEY_MONITORING_STATUS = "monitoring_status"
KEY_MONITORING_TRAFFIC_STATISTICS = "monitoring_traffic_statistics"
KEY_WLAN_HOST_LIST = "wlan_host_list"

BINARY_SENSOR_KEYS = {KEY_MONITORING_STATUS}

DEVICE_TRACKER_KEYS = {KEY_WLAN_HOST_LIST}

SENSOR_KEYS = {
Expand All @@ -27,4 +30,4 @@

SWITCH_KEYS = {KEY_DIALUP_MOBILE_DATASWITCH}

ALL_KEYS = DEVICE_TRACKER_KEYS | SENSOR_KEYS | SWITCH_KEYS
ALL_KEYS = BINARY_SENSOR_KEYS | DEVICE_TRACKER_KEYS | SENSOR_KEYS | SWITCH_KEYS