Skip to content
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

Move more MQTT platforms to config entries #16918

Merged
merged 2 commits into from
Sep 28, 2018

Conversation

OttoWinter
Copy link
Member

Description:

#16904, but for these new platforms:

  • binary_sensor
  • camera
  • cover
  • light
  • sensor
  • switch
  • alarm_control_panel

fan and lock need to be dealt with later because their base classes haven't been converted to config entries yet.

Checklist:

  • The code change is tested and works locally. It's a rather simple change, and I have tested the sensor platform quite well.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

async_discover)


async def _async_setup_platform(hass, config, async_add_entities,
Copy link
Member

Choose a reason for hiding this comment

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

It should just be called _async_setup_entity(hass, async_add_entities, config) (in all the platforms).

Inside async_setup_platform you know that it's always config and no longer discovery_info (as that's moved to config entries). In the setup entry discover, you know that config is always the config.

@OttoWinter OttoWinter force-pushed the mqtt-config-entry-platforms branch from 9dbd7b9 to e60e2b0 Compare September 28, 2018 10:21
"""Discover and add an MQTT alarm control panel."""
config = PLATFORM_SCHEMA(discovery_payload)
await _async_setup_entity(hass, config, async_add_entities,
discovery_payload[ATTR_DISCOVERY_HASH])
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the discovery_hash attribute into this code block since it only affects components discovered through MQTT discovery. If you wish, I could also do it like this:

async def async_setup_entry(hass, config_entry, async_add_entities):
    async def async_discover(discovery_payload):
        config = PLATFORM_SCHEMA(discovery_payload)
        await _async_setup_entity(hass, config, async_add_entities)

async def _async_setup_entity(hass, config, async_add_entities):
    async_add_entities([MqttAlarm(
        # ...
        config.get(ATTR_DISCOVERY_HASH)
    )])


async def _async_setup_entity(hass: HomeAssistantType, config: ConfigType,
async_add_entities, discovery_hash=None):
"""Set up MQTT sensor."""
Copy link
Member Author

Choose a reason for hiding this comment

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

Also changed the method name for sensor since it's more or less a simple rename. Let me know if I should split it off into another PR. I think I need to re-learn what is acceptable to be in one PR and what's not 😅

@balloob balloob merged commit af89e7c into home-assistant:dev Sep 28, 2018
@ghost ghost removed the in progress label Sep 28, 2018
@emontnemery
Copy link
Contributor

emontnemery commented Sep 29, 2018

@OttoWinter After this is merged, MQTT discovery only accepts a single device per platform (one binary_sensor, one light, one switch, etc).
Is it a bug, or does the configuration need to be udpated somehow?

Example of error of the 2nd switch (and all subsequent):

2018-09-29 09:58:19 ERROR (MainThread) [homeassistant.config_entries] Error setting up entry configuration.yaml for switch
Traceback (most recent call last):
  File "/mnt/d/development/github/home-assistant_fork/homeassistant/config_entries.py", line 225, in async_setup
    result = await component.async_setup_entry(hass, self)
  File "/mnt/d/development/github/home-assistant_fork/homeassistant/components/switch/__init__.py", line 84, in async_setup_entry
    return await hass.data[DOMAIN].async_setup_entry(entry)
  File "/mnt/d/development/github/home-assistant_fork/homeassistant/helpers/entity_component.py", line 108, in async_setup_entry
    raise ValueError('Config entry has already been setup!')
ValueError: Config entry has already been setup!

@OttoWinter
Copy link
Member Author

@emontnemery Hmm, it only happens to platforms that you also have entries in your configuration.yaml for, right?

It seems like the check in entity_component.py when forwarding a condig entry is because the component is already loaded as a normal platform through the config file, but not as a config entry. That check probably needs to be fixed in the entity component file.

@emontnemery
Copy link
Contributor

@OttoWinter It's reproduced also with an empty configuration.yaml
Should I open an issue for this with some dumps of mqtt discovery data to reproduce?

@OttoWinter
Copy link
Member Author

@emontnemery Yes that would be good. I can't replicate the issue here.

@emontnemery
Copy link
Contributor

@OttoWinter Perhaps it is timing related: There's no issue if I manually publish discovery topics, but I can reproduce if the discovery messages are retained when hass starts. Issue here: #16968

@emontnemery
Copy link
Contributor

@OttoWinter Potential fix in #16987

@MartinHjelmare
Copy link
Member

Merged PRs should not be used for enhancement discussion or bug reports. If you've found a bug it's ok to make a review with inline comments and link to an issue that reports the bug.

Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Sep 30, 2018
@OttoWinter OttoWinter deleted the mqtt-config-entry-platforms branch December 16, 2018 14:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants