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

Config structure for embedded platforms #142

Open
balloob opened this Issue Feb 7, 2019 · 14 comments

Comments

Projects
None yet
6 participants
@balloob
Copy link
Member

balloob commented Feb 7, 2019

It's nice that we streamline the ways of introducing new component and/or platform modules to home assistant. Now, we know that there should always be a new component package and a platform should go under that component.

But what about config structure? Should we keep the platforms (itach) under a domain (remote) config structure also for new platforms? Or should we ask to set up a component that has the config?

Originally posted by @MartinHjelmare in home-assistant/home-assistant#20809 (comment)

@balloob

This comment has been minimized.

Copy link
Member Author

balloob commented Feb 7, 2019

If we have itach/remote.py, I think that it would make sense to configure it via an itach component moving forward.

@MartinHjelmare MartinHjelmare referenced this issue Feb 7, 2019

Merged

Move weather.ipma into a component #20706

9 of 9 tasks complete
@dgomes

This comment has been minimized.

Copy link
Member

dgomes commented Feb 7, 2019

I think the rule should apply to new components and leave as is for the remainders.

After all this will be a breaking change to many setups.

@pvizeli

This comment has been minimized.

Copy link
Member

pvizeli commented Feb 7, 2019

I think the future of config is the config flow and they should be preferred for new integration. The old way for config should be migrated step by step, and we don't change things on the old world.

@balloob

This comment has been minimized.

Copy link
Member Author

balloob commented Feb 7, 2019

@dgomes for sure! Ain't nobody got time to even do all those breaking changes.

@OttoWinter

This comment has been minimized.

Copy link
Member

OttoWinter commented Feb 8, 2019

Is there an example for how this would look like config-wise? Something like this?

itach:
  # itach settings
  remotes:
  - # itach remote setting

What would this look like for components that have platforms in multiple domains (for example MQTT; I'm in no way suggesting breaking change there :D )

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 8, 2019

See the rainmachine component for example.

@amelchio

This comment has been minimized.

Copy link
Member

amelchio commented Feb 14, 2019

If we have itach/remote.py, I think that it would make sense to configure it via an itach component moving forward.

+1. I agree that it would make sense. My reasoning is one of consistency, though ... I don't think the source code layout should dictate the configuration format.

For contributors, I also think it would be very good to always start out on the right path. I can be quite discouraging to throw away work and make a breaking change ("graduate to component"), maybe just 1 release after the initial support.

Config-wise, I want to suggest using the domain name for subsections. So "remote" rather than "remotes" (and "sensor" rather than the "sensors" of rainmachine). This will avoid a breaking change when moving from supporting a single entity to several. I also think it matches the existing YAML well and it avoids some hard questions about the plural naming of things like "notify" and "climate".

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 20, 2019

So the proposed config structure would be:

itach:
  # itach settings
  remote:
    # itach remote settings
  sensor:
    # itach sensor settings

Are we ready to enforce this for new integrations, or should we hold off?

@amelchio

This comment has been minimized.

Copy link
Member

amelchio commented Mar 20, 2019

Well, I do think we can go with this but I tried to migrate netgear_lte (home-assistant/home-assistant#22105) and I guess some things are still not clear to me.

  1. One can control several LTE modems so the component config becomes a list. I assume this is okay.
  2. How should multiple sensors be specified? (i.e. an answer to #13 #12 in this new world)
  3. There is no longer a (general) way to specify scan_interval. This may be okay but I think we need a prepared response as to why it is being removed (my own answer currently mentions a time_pattern automation calling homeassistant.update_entity)

My current layout is this:

netgear_lte:
  - host: !secret router_hostname
    password: !secret router_password
    notify:
      - name: sms
        recipient: !secret admin_phone
    sensor:
      monitored_conditions:
        - usage
        - sms
  - host: !secret failover_modem_hostname
    password: !secret failover_modem_password
    notify:
      - name: failover_sms
        recipient: !secret admin_phone
    sensor:
      monitored_conditions:
        - usage
@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 20, 2019

I don't understand question 2. Can you elaborate?

@amelchio

This comment has been minimized.

Copy link
Member

amelchio commented Mar 20, 2019

Oops, that was a typo. I meant to reference #12.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 20, 2019

Sorry, I'm a bit slow today it seems. I don't see why there would be more than one sensor key per host, taking your example as base. Can you post what the alternatives would be in our case here?

@amelchio

This comment has been minimized.

Copy link
Member

amelchio commented Mar 20, 2019

Sure, here are two examples.

netgear_lte:
  sensor:
    - usage
    - sms
netgear_lte:
  sensor:
    usage:
      #scan_interval: 300
    sms:
      #unread: True

Are you thinking that anything below sensor: should just be up to the integration, like in #12?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 20, 2019

Yeah, I think that can be up to each integration to decide what fits best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.