Add support for sending realtime UDP frames #67

magicus · 2021-01-02T17:10:02Z

Adds "rt" mode to set_mode() and send_realtime_frame() which takes a byte array with a raw frame and sends it using UDP.

This function is tested for the single packet UDP case, but not for the multipacket case. Testing help is needed!

You can use it like this: (Example from my GRBW 210 led Twinkly):

    discovered_device = xled.discover.discover()
    ip = discovered_device.ip_address
    hw = discovered_device.hw_address

    control = xled.ControlInterface(ip, hw)
    control.set_mode('rt')

    for i in range(0, 255):
        with io.BytesIO() as output:
            bytes_str = struct.pack(">BBBB", 0, 0, i, 0)
            for position in xrange(210):
                output.write(bytes_str)
            output.seek(0)
            ba = output.read()
            control.send_realtime_frame(210, 4, ba)
            time.sleep(0.1)

magicus · 2021-01-02T17:13:07Z

Ideally this should be complemented by a high level function that can extract the number of leds and bytes per led, and perhaps provide some kind of iterator pattern for sending frames. This needs some preparatory work, like better modelling of different led layouts (RGB, RGBW, AWW, more..?).

scrool

I think of control.py as application logic that doesn't use network at all. If it needs to send something it uses external module. Originally I got tests on my mind.

I'm thinking of slightly different design - code in the control would prepare the device for sending data and initialize a new object with an interface that accepts data. And separately there would be a code that interacts only with network. Maybe in or alongside of udp_client.py.

What do you think?

xled/control.py

magicus · 2021-01-02T18:30:15Z

I see your point, but I also think it's easy to overcomplicate things. It's after all just a single method less than a page long.

But sure, I can move it to a separate class. The current udp_client.py does something completely different so it'd have to be a new file then.

magicus · 2021-01-02T19:59:54Z

I think this actually got better, if not for anything else so for the fact that you only need to setup num_leds and bytes_per_led once.

scrool

I like separate class better. You have managed to identify some simplifications where an object can store separate state.

On the other hand I believe the API is rather cumbersome to use - we start but never stop (only timeout). Send function wraps packet construction and cannot be tested separately.

Edit: one more note - this is first code that would deal with multiple generations and/or firmware versions. No other code deals with that yet. I think that existing code might need to be gradually refactored to allow various features available in different models. High level interfaces should be able to construct correct class based on the device that they are talking to. I was thinking about different classes with inheritance but I'm not sure how well this would map to existing code.

Please see inline comments and let me know what do you think.

scrool · 2021-01-06T19:59:20Z

xled/control.py

@@ -411,7 +411,7 @@ def set_timer(self, time_on, time_off, time_now=None):
        assert all(key in app_response.keys() for key in required_keys)


-class HighControlInterface(ControlInterface):
+xclass HighControlInterface(ControlInterface):


x here is an typo.

scrool · 2021-01-06T20:02:01Z

xled/realtime.py

+        data_size = self.leds_number*self.bytes_per_led
+        assert len(data) == data_size
+        with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as sock:
+            if data_size < 900 and self.leds_number < 256:


As I have found recently (see https://github.com/scrool/xled-docs/pull/13 ) very first packet doesn't depend on the number of the LEDs but rather on the firmware version. I have Generation II device with 250 LEDs that initially used protocol version 2 and only later switched to protocol version three (like bellow).

scrool · 2021-01-06T20:03:46Z

xled/realtime.py

+        with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as sock:
+            if data_size < 900 and self.leds_number < 256:
+                # Send single frame
+                packet = bytearray(b'\x01')


I would like to see construction of the packet in network-independent code. Ideally also together with tests.

With this approach, in a single function, we would unecesarily need to mock networking just to test out building of a packet.

scrool · 2021-01-06T20:04:17Z

xled/realtime.py

+                sock.sendto(packet, (self.control.host, REALTIME_UDP_PORT_NUMBER))
+            else:
+                # Send multi frame
+                packet_size = 900//self.bytes_per_led


Same reasoning as above with protocol version and separation of the packet construction.

scrool · 2021-01-06T20:14:14Z

xled/realtime.py

+        self.leds_number = leds_number
+        self.bytes_per_led = bytes_per_led
+
+    def start_realtime(self):


I'm not sure about this interface / API. I understand that it might be useful to have it here.

On the other hand if we have start here we would need a stop here - otherwise we end up with a device that stops getting new data and displays just last frame for couple of seconds until it timeouts.

And how would the stop look?
a) it could be either 'movie', 'effect' or 'playlist' depending on the desirable state which would be decided on a caller
b) or it could store state before realtime start end switch to original one at the end.

In the case of a) one this would just duplicate corresponding methods from control one. Or leak underlying API. In the end - do we need start_realtime() if the caller could manage to do that on their own?

b) might be interface that is easier for a caller. But in the end it would need much more logic for a class that is indended for RealtimeChannel. So I guess in that case it might be better to have all the logic in higher level interface where one would not call start nor stop and only send data. Maybe even define Context manager (with with keyword).

rec · 2021-04-17T11:18:09Z

xled/control.py

-
-
-class HighControlInterface(ControlInterface):
+xclass HighControlInterface(ControlInterface):


rec · 2021-04-17T12:25:41Z

xled/control.py

@@ -410,6 +416,36 @@ def set_timer(self, time_on, time_off, time_now=None):
        required_keys = [u"code"]
        assert all(key in app_response.keys() for key in required_keys)

+    def send_realtime_frame(self, leds_number, bytes_per_led, data):


The performance of this is surprisingly good, but it could be improved even more by not opening a new socket connection each time but having a long-lasting socket connection served by a daemon thread and a Queue.

I don't want to delay this pull request but I could knock this out pretty fast once this is committed.

rec · 2021-04-17T15:51:43Z

xled/control.py

+                # Send multi frame
+                packet_size = 900//bytes_per_led
+                for i in range(0, math.ceil(data_size/packet_size)):
+                    packet_data = data[:(900//bytes_per_led)]


Oops! The packet data it sends is always the same initial segment. This expression needs to depend on i.

I can't test this case unfortunately because I have 250 x RGB lights.

Also, there's another issue with this where it does all these bytearray concatenations on every frame. Most of the packet is the same every time it's sent, so an optimization would be to have a fixed sized packet and simply copy in the data.

(Sorry for the single comments one at a time, I'm writing code as I go. :-)

EDIT: OOPS! I was wrong about this - there's surgery on that list, cutting off the initial portions.

Anders-Holst · 2021-11-15T22:50:34Z

Hi @magicus ,
Now the UDP realtime support is in place too! So now this pull request of yours (from which I used your code as a starting point) is obsolete and can be safely closed.
The function is set_rt_frame_socket in ControlInterface.

magicus · 2021-11-16T14:27:50Z

@Anders-Holst Great! Thank you for your hard work.

Add support for sending realtime UDP frames

570dfae

scrool reviewed Jan 2, 2021

View reviewed changes

xled/control.py Outdated Show resolved Hide resolved

Add colon to comment

bc595c2

Move realtime to separate class

6fcfee9

scrool requested changes Jan 6, 2021

View reviewed changes

scrool mentioned this pull request Apr 17, 2021

How to send messages in rt mode? #62

Closed

rec reviewed Apr 17, 2021

View reviewed changes

xled/control.py

class HighControlInterface(ControlInterface):

xclass HighControlInterface(ControlInterface):

Copy link

Contributor

rec Apr 17, 2021

Choose a reason for hiding this comment

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

Ditto!!!

rec reviewed Apr 17, 2021

View reviewed changes

rec mentioned this pull request Apr 18, 2021

Add support for sending realtime UDP frames #71

Closed

magicus mentioned this pull request Oct 10, 2021

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

Closed

scrool mentioned this pull request Nov 15, 2021

Example of movie file/stream? #89

Closed

magicus closed this Nov 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sending realtime UDP frames #67

Add support for sending realtime UDP frames #67

magicus commented Jan 2, 2021

magicus commented Jan 2, 2021

scrool left a comment

magicus commented Jan 2, 2021

magicus commented Jan 2, 2021

scrool left a comment •

edited

Loading

scrool Jan 6, 2021

scrool Jan 6, 2021

scrool Jan 6, 2021

scrool Jan 6, 2021

scrool Jan 6, 2021

rec Apr 17, 2021

rec Apr 17, 2021

rec Apr 17, 2021 •

edited

Loading

Anders-Holst commented Nov 15, 2021

magicus commented Nov 16, 2021



		class HighControlInterface(ControlInterface):
		xclass HighControlInterface(ControlInterface):

Add support for sending realtime UDP frames #67

Add support for sending realtime UDP frames #67

Conversation

magicus commented Jan 2, 2021

magicus commented Jan 2, 2021

scrool left a comment

Choose a reason for hiding this comment

magicus commented Jan 2, 2021

magicus commented Jan 2, 2021

scrool left a comment • edited Loading

Choose a reason for hiding this comment

scrool Jan 6, 2021

Choose a reason for hiding this comment

scrool Jan 6, 2021

Choose a reason for hiding this comment

scrool Jan 6, 2021

Choose a reason for hiding this comment

scrool Jan 6, 2021

Choose a reason for hiding this comment

scrool Jan 6, 2021

Choose a reason for hiding this comment

rec Apr 17, 2021

Choose a reason for hiding this comment

rec Apr 17, 2021

Choose a reason for hiding this comment

rec Apr 17, 2021 • edited Loading

Choose a reason for hiding this comment

Anders-Holst commented Nov 15, 2021

magicus commented Nov 16, 2021

scrool left a comment •

edited

Loading

rec Apr 17, 2021 •

edited

Loading