Read all data from the device, disable double-encoding, implement more APIs, refactor querying, update README #11

rytilahti · 2016-12-01T19:57:38Z

The commit messages are pretty self-explanatory, so here's just some extra info:

My HS110 doesn't seem to send all the data for requests, causing incomplete JSON as response.

DEBUG:pyHS100.pyHS100:> (56) {"emeter": {"get_daystat": {"month": 11, "year": 2016}}}
DEBUG:pyHS100.pyHS100:Got chunk 1448
DEBUG:pyHS100.pyHS100:Got chunk 161
DEBUG:pyHS100.pyHS100:Got chunk 0
DEBUG:pyHS100.pyHS100:< (1605) {"emeter":{"get_daystat":{"day_list":[{"year":2016,"month":11,"day":1,"energy":0.404000},

Casting month & year to strings the following, could this be device specific? The older version used by Home Assistant seems to work fine on this case.

DEBUG:pyHS100.pyHS100:> (60) {"emeter": {"get_daystat": {"month": "12", "year": "2016"}}}
DEBUG:pyHS100.pyHS100:< (71) {"emeter":{"get_daystat":{"err_code":-3,"err_msg":"invalid argument"}}}

kirichkov · 2016-12-01T22:38:41Z

Can you give us a dump of the system info for your plug? You could be having different firmware version than mine which leads to those differences.

rytilahti · 2016-12-01T22:46:34Z

Threre you go, I masked some information with XXXXXes.

DEBUG:pyHS100.pyHS100:< (580) {"system":
{"get_sysinfo":{"err_code":0,
"sw_ver":"1.0.8 Build 151101 Rel.24452",
"hw_ver":"1.0",
"type":"smartplug",
"model":"HS110(EU)"
,"mac":"50:C7:BF:XXXXXX",
"deviceId":"8006588E50AD38930XXXXXXXXXXXXXX",
"hwId":"45E29DA8382494D2EXXXXXXXX",
"fwId":"3A1C9C60B93A090DF074A74960BA60C0",
"oemId":"3D341ECE302C0642C99E31CEXXXXXXXXXXX",
"alias":"Media",
"dev_name":"Wi-Fi Smart Plug With Energy Monitoring",
"icon_hash":"",
"relay_state":1,
"on_time":9921,
"active_mode":"schedule",
"feature":"TIM:ENE",
"updating":0,
"rssi":-51,
"led_off":0,"latitude":XXXXX,"longitude":XXXXXX}}}

edit: made it readable
edit: to add, the responses are not always chunked, it just happens now and then (although more often than not it seems).

kirichkov · 2016-12-02T12:00:47Z

I did a quick test (using the cli.py file) and your code seems to be working on both python 2 and 3. I think it'd be also good to bump the version to 0.2.2 in /setup.py and merge this.

EDIT: I'm not sure the cli.py should be part of the module, it does help, but it might be better used as an example in the README, rather than a standalone python script.

Thanks for sharing the model info. My plug appears to be the exact same hardware and software version like yours.
I haven't noticed the issue that you are describing with the chunked data, but I always suspected that socket read/writes are somehow related to the issue me and mweinelt are discussing in #3 .

rytilahti · 2016-12-02T15:22:31Z

I agree about cli.py, although maybe it could be placed into a separate dir like examples, and include a shorter usage example in README.md?

Unfortunately I don't have any historical data left as I moved my plug and cleared the device, but if you create a loop for daily you'd probably see the device delivering incomplete packets sometimes.

Unfortunately I have no insight what may cause the behavior in #3, the sockets seem to be closed properly, but in case it's not also done on the plug's end, maybe it runs out of file descriptors? One could test that by doing lots of queries and see if it stops being responsive.

rytilahti · 2016-12-02T18:23:19Z

Uhuh, I may have made a mess by working on the same initial branch for new set of patches. If that's okay then fine, if not, I'll have to figure out how to rebase and split these changes. :)

edit: looks like I messed up some changes from others... Will have to check that out when I get some time for that. For now this can be put in pending mode.

mweinelt · 2016-12-02T19:03:18Z

Can you please rebase your commits onto the current state of the master branch?

mweinelt · 2016-12-02T19:04:40Z

pyHS100/pyHS100.py

+            chunk = sock.recv(4096)
+            buffer += chunk
+            #_LOGGER.debug("Got chunk %s" % len(chunk))
+            if len(chunk) == 0:


if not chuck:

Good catch, didn't recall for sure whether empty b'' would've matched that, but it does.

mweinelt · 2016-12-02T19:06:23Z

pyHS100/pyHS100.py

@@ -318,12 +318,20 @@ def query(host, request, port=9999):

        sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        sock.connect((host, port))
+        _LOGGER.debug("> (%i) %s" % (len(request), request))


String interpolation is deprecated, use str.format() instead.

mweinelt · 2016-12-02T19:06:39Z

pyHS100/pyHS100.py

        sock.shutdown(socket.SHUT_RDWR)
        sock.close()

-        response = TPLinkSmartHomeProtocol.decrypt(buffer)
+        response = TPLinkSmartHomeProtocol.decrypt(buffer[4:])
+        _LOGGER.debug("< (%i) %s" % (len(response), response))


Interpolation here as well.

mweinelt · 2016-12-02T19:07:17Z

cli.py

+logging.info("Sysinfo: %s" % hs.get_sysinfo())
+has_emeter = hs.has_emeter
+if has_emeter:
+  logging.info("== Emeter ==")


Please indent with 4 spaces.

mweinelt · 2016-12-02T19:07:28Z

cli.py

+logging.basicConfig(level=logging.DEBUG)
+
+if len(sys.argv) < 2:
+  print("%s <ip>" % sys.argv[0])


Please indent with 4 spaces.

mweinelt · 2016-12-02T19:19:23Z

pyHS100/pyHS100.py

+        :return:
+        """
+        keys = ["sw_ver", "hw_ver", "mac", "hwId", "fwId", "oemId", "dev_name"]
+        return {key:self.sys_info[key] for key in keys}


add a whitespace after the colon.

mweinelt · 2016-12-02T19:19:44Z

pyHS100/pyHS100.py

+        Location of the device, as read from sysinfo
+        :return:
+        """
+


remove the empty line

mweinelt · 2016-12-02T19:19:50Z

pyHS100/pyHS100.py

+        Returns WiFi signal strenth (rssi)
+        :return: rssi
+        """
+


remove the empty line

mweinelt · 2016-12-02T19:20:21Z

pyHS100/pyHS100.py

+        Sets new mac address
+        :param mac: mac in hexadecimal with colons, e.g. 01:23:45:67:89:ab
+        """
+


remove the empty line

mweinelt · 2016-12-02T19:21:11Z

pyHS100/pyHS100.py

+        Content for hash and icon are unknown.
+        :param icon:
+        """
+


remove the empty line

rytilahti · 2016-12-02T20:06:18Z

Thanks for the review and insightful comments :) I hope I managed to clean-up the mess I made by merging through github in my repo..

mweinelt · 2016-12-02T20:07:17Z

I wouldn't expect anyone to implement a call to that function when it's not implemented in the first place.

…

On 12/02/2016 08:59 PM, Teemu R. wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In pyHS100/pyHS100.py <#11>: > + def icon(self): + """ + Returns device icon + Note: this doesn't seem to work when not using the cloud service, not tested with it either. + :return: + """ + return self._query_helper("system", "get_dev_icon") + + @icon.setter + def icon(self, icon): + """ + Content for hash and icon are unknown. + :param icon: + """ + + raise SmartPlugNotImplementedException() I was thinking that it allows clients to catch SmartPlugException (as that inherits from it) without needing separate handling. Does that make sense to you? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#11>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAICD09Nf_DXFOm-FRXKP1qfN2RheUX7ks5rEHiZgaJpZM4LB0ly>.

rytilahti · 2016-12-02T20:28:49Z

Agreed. The newest commit simplifies state handling by converting from strings to bools, e.g.
plug.state = "ON" is now plug.state = True

A couple of questions:

As all "active" actions do throw an exception in case of failure, it's probably a good idea to add :raises SmartPlugException to those for clients?
In case of description containing already what it returns, is it necessary to have that type of boilerplate there in :returns too?

mweinelt · 2016-12-02T23:00:44Z

pyHS100/pyHS100.py

@@ -23,8 +23,6 @@
 _LOGGER = logging.getLogger(__name__)

 # switch states
-SWITCH_STATE_ON = 'on'
-SWITCH_STATE_OFF = 'off'


Let's keep those and let people refer to them, just in case we decide to change the representation again.

mweinelt · 2016-12-02T23:01:15Z

pyHS100/pyHS100.py

        else:
-            _LOGGER.warning("Unknown state %s returned.", relay_state)
+            _LOGGER.warning("Unknown state %s returned.".formaat(relay_state))


kirichkov · 2016-12-03T07:48:39Z

The reason why "ON" and "OFF" are used instead of Booleans is because the module is modeled after the D-Link module handling D-Link's smart plugs.

Also from readability stand point if we are to replace ON/OFF with True/False the name of state should be changed to something like is_on. I'm fine with keeping the internal representation like a Boolean, but the state property should still return "ON" / "OFF" first for backwards compatibility and second for readability. Two new methods can be added is_on and is_off Which return True/False.

mweinelt · 2016-12-05T01:42:22Z

Yes, we should document raises: where necessary, so developers quickly see what they have to expect.

Regarding returns:, I think it should be added everywhere, where return is not None. The reasoning is consistency.

HS110 sends sometimes datagrams in chunks especially for get_daystat, this patch makes it to read until there is no more data to be read. As json.dumps() does JSON encoding already, there's no need to str() the year or month either.

This commit adds access to new properties, both read & write, while keeping the old one (mostly) intact. Querying is refactored to be done inside _query_helper() method, which unwraps results automatically and rises SmartPlugException() in case of errors. Errors are to be handled by clients. New features: * Setting device alias (plug.alias = "name") * led read & write * icon read (doesn't seem to return anything without cloud support at least), write API is not known, throws an exception currently * time read (returns datetime), time write implemented, but not working even when no error is returned from the device * timezone read * mac read & write, writing is untested for now. Properties for easier access: * hw_info: return hw-specific elements from sysinfo * on_since: pretty-printed from sysinfo * location: latitude and longitued from sysinfo * rssi: rssi from sysinfo

Following issues are addressed by this commit: * All API is more or less commented (including return types, exceptions, ..) * Converted state to use * Added properties is_on, is_off for those who don't want to check against strings. * Handled most issues reported by pylint. * Adjusted _query_helper() to strip off err_code from the result object. * Fixed broken format() syntax for string formattings.

houndci-bot · 2016-12-06T00:34:47Z

pyHS100/pyHS100.py

+        while True:
+            chunk = sock.recv(4096)
+            buffer += chunk
+            #_LOGGER.debug("Got chunk %s".format(len(chunk)))


block comment should start with '# '

Really? Can fix though, I don't see it helping much in case of such debug messages, which may sometimes be useful to have. Better strip it off completely?

Yes, better drop it.

houndci-bot · 2016-12-06T00:34:47Z

pyHS100/pyHS100.py

+        :rtype: dict
+        """
+
+        return {"latitude": self.sys_info["latitude"], "longitude": self.sys_info["longitude"]}


line too long (95 > 79 characters)

houndci-bot · 2016-12-06T00:34:49Z

pyHS100/pyHS100.py

-
-        if response['err_code'] != 0:
-            return False
+        response = self._query_helper("emeter", "get_monthstat", {'year': year})


line too long (80 > 79 characters)

houndci-bot · 2016-12-06T00:34:49Z

pyHS100/pyHS100.py

-
-        if response['err_code'] != 0:
-            return False
+        response = self._query_helper("emeter", "get_daystat", {'month': month, 'year': year})


line too long (94 > 79 characters)

houndci-bot · 2016-12-06T00:34:49Z

pyHS100/pyHS100.py

+
+        result = response[target][cmd]
+        if result["err_code"] != 0:
+            raise SmartPlugException("Error on {}.{}: {}".format(target, cmd, result))


line too long (86 > 79 characters)

I'd prefer to keep this again for the sake of readability. Moving the call to format to a new line or splitting up the parameter list doesn't really improve reaadability.

houndci-bot · 2016-12-06T00:34:49Z

pyHS100/pyHS100.py

+
+        :param target: Target system {system, time, emeter, ..}
+        :param cmd: Command to execute
+        :param arg: JSON object passed as parameter to the command, defualts to {}


line too long (82 > 79 characters)

Also typo "defualts"

Fixed "defualts".

houndci-bot · 2016-12-06T00:34:50Z

pyHS100/pyHS100.py

@@ -22,6 +22,11 @@

 _LOGGER = logging.getLogger(__name__)

+class SmartPlugException(Exception):


expected 2 blank lines, found 1

houndci-bot · 2016-12-06T00:34:50Z

pyHS100/__init__.py

@@ -1 +1 @@
-
+from pyHS100.pyHS100 import SmartPlug


'SmartPlug' imported but unused

False alarm. import pyHS100 allows using pyHS100.SmartPlug as well as from pyHS100 import SmartPlug for library users. Can be reverted though, if from pyHS100.pyHS100 import SmartPlug is more wanted.

mweinelt · 2016-12-06T00:44:24Z

pyHS100/pyHS100.py

+    def _query_helper(self, target, cmd, arg={}):
+        """
+        Query helper, returns unwrapped result object.
+        Raises SmartPlugException in case of error reported by the plug.


Use :raises and move after last param for consistency.

It's later on there. This is just the descriptive string for it.

rytilahti · 2016-12-06T00:46:21Z

Fixed quite a few things, as earlier, commit message contains the changes. Btw, pylint complains that pyHS100 is not a valid name :-)

houndci-bot · 2016-12-06T00:46:56Z

pyHS100/pyHS100.py

+        :rtype: datetime
+        """
+        return datetime.datetime.now() - \
+               datetime.timedelta(seconds=self.sys_info["on_time"])


continuation line over-indented for visual indent

Again for readability's sake. If wanted, we could import datetime & timedelta directly and avoid typing datetime twice/at all, if wanted.

houndci-bot · 2016-12-06T00:46:56Z

pyHS100/pyHS100.py

+        """
+        response = self._query_helper("time", "get_time")
+        return datetime.datetime(response["year"], response["month"], response["mday"],
+                                 response["hour"], response["min"], response["sec"])


line too long (84 > 79 characters)

houndci-bot · 2016-12-06T00:46:57Z

pyHS100/pyHS100.py

+        :raises SmartPlugException: on error
+        """
+        response = self._query_helper("time", "get_time")
+        return datetime.datetime(response["year"], response["month"], response["mday"],


line too long (87 > 79 characters)

Please, it's already splitted. Changing response to rsp or r doesn't improve readability.

houndci-bot · 2016-12-06T00:46:57Z

pyHS100/pyHS100.py

+        """
+        Returns device icon
+
+        Note: this doesn't seem to work (at least) when not using the cloud service.


line too long (84 > 79 characters)

Better remove this altogether, if this is an issue. It's for there for devels to see/note that there may be an issue there..

houndci-bot · 2016-12-06T00:46:57Z

pyHS100/pyHS100.py

+
+        :param target: Target system {system, time, emeter, ..}
+        :param cmd: Command to execute
+        :param arg: JSON object passed as parameter to the command, defaults to {}


line too long (82 > 79 characters)

mweinelt · 2016-12-06T00:49:52Z

Yes, the name should be PyHS100 instead - or at least start with a captical character. But let's keep that for another PR, this one's large enough already.

rytilahti · 2016-12-06T00:52:35Z

Yeah, it's my failure to mess up that much with using proper feature branches, wasn't thinking it would gain so many commits in the end.
edit: btw, it'd make sense to have unit tests for the state machine & error handling functions, although I wouldn't like to have that either in this commit series, but do it separately.

mweinelt · 2016-12-06T01:02:25Z

Agreed, unit testing makes total sense.

rytilahti · 2016-12-06T01:02:56Z

On related note, perhaps LED api should also follow the "legacy" ON/OFF for state values, even if I don't really like strings as return values/parameters?

mweinelt · 2016-12-06T01:05:15Z

I agree True/False should be used, not strings.

houndci-bot · 2016-12-06T01:41:39Z

pyHS100/pyHS100.py

+        :raises SmartPlugException: on error
+        """
+        raise NotImplementedError("Fails with err_code == 0 with HS110.")
+        """ here just for the sake of completeness / if someone figures out why it doesn't work.


line too long (88 > 79 characters)

houndci-bot · 2016-12-06T01:41:39Z

pyHS100/pyHS100.py

-        return response['err_code'] == 0
+        self.initialize()
+
+        # As query_helper raises exception in case of failure, we have succeeded when we are this far.


line too long (102 > 79 characters)

houndci-bot · 2016-12-06T01:41:39Z

pyHS100/pyHS100.py

+
+        :param target: Target system {system, time, emeter, ..}
+        :param cmd: Command to execute
+        :param arg: JSON object passed as parameter to the command, defaults to {}


line too long (82 > 79 characters)

rytilahti · 2016-12-06T01:45:10Z

Adjusted led setter to use True/False instead of ints, fix some styling issues (keep whitespaces in docstrings between headline & other elements. Outed initialization out from init() and force-calling it after all API calls changing the state of the device.

I don't really like the initialize solution, maybe sysinfo content shouldbe kept locally and just updated based on responses from the plug. Opinions?

mweinelt · 2016-12-11T19:05:44Z

@GadgetReactor Can we get this merged?

GadgetReactor · 2016-12-12T09:14:11Z

thanks!

rytilahti · 2016-12-12T20:40:39Z

Thanks for merging, next step is to clean the code base a bit & prepare for easier testing :-)

rytilahti force-pushed the master branch from 8eade34 to 2b6efd8 Compare December 2, 2016 18:25

rytilahti changed the title ~~Read all data from the device, disable double-encoding~~ Read all data from the device, disable double-encoding, implement more APIs, refactor querying, update README Dec 2, 2016

mweinelt requested changes Dec 2, 2016

View reviewed changes

rytilahti force-pushed the master branch 2 times, most recently from 2b6efd8 to 639d9fa Compare December 2, 2016 19:49

mweinelt reviewed Dec 2, 2016

View reviewed changes

rytilahti added 9 commits December 6, 2016 01:25

Add cli.py, a simple script to query devices for debugging purposes.

4e06dbf

allow easier importing with from pyHS100 import SmartPlug

134fa58

move cli.py to examples, add short usage into README.md

c979e4f

Update README.md with examples of available features.

ded0ba6

Handle comments from mweinelt

c3213e6

Refactor state handling, use booleans instead of strings

4df8f7c

rytilahti force-pushed the master branch from c9befad to 742e76c Compare December 6, 2016 00:34

houndci-bot reviewed Dec 6, 2016

View reviewed changes

mweinelt reviewed Dec 6, 2016

View reviewed changes

Fix ci woes plus one typo.

fad3d8f

houndci-bot reviewed Dec 6, 2016

View reviewed changes

rytilahti mentioned this pull request Dec 6, 2016

Fully implement the TP-Link Smart Home Protocol #10

Closed

Do initialization after changing device properties, fix nits.

8140349

houndci-bot reviewed Dec 6, 2016

View reviewed changes

rytilahti mentioned this pull request Dec 9, 2016

Tplink switch fails to load home-assistant/core#4601

Closed

GadgetReactor merged commit 05a6bbb into GadgetReactor:master Dec 12, 2016

		@@ -22,6 +22,11 @@

		_LOGGER = logging.getLogger(__name__)

		class SmartPlugException(Exception):

Read all data from the device, disable double-encoding, implement more APIs, refactor querying, update README #11

Read all data from the device, disable double-encoding, implement more APIs, refactor querying, update README #11

Conversation

rytilahti commented Dec 1, 2016

kirichkov commented Dec 1, 2016

rytilahti commented Dec 1, 2016 • edited Loading

kirichkov commented Dec 2, 2016 • edited Loading

rytilahti commented Dec 2, 2016

rytilahti commented Dec 2, 2016 • edited Loading

mweinelt commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rytilahti commented Dec 2, 2016

mweinelt commented Dec 2, 2016 via email

rytilahti commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirichkov commented Dec 3, 2016

mweinelt commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rytilahti commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mweinelt commented Dec 6, 2016 • edited Loading

rytilahti commented Dec 6, 2016 • edited Loading

mweinelt commented Dec 6, 2016

rytilahti commented Dec 6, 2016

mweinelt commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rytilahti commented Dec 6, 2016

mweinelt commented Dec 11, 2016

GadgetReactor commented Dec 12, 2016

rytilahti commented Dec 12, 2016

rytilahti commented Dec 1, 2016 •

edited

Loading

kirichkov commented Dec 2, 2016 •

edited

Loading

rytilahti commented Dec 2, 2016 •

edited

Loading

mweinelt commented Dec 6, 2016 •

edited

Loading

rytilahti commented Dec 6, 2016 •

edited

Loading