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

Migrate to generic discovery method #2271

Merged
merged 2 commits into from
Jun 12, 2016
Merged

Migrate to generic discovery method #2271

merged 2 commits into from
Jun 12, 2016

Conversation

balloob
Copy link
Member

@balloob balloob commented Jun 11, 2016

The only thing that has changed… is everything.

@kellerza introduced a generic load platform discovery mechanism for the Entity Component a couple of releases ago. This makes our existing hardcoded platform discovery obsolete so this PR removes it.

This is my first pass at it. I bet I made some mistakes…

@balloob balloob force-pushed the remove-normal-discovery branch 3 times, most recently from 2623775 to 4a31cfc Compare June 11, 2016 07:59
@kellerza
Copy link
Member

Going through it it looks good. Even added a tox test for load_platform. I never got to it and must say not sure I understand the mock calls yet.

I was wondering if load_platform should not rather be in homeassistant.util or homeassistant.helper?

When I started out discovery through discovery: in the config and all this "discovery" which is actually just a way of loading platforms from components was quite confusing. Even some of the component help on the websites adds to this confusion.

Some used discover, some not (but seems like you got them all here now) and overall its more uniform.

The fact that EntityComponent, which is part of the core, is dependant on a homeassistant.component.discovery feels wrong. homeassistant.components adds functionality, but core functionality should not be dependant on homeassistant.components in my view

@balloob balloob force-pushed the remove-normal-discovery branch from 4a31cfc to 25b7611 Compare June 11, 2016 20:18
@balloob
Copy link
Member Author

balloob commented Jun 11, 2016

You are so right. I will be moving all the discovery things to a discovery helper. The discovery component will solely exist to interface with netdisco and forward discoveries to the correct component/platform.

@balloob balloob force-pushed the remove-normal-discovery branch from 25b7611 to d00d0d7 Compare June 11, 2016 20:35
@kellerza
Copy link
Member

Great, if you can please add qwikswitch.py import this should be good. Did a search its the only component that uses it atm.

@kellerza
Copy link
Member

One extra bit that would be nice is a way to pass objects around, I know you said you only want it serializable on the event bus.

I add the below code to some of my custom components (qwikswitch is only used since it can be imported, custom components not) and it then gives me a global variable -- there might be a better way?

But with this custom component also have a way to pass objects around. (and so do some/lots of other components, without keeping their own globals)

def push_obj(obj):
    import homeassistant.components.qwikswitch as qwikswitch
    if qwikswitch.QSUSB is None:
        qwikswitch.QSUSB = {}
    oid = id(obj)
    qwikswitch.QSUSB[oid] = obj
    return oid

def pop_obj(id):
    import homeassistant.components.qwikswitch as qwikswitch
    return qwikswitch.QSUSB.pop(id)

I also based this on a weakref dictionary once, but could not find that code now. I think weakref mght be important, but not sure how solid my logic is there

@balloob
Copy link
Member Author

balloob commented Jun 11, 2016

All events on the eventbus have to be serializable because they are also being sent over MQTT and HTTP.

@kellerza
Copy link
Member

So the bit of code above still allows it to be serializable, since you only send the oid

-The component push_obj and sends the oid in the params (all serializable)
-The platform receives the params with oid and uses the oid to pop_obj

@balloob
Copy link
Member Author

balloob commented Jun 11, 2016

Sorry, did not read your full comment because I was also watching a series 💃

I don't want to push objects over the event bus. Instead we should add some sort of global accessible objects to the hass instance. (captured in this pivotal story)

@kellerza
Copy link
Member

farcy is always vigilant 👓 , even when you're watching series 😄

@balloob balloob force-pushed the remove-normal-discovery branch from 358e623 to 79874ab Compare June 12, 2016 00:07
@balloob balloob merged commit 30f74bb into dev Jun 12, 2016
@balloob balloob deleted the remove-normal-discovery branch June 12, 2016 00:43
@dcrypt3d dcrypt3d mentioned this pull request Jun 24, 2016
1 task
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants