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

Add support for sending realtime UDP frames #71

Closed
wants to merge 3 commits into from
Closed

Add support for sending realtime UDP frames #71

wants to merge 3 commits into from

Conversation

rec
Copy link
Contributor

@rec rec commented Apr 18, 2021

Intended to replace #67, tweaking to get rid of the typo!

@rec
Copy link
Contributor Author

rec commented Apr 18, 2021

@magicus You will recognize these commits. :-D

@magicus
Copy link
Contributor

magicus commented Apr 26, 2021

@rec Thanks for trying to revive the code! I don't know what's up with the typo, I must have made some unintentional search/replace before committing.

I'ts good to know the code still easily applies and works. However, the problem with getting it into mainline was more the high-level comments from @scrool.

I really should address those, but when the holiday season ended, my Twinkly got stowed away, and my spare time got diverted toward other interests.

I don't care much about credit; if you can get my code fixed so that @scrool accept it, that's fantastic. Otherwise I guess it will lay dormant until late November, or longer...

@rec
Copy link
Contributor Author

rec commented Apr 27, 2021 via email

@Anders-Holst
Copy link
Contributor

Hi @rec and @magicus,

For your information, I have in turn taken your code and debugged and tweaked it some more. I will just polish it before updating my previous pull request, with this fix included.

In my version I actually suggest to put it back into control.py - I read scrool's comments carefully (had to read them several times really), and think I now understand what he means: rt-support need not be in its own file. Its only that control.py don't do any low level communication itself, but out-sources this to the Session class which takes care of the restful stuff. Similarly, the UDP-socket stuff should also be out-sourced, and there is already a file udp_client.py included which can do this. So similar to the ControlInterface creating a session member, it now also creates an udpclient member (lazily, when needed) which takes care of the socket communication.

I had to tweak it a little to work on both pyton-2 and python-3, and add the version 2 protocol (which by the way appear to be wrong in the docs). I just posted a test-example of my version of your code in #81 (there still using the socket directly, and not through udp_client though).

Best Regards
Anders Holst

(By the way, Magnus, I live in Stockholm too.)

@rec
Copy link
Contributor Author

rec commented Sep 27, 2021 via email

@Anders-Holst
Copy link
Contributor

Are people really using Python 2 for new development?

Well, I don't know. But it says in the requirements for contributing to xled that it should be compatible with python-2. I suppose the idea is that some people have some separate hardware for playing with IoT, maybe with limited performance and not connected to the external web and seldom upgraded, and xled should work for them too out of the box, even if they have an old python. I you have made major hacks on your system, upgrading may be a pain.

was there a bug in my code?

Well, I don't want to focus on the bugs of course - rather thank you again for providing the code, since I am not a communication guy and would not have figured out myself how to assemble those packages, nor how to use a socket.
But if you want to know, to adjust the code on your side:

Two bugs affecting the "version 3" protocol (you probably only use the "version 1" on your side so you didn't notice.):

  • First, packet becomes a list and not a "bytes" string on line 63, so sock.send chokes on line 66.
  • Then, packet_size is the number of bytes, not the number of pixels, so should not be divided by bytes_per_led on lines 59,61,62.

Then two adjustments to make it work in python-2:

  • bytes([num_leds]) means the same as str([num_leds]) in python-2, neatly producing the string '[250]' which was not intended. Lines 54 and 64.
  • for some reason "with socket... as sock" does not work in python-2 but I don't know why so I just replaced it.

And finally, as Scrool also pointed out:

  • The condition for when to use what protocol is wrong. Really, you should have used version 3 yourself and not version 1 (assuming you have generation II with a fairly updated firmware)... Version 1 is only for generation I, version 2 is for older firmware of generation II and version 3 for newer firmware. To your defense though, there seem to be a bug in the documentation for version 2: I had to add an extra null byte after the access token to make it work. And Twinkly appear to be mostly backward compatible, so version 1 and 2 still works for version 3 devices.

If you'd like a b̶r̶u̶t̶a̶l̶ thorough code review, let me know.

That's very kind of you, maybe I will. I'm packaging it up for a pull request but need some more time.

@rec
Copy link
Contributor Author

rec commented Sep 28, 2021 via email

@scrool
Copy link
Owner

scrool commented Sep 28, 2021

It might be that the maintainer isn't going to review this pull request. We could consider starting a fork, in that case...!

I have reviewed #67 and I don't see my issues addressed in that nor this PR.

@Anders-Holst
Copy link
Contributor

Hi Pavol, nice to hear from you!

As mentioned, I am weaving this into my pull request #80 (making it even more huge), and I think I have managed to meet most of your concerns. Just hang on a little, I'm in the polishing phase.

Best Regards
Anders Holst

@rec
Copy link
Contributor Author

rec commented Sep 28, 2021 via email

@Anders-Holst
Copy link
Contributor

Hi @rec and @magicus

I have now polished, documented, tested, and committed my changes including the real-time support in pull request #80 .
If you like you can have a look at the code, but its quite a lot, so you don't have to.

If you fell like it and have the time, there is one thing I would need some help with though, related to the real-time stuff: The interactive color picker that is included in the pull request. It is an example of how the real time interface can be used. However, it is based on matplotlib, and I sometimes have a problem to make platform independent code with matplotlib, so I would need help to check if it looks right.

If so, you need to download my branch, and then try "python -m test_colorsphere". Make sure your leds are connected. You should then on your computer see a window with a picture of a colored sphere. It is a depiction of the color body. Rotate it by dragging on the surface. Select a color by clicking the surface. Quit by pressing q in the window. If it looks ugly, take a screenshot and send me back. But if you don't have time, don't worry, I'll test it myself at some different platforms.

@rec rec closed this Oct 9, 2021
@rec rec deleted the add-realtime-udp-support branch October 9, 2021 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
@rec @magicus @Anders-Holst @scrool