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

Reconfigure MQTT binary_sensor component if discovery info is changed #18169

Merged
merged 10 commits into from Nov 19, 2018

Conversation

Projects
None yet
5 participants
@emontnemery
Contributor

emontnemery commented Nov 3, 2018

Description:

Reconfigure component if discovery info is changed.
This is bullet 2 in home-assistant/architecture#70

PRs for other platforms will be opened separately.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox.
  • Tests have been added to verify that the new code works.
Show resolved Hide resolved homeassistant/components/mqtt/__init__.py
@@ -864,9 +864,14 @@ def available(self) -> bool:
class MqttDiscoveryUpdate(Entity):
"""Mixin used to handle updated discovery message."""
def __init__(self, discovery_hash) -> None:
def __init__(self, discovery_hash, discovery_payload=None,
async_discover=None) -> None:

This comment has been minimized.

@OttoWinter

OttoWinter Nov 4, 2018

Contributor

I don't like this that much. Passing this async_discover callback will become quite a mess once we start implementing MQTT config entry unloading. For now it's probably ok though

This comment has been minimized.

@emontnemery

emontnemery Nov 4, 2018

Contributor

Fixed (I think?)

Show resolved Hide resolved homeassistant/components/binary_sensor/mqtt.py Outdated
Show resolved Hide resolved homeassistant/components/mqtt/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/mqtt/__init__.py Outdated
@emontnemery

This comment has been minimized.

Contributor

emontnemery commented Nov 4, 2018

@OttoWinter Thanks a lot for the comments!
The code is modified as you proposed such that the component is updated instead of removed+added.

Some questions:

  • Does it make sense to add a method _setup_from_config also to MqttAvailability?
  • What to do if device is changed, add some handling to MqttEntityDeviceInfo?
  • What to do if unique_id is changed?
  • In MqttDiscoveryUpdate I'm checking if self._discovery_payload != payload, but I think that's quite redundant, no need to check?

@emontnemery emontnemery force-pushed the emontnemery:mqtt_discovery_update_binary_sensor branch from aa53d67 to ac19a0b Nov 4, 2018

@emontnemery emontnemery changed the title from Recreate binary_sensor component if discovery info is changed to Reconfigure MQTT binary_sensor component if discovery info is changed Nov 4, 2018

@emontnemery emontnemery force-pushed the emontnemery:mqtt_discovery_update_binary_sensor branch from b357dce to 61541a0 Nov 4, 2018

@emontnemery emontnemery force-pushed the emontnemery:mqtt_discovery_update_binary_sensor branch from 61541a0 to 0542b2d Nov 4, 2018

self._unique_id = config.get(CONF_UNIQUE_ID)
# TODO: Handle changed device?
# config.get(CONF_DEVICE),

This comment has been minimized.

@emontnemery

emontnemery Nov 7, 2018

Contributor

@OttoWinter What do you suggest to do if device info is changed, just update the MqttEntityDeviceInfo?

This comment has been minimized.

@OttoWinter

OttoWinter Nov 7, 2018

Contributor

Why not the same as availability_discovery_update, just called device_info_discovery_update - this method would set the internal self._device_info object to the latest values.

(It might be the case though that the device_info is only read when the device is initially added to hass, then I guess we ignore the update to this attribute, as it's probably not used too often)

This comment has been minimized.

@emontnemery

emontnemery Nov 8, 2018

Contributor

It might be the case though that the device_info is only read when the device is initially added to hass

Yes, this seems to be the cased based on some quick testing.
Either just ignore this case as you suggest, or delete + re-add the component.

Show resolved Hide resolved homeassistant/components/mqtt/discovery.py
self._unique_id = config.get(CONF_UNIQUE_ID)
# TODO: Handle changed device?
# config.get(CONF_DEVICE),

This comment has been minimized.

@OttoWinter

OttoWinter Nov 7, 2018

Contributor

Why not the same as availability_discovery_update, just called device_info_discovery_update - this method would set the internal self._device_info object to the latest values.

(It might be the case though that the device_info is only read when the device is initially added to hass, then I guess we ignore the update to this attribute, as it's probably not used too often)

@bind_hass
async def async_subscribe_topics(hass: HomeAssistantType, sub_state: dict,
new_topics: dict,
msg_callback: MessageCallbackType,

This comment has been minimized.

@balloob

balloob Nov 9, 2018

Member

We should not have a generic msg callback for all topics. For example, this won't work for the MQTT light, as it has a different callback for brightness and state.

I would expect new_topics to be called topics (as there might nothing be new about it) and be defined like this:

{
  '/home/light/kitchen/state': {
    'qos': 2,
    'encoding': 'utf-8',
    'msg_callback': state_received
  }
}

This comment has been minimized.

@emontnemery

emontnemery Nov 10, 2018

Contributor

Fixed.

State is kept in sub_state.
"""
if sub_state is None:
sub_state = {'topics': {}}

This comment has been minimized.

@balloob

balloob Nov 9, 2018

Member

No reason to wrap it in a dictionary. It's an opaque object to the outside world, inside this method we always know the implementation.

This comment has been minimized.

@emontnemery

emontnemery Nov 10, 2018

Contributor

OK, fixed.

for key, topic in new_topics.items():
if key not in old_topics and topic is not None:
unsub = await mqtt.async_subscribe(hass, topic, msg_callback, qos)
old_topics[key] = (topic, unsub)

This comment has been minimized.

@balloob

balloob Nov 9, 2018

Member

it's kinda confusing that we have an object called old_topics that we're writing to. That's also why we need to wrap the for-loop with a list around values.

We should rename old_topic to cur_topics and create a new state dictionary. Then just while iterating over the new_topics param, we pop keys from cur_topics. When done with the for-loop, anything that's left in cur_topics can be unsubscribed.

This comment has been minimized.

@emontnemery

emontnemery Nov 10, 2018

Contributor

Ok, rewritten as you suggest.

@emontnemery

This comment has been minimized.

Contributor

emontnemery commented Nov 10, 2018

@balloob Thanks a lot for the review! subscription.py rewritten as you suggest, and tests added.

@emontnemery

This comment has been minimized.

Contributor

emontnemery commented Nov 15, 2018

@balloob Do you consider this ready for merge now, or are additional changes needed?

@balloob balloob merged commit de9bac9 into home-assistant:dev Nov 19, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 93.066%
Details

@wafflebot wafflebot bot removed the in progress label Nov 19, 2018

@balloob

This comment has been minimized.

Member

balloob commented Nov 19, 2018

Looks good! Delayed because I was taking a week off GitHub :)

mxworm added a commit to mxworm/home-assistant that referenced this pull request Nov 19, 2018

Merge branch 'dev' of https://github.com/home-assistant/home-assistant
…into dev

* 'dev' of https://github.com/home-assistant/home-assistant:
  Bumped ghlocalapi to 0.1.0 (home-assistant#18584)
  Prefix all xiaomi_aqara events (home-assistant#17354)
  Fix MQTT async_added_to_hass (home-assistant#18575)
  Add mikrotik SSL support (home-assistant#17898)
  Reconfigure MQTT binary_sensor component if discovery info is changed (home-assistant#18169)
  Darksky: Expose missing conditions for day 0 forecast (home-assistant#18312)
  Update pyhomematic to 0.1.52 and add features for lights (home-assistant#18499)
  Fix for epson state not updating (home-assistant#18357)
  Support for Point component (home-assistant#17466)
  Template binary sensor to not track all state changes (home-assistant#18573)
  Correct cached stale device tracker handling (home-assistant#18572)
  Add support for sessions (home-assistant#18518)
  Remove turn_on and turn_off feature for clients (home-assistant#18234)
  Log delay and wait_template steps in scripts (home-assistant#18448)
  Logbook speedup (home-assistant#18376)
  Avoid race in entity_platform.async_add_entities() (home-assistant#18445)
  Fix small issue related to topic prefix (home-assistant#18512)
  Enable native support + ADB authentication for Fire TV (home-assistant#17767)
  Re-adding the season attribute (home-assistant#18523)

emontnemery added a commit to emontnemery/home-assistant that referenced this pull request Nov 28, 2018

emontnemery added a commit to emontnemery/home-assistant that referenced this pull request Nov 28, 2018

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

OttoWinter added a commit that referenced this pull request Nov 30, 2018

balloob added a commit that referenced this pull request Nov 30, 2018

@balloob balloob referenced this pull request Nov 30, 2018

Merged

0.83.2 #18857

mxworm added a commit to mxworm/home-assistant that referenced this pull request Dec 1, 2018

Merge branch 'dev' of https://github.com/home-assistant/home-assistant
…into dev

* 'dev' of https://github.com/home-assistant/home-assistant: (122 commits)
  Reintroduce unique_id for Netatmo sensor (home-assistant#18774)
  Fix ordinal filter in template (home-assistant#18878)
  fixed state case for rtorrent (home-assistant#18778)
  Updated UVC camera component to support SSL connections (home-assistant#18829)
  Added new filters for templates (home-assistant#18125)
  Add support for zha device registry (home-assistant#18755)
  Add tests for directv platform (home-assistant#18590)
  Fix issues with 17track.net sensor names (home-assistant#18860)
  Bump py17track to 2.1.1 (home-assistant#18861)
  RFC: Call services directly (home-assistant#18720)
  Fix flaky geofency test (home-assistant#18855)
  Update of volvooncall component (home-assistant#18702)
  Always set hass_user (home-assistant#18844)
  Hotfix for Fibaro wall plug (home-assistant#18845)
  Add services description (home-assistant#18839)
  update netatmo library (home-assistant#18823)
  Revert change to MQTT discovery_hash introduced in home-assistant#18169 (home-assistant#18763)
  bump gtts-token to 1.1.3 (home-assistant#18824)
  Add Entur departure information sensor (home-assistant#17286)
  Bump python_awair to 0.0.3 (home-assistant#18819)
  ...

@emontnemery emontnemery deleted the emontnemery:mqtt_discovery_update_binary_sensor branch Dec 2, 2018

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