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

Use device timestamp for communication #13

Closed
wants to merge 3 commits into from

Conversation

SchumyHao
Copy link
Contributor

Hi Rytilahti

Thank you for your brilliant work on miio.

I want to use your code to control some other Xiaomi devices.
But I found some xiaomi devices time stamp is not sync with network, it reset to zero when device power-off.
So when using your code to send message to those device will got recv timeout.

I pushed this patch, this patch will set message time stamp according to device time stamp. And increase time stamp when send next message.

This can work on vacumm device too.

May you merge this commit to your code and release to PIP?

Signed-off-by: SchumyHao <bob-hjl@126.com>
Signed-off-by: SchumyHao <bob-hjl@126.com>
Signed-off-by: SchumyHao <bob-hjl@126.com>
Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't yet tested this out but wrote some comments on the code. A question: would it simplify this code if you'd check if the clock returned by the device is too far off from the local one, and if yes just adjust that timestamp for sending?

self.__enter__() # when called outside of cm, initialize.
delta_ts = int(time.mktime(datetime.datetime.utcnow().timetuple())) - self._ts_server
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this duplicate from the above?

@@ -22,13 +23,17 @@ def __init__(self, ip, token, debug=0):
self.__id = 0
self._devtype = None
self._serial = None
self._ts = None
self._ts_server = int(time.mktime(datetime.datetime.utcnow().timetuple()))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_ts_server indicates that it is a server timestamp, and if so, initializing it here sounds wrong? In anycase it's better to use datetime objects instead of converting it to an int.

@@ -1,6 +1,7 @@
import logging
import socket
import datetime
import time
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary, see below.

return m.data.value["result"]
except Exception as ex:
_LOGGER.error("got error when receiving: %s" % ex)
self.__enter__()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this? I don't think an exception handler should be doing this kind of logic.

@@ -125,9 +133,12 @@ def send(self, command, parameters=None):
_LOGGER.debug("%s:%s (ts: %s) << %s" % (self.ip, self.port,
m.header.value.ts,
m.data.value))
self._ts = m.header.value.ts
self._ts_server = int(time.mktime(datetime.datetime.utcnow().timetuple()))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weren't these set already above to proper values?

@@ -85,8 +90,10 @@ def discover(addr=None):

def send(self, command, parameters=None):
"""Build and send the given command."""
if self._devtype is None or self._serial is None:
delta_ts = int(time.mktime(datetime.datetime.utcnow().timetuple())) - self._ts_server
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timedelta object has total_seconds() which avoids casting to int and allow using datetime objects.

@rytilahti rytilahti changed the title Rytilahti Use device timestamp for communication Jun 4, 2017
@rytilahti
Copy link
Owner

rytilahti commented Jun 4, 2017

As a general comment, as the protocol is the same or at least similar, I think it would make sense to create separate classes (and cli tools) for separate devices (and maybe rename this project later to reflect the change) for controlling those too?

edit: to add, at least the air purifier and maybe also the bulbs when not using developer mode use the same protocol, probably many other xioami devices do so too.

@SchumyHao
Copy link
Contributor Author

Hi rytilahti
I create a new branch in my git repo and separate vacumm to a general device(called miio)
And I create a new python lib called python_miio.

Most of the code is forked your code. Do you mind what I have done?

We cannot get token by invoke the discover function. But there is still another way to get those token.
I got the token by root an Android device, running MiJia app on this device and pull the app data out.
All device token can be found by this way.

@rytilahti
Copy link
Owner

Hi SchumyHao, just wondering, would it not make more sense to start adding features to this library (and considering renaming the library when the time comes) instead of having a fork? Anyway, as the code is GPLv3 it's of course okay to fork as you want, in my eyes it would make more sense to concentrate the effort :-)

I'm glad to hear about possibility to get the token from the app data, could you perhaps explain how to do that to help others who do not want to do a reset and have rooted devices available?

@SchumyHao
Copy link
Contributor Author

Because my purpose is separate send and discover function into a general device(call is miio device). Vacuum or other device can inherit this general device.
And if someone just want to use this miio device in their code, some function in Vacuum class is useless.
Just like what I did before, I just use send and discover function, create a XiaoMi fan component in Home Assistant.

So I think if you separate general device function and vacuum device function. I can add more device support to this lib. And I'm willing to do this.

About HACK MiJia app things, you can read my step-by-step guide
And if you have xiaomi fan or xiaomi IR remote, you can take a look this repo. I'll go-on integrate some devices I have to this repo. And maybe some other home assistant users in China will push their code too.

@jcastro
Copy link

jcastro commented Jun 5, 2017

Guys not sure if you saw this but there's an entire Xiaomi component working pretty nicely on the forum https://community.home-assistant.io/t/beta-xiaomi-gateway-integration/8213/1. (and its github repo https://github.com/lazcad/homeassistant) I've been using it for weeks for my Xiaomi sensors and is super nice.

Not sure if it's worth integrate all of it?

@rytilahti
Copy link
Owner

@jcastro the gateway is indeed a completely different beast, and I suppose it makes no sense to put effort on that as long as the developer mode keeps working. The same applies for yeelight bulbs.

@SchumyHao I have refactored the communication handling and created a new Device superclass to allow easier extension of the library. The vacuum class is still there, but you could now just inherit from Device for other types of devices. I also made tokens device-specific so no more problems trying to access multiple devices with different tokens.

I modified the README to contain some information about adding support for new devices, and added a link to your instructions on how to get the token from rooted devices, thanks!

@SchumyHao
Copy link
Contributor Author

@jcastro the repo you motioned is nice work, we using this repo to add gateway related devices.

@rytilahti Thank you for your work. I'll try to push what I have done to your repo.

@SchumyHao SchumyHao closed this Jun 7, 2017
@SchumyHao SchumyHao deleted the rytilahti branch June 7, 2017 10:24
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.

None yet

4 participants