add sending 0xDD per-channel priority

[mthespian]

Note: 0xDD packets are not sync-sensitive, so they do not manual flush

[Hundemeier]

Even if it makes me seem rude, but I don't think this change should be made upstream into this library as is.

I have several problems with it:

1. It includes features outside of the scope of the ANSI E1.31 protocol. Note that the value 0xDD as a DMX start code is never mentioned in the document.
2. I think the current implementation violates the separation of concerns principle. The current implementation of sender and receiver are limited to features directly mentioned in the E1.31 protocol document and I would like to keep it that way.
3. If the functionality to send out specific priority information is implemented on the sender (output.py and output_thread.py) it should also be implemented on the receiving side (receiver.py and receiver_thread.py) (might be a future step; minor issue).
4. Where does the value of INTERVAL_CORRECTION_FACTOR = 0.984 come from? Could you provide a link or resource which explains this value?

Keep in mind that I am not against the core idea behind this pull request (implementing per-channel priority), but I would do it differently.

I suggest a couple of things:

1. Create a separate pull request for your changes on data_packet.py, since this fixes a long standing issue, I would like to merge it.
2. Separate the handling of 0xDD start code packets by creating a separate class (potentialy inheriting Output), although I am not quite sure about the changes to the OutputThread; maybe a second separate class?

[mthespian]

I don't consider your response rude at all. Thank you for the suggestions and pointers.

A few responses:

1. E1.31 allows for alternate start codes for different data packets. It's a network packet structure around E1.11 https://tsp.esta.org/tsp/documents/docs/ANSI-ESTA_E1-11_2008R2018.pdf. 0xDD is a registered alternate start code https://tsp.esta.org/tsp/working_groups/CP/DMXAlternateCodes.php for E1.11 packets assigned to Electronic Theatre Controls. Usage is summarized in the Alternate Start Code listing and detailed https://etclabs.github.io/sACN/docs/head/per_address_priority.html on ETC's github.
2. I considered additional classes, but ran into exactly the question you mentioned regarding Output Thread. Avoiding duplication led me to integrating within the existing classes. If you prefer to keep them separate, I can certainly do as you suggest and add those capabilities to my own derivative branch.
3. I had left the receiver side as a separate future step. My current project only required the sending side implementation (The perils of open source development, I know.)
4. I knew someone was going to ask about that. The answer is: it's an ugly hack. I derived the number by setting a 1 second timeout between packets and observing what actually came out -- then tweaked the multiplier until the inter-packet timing never exceeded one second on my specific machine. Hardly empirical, I understand. The goal was to allow the various intervals to be nominal values but respect those as maximums. I'm all for a better way to do this if you have any ideas. Obviously the best approach is to move every operation possible out of the direct sending path so they can be setup and ready.

Regarding item 4 above, I'm curious about the choices to calculate target multicast address and to set the multicast socket options on every packet.

1. Would you be averse to putting the calculated target into data_packet? Or does that similarly fail the separation of concerns? (It is outside the enumerated packet structure though within the scope of the packet's content.) I suppose the alternative is to keep the address in the output.)
2. Also, I'm a lightweight on how to handle socket options. It seems we set the multicast options on every send operation. Is that necessary? Or could it be set once on the socket and left alone afterward?

I'll look at the data_packet fixups as a separate pull-request as you suggest. Past that one, would you be interested in a separate pull request to clean up a bunch of flake 8 warnings?

Thank you for your thoughts.

On Mon, Mar 8, 2021 at 11:17 AM Hundemeier notifications@github.com wrote:

Even if it makes me seem rude, but I don't think this change should be made upstream into this library as is.

I have several problems with it:

1. It includes features outside of the scope of the ANSI E1.31 protocol https://tsp.esta.org/tsp/documents/docs/ANSI_E1-31-2018.pdf. Note that the value 0xDD as a DMX start code is never mentioned in the document.
2. I think the current implementation violates the separation of concerns https://en.wikipedia.org/wiki/Separation_of_concerns principle. The current implementation of sender and receiver are limited to features directly mentioned in the E1.31 protocol document and I would like to keep it that way.
3. If the functionality to send out specific priority information is implemented on the sender (output.py and output_thread.py) it should also be implemented on the receiving side (receiver.py and receiver_thread.py) (might be a future step; minor issue).
4. Where does the value of INTERVAL_CORRECTION_FACTOR = 0.984 come from? Could you provide a link or resource which explains this value?

Keep in mind that I am not against the core idea behind this pull request (implementing per-channel priority), but I would do it differently.

I suggest a couple of things:

1. Create a separate pull request for your changes on data_packet.py, since this fixes a long standing issue, I would like to merge it.
2. Separate the handling of 0xDD start code packets by creating a separate class (potentialy inheriting Output), although I am not quite sure about the changes to the OutputThread; maybe a second separate class?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Hundemeier/sacn/pull/26#issuecomment-792916031, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHRDGJPWDH5QBWVM2NJ2KTTCUBCJANCNFSM4YYIOTKQ .

[Hundemeier]

1. E1.31 allows for alternate start codes ...

Thanks for the additional resources! Since I never heared of the start code 0xDD before, I surely will read and try to understand the described steps by ETC.

2. I considered additional classes ...

Quite a difficult problem to tackle, I will think about that one for a couple of days and hope I get a good idea for this. Otherwise the current approach might be the pragmatic one.

3. I had left the receiver side as a separate future step. ...

I'm with you on that one. I think this should only be implemented in the future if needed.

4. I knew someone was going to ...

I still need to understand the 0xDD intricacies, but line 69ff in output_thread.py might be interesting? Would it be possible to define a "fps" count for those specific packets?

1. Would you be averse to putting the calculated target into data_packet?

I'm not sure I can follow you, but there is already a method for calculating the multicast target address via the universe in data_packet.py line 167ff.

It seems we set the multicast options on every send operation. Is that necessary?

I think there is no other choice. If i remember correctly only one socket per port can be created, and if a packet requires specific settings, they need to be set before the send operation. I don't think this is a performance critical part in the process, otherwise it would have probably been addressed in #25 .

Past that one, would you be interested in a separate pull request to clean up a bunch of flake 8 warnings?

If you mean warnings in the master branch: of course! If the changes are only relevant for this branch, keep it here for now.

[mthespian]

Great thank you! I'll be getting the data_packet cleanup ready for you in the next day or so.

I'm asking about the choice to calculate the multicast address on every send iteration. If that value was calculated when the universe property was set, we could store it and just retrieve it on send. Bit operations aren't super expensive, but reading a stored variable would be faster. Since a universe number rarely changes, you're trading a tiny amount of extra stored data for potentially 60-120 bit operations and string substitutions a second.

If we chose to store it, we could either add it into data_packet, or put it into output. But applying it in output suggests a need to bring target universe up into the output class so we can update the multicast destination when the universe changes.

About the socket options: I'm not seeing them ever unset anywhere. Once applied to the socket, don't they stay? Must they be reasserted before each output packet?

On Mon, Mar 8, 2021 at 3:38 PM Hundemeier notifications@github.com wrote:

1. E1.31 allows for alternate start codes ...

Thanks for the additional resources! Since I never heared of the start code 0xDD before, I surely will read and try to understand the described steps by ETC.

1. I considered additional classes ...

Quite a difficult problem to tackle, I will think about that one for a couple of days and hope I get a good idea for this. Otherwise the current approach might be the pragmatic one.

1. I had left the receiver side as a separate future step. ...

I'm with you on that one. I think this should only be implemented in the future if needed.

1. I knew someone was going to ...

I still need to understand the 0xDD intricacies, but line 69ff in output_thread.py https://github.com/Hundemeier/sacn/blob/a25b6bef85e09653224b828a70c6c3ebc7418e2e/sacn/sending/output_thread.py#L69 might be interesting? Would it be possible to define a "fps" count for those specific packets?

1. Would you be averse to putting the calculated target into data_packet?

I'm not sure I can follow you, but there is already a method for calculating the multicast target address via the universe in data_packet.py line 167ff https://github.com/Hundemeier/sacn/blob/a25b6bef85e09653224b828a70c6c3ebc7418e2e/sacn/messages/data_packet.py#L167 .

It seems we set the multicast options on every send operation. Is that necessary?

I think there is no other choice. If i remember correctly only one socket per port can be created, and if a packet requires specific settings, they need to be set before the send operation. I don't think this is a performance critical part in the process, otherwise it would have probably been addressed in #25 https://github.com/Hundemeier/sacn/pull/25 .

Past that one, would you be interested in a separate pull request to clean up a bunch of flake 8 warnings?

If you mean warnings in the master branch: of course! If the changes are only relevant for this branch, keep it here for now.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Hundemeier/sacn/pull/26#issuecomment-793098610, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHRDGLYTJH3PSNGB54XBLDTCU7XBANCNFSM4YYIOTKQ .

[Hundemeier]

If you are still up for it, you could now integrate your changes into the new sending implementation. I suggest inheriting from output.py, sender.py and sender_handler.py. This should be feasible now, as you can now "hook" into the sending loop of the sender by inheriting from SenderHandler and overriding on_periodic_callback while also calling the parent super().on_periodic_callback implementation.

---

About the socket options: I'm not seeing them ever unset anywhere. Once applied to the socket, don't they stay? Must they be reasserted before each output packet?

They should propably be reasserted before each output packet, but this actually should only apply to the TTL-setting.

[tresler]

I was doing my fork of this repository on my_github and I rewrite it for manual flash sending with channel priority with 0xDD. It is meaby not correctly and clear, but it works. You can check and try it with my own example.