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

Add emulated_roku component #17596

Merged
merged 14 commits into from Jan 11, 2019

Conversation

Projects
None yet
6 participants
@mindigmarton
Copy link
Contributor

mindigmarton commented Oct 18, 2018

Description:

Adds a component for emulating a Roku API, allowing Roku remotes to control Home Assistant.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#6928

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@mindigmarton mindigmarton requested a review from home-assistant/core as a code owner Oct 18, 2018

@homeassistant

This comment has been minimized.

Copy link

homeassistant commented Oct 18, 2018

Hi @mindigmarton,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@wafflebot wafflebot bot added the in progress label Oct 18, 2018

@mindigmarton mindigmarton referenced this pull request Oct 18, 2018

Merged

Add emulated_roku docs #6928

2 of 2 tasks complete

@homeassistant homeassistant added cla-signed and removed cla-needed labels Oct 18, 2018

@Kane610

This comment has been minimized.

Copy link
Contributor

Kane610 commented Oct 18, 2018

Awesome to finally see this as a PR!

First off; you'll gonna need to write tests, at least for the parts related to config entries.

Second; is it really any benefit to support the old configuration path?

"""Handle a flow initialized by the user."""
return await self.async_step_init(user_input)

async def async_step_init(self, user_input=None):

This comment has been minimized.

@Kane610

Kane610 Oct 18, 2018

Contributor

You don't need step init, just replace it with step user

This comment has been minimized.

@mindigmarton

mindigmarton Oct 18, 2018

Author Contributor

Thanks. Seen the docs, however I could not find any components that didn't use it. Thought it might be needed for compatibility.

@mindigmarton

This comment has been minimized.

Copy link
Contributor Author

mindigmarton commented Oct 18, 2018

Awesome to finally see this as a PR!

First off; you'll gonna need to write tests, at least for the parts related to config entries.

Second; is it really any benefit to support the old configuration path?

Yeah, no probs,
Is HA moving away from yaml config? I haven't seen a component that didn't offer yaml configuration.

@Kane610

This comment has been minimized.

Copy link
Contributor

Kane610 commented Oct 18, 2018

If you want to support it it's fine. It was mainly a question.

@mindigmarton

This comment has been minimized.

Copy link
Contributor Author

mindigmarton commented Oct 18, 2018

Doesn't take much to support both. The current implementation doesn't import the yaml entry to config entries, which might be confusing as it won't appear in the UI, but importing doesn't make sense for a new component. So I'm not sure...

@Kane610

This comment has been minimized.

Copy link
Contributor

Kane610 commented Oct 18, 2018

Well the emphasis on hass is to configure through gui, read the IQS https://developers.home-assistant.io/docs/en/integration_quality_scale_index.html

I'm removing the documentation for my components as to not invite people to use the configuration.yaml. On new components I don't add the support.

At least I think you should import them as config entries, different ways of configuring but one way to have it registered in HASS. Problem is that there is no easy way to disable the config entry if you later remove the configuration from configuration.yaml. You'd still need to go through the integrations page to remove it

@mindigmarton

This comment has been minimized.

Copy link
Contributor Author

mindigmarton commented Oct 19, 2018

For old components, importing makes sense as a migration step that makes the yaml entry obsolete. When configuring a new component, I wouldn't expect the config entry I just wrote to not work after starting HA once. I guess it's better to remove the yaml config support.

@mindigmarton mindigmarton changed the title Add emulated_roku component WIP: Add emulated_roku component Oct 19, 2018

@mindigmarton mindigmarton changed the title WIP: Add emulated_roku component Add emulated_roku component Oct 20, 2018

@mindigmarton

This comment has been minimized.

Copy link
Contributor Author

mindigmarton commented Oct 20, 2018

Updated the tests and removed the yaml configuration support.
pylint in Travis seem to have failed for some random reason (for other PRs too it looks like)

@mindigmarton mindigmarton force-pushed the mindigmarton:emulated_roku branch from 372984d to 4317a40 Oct 22, 2018

step_id='user',
data_schema=vol.Schema({
vol.Required(CONF_NAME): str,
vol.Required(CONF_LISTEN_PORT): vol.Coerce(int),

This comment has been minimized.

@balloob

balloob Nov 1, 2018

Member

why can't we pick a port?

This comment has been minimized.

@mindigmarton

mindigmarton Nov 1, 2018

Author Contributor

Do you mean defaulting for the first instance? 8060 is what Roku uses, could use that.

This comment has been minimized.

@balloob

balloob Nov 2, 2018

Member

yep, let's pick 8060

This comment has been minimized.

@mindigmarton

mindigmarton Nov 6, 2018

Author Contributor

Defaults to 8060, incrementing with each new instance by default.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 13, 2018

There are merge conflicts too, that need to be resolved.

mindigmarton added some commits Dec 13, 2018

Merge remote-tracking branch 'upstream/dev' into emulated_roku
# Conflicts:
#	requirements_test_all.txt
#	script/gen_requirements_all.py
@mindigmarton

This comment has been minimized.

Copy link
Contributor Author

mindigmarton commented Dec 14, 2018

Thanks! Fixed the comments and the conflict.

@@ -0,0 +1,21 @@
{

This comment has been minimized.

@balloob

balloob Dec 14, 2018

Member

These are autogenerated files and will be overridden, only strings.json matters.

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good! Can be merged when build passes.

@mindigmarton

This comment has been minimized.

Copy link
Contributor Author

mindigmarton commented Dec 15, 2018

Just retested everything, one of the commits which removed the default value from upnp_bind_multicast broke SSDP discovery so I've pushed a new version of the library to prevent that from happening. (should pretty much be always True)

Bumped the version to 0.1.6.

https://gitlab.com/mindig.marton/emulated_roku/commit/e42bb3c7d5ff4cfb94bd943bb781684249a0e9f1

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 15, 2018

Had a look in the library. There's a mix of async def and use of asyncio.coroutine decorator within the same module, and sometimes even in the same function.

https://gitlab.com/mindig.marton/emulated_roku/blob/master/emulated_roku/__init__.py#L179

@mindigmarton

This comment has been minimized.

Copy link
Contributor Author

mindigmarton commented Dec 15, 2018

Yeah, I think I've started to migrate when hass started using the async syntax, and haven't got around to finishing it. Will take a look next weekend.

@mindigmarton

This comment has been minimized.

Copy link
Contributor Author

mindigmarton commented Dec 20, 2018

I've been refactoring the library, I've fixed couple of other issues too.

It should be noted that the API is without authentication. I've limited access to local networks and only by advertised ip, so DNS rebinding should not work.

An IP whitelist config option could be added for people with static IPs for their Harmony Hubs, or
using a proxy (bind to localhost, advertise the proxy address) with a whitelist could also work and would probably be more flexible / safe. (I'm not sure which is preferred)
Any thoughts?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 20, 2018

Sorry, I'm not a network expert.

Make sure the library is compatible with Python 3.5.3+. This syntax looks like Python 3.6+:
https://gitlab.com/mindig.marton/emulated_roku/blob/e438f913baa8483d94dc5960bfc667ce4f115943/emulated_roku/__init__.py#L146

There's a merge conflict now.

@mindigmarton

This comment has been minimized.

Copy link
Contributor Author

mindigmarton commented Dec 20, 2018

Python syntax versions are starting to get confusing, looks like that's PEP 526, thanks. I'll update the MR when the library is final, I've changed a lot.

I'd also like to keep the config simple, so I rather avoid adding new config options especially related to networking, as it's much easier misconfigure when it's all over the place.
Considering not many people use static ips, a whitelist wouldn't be appropriate in a lot of cases either, so I'm leaning toward supporting proxies in advanced configurations.

mindigmarton added some commits Jan 2, 2019

@mindigmarton

This comment has been minimized.

Copy link
Contributor Author

mindigmarton commented Jan 2, 2019

I've finished refactoring the library. Should support reverse proxies now, if anyone wants to use IP whitelists.
Also, I've refactored the stop / start methods so it cleans up properly in case of errors.

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good! The test failure is unrelated.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 10, 2019

@mindigmarton should we merge?

@mindigmarton

This comment has been minimized.

Copy link
Contributor Author

mindigmarton commented Jan 10, 2019

Yeah, it's ready. Thanks!

@MartinHjelmare MartinHjelmare merged commit 31d9268 into home-assistant:dev Jan 11, 2019

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA

@wafflebot wafflebot bot removed the in progress label Jan 11, 2019

@balloob balloob referenced this pull request Jan 23, 2019

Merged

0.86.0 #20354

alandtse added a commit to alandtse/home-assistant that referenced this pull request Feb 12, 2019

Add emulated_roku component (home-assistant#17596)
* Add emulated_roku component

* Add emulated_roku config tests

* Fix emulated_roku test dependencies

* Remove emulated_roku yaml support, add tests

* Add yaml support, simplify config flow

* Improve emulated_roku code quality

* Fix emulated_roku translation, improve code quality

* Fix emulated_roku translation

* Bump emulated_roku to 0.1.6 to fix SSDP discovery

* Bump emulated roku to 0.1.7, refactor component start/stop methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment