-
-
Notifications
You must be signed in to change notification settings - Fork 568
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
Zigbee suqare button and cube support via fake_device #709
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bskaplou nice, can't wait to test this.
Unfortunately I do not own a squarebutton or cube :(
Would it be possible to implement this for the magnet device?
I made some sugestions for improvements, hope you are open for it.
If you like I could do a more extensive revieuw after you proccesed these sugestions.
@@ -201,6 +218,38 @@ def discover_devices(self): | |||
|
|||
return self._devices | |||
|
|||
def x_del(self, script_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename "x_del" to "unsubscribe_event"
and "script_id" to "subscription_id"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x_del is name of method... python method name mimics MIIO method... Less conversions => more comprehension... x_del is not executed directly, it's low-level method which is called from uninstall ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright I understand, you are probably right if this function is only called by other functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this should be named otherwise, especially if this can be useful for other use cases. Otherwise it is probably simpler to simply use self.send
where necessary (or at least rename this to something more descriptive, and prefix it with _
to indicate that it's a private method not to be used by others.
miio/gateway.py
Outdated
@@ -738,6 +787,21 @@ def get_firmware_version(self) -> Optional[int]: | |||
) | |||
return self._fw_ver | |||
|
|||
@command() | |||
def uninstall_scripts(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this uninstalls all scripts right?
I would then rename this to "Unsubscribe_all_events"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actual installation of program into gateway memory.... installation and deletion...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what you can do with loading such a "program" into gateway memory?
Can you only get callbacks when an event happens/property changes or can it do more?
It would be really nice if you could include docstrings """explanation"""" for all new functions that briefly explain what the function can do and how it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what you can do with loading such a "program" into gateway memory?
Gateway is able to store limited amount of such "scripts". Scripts are permanent upon restarts. External device call parameters are also encoded with script (i.e. rotate)...
For each devices receiving messages from scripts gateway maintains state... It means gateway is sending ping requests and will not send actual packages in pings are faulty..
Can you only get callbacks when an event happens/property changes or can it do more?
Unfortunately we have no callbacks... Script allows to send package to other miio device as a result of action....
docs are added for all methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subscription mechanism can be implement by scripts if you use target with ip is an ip of python-miio running device... But in xiaomi scripts are not expected to be used as callbacks. It was designed to make gateway implement complete automation listen=>process=>send data to other devices... Such behavior is still possbile. You can create script which turns on wi-fi power plug in cube rotation without processing of callback in hass.
miio/gateway.py
Outdated
|
||
@command() | ||
def install_move_script(self): | ||
return self._gw.install_script(self.sid, "move", build_move) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename all of these to "subscribe_to_move" etc.
And make an aditional function "subscribe_to_all_events" that calls all these functions in one go.
maybe consider removing all these individual functions and just make one list "events" like the "properties" that contains all events that can be subscribed to ["move", "rotate", "shakeair", "flip90", "taptap", "flip180"] and then just make one function "subscribe_to_all_events" that subscibes to all events in a for loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed most of code duplication... Further reducing requires deep work with click....
miio/gateway_scripts.py
Outdated
lumi, source_id = source_sid.split(".") | ||
method_name = f"move_{source_id}" | ||
|
||
move = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move -> subscription_info
miio/gateway_scripts.py
Outdated
|
||
move = [ | ||
[ | ||
action_id["move"](source_sid), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move --> event
miio/gateway_scripts.py
Outdated
"0", | ||
{ | ||
"did": source_sid, | ||
"extra": "[1,18,2,85,[6,256],0,0]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 256 is specific for the move event, so make a mapping dict at the top of this file that maps events to these specific values:
extra_parameter_mapping = {"move": 256, "flip90": 64} etc.
miio/gateway_scripts.py
Outdated
{ | ||
"did": source_sid, | ||
"extra": "[1,18,2,85,[6,256],0,0]", | ||
"key": "event." + source_model + ".move", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"event." + source_model + ".move"-> f"event.{source_model}.{event}"
miio/gateway_scripts.py
Outdated
return dumps(move) | ||
|
||
|
||
def build_flip90( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of these functions are then not needed anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to implement this for the magnet device?
@starkillerOG Yes sure! Can you sniff interaction between mobile app and magnet during creation of automation involving magnet?
miio/gateway.py
Outdated
|
||
@command() | ||
def install_move_script(self): | ||
return self._gw.install_script(self.sid, "move", build_move) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed most of code duplication... Further reducing requires deep work with click....
target_id=fake_device_id, | ||
source_model="lumi.sensor_cube.v1", | ||
message_id=0, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented
@bskaplou I have not yet been able to sniff local direct traffic to or from my gateway. On my phone I also have an capture app "Packet Capture", that worked greath for another application, but with the MiHome app it just captures hundreds of packets to outside servers (I pressume the Xioami Cloud servers, but I do not see local packets). What app are you capturing, is it the MiHome app? |
target_ip, | ||
target_model=fake_device_model, | ||
target_id=fake_device_id, | ||
source_model="lumi.sensor_switch.aq3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe consider adding "lumi.sensor_switch.aq3" as a property of the subdevice class:
@property
def model(self):
"""Return the model of the device."""
return "lumi.sensor_switch.aq3"
also include a default for the base class:
@property
def model(self):
"""Return the model of the device."""
return "lumi.unknown"
I think this can be usefull for more functions in the future and this can be used to show in HomeAssistant as the model in the device info in the GUI.
That property can than be passed to these functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably someone will eventually figure out how to get the model directly from the gateway, I have tryed but not suceeded yet since I cannot yet capture packets.
I know the models are shown in the MiHome app if you go to the gateway device --> click ... --> about --> Hub info.
That shows a list containing dicts that have model, sid, and name, would be very usefull to get that list since we would then also have nice default names for inside HomeAsssitant while setting up the integration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe consider adding "lumi.sensor_switch.aq3" as a property of the subdevice
I actually hope at least some of these methods will be reusable between devices for example between single/double/triple button switch...
Probably someone will eventually figure out how to get the model directly from the gateway
:) Impossible without new firmware... As far as I get original idea is use of independent agents instead if client<->server ... All calls between devices are independent...
Significantly updated... |
How do you sniff miio trafic? |
I think this looks very good! Could you possibly make an script for the gateway alarm? |
@starkillerOG
@rytilahti waiting for PR approve and release :) |
@rytilahti is everything fine with PR? |
@bskaplou great work with this code! In the mean time I have been able to sniff traffic and test the code. Your code worked mostly nicely and I was able to capture the magnet sensor open event. The two major things that I think still need to improve in this code are:
|
@starkillerOG removed token from code... passed as cli args now... |
…u/python-miio into zigbee_suqarebutton_cube
Homeassistant will almost certanly require the server/fake_device code to be implemented in miio library instead of in HASS itself, since they always want to push as much as code possible to the libs. Besides it is convenient to have the server code in miio lib because then things like open/closed state can be tracked internally in the miio lib. @rytilahti what do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, and my apologies for the belated answer. I'm currently very busy with some other things, and as I realized that this will require some planning, I have been postponing the review and this is also a short not comprehensive one...
The main critique from my side are:
- There is plenty of repetition, the event system interface should be very generic and applicable to new devices without touching any other code besides the code of that new device. This means, that we need a way to expose the available events that can be bound to "handlers/actions". In the perfect case we will have a method with a signature not unlike
install_event_handler(source_device, event, target_device, target_method)
.
Let me know if any of my thoughts make sense to you :-)
- The data structures need to be converted to attrs classes. This will allow generating the "payload" directly from a class instance without any manual dict/list handing.
), | ||
), | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads really, really complicated.. What for this is and is it necessary? I suppose for binding on multiple interfaces? If yes, that could also be potentially useful for discovery (iirc there is an issue about not being able to discover devices from other subnets). Is there maybe a library that could be used to achieve this?
class FakeDevice: | ||
_device_id = None | ||
_address = None | ||
_token = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to define these as class attributes.
build_shake, | ||
build_shakeair, | ||
build_singlepress, | ||
build_taptap, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is waaaaay too specific to be inside the gateway class. We need a way to expose subdevice based actions some other way.
@@ -201,6 +218,38 @@ def discover_devices(self): | |||
|
|||
return self._devices | |||
|
|||
def x_del(self, script_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this should be named otherwise, especially if this can be useful for other use cases. Otherwise it is probably simpler to simply use self.send
where necessary (or at least rename this to something more descriptive, and prefix it with _
to indicate that it's a private method not to be used by others.
) | ||
def subdevice_command(self, sid, command, encoded_token): | ||
"""Send command to subdevice.""" | ||
self.discover_devices() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to do the discovery for this? If yes, I think it would be better to modify the discover_devices to return a dictionary of {sid: SubDevice}
. This would allow dropping the next line and make it much more easier to comprehend.
def uninstall_scripts(self, encoded_token): | ||
return dict( | ||
map( | ||
lambda action: ( | ||
action, | ||
( | ||
action_id[action](self.sid), | ||
self._gw.x_del(action_id[action](self.sid)), | ||
), | ||
), | ||
action_id.keys(), | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is way coo complicated and rather hard to interpret.
Is the goal to remove all scripts? Is there a way to get a list of current events, that could be iterated and then removed? How does removal/overwriting of a single script look like?
How is the encoded token used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes... goal is to remove all scripts at least potentially could be installed in device... As far as i get list of installed scripts is not exposed through miio protocol... Here I xdel all scripts for all possible scripts to avoid memory leak on gw
Encoded token is used during script installation and somehow linked with real token. Unfortunately algo is unknown...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think there were some sort of list functionality related to scenes in the gateway implementation at some point, that could be related to this?
Anyway, I pointed out the encoded_token here as it is given as a parameter but never used in the method itself?
|
||
def install_script(self, sid, builder, encoded_token, ip=None): | ||
"""Install script for by building script source and sending it with miio method. You need to run fake or real device to capture script execution results.""" | ||
if ip is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see this being much simpler, in my opinion the API should be more something like:
gw = Gateway()
cube = gw.get_device_by_id("cube id")
some_other_device = gw.get_device_by_id("my light")
cube.install_event_handler(cube, 'shake', some_other_device, 'some action')
Likewise, if I have a cube instance, I would like to query the list of available events and already installed scripts:
print("available events: %s" % cube.events) # I mention this way of exposing the available events in some other comment
# if there is only one script possible, otherwise we need a way to get a list of already installed scripts to make it possible for the user to decide which one to remove
cube.remove_script("shake")
# the other case
handlers = cube.installed_event_handlers
for handler in handers:
print("handler %s")
properties = [] | ||
|
||
@command() | ||
def install_singlepress_script(self, encoded_token): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment about how to expose events to users.
target_encoded_token, | ||
target_model=fake_device_model, | ||
target_id=fake_device_id, | ||
source_model="lumi.sensor_cube.v1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that these seem to be passed everywher, I think the best approach is to create a new class, EventHandler
could be a possible name. This would contain all the necessary information for the handler, so just to give you an idea:
@attr.s
class EventHandler:
source_sid = attr.ib()
target_ip = attr.ib()
target_token = attr.ib()
target_model = attr.ib() # is this necessary?!
target_did = attr.ib() # should this be `target_sid`?
target_model = attr.ib() # is this necessary?
Potentially a lot of these are already known by the target/source devices, so potentially passing the source "device" and target "device" should be enough for making the connections.
[ | ||
"1.0", | ||
randint(1590161094, 1590162094), | ||
[ | ||
"0", | ||
{ | ||
"did": source_sid, | ||
"extra": extra, | ||
"key": "event." + source_model + "." + event, | ||
"model": source_model, | ||
"src": "device", | ||
"timespan": ["0 0 * * 0,1,2,3,4,5,6", "0 0 * * 0,1,2,3,4,5,6"], | ||
"token": "", | ||
}, | ||
], | ||
[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be done using a container class, otherwise it is way too complicated and hard to interpret. See my comment above, I think some of this data can already be stored in the devices what we have, the event handler (or eventscript? or script? I'm not sure) needs to simply have the potential extra fields (like the timespan
).
As #1446 #1459 are merged now, I think this can be closed. Thanks @bskaplou and @starkillerOG! |
Add support for square button and cube actions via script installation on gate and events receiving with fake_device.
Also added zigbee_command to make functionality avialable through CLI