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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 15 commits into from
Closed

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

wants to merge 15 commits into from

Conversation

Anders-Holst
Copy link
Contributor

Enhancement of both the low and high control interfaces, and addition of a color model, various patterns, and an interactive color picker.

@Anders-Holst
Copy link
Contributor Author

Pavol, I have tried to send you a couple of emails but am not sure if they made it to you. So I repeat some of it here, to explain what I have done.

Already when I bought my lights a couple of months ago, I knew I would like to have much more control over them than is possible by the app. I had several ideas, many in the genre of randomly generated, slowly evolving, never repeating patterns. I was therefore very happy to find the xled package, without which it would not have been possible - I am not an IoT or communication protocol guy, I am more of a color guy.

Maybe against better wisdom I have added several of my suggested changes into one huge pull request, rather than slicing it up in several smaller ones. This is because I wanted to show where I am going, to make it easier to see why each step is needed, and to get a complete package with final examples that works out of the box. Please have a look and tell me what you like and not, and I can prepare a more specific version. (Note: there are no endlessly evolving non-repeating effects included here - I wait with those until the real-time mode is in place, which I see is waiting in another pull request.)

The file Changes at the top level explains in detail my suggested changes, so I need not repeat it all here. Just as a summary: To do what I wanted I first needed a more powerful control interface which allowed full control over patterns and movies - a solid static color is just not enough. Thus I have extended the HighControlInterface with several primitives for creating and manipulating patterns and movies. Next I needed a color model to correct the white balance of the leds and make it easier to select a color of a specific hue and brightness. And then there are some examples of what you can achieve on top of those primitives, that is, how you with sometimes just one or two lines of code can produce fairly advanced patterns and movies. I also threw in an experimental interactive color picker, for those of our families and friends who might want to help select a daily color scheme but don't enjoy programming python.

Best Regards
Anders Holst

@Anders-Holst Anders-Holst mentioned this pull request Aug 29, 2021
Closed
4 tasks
@Anders-Holst Anders-Holst mentioned this pull request Sep 28, 2021
Closed
@Anders-Holst
Copy link
Contributor Author

Anders-Holst commented Oct 3, 2021 •
edited
Loading

Hi @scrool,

I have now committed the rest of my suggested changes to the high and low control interfaces, including the real-time support (as promised the other day - thanks goes to magicus, rec, and bigboss97).

The changes make the low-level interface complete - now all available restful calls are supported (except a few I could not figure out how to use). The high-level interface now provides an almost device-independent interface which hides the technical details of the low-level calls and provides a small set of primitives from which it should be possible to create any kind of effects. The complete list of changes is in the file Changes.

Please focus primarily on control.py (and the follow up changes in util.py and security.py) - all the new files are stand-alone features that build on control.py, so they can easily be omitted from this pull request and added at a later stage when control.py is in shape.

Best Regards
Anders

rec
rec reviewed Oct 3, 2021
View reviewed changes
Copy link
Contributor

@rec rec left a comment

Choose a reason for hiding this comment

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

Well, this code review is pretty mind-blowing and my hat is off to you!

(I haven't run the code yet, I just read it.)

There's a huge amount to go through here. You seem to have filled in pretty well all the features that the Twinklys offer, and added several brand-new ones, like that colorpicker, which is simply aces!

I have a huge amount of good things to say about this, but :-D I'm going to concentrate on the fairly small number of less-than-good things. Also, TBH, I haven't fully understood big parts of this.

1. Organizing the pull request!

I'm going to start with the dullest part. :-D

But the way it's arranged into commits is going to make it hard for the owner to review and approve.

What you have is a historical record of your development, but each commit contains changes to most of the files.

So the only way to review, or to approve or ask for change, is in one great huge blob! If I were the owner of this, I'd be quaking quite a bit right about now. ;-)

You have multiple independent features. I think you should be splitting this whole branch into separate, independent commits, each as small as possible while still implementing a complete feature and having separate pull requests.

....and before you say, "My git isn't up to that", don't worry!

I can split these commits up for you and then explain to you how to get them back into your branch.

Note: I'm fairly sure the code owner follows this classic article on Git commit messages: https://chris.beams.io/posts/git-commit/

I do too - it's pretty standard these days.

2. Huge duplication of code in control.py

You didn't create it, but you expanded it, and now control.py is out of control!

The file is 1720 lines long.

There's a code pattern much like this which repeats approximately 45 times in the code:

# One or two lines here
url = urljoin(self.base_url, ...)
response = self.session.get(url)
# One or two lines here
app_response = ApplicationResponse(response)
required_keys = [...]
assert all(key in app_response.keys() for key in required_keys)
return app_response

Code duplication in mass quantities is quite undesirable.

It makes the code harder and harder to maintain every time you add a new feature.

Worse, it makes the code hard to read and understand, because nearly all of it is duplication and each individual method is like a little puzzle where you have to work out what the tiny difference is from the previous method.

Since I believe you should never suggest something you aren't willing to do yourself, I have your branch in-client here and I'll make a sketch of what it could look like...

I'd be up for putting that work as a refactoring at the end of one of your commits.

@rec
Copy link
Contributor

rec commented Oct 3, 2021

I sketched out the refactoring, and it's here: rec@efd948f

The code adds 27 lines at the end, but would lose 5*45 lines, for a net loss of about 200 lines. More, the tiny differences are much clearer, including the one bug I added. :-D

Beyond this, there's still another issue with the code: all new functionality goes into this one huge class ControlInterface!

It's called The God Object: https://en.wikipedia.org/wiki/God_object

I love objects as much as the next guy, but this goes too far. ;-)

Instead of having dozens of methods on one object, they can be free standing functions which take a ControlInterface object as their first argument.

rec@d0cf416

@rec
Copy link
Contributor

rec commented Oct 3, 2021

Oh, and this breaks the continuous integration, perhaps because you missed out on the netaddr library in requirements.txt?

@rec rec mentioned this pull request Oct 3, 2021
Closed
@Anders-Holst
Copy link
Contributor Author

Anders-Holst commented Oct 3, 2021 •
edited
Loading

Thanks for looking at it, Tom!

I need to make this clear first, before examining the details: I completely agree with most what you say:

  1. It is an awkward pull request to review, and should be split up in pieces. (Thats why I say "Please focus on control.py for now" but even so it might be large.)
  2. There is huge duplication of code, and most of the low level interface could be done with three functions for get, post and delete, with suitable parameters. My fingers were itching to do that rewrite too.
  3. It is a huge file by now, but actually it is two classes: the ControlInterface and the HighControlInterface. My intuition would be to at least put them in separate files. I would hesitate more about further subdivision though. Each part is fairly self contained but simultaneously fairly tightly connected internally.

The reason for 1. is that I feared (maybe unnecessarily) that if I just say: "I would like to propose these extra features of the control interface" someone could say that it does not seem very useful. So I wanted to show the larger picture of where I am going. However, now I regret that I threw in those color-model and pattern stuff, I should have waited with them. (On the other hand, the color-picker is a good example of why real-time is so important and urgent, and by showing what is possible to do I hoped to increase the possibility of acceptance.) Thus I wanted to show the vision, and then open a dialogue on exactly how to achieve it. I am quite expecting to have to withdraw this pull request and prepare a series of much smaller one. But first I want some feedback from @scrool , to have a dialogue on the best way forward, and which parts are acceptable and which not.

The reason for 2 and 3 is that although I really would have liked it, it would mean a more or less complete rewrite from scratch of control.py. After all, this is scrool's package, and I tried to follow his style and partitioning in files as closely as possible. If he approves, we can then do that refactoring. That was my plan anyway.

@Anders-Holst
Copy link
Contributor Author

Oh... netaddr? requirements? Probably you are right since I do not understand what you are talking about.

Anders

@Anders-Holst
Copy link
Contributor Author

Now I saw your refactoring. Yes we are thinking the same way here. But I still want to await scrool's comments.

I am more hesitant to your suggestion to break out functions from the class. It will not really solve anything - a huge class or a huge collection of functions (ok, spread out over several files, but conceptually they belong together as one abstraction layer).

I am rather thinking another way: Separate the low and high level interfaces into separate files, and then make the low level a member rather than a subclass of the higher. Then the higher can hide some of the functions in the lower, or replace it with more intuitive versions. The higher level should provide a small but sufficient set of powerful primitives to do everything one would like. I would prefer to keep that high level interface one class though.

@rec
Copy link
Contributor

rec commented Oct 4, 2021 via email

@rec
Copy link
Contributor

rec commented Oct 4, 2021 via email

@rec
Copy link
Contributor

rec commented Oct 4, 2021 via email

@Anders-Holst
Copy link
Contributor Author

I see where you are going: showing arbitrary gifs on the leds. I certainly share that vision, it would be cool. See my next comment for this.

However, it is not the save_movie and load_movie functions. Most image formats, such as gif, assumes a regular raster of pixels, whereas the leds can be positioned fairly arbitrarily (constrained by the string of course). This limits the usability of save_movie / load_movie to random or uniform patterns, or possibly patterns following the string. (Nevertheless I have already had use for it, experimenting with a pattern on one of my lights, transferring it to the other when I was happy with it.)
You might remark that it is not a problem to have a 1x250 pixel gif. Then I may counter that "what about RGBW profiles which needs an extra channel" (which in the code is always set to zero for now though, but who knows in the future). You might suggest that maybe we can reuse the alpha channel, and I would then start to twist uneasily... (Just kidding of course, I know you would never suggest such a thing.) Anyway, we are now moving quickly away from the vision of using existing gifs.

There is another candidate for "existing format": a dump of the raw string. Such a file can then be fed directly to the low-level rest call. Indeed, it seems that the command line interface supports this kind of upload of a raw dump, without mentioning anywhere how to create such dump. This is the asymmetry I wanted to resolve with these functions. However, I wanted a header stating some parameters of the movie, such as the led-profile bytes and also the speed, since arguably the speed is part of the movie experience and as such the recommended speed should be stored with the movie. Since I'm on linux I like human readable files, and not a lot of zero bytes, so I used an ascii format with hexadecimal coding. Simplest possible to write and parse, no overkill. But I can change it if there is something more appropriate.

@Anders-Holst
Copy link
Contributor Author

When it comes to the vision of showing gifs on the leds, its slightly more complicated, I believe. However, with your gif-code (a component I was missing) and my code in ledcolor and the layout stuff, we may actually have come three fourths of the way!

You indicate that you have a python package to parse gif-files so we can easily access the parameters and individual pixel values? That is the first step. Check.
Then we need to map led positions to pixels. If one use the app to map the leds, and then the new suggested code to retrieve the led layout, and the function make_layout_pattern, we can easily build a pattern based on those positions. Check.
However, the gif probably contains gamma-corrected RGB values, to display on a normal screen. The leds need no gamma correction. Without compensating for this difference, colors will be exaggerated and unnatural. The function image_to_led_rgb in ledcolor.py can do this conversion. Check.

The only thing missing is a smart way of finding the color at a specific position in the gif. The naive way (and maybe sufficient for a start) is to take the pixel right under that position. However, the leds are probably much more sparse than the pixels. The irregular placement may give strange effects (if there is a regular pattern in the gif, or small details). Maybe one would need some antialiasing technique, like a weighted average over neighboring pixels. How wide might depend on the density of leds over the picture. However, some preliminary solution should be possible to achieve fairly easy.

I think the idea is exciting. Should we make an attempt at it?

@rec
Copy link
Contributor

rec commented Oct 6, 2021 via email

@Anders-Holst
Copy link
Contributor Author

Github is very clever in masking email addresses with stars. Anyway, I have anders dot holst at ri dot se.

@rec
Copy link
Contributor

rec commented Oct 6, 2021 via email

scrool
scrool requested changes Oct 6, 2021
View reviewed changes
Copy link
Owner

@scrool scrool left a comment

Choose a reason for hiding this comment

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

First thing first - it is really huge amount of work. Kudos for that @Anders-Holst!

I'll try to describe my way of thinking - I wanted to be able to control lights with CLI and for that I needed some low level communication channel which ended up to be a ControlInterface. I didn't care about code duplication at that moment. CLI needed some high level abstractions which is why HighControlInterface appeared.

I'm not entirely against implementation of all API calls that devices can handle. And that part of PR is pretty fine.

You mentioned that documentation is sometimes incomplete or even wrong. Please open a PR against https://github.com/scrool/xled-docs . There are couple of projects that use this code-independent description and they might benefit out of those changes as well. Once those are merged we don't need to call out changes in xled as undocumented. :)

I would prefer split of this PR into smaller pieces. Please start with ControlInterface which is rather self-contained and might have the lowest amount of issues.

With this comment I'm going to submit inline comments as well - please see them too.

xled/control.py
if brightness is not None:
if relative:
assert brightness in range(-100, 101)
json_payload = {"value": brightness, "type": "R"} # Relative
Copy link
Owner

Choose a reason for hiding this comment

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

So far we don't have relative brightness documented: https://xled-docs.readthedocs.io/en/latest/rest_api.html#set-brightness could you open a PR against https://github.com/scrool/xled-docs please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

xled/control.py Outdated

def set_led_movie_full(self, movie):
def set_movies_current(self, id):
Copy link
Owner

Choose a reason for hiding this comment

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

id is python's built-in. Please use different variable name.

xled/control.py Outdated
"""
assert isinstance(time_on, int)
assert time_on >= -1
assert isinstance(time_off, int)
assert time_off >= -1
if time_now is None:
time_now = xled.util.seconds_after_midnight()
time_now = seconds_after_midnight()
Copy link
Owner

Choose a reason for hiding this comment

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

Why direct import of this function and not through a module like before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an unnecessary change. I will revert it.
It was because around line 33 where modules are imported, I noted that all from all packages except util specific functions were imported. So I changed this to follow the same pattern. As a positive side effect, some very long lines became shorter.
By the way, I now note that from xled.security only encrypt_wifi_password is imported, whereas xled.security.sha1sum is called from update_firmware (in the old code). This could not have worked, could it? Strange that flake8 did not catch it. I will fix it too while I'm at it.

Copy link
Owner

Choose a reason for hiding this comment

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

By the way, I now note that from xled.security only encrypt_wifi_password is imported, whereas xled.security.sha1sum is called from update_firmware (in the old code). This could not have worked, could it? Strange that flake8 did not catch it. I will fix it too while I'm at it.

That's nice catch of inconsistency! Thanks for taking a care of that.

xled/control.py Outdated
for stage in (0, 1):
fw_images = [stage0, stage1]
fw_funcalls = [self.firmware_0_update, self.firmware_1_update]
stages = [0, 1] if self.family == "D" else [0]
Copy link
Owner

Choose a reason for hiding this comment

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

I think this might be better to have in a helper function that determines if device family accepts one or two stages. This could be used by caller of update_firmware() itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean something like this:
def firmware_num_stages(self): return 2 if self.family == "D" else 1
So that the caller can get access to this too and do the right thing? Yes I can add such a function.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, something like that. And in the case of a method it doesn't need to be ternary one liner as multi line might be more readable.

xled/control.py Show resolved Hide resolved
xled/control.py
return self.set_mode("off")

def is_on(self):
"""
Returns True if device is on
"""
return self.get_mode()["mode"] != "off"

def set_mode(self, mode):
Copy link
Owner

Choose a reason for hiding this comment

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

Please add docstring.

xled/control.py Outdated
return self.set_mode("off")

def is_on(self):
"""
Returns True if device is on
"""
return self.get_mode()["mode"] != "off"

def set_mode(self, mode):
if mode in ("movie", "playlist", "rt", "demo", "effect", "off"):
Copy link
Owner

Choose a reason for hiding this comment

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

Why this condition if there is no else? Shouldn't have this been assert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I can change it.

xled/control.py Outdated
self.last_rt_time = time.time()
super(HighControlInterface, self).set_mode(mode)

# Functions for selecting what to show
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure why this comment is here. show_* methods are more down in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conceptually I consider clear_movies and upload_movie to be closely connected to the "show"-functions, because you may need them to set up a playlist before calling "show_playlist". That's why I put them in the same section. But I can solve the problem by putting them last in the section instead, not to confuse the reader.

xled/control.py Outdated
else:
self.set_rt_frame_socket(frame, 3)

def show_effect(self, id):
Copy link
Owner

Choose a reason for hiding this comment

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

id is python's keyword. Please use different variable name.

xled/control.py Outdated
if self.curr_mode != 'effect':
self.set_mode('effect')

def show_demo(self, id=None):
Copy link
Owner

Choose a reason for hiding this comment

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

Here again - please don't use id.

@Anders-Holst
Copy link
Contributor Author

Thanks for your comments! Most of them are straightforward to fix. I will as you suggest prepare a PR for xled-docs, and a new one for xled with just the changes to the low-level ControlInterface. Give me a couple of days.

I will write some inline comments back to you. In one case, set_formatted_timer, I am uncertain about the best way to go.

Eventually I suppose I will withdraw this too-long pull request. However, can I keep it for a while, so I can check in the changes here too and you can easier see what I have changed? Maybe I can even commit some additional files here, to showcase the larger picture as we work piece by piece in the parallell sequence of PRs?

@rec
Copy link
Contributor

rec commented Oct 9, 2021 via email

@rec
Copy link
Contributor

rec commented Oct 9, 2021 via email

@Anders-Holst
Copy link
Contributor Author

Thanks for testing it!

The discovery code still appears somewhat unreliable, doesn't it? There are some spread reports (in the issues section) of it hanging unexplicably and in ways difficult to reproduce. I have not touched that code yet, so don't know why. Maybe some race condition? Missed ping reply if it is too fast? (Just speculating.)

Number 2 is clearly a bug. Thanks for detecting it. Probably an event offset interpreted relative the wrong anchor point. It works ok here, but I will see what I find. Would it be possible to send me (on email) a screen dump of the window, including its decoration, so I can see what it looks like?
And resizing is as you noted not supported yet - I need to know exactly which pixel in the image the mouse is over, so there is some effort involved in making resizing work correctly. But it should not be impossible.

Number 1 and 3 are on purpose, but shows that the interface is not sufficiently self explanatory yet. Need to add some hints or instructions.

1: You probably clicked on some yellow color in the sphere. That is how you "select" that color for your leds, so they will keep it when you exit the colorpicker. If you have not yet clicked a color, the leds are supposed to return to the same mode they were in before entering the sphere.

3: It is a 3D sphere. Grab the surface and drag it to see other sides of it. (You can use the scroll wheel too, alone for vertical rotation, shift for horizontal, and control for "going inside" the sphere, but currently that code is slower - working on it.)

@rec
Copy link
Contributor

rec commented Oct 9, 2021 via email

@rec
Copy link
Contributor

rec commented Oct 9, 2021 via email

@rec
Copy link
Contributor

rec commented Oct 9, 2021 via email

@Anders-Holst
Copy link
Contributor Author

Hi @scrool ,
I have now committed the responses to your comments to this pull request, but I suppose you want me to create a new pull request, for just the low-level ControlInterface changes, to start with. I will do that.

Into this pull request I also committed two files I was not sure what do with: test_low_control.py and test_high_control.py. They are my "manual unit tests" so to say. I did not dare to put them under "tests" because they are the wrong format and might break some automated script. Indeed, I have no idea how to create real automated unit tests... Nevertheless, I have collected in those two files a sequence of commands that I went through manually observing the effect on the light and the outputs. I have done this on both my lights, one still at firmware 2.2.1 and the other at 2.7.1, and using both python2 and python3. Those commands jog every call (new, modified, and old) in the ControlInterface and HighControlInterface respectively, except the firmware and the set_network_status_* ones (the former because I have no firmware images to test with, the latter because it would disrupt the testing but I have tested them here at my place).
I though these files could be useful. The existing test_control.py is not very complete. With the commands in the new files you can verify that the new commands added to the documentation actually works the way I claim there. And of course check that the code works as intended on other families and versions.

@Anders-Holst Anders-Holst mentioned this pull request Oct 12, 2021
Closed
@rec
Copy link
Contributor

rec commented Oct 13, 2021 via email

@Anders-Holst
Copy link
Contributor Author

I strongly feel that I need to clarify:

Rec is talking about the experimental colorsphere code, which requires matplotlib and numpy. It is included to illustrate what you can do with the new color model and the real-time interface. It is not critical for anything else.

All the rest of the code in this branch is compatible with both python2 and python3 and should fulfill all requirements.

Anders

@rec
Copy link
Contributor

rec commented Oct 13, 2021 via email

@Anders-Holst
Copy link
Contributor Author

Hi @scrool
I guess you noticed that I have started to dismantle this too big pull request by creating another one with just the hopefully less controversial changes to the low-level interface, #84 ?
Once that is accepted, I will produce another one for the high-level interface. Maybe there are a few issues to settle there,for example we did not agree about the set_formatted_timer (but its not so complicated, and worst case I will just remove it until we know what to do about it).
When eventually both the low and high interfaces are in place, this will form a foundation upon which other functionality can be built. I know that several people out there would benefit from these changes. Indeed, I think that half of the open issues can be closed by then. What we eventually do with the rest of my code, i.e whether it fits in here or in another repository, is of less importance.
(Nevertheless, I will continue to push things into this pull request too - just for the complete picture. It will eventually be withdrawn, when we decide where the different parts of the code fits best.)

Best Regards
Anders

scrool
scrool requested changes Oct 27, 2021
View reviewed changes
Copy link
Owner

@scrool scrool left a comment

Choose a reason for hiding this comment

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

I have looked into test_low_control.py as you asked for in #84. I don't like testing calls against real device. This should be rewritten to work against mocked data, "fake" device. To collect data in the first place we should have a way to turn off mocking and ask specific device on the network - so we could test this against various models and firmwares. But once we collect those data it shouldn't touch the network again nor expect devices to be available.

xled/test_low_control.py
return [{'x': 1.0-2*ele['y'], 'y': (ele['x']+1.0)/2, 'z': ele['z']}
for ele in coords]

dev = discover()
Copy link
Owner

Choose a reason for hiding this comment

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

If I got this correctly you are connecting to real device here? That's not correct approach for a test. This should be mocked out - see tests/test_control.py.

xled/test_low_control.py

ctr = ControlInterface(dev.ip_address, dev.hw_address)

ctr.check_status()._data
Copy link
Owner

Choose a reason for hiding this comment

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

This should check return value to known, expected state of mocked device.

xled/test_low_control.py

ctr.check_status()._data

ctr.firmware_version()._data
Copy link
Owner

Choose a reason for hiding this comment

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

Same here - check return value of this call.

xled/test_low_control.py

ctr.firmware_version()._data

vers = tuple(map(int, ctr.firmware_version()['version'].split('.')))
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure what is tested here - that a version can be split by dots? Rather than this test I'd expect test that there are some dots. But better would be to compare a version returned from this call to a known (mocked) value of a device.

xled/test_low_control.py

vers = tuple(map(int, ctr.firmware_version()['version'].split('.')))

ctr.get_device_info()._data
Copy link
Owner

Choose a reason for hiding this comment

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

From here on gets should be tested the same way as I wrote above.

xled/test_low_control.py

name = ctr.get_device_name()['name']

ctr.set_device_name("Dummyname")._data
Copy link
Owner

Choose a reason for hiding this comment

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

This is first set - doing this is fine. But I don't see a test that a call run against a device, which should be mocked device.

xled/test_low_control.py

ctr.network_scan()._data

time.sleep(1)
Copy link
Owner

Choose a reason for hiding this comment

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

Sleeping for fixed period of time, especially this low might not be enough. At least for my real-world device 1 second wasn't enough to find SSID I was intereseted in. This could be again mocked if needed but I'm not sure if it would be necessary for mocked one.

@Anders-Holst
Copy link
Contributor Author

Thanks for looking at it, @scrool
However, I think I need to clarify what type of test it is - why I call it "manual unit test" and have not put it in the directory "tests" along with the others.
The file contains the lines I ran on both my lights, with different firmware versions. The focus was to verify that each function I had added or modified actually had the desired effect on the lights. Crashes in python was of course also interesting to catch, but most of them were sieved out earlier, so this was not the focus. So I pasted each line into python, and observed the visible effects on the lights, and the return values of all those lines where you wanted comparisons with known states. The line "vers = ..." you reacted on is thus not part of the test, but a way to get the actual devices firmware number in tuple format, so it can be compared later down. (It can't be put before the start of the test because "ctr" is not defined at that point.)

So I published it here for several purposes:

  • To show that I have actually thoroughly tested all those functions I added and made sure they work as they should.
  • To provide a way for you to verify that they actually have the effect on the lights that are claimed in the documentation (so you can make sure I did not just dream up some of the new functionality).
  • To provide a way to verify that it all works on further families or firmware than those I have access to. Or newer versions in the future.
  • To give examples of how the functions can be called in a way known to work (if someone has a problem with some function or is uncertain what to provide as arguments, as complement to the documentation).

I asked you to check it out, because I thought that the reason you wanted tests specifically for the real time functionality, was that you wanted to make sure they had the correct (visual) effect on lights across several versions and string sizes. That is what these tests are designed for. In contrast, the unit tests that you now ask for, which run against mock data, will only be able to discover if someone in the future accidentally modifies something in the code that either makes it crash or changes the data sent to the device or returned from the function. (If I record the current behavior, it might even be the wrong behavior, if I hadn't checked it visually.) Of course, I should not deny the usefulness of those tests too, but I am confused why such tests are needed specifically for the real time functionality, when the current unit tests in xled are so very limited. For example, the current "tests/test_control.py" ever only checks set_mode and get_mode. "tests/test_security.py" only tests xor_strings.

In addition, I am not sure I have the tools or knowledge to produce such tests. For example, I do not understand how to produce the "casettes" mentioned in current test files. Or when you mention to "turn off mock" to collect data. I will certainly need help wit this if you insist that these tests are needed.

@scrool
Copy link
Owner

scrool commented Oct 28, 2021 •
edited
Loading

Thanks for looking at it, @scrool However, I think I need to clarify what type of test it is - why I call it "manual unit test" and have not put it in the directory "tests" along with the others. The file contains the lines I ran on both my lights, with different firmware versions. The focus was to verify that each function I had added or modified actually had the desired effect on the lights. Crashes in python was of course also interesting to catch, but most of them were sieved out earlier, so this was not the focus. So I pasted each line into python, and observed the visible effects on the lights, and the return values of all those lines where you wanted comparisons with known states. The line "vers = ..." you reacted on is thus not part of the test, but a way to get the actual devices firmware number in tuple format, so it can be compared later down. (It can't be put before the start of the test because "ctr" is not defined at that point.)

Got it. Thanks for the explanation. That makes a sense.

I asked you to check it out, because I thought that the reason you wanted tests specifically for the real time functionality, was that you wanted to make sure they had the correct (visual) effect on lights across several versions and string sizes. That is what these tests are designed for. In contrast, the unit tests that you now ask for, which run against mock data, will only be able to discover if someone in the future accidentally modifies something in the code that either makes it crash or changes the data sent to the device or returned from the function. (If I record the current behavior, it might even be the wrong behavior, if I hadn't checked it visually.)

I believe it is possible to combine advantages of both approaches. A code that is able to control lights could be tested visually as well. Whoever runs test for the first time they see how lights behave. Interaction is recorded. Then this can be described within the code - most likely with nicely tests names but it could be also improved with additional comments. Think of a test "turn on single first white led in RGBW device". From there on once could rerun tests anytime a code changes to make sure that it still interacts with mocked device as before. That's how you understood it as well. In the future, if somebody would like to verify that Twinkly didn't break their interface and still displays first white led, that mocked data could be disabled and one could test it against real device again and visually inspect.

Of course, I should not deny the usefulness of those tests too, but I am confused why such tests are needed specifically for the real time functionality, when the current unit tests in xled are so very limited. For example, the current "tests/test_control.py" ever only checks set_mode and get_mode. "tests/test_security.py" only tests xor_strings.

Restfull APIs are "easy" to double check externally to xled. Once has to get correct authentication token and then use e.g. curl for these calls. That's why I wasn't so eager to test each and every call there.

Just to make sure - I was under impression that test_low_control.py was initial approach to increase test coverage of restfull API calls. Now, that I understand what you meant by that feel free to ignore those comments about improvements of that script. On the other hand I would rather keep this script away from the code base for now.

Security module is slightly more complicated and I'd love to have it covered much deeper. There are already few differences between various devices and until we have support for multiple python versions it is harder to not break it with changes.

Finally UDP communication is again something that one has to properly construct in a code because it is proprietary protocol. That's why there is at least minimal test to make sure it can be run against various environments.

This new UDP client extends proprietary protocol which, without proper test, might break in the future. I understand that mocking UDP communication might not be as easy without a helper library (that we have for restful APIs).

In addition, I am not sure I have the tools or knowledge to produce such tests. For example, I do not understand how to produce the "casettes" mentioned in current test files. Or when you mention to "turn off mock" to collect data. I will certainly need help wit this if you insist that these tests are needed.

If you are wondering about existing mocked data - code uses vcrpy-unittest. Easiest would be to follow documentation there or in vcrpy one.

PS: I won't be around my Twinkly lights for upcoming two weeks again. I might have also less time to look into your code changes or to comments. Please keep up doing great work, I'll look into that once I come back and get a mess settle on my end.

@Anders-Holst
Copy link
Contributor Author

Anders-Holst commented Oct 31, 2021 •
edited
Loading

Hi @scrool , three quick comments (even if I know you might not see it until later)

I googled a little and to my own surprise I think I managed to produce a unit test for the socket calls. I may actually convert my "manual tests" into unit tests too, although I still maintain that both kinds of tests are needed. (And I have not figured out how to easily do both by the same file yet. The order and number of calls you want to do may differ.)
The realtime unit test is checked into this branch as "tests/test_control_realtime.py" if you want to check it out. (It makes no sense to check it into the "complemented low control" branch, since you wanted me to remove the udp realtime stuff from there, right? Do you still want that in a separete PR or does this unit test solve it?)

This new UDP client extends proprietary protocol which, without proper test, might break in the future.

I am not sure I understand what you mean by "extends". Note that my implementation of the socket based realtime functionality relies entirely on the existing "udp_client.py", which presumably follows the correct protocol. The discovery code already relies on it. The only "new protocol" concerns how to assemble the contents of the udp packets, and that is specified by Twinkly. (And if you look at my implementation you will see that it (deliberately) has a line-by-line correspondence to the protocol details in xled-docs.) Well, I have the unit test now so maybe it doesn't matter anymore.

Finally, I forgot to tell you how to trigger the crash in security.encrypt_wifi_password. That is easy. Just call the function from python3 with any suitable arguments. Here are two logs using python2 (which works) and python3 which crashes:

aho@lysithea:~$ python2
Python 2.7.17 (default, Feb 27 2021, 15:10:58) 
[GCC 7.5.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from xled.security_orig import encrypt_wifi_password
>>> encrypt_wifi_password('Banana', '84:0d:8e:d6:49:d9')
'kRtYWrvokYbYx8YJ1InMImw07Jns08TOTTJztetnStI5HYdcIfFDuYRVPM8cLFvROT7m29xhIuLhBKAzefGYGw=='
>>> quit()


aho@lysithea:~$ python3
Python 3.6.9 (default, Jan 26 2021, 15:33:00) 
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from xled.security_orig import encrypt_wifi_password
>>> encrypt_wifi_password('Banana', '84:0d:8e:d6:49:d9')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/aho/.local/lib/python3.6/site-packages/xled/security_orig.py", line 149, in encrypt_wifi_password
    secret_key = derive_key(key, mac_address)
  File "/home/aho/.local/lib/python3.6/site-packages/xled/security_orig.py", line 85, in derive_key
    return xor_strings(shared_key, mac.packed)
  File "/home/aho/.local/lib/python3.6/site-packages/xled/security_orig.py", line 67, in xor_strings
    ciphered.append(m_char ^ k_char)
TypeError: unsupported operand type(s) for ^: 'str' and 'int'
>>> quit()

And the same thing with my proposed fix:

aho@lysithea:~$ python2
Python 2.7.17 (default, Feb 27 2021, 15:10:58) 
[GCC 7.5.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from xled.security import encrypt_wifi_password
>>> encrypt_wifi_password('Banana', '84:0d:8e:d6:49:d9')
'kRtYWrvokYbYx8YJ1InMImw07Jns08TOTTJztetnStI5HYdcIfFDuYRVPM8cLFvROT7m29xhIuLhBKAzefGYGw=='
>>> quit()


aho@lysithea:~$ python3
Python 3.6.9 (default, Jan 26 2021, 15:33:00) 
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from xled.security import encrypt_wifi_password
>>> encrypt_wifi_password('Banana', '84:0d:8e:d6:49:d9')
b'kRtYWrvokYbYx8YJ1InMImw07Jns08TOTTJztetnStI5HYdcIfFDuYRVPM8cLFvROT7m29xhIuLhBKAzefGYGw=='
>>> quit()

@Anders-Holst
Copy link
Contributor Author

Hi @scrool
I noticed the other day that you tried to run the realtime unit test, but it was interrupted because apparently it was hanging. That's no good... Presumably it is the socket.recv call which hangs indefinitely if the expected data doesn't appear on the socket. So it seems my socket code was not sufficiently platform independent to work on the github servers, although it worked perfectly locally. (Maybe it was as simple as me assuming that the localhost is called 127.0.0.1, which might not be true on the github servers?) Even if the sockets could be made to work properly on github, a more serious problem is that if something goes wrong in a test, it would also hang instead of reporting that problem... So the solution would not work regardless.

So I have made a new attempt with a simpler solution: All socket communication is performed by a UDPClient object in the ControlInterface. This test just replaces that UDPClient with a fake UDP client instead, so no real socket traffic is ever generated. Again it works nicely locally. Maybe better luck on github with this version.

Anders

@Anders-Holst
Copy link
Contributor Author

And now I just checked in unit tests for all the rest of the functions in ControlInterface.

@rec
Copy link
Contributor

rec commented Nov 13, 2021 via email

@scrool scrool mentioned this pull request Nov 15, 2021
Closed
scrool
scrool reviewed Nov 20, 2021
View reviewed changes
xled/control.py
return self.set_mode(self.last_mode)
else:
if self.family == 'D' or self.version < (2, 5, 6):
response = self.get_led_movie_config()['frames_number']
Copy link
Owner

Choose a reason for hiding this comment

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

A note (to myself): this is a workaround for #66 .

@Anders-Holst Anders-Holst mentioned this pull request Nov 30, 2021
Closed
@Anders-Holst
Copy link
Contributor Author

This pull request is now obsolete so I withdraw it.

@Anders-Holst Anders-Holst deleted the aho-features branch December 4, 2021 14:14
@Anders-Holst Anders-Holst mentioned this pull request Dec 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rec rec rec left review comments

@scrool scrool scrool requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Anders-Holst @rec @magicus @scrool