Complemented low control #84

Anders-Holst · 2021-10-12T22:38:28Z

This pull request contains the changes to the low-level control interface, ControlInterface, suggested in the unwieldy pull request #80 .

(I really tried to make the commit messages more specific this time. But with so many changes, some are still kind of vague.)

/ Anders

Anders-Holst · 2021-10-25T12:07:23Z

I noticed that you have an old branch with mqtt support added. Why did you not merge it? I noticed that your solution with the parameters were more flexible than mine, so I adjusted my version to look more like yours.
I also added the security.py fixes here - they belong here since they are needed for the low level 'set_network_mode_station'. I have not solved the 48 byte encryption issue though since it is not my field.

scrool · 2021-10-26T17:55:17Z

I noticed that you have an old branch with mqtt support added. Why did you not merge it? I noticed that your solution with the parameters were more flexible than mine, so I adjusted my version to look more like yours.

I don't recall details but I think it was either that I haven't tested that enough or I wasn't sure about usefullness/user friendliness of CLI interface.

I also added the security.py fixes here - they belong here since they are needed for the low level 'set_network_mode_station'.

This part is the most tricky one. We have unit tests for this part of the code. I didn't know that this code is failing. Have you managed to get those failed?

I have not solved the 48 byte encryption issue though since it is not my field.

I'm not sure what you mean by "48 byte encryption issue". Has somebody reported issue that I haven't noticed?

scrool

Overall I like these changes. I have commented inline for those parts that I would like to change or I wasn't sure about a reason for a change.

I would like you to leave out realtime UDP client from this pull request. I would like to see that in a separate PR, preferrably with some unit tests that cover all those versions.

xled/control.py

scrool · 2021-10-26T17:30:52Z

xled/control.py

+        if password:
+            json_payload = {
+                "mode": 2,
+                "ap": {"password": password, "enc": 4}


Let's make this easier to read.

Since {"mode": 2} is always sent that could be a default. And only if a password is set we could add second dict entry - "ap".

xled/control.py

scrool · 2021-10-26T17:34:07Z

xled/control.py

-        }
+        if ssid and password:
+            encpassword = xled.security.encrypt_wifi_password(password, self.hw_address)
+            json_payload = {


Same as above - let's use default with single dict entry "mode": 1 and only if password and ssid are available add those.

xled/control.py

scrool · 2021-10-26T17:46:31Z

xled/control.py

+
+    def get_led_effects(self):
+        """
+        Get the number of effects and their unique_ids


Let's use "Gets" instead of "Get" similarly to other API docstrings.

scrool · 2021-10-26T17:46:35Z

xled/control.py

+
+    def get_led_effects_current(self):
+        """
+        Get the current effect index


Let's use "Gets" instead of "Get" similarly to other API docstrings.

scrool · 2021-10-26T17:46:46Z

xled/control.py

+
+    def delete_playlist(self):
+        """
+        Clear the playlist


Let's use "Clears" instead of "Clear" similarly to other API docstrings.

scrool · 2021-10-26T17:46:57Z

xled/control.py

@@ -74,6 +74,52 @@ def session(self):
            assert self._session
        return self._session

+    def check_status(self):
+        """
+        Check that the device is online and responding


Let's use "Checks" instead of "Check" similarly to other API docstrings.

Sorry for all the doc-string oversights. Fixed.

scrool · 2021-10-26T17:49:56Z

xled/control.py

+        assert all(key in app_response.keys() for key in required_keys)
+        return app_response
+
+    def set_mqtt_config(self, br_host, br_port, client_id, user, interval):


This seems to be duplicated from earlier commit ee7114c .

Anders-Holst · 2021-10-26T22:50:14Z

I also added the security.py fixes here - they belong here since they are needed for the low level 'set_network_mode_station'.

This part is the most tricky one. We have unit tests for this part of the code. I didn't know that this code is failing. Have you managed to get those failed?

If you refer to test_security.py, it appears to only test security.xor_strings, not security.encrypt_wifi_password which is the culprit. Yes I tested it and the old code works in python 2.7 but fails in python 3.6. The new code works in both.

I have not solved the 48 byte encryption issue though since it is not my field.

I'm not sure what you mean by "48 byte encryption issue". Has somebody reported issue that I haven't noticed?

I refer to issue #70 opened by someone called scrool on January 3... ;-)

I would like you to leave out realtime UDP client from this pull request. I would like to see that in a separate PR, preferrably with some unit tests that cover all those versions.

That will take some delicate operation again. I'll do it but not tonight. Meanwhile, have you checked out the file test_low_control.py checked into my (soon obsolete) pull request #80? It contains test for all low level interface calls (except firmware and change network mode) including all versions of the realtime calls. You can also run
python -m xled.test_colorsphere also from that pull request, to see the realtime interface in intense action. Otherwise, you can try this snippet as a visual unit test (assuming RGB led profile). Since Twinkly appears to be backward compatible, all protocol versions work on newer firmwares (and not family D), so then the colors should be in sequence: green, yellow, lime, and orange. On older firmware (family F) the last color will come out wrong (since its the wrong protocol). On family D, presumably the last two colors will come out wrong or not at all.

from xled.control import ControlInterface
from xled.discover import discover
import io
import struct
import time

dev = discover()
ctr = ControlInterface(dev.ip_address, dev.hw_address)
num_leds = ctr.get_device_info()['number_of_led']

def make_solid_movie(num, r, g, b):
    pat = [struct.pack(">BBB", r, g, b)] * num
    movie = io.BytesIO()
    movie.write(b''.join(pat))
    movie.seek(0)
    return movie

ctr.set_mode("rt")

print("Green")
ctr.set_rt_frame_rest(make_solid_movie(num_leds, 0, 255, 0))
time.sleep(5)

print("Warm yellow")
ctr.set_rt_frame_socket(make_solid_movie(num_leds, 230, 170, 0), 1, min(255, num_leds))
time.sleep(5)

print("Lime")
ctr.set_rt_frame_socket(make_solid_movie(num_leds, 100, 255, 0), 2)
time.sleep(5)

print("Orange")
ctr.set_rt_frame_socket(make_solid_movie(num_leds, 230, 85, 0), 3)
time.sleep(5)

Anders-Holst · 2021-11-13T22:18:03Z

@scrool , I have now removed the realtime UDP protocols from this pull request, as you wished.

I am standing by to add it again, together with the new unit test, once you have merged this one.

And in line after that are all the other low-level unit tests.

scrool · 2021-11-14T20:51:57Z

@Anders-Holst I have noticed that you haven't run black linter on your changes. Usually it would be run automatically with pre-commit hook - see https://github.com/scrool/xled/blob/develop/CONTRIBUTING.rst how to set it up:

$ pip install -r requirements_dev.txt
$ pre-commit install

To not miss this next time I have updated GitHub actions to run linters for pull requests but this doesn't run against this PR because it is based on develop branch before I added #88.

Please:

rebase your changes against develop
make sure you run black and commit all its changes.

…seem to have changed

Anders-Holst · 2021-11-14T23:24:29Z

OK, you are right, I had missed black. Tried to rebase and apply black. I thought I had fixed it all now. (Obviously did a small mistake when committing, and git rebase seemed confused so I was not able to just "drop" my last commit.)
But when pushing I still get complaints from "lint". Why? I can not find any log saying what it complains on.
Getting very tired, need to sleep...

scrool · 2021-11-14T23:45:28Z

From what I can tell, black passes now. 👍

There is one error caught by flake8 though - see https://github.com/scrool/xled/runs/4205988557?check_suite_focus=true#step:4:56

xled/control.py:33:1: F401 'xled.security.encrypt_wifi_password' imported but unused

that's because you import directly method encrypt_wifi_password but later you use xled.security.encrypt_wifi_password.

Anders-Holst · 2021-11-15T10:26:35Z

New try. Fixed the problem, passes black and flake8 here. Hope it passes on your side too.
(I really tried to undo the two confused commits from last night, but the first seem to be "special" as it was the result of trying to solve conflicts appearing during rebase, so can't be easily removed. So I let it be, although the history looks ugly...)

magicus

From what I can tell, this looks like a very good change.

xled/control.py

scrool · 2021-11-15T12:23:05Z

I have skipped those commits which didn't provide any changes and merged the rest into develop.

Thanks again! 🥳

Up until now see merged #84, #90, #91 for the contributions.

Anders-Holst added 5 commits October 13, 2021 00:01

Addition of several missing rest calls

fe876e5

Make firmware_update compatible with Generation II

ba86573

Enable relative values in set_brightness

ee3a0c2

Enable options in set_network_mode_ap and _station

4b68c0e

Making all rest calls consistent vrt validation and return

daa5850

Anders-Holst mentioned this pull request Oct 25, 2021

Thorough enhancement of the control interface, and some additions #80

Closed

scrool requested changes Oct 26, 2021

View reviewed changes

Anders-Holst added 3 commits November 13, 2021 23:05

More flexible parameters to set_mqtt_config

b66c289

Make encrypt_wifi_password work also with python3

88c5b7c

Adjusted after comments

e76cddf

Anders-Holst force-pushed the complemented-low-control branch from 9138705 to e76cddf Compare November 13, 2021 22:12

Anders-Holst added 12 commits November 14, 2021 23:38

Merge branch 'scrool:develop' into complemented-low-control

4cc9f9e

Addition of several missing rest calls

78b1a4c

Make firmware_update compatible with Generation II

3ec95a1

Enable relative values in set_brightness

e592f37

Enable options in set_network_mode_ap and _station

8132a49

Making all rest calls consistent vrt validation and return

885d4d7

More flexible parameters to set_mqtt_config

5eb1136

Make encrypt_wifi_password work also with python3

04e6640

Adjusted after comments

0a9a258

Run black on control.py

4ee45c1

Hmm, some rebasing conflict wanted me to do commit again but nothing …

c55e5ad

…seem to have changed

Sorry, tried to undo last commit

811637a

Corrected import of security, and removed some old comments

ac72519

magicus approved these changes Nov 15, 2021

View reviewed changes

xled/control.py Show resolved Hide resolved

scrool closed this Nov 15, 2021

Anders-Holst deleted the complemented-low-control branch November 15, 2021 20:03

scrool added a commit that referenced this pull request Nov 27, 2021

Add Anders Holst (@Anders-Holst) to contributors

209591e

Up until now see merged #84, #90, #91 for the contributions.

scrool mentioned this pull request Nov 27, 2021

Add Anders Holst (@Anders-Holst) to contributors #97

Merged

scrool added a commit that referenced this pull request Nov 27, 2021

Add Anders Holst (@Anders-Holst) to contributors (#97)

526940f

Up until now see merged #84, #90, #91 for the contributions.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complemented low control #84

Complemented low control #84

Anders-Holst commented Oct 12, 2021

Anders-Holst commented Oct 25, 2021

scrool commented Oct 26, 2021

scrool left a comment

scrool Oct 26, 2021

scrool Oct 26, 2021

scrool Oct 26, 2021

scrool Oct 26, 2021

scrool Oct 26, 2021

scrool Oct 26, 2021

Anders-Holst Oct 26, 2021

scrool Oct 26, 2021

Anders-Holst commented Oct 26, 2021

Anders-Holst commented Nov 13, 2021

scrool commented Nov 14, 2021

Anders-Holst commented Nov 14, 2021

scrool commented Nov 14, 2021 •

edited

Loading

Anders-Holst commented Nov 15, 2021

magicus left a comment

scrool commented Nov 15, 2021

Complemented low control #84

Complemented low control #84

Conversation

Anders-Holst commented Oct 12, 2021

Anders-Holst commented Oct 25, 2021

scrool commented Oct 26, 2021

scrool left a 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

Anders-Holst commented Oct 26, 2021

Anders-Holst commented Nov 13, 2021

scrool commented Nov 14, 2021

Anders-Holst commented Nov 14, 2021

scrool commented Nov 14, 2021 • edited Loading

Anders-Holst commented Nov 15, 2021

magicus left a comment

Choose a reason for hiding this comment

scrool commented Nov 15, 2021

scrool commented Nov 14, 2021 •

edited

Loading