Reconfigure MQTT binary_sensor component if discovery info is changed

[emontnemery]

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.

[OttoWinter]

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

[emontnemery]

Fixed (I think?)

[emontnemery]

@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]

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

[OttoWinter]

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)

[emontnemery]

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.

[OttoWinter]

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)

[balloob]

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
  }
}

[emontnemery]

Fixed.

[balloob]

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.

[emontnemery]

OK, fixed.

[balloob]

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.

[emontnemery]

Ok, rewritten as you suggest.

[emontnemery]

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

[emontnemery]

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

[balloob]

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