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

Add radio stop/play/channels/select_channel/info #698

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add radio stop/play/channels/select_channel/info #698

wants to merge 2 commits into from

Conversation

bskaplou
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented May 19, 2020

Coverage Status

Coverage decreased (-0.02%) to 74.173% when pulling 3371762 on bskaplou:radio into fe031c5 on rytilahti:master.

@starkillerOG
Copy link
Contributor

starkillerOG commented May 19, 2020

@bskaplou I would use the radio class to add this functionality

@bskaplou
Copy link
Contributor Author

bskaplou commented May 19, 2020

If we put subdevice code into classes like GatewayRadio we can't call them with CLI.

$cli gatewayradio --ip 192.168.2.109 --token 824ee1dc7b8dc72be02151490e5c1337 info
Traceback (most recent call last):
  File "./build/lib/miio/cli.py", line 48, in <module>
    create_cli()
  File "./build/lib/miio/cli.py", line 44, in create_cli
    return cli(auto_envvar_prefix="MIIO")
  File "/Users/bskaplou/github/python-miio.orig/miio/click_common.py", line 59, in __call__
    return self.main(*args, **kwargs)
  File "/Library/Python/3.7/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/Library/Python/3.7/site-packages/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Library/Python/3.7/site-packages/click/core.py", line 1256, in invoke
    Command.invoke(self, ctx)
  File "/Library/Python/3.7/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Library/Python/3.7/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/Library/Python/3.7/site-packages/click/decorators.py", line 21, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/bskaplou/github/python-miio.orig/miio/click_common.py", line 242, in group_callback
    ctx.obj = self.device_class(*args, **kwargs)
TypeError: __init__() got an unexpected keyword argument 'ip'

If we put all subdevice methods into Gateway it will be too fat....

$cli gateway --ip 192.168.2.109 --token 824ee1dc7b8dc72be02151490e5c1337 radio_info
Running command radio_info
{'current_program': 527782010, 'current_progress': 0, 'current_volume': 40, 'current_status': 'pause'}

What you think about implementing subdevices as mixins for gateway?
We'll still have prefixes like radio_/relay_ but these method will be implemented in related mixins.

Is this approach ok?

@rytilahti
Copy link
Owner

rytilahti commented May 19, 2020

Thanks for the PR! The proper way is to fix the cli here instead of moving the methods out to the main class. I think it's also broken for other "subdevices" like the gatewayalarm :-(

edit: about the mixins; I'm welcoming to any solution for this, the only concern I had in my mind is that the services offered by the gateway device should be accessible for anyone acting on an object of Gateway. I'm not sure if those subdevices should even be exposed as themselves to the cli (that is, whether they should inherit Device class at all), but rather be accessed underneath the gateway as sub-commands (miiocli gateway radio <command>).

@starkillerOG
Copy link
Contributor

I am currently working on properly discovering and intializing subdevices (zigbee connected devices) using the base subdeviceclass and extendig that with device specific methods and properties.

A PR will be coming in a few days

@starkillerOG
Copy link
Contributor

@bskaplou how did you figure out which properties are accesible for a device?

@starkillerOG
Copy link
Contributor

@bskaplou just finished the initial version, PR is here: #700
Could you test if your 2 channel relay works?

@bskaplou
Copy link
Contributor Author

@starkillerOG sure! will check it in a hour!

Meanwhile may I ask your opinion on this #699?
Have you tried proposed way already?

@bskaplou
Copy link
Contributor Author

@starkillerOG yes sure I can sniff anything on devices around...

@bskaplou
Copy link
Contributor Author

Dear colleagues, let me in :)
I've bought devices, found 'mi home' as non-usable bullshit and playing home-assistant to get some fun. I'm ready to roll and hack :)
I follow three steps in development: 1 - make it work, 2 - make it right, 3 - make it fast

With current PR we are on step 1....
Let me in and following steps will follow.

@rytilahti
Copy link
Owner

rytilahti commented May 20, 2020

@bskaplou would it work for you if we wait until the refactoring PR is done, and then rebase these improvements over that?

Also, I'm not sure what do you mean by letting you in? I'm really happy that you want to contribute and we will be reviewing the PRs as they come! :-)

edit: if you want to discuss in a more informal and synchronous manner, feel free to join the matrix room linked in the readme!

@bskaplou
Copy link
Contributor Author

bskaplou commented May 21, 2020

@rytilahti yes sure :) Waiting for #700

About letting me in ... I'm just a bit frustrated with a bunch of pending PRs all around ...
Some of them are accepted home-assistant/core@37f9b24 home-assistant/core@37cabe5 #696

Some other are too sloooowwwww this one, kennedyshead/aioasuswrt#39 home-assistant/core#35778

By the way if you are mac user might give a look https://github.com/bskaplou/home-assistant-electron/

@rytilahti
Copy link
Owner

Okay, let's get 700 first done and then extend the radio support! Btw, those are completely different projects, so there isn't much we can do here now, can we? :-) I know that the review cycles on homeassistant can take a while, as there are only so many reviewers and plenty of PRs to review, so hang in tight there!

@rytilahti
Copy link
Owner

@bskaplou the restructuring PR is now merged, feel free to rebase this on top of it.

@starkillerOG
Copy link
Contributor

@bskaplou now that version 0.5.1 of Python-miio is released work can begin to add support for sub devices in HomeAssistant.

Do you want to create a PR, or shall I start working to integrate sub devices into HomeAssistant?

@gheesung
Copy link

gheesung commented Jun 12, 2020

This will not work because "play_specify_fm" requires a json object of the channel to play

def radio_select_channel(self, id):
  return self.send("play_specify_fm", [int(id)])

id should be json e.g json.loads('{"id": 764, "type": 0,"url": "http://live.xmcdn.com/live/764/64.m3u8"}')

def play_specify_fm(self, id):
  return self._gateway.send("play_specify_fm", id)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants