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.

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

Add Kinet portout support, closes #1544 #1787

Merged
merged 51 commits into from
Jun 21, 2023

Conversation

peternewman
Copy link
Member

No description provided.

@peternewman peternewman linked an issue Sep 10, 2022 that may be closed by this pull request
@peternewman
Copy link
Member Author

@DaAwesomeP @jmusarra I think you've previously had some knowledge/use of KiNet/access to KiNet kit, so any help with testing this would be appreciated! I've only got access to one third party device which seems to have a somewhat questionable implementation.

Some clarification would also be good

  • I'm assuming in PORTOUT mode there are effectively multiple strings of pixels or whatever, one off each port, so you'd want to send data to ALL the ports on an IP?
  • Does anyone use the KiNet universe? Other stuff seems to just set it to 0xffffffff which I assume is some sort of broadcast option?
  • One alternative device seems to implement a port 0 which sends to ALL ports on that IP (just by sending similar packets lots of times). Is that useful? Would it be better as a separate config option rather than a physical port with lots of confusing overlap?
  • Would polling/discovery of the remote devices be useful where they support it (a bit like some of our ArtNet stuff)?
  • Does anyone want KiNet input (or is just output enough)?
  • If so does anyone really care about discovery or device info within Quickplay Pro or similar tools. It half works with our kinet.cpp code, but doesn't seem to pop up immediately currently for some reason (I'd need more captures of real devices for this) Or are you happy to just fire data at given IPs?

If we want multiple ports, I guess ideally we want multiple devices too (just so the OLA ports match the KiNet ports really):
Device 9: KiNet Device - 10.1.1.95 - Portout
port 0, OUT Power Supply: 10.1.1.95, patched to universe 1
port 1, OUT Power Supply: 10.1.1.95, patched to universe 2
port 2, OUT Power Supply: 10.1.1.95, patched to universe 3
port 3, OUT Power Supply: 10.1.1.95, patched to universe 4
port 4, OUT Power Supply: 10.1.1.95, patched to universe 5
port 5, OUT Power Supply: 10.1.1.95, patched to universe 6
port 6, OUT Power Supply: 10.1.1.95, patched to universe 7
port 7, OUT Power Supply: 10.1.1.95, patched to universe 8
port 8, OUT Power Supply: 10.1.1.95, patched to universe 9
port 9, OUT Power Supply: 10.1.1.95, patched to universe 10
port 10, OUT Power Supply: 10.1.1.95, patched to universe 11
Device 10: KiNet Device - 10.1.1.96 - DMX
port 0, OUT Power Supply:

@DaAwesomeP
Copy link
Member

I have some CK devices at home and at work; I'll take a look next week!

@peternewman
Copy link
Member Author

As sort of mentioned, currently its just some WIP code (I didn't think anyone would reply so quickly). Changing

return m_node->SendDMX(m_target, buffer);
to SendPortOut and adding a port number after the IP and recompiling would make it output on that port number.

But more generally as someone who has or has used the real kit, input on some of the more general operational queries above would be appreciated. Having not really had hands on with proper kit I don't know if we need to actually implement universes for example.

@DaAwesomeP
Copy link
Member

DaAwesomeP commented Sep 13, 2022

Disclaimer: I am not super knowledgeable about the technical details of the protocol, but I can help with the functions and try things out.

Introducing some Color Kinetics-specific vocabulary to avoid confusion:

  • A Color Kinetics (CK) "power supply" is a device that converts DMX+power or Artnet+power or KiNET+power into the CK LED fixture protocol+power. They are configured via the QuickPlay Pro software. Basically it's a KiNET to CK fixture gateway with power and extra steps.
  • A "port" is a physical connection that you plug a chain of fixtures into on a power supply.

I'm assuming in PORTOUT mode there are effectively multiple strings of pixels or whatever, one off each port, so you'd want to send data to ALL the ports on an IP?

So with two strings of 50 fixtures each plugged into my PDS-60ca, light 1 on port 1 is address 1. Light 1 on port 2 is address 151. So for this particular case you would not need specific port packets or multiple universes. However, if you have more channels on the strings than fit in one universe then you would need another method; in that case I think you have to use the port out commands.

I think in most situations you would send to each port of a power supply separately because you usually have more than a few lights on a port. There are definitely cases were you only use one port on a power supply (if you have only one string of fixtures or they fit in one universe of non-port out commands), but if you are using both ports it is very likely that you want separate data to them with separate port out commands.

Does anyone use the KiNet universe? Other stuff seems to just set it to 0xffffffff which I assume is some sort of broadcast option?

On the PDS-60ca I have in front of me at the moment, I can set a universe for the whole device but it applies to both ports. This means that in order to control each port separately, you need to use the port out commands or have few enough lights to fit on one universe. The OLA that currently ships with Debian 11 writes to the single universe no matter which universe number is assigned in OLA or on the PDS.

The universes I think come in handy for fixtures that are combination KiNet+fixture all-in-one, like a ColorBlaze TRX. In this configuration KiNet functions not that different from an individual sACN fixture where each fixture has a universe and a starting address and I am pretty sure packets are sent multicast. I should note that the TRX is more of a live lighting/live event light and not a device targeted at installation like the PDS-60ca.

One alternative device seems to implement a port 0 which sends to ALL ports on that IP (just by sending similar packets lots of times). Is that useful? Would it be better as a separate config option rather than a physical port with lots of confusing overlap?

So there seem to be a few different methods of sending data:

  1. The current (I think unicast) method where the PDS appears as one universe with as many lights as will fit in one universe. Basically the PDS-60ca appears as a single device that consumes many channels in the KiNet universe. Each device appears separate in OLA (because it is unicast) and the universe is determined by the mapping in OLA, not by the universe number the device is configured to.
  2. The same as above but broadcast multicast, which is how the live lighting KiNet lights work and is similar to sACN. This would function identically to the way that sACN shows up where the number of the universe in OLA aligns with the KiNet universe. This method is definitely needed to realistically use devices like the TRX and is the simplest way for PDS devices if your lights fit in one universe.
  3. Port out to each individual port. This method is required to realistically use PDS installation devices. In this method, each physical port of strips of fixtures is controlled individually, I believe irrespective of the universe.
  4. Port out to all ports simultaneously (each port gets the same data). I'm sure that someone somewhere uses this, but I think it would be pretty rare that you would actually want to send the same signal to both ports/chains of lights on one device.

So methods 1 & 2 are more traditional universe and 3 & 4 are by port. I believe methods 1 & 2 are mutually exclusive from 3 & 4, but I don't think this needs to be represented as mutually exclusive in the UI. I'm thinking that instead one should show up as "KiNet universes" and one as "KiNet Port Out." So by default simply "enabling KiNet" causes options similar to sACN to show up. If you add in a specific IP address, then it would additionally show methods 1 and 3 (and possibly 4). For method 3, it should show each port separately. I think you can encompass method 4 by simply checking both method 3 ports into one universe in OLA.

Would polling/discovery of the remote devices be useful where they support it (a bit like some of our ArtNet stuff)?

Definitely. This is what Quick Play Pro does. It detects the IPs of devices. It can also initiate a fixture scan on the PDS where the PDS figures out how many lights are plugged into it, but that is less necessary. The most important parts are finding the IPs and how many ports are available on each IP.

Does anyone want KiNet input (or is just output enough)?

I can only see this being necessary if you are interfacing with a CK or other brand of playback device. I suppose it would be very useful for debugging but probably not as much as a missing feature as the above parts. You would probably only want to implement methods 1 & 2 as input or you would have to add some strange port logic I suppose to OLA.

If so does anyone really care about discovery or device info within Quickplay Pro or similar tools. It half works with our kinet.cpp code, but doesn't seem to pop up immediately currently for some reason (I'd need more captures of real devices for this) Or are you happy to just fire data at given IPs?

I am personally happy to just fire data at it as if it was sACN gateway (I suppose without RDM). You definitely need to know how many ports the PDS has and be able to assign them to separate universes, but I don't see a use for scanning the fixtures on the PDS or other configuration parameters. I feel like that will be a constant back-and-forth of reverse engineering as new devices come to market.

As far as OLA appearing in Quick Ply Pro, personally I can't see a need for it. Quick Play has a dropdown for specific devices from CK, so I could see that breaking with OLA pretty easily. I think it would be strange to go to the trouble to install a whole system and use a CK playback unit and need it to talk to OLA.

@DaAwesomeP
Copy link
Member

TLDR:

  1. You can send data as a single universe via multicast.
  2. You can send data as a single numbered universe as broadcast (like sACN).
  3. You can send data unicast to specific physical output ports.
  4. You can send the same data unicast to all physical output ports on a device at once.

Method 2 is most necessary for devices that are effectively self-contained fixtures where multiple KiNet devices sit on one universe (like sACN universes). Method 3 is most necessary for install systems with long chains of fixtures plugged into them. For method 3, each port should be able to be assigned to a universe in OLA (and preferably multiple ports on one universe).

@DaAwesomeP
Copy link
Member

DaAwesomeP commented Sep 13, 2022

I apologize for the message spam, but I hope this is all immensely helpful since I have the devices hands-on.

Talking to some acquaintances who also work with these devices, the KiNet spec is under NDA (currently shared with other close-source providers that implement it), but they also said may not actually hurt to ask CK about open sourcing at least part of it given how much progress has been made in reverse-engineering the V2 spec. This pull request is a good demonstration of that progress. Personally I have not seen the spec.

I have also confirmed that the universe method is v1 and called "DMXOUT" and the v2 method is "PORTOUT." The PDS-60ca supports both KiNet v1 (universes) and v2 (ports) so I can effectively test everything with it. The PDS-480ca, which I may use in a few months, only supports v2, so I am very interested in that.

I have confirmed that the DMXOUT mode on the PDS-60ca performs as I tested it: lights from both ports are placed back-to-back on one universe. But apparently this is deprecated and PORTOUT is the way to go.

I'm thinking that treating KiNet v1 and v2 entirely separate (separate config files in OLA) is the way to go. That way you can mix the devices if you need to. The two protocols also work very differently (universes vs devices w/ ports).

@peternewman
Copy link
Member Author

That's incredibly useful already thanks @DaAwesomeP as you say knowing the terminology and the explanation from using it in practise is something I've not had the benefit of. I'll reply in more depth to all your comments soon.

So in terms of testing, this PR should currently just implement options 3 (and 4 as you say by patching them all to the same OLA universe). Currently it generates 16 ports (from 1-16) for each PSU IP (with those configured in the same way as we used to for DMX out PSUs. I'd appreciate it if you get a chance to test that (I've checked my data against packets received from my test, non-CK, fixture, but I've not actually tried sending data back to it yet). In theory it should work though... 🤞

If so then I can work on finenessing all the other options too...

@DaAwesomeP
Copy link
Member

Great! I'll try building it tomorrow.

@peternewman
Copy link
Member Author

Disclaimer: I am not super knowledgeable about the technical details of the protocol, but I can help with the functions and try things out.

As mentioned, still super useful.

So with two strings of 50 fixtures each plugged into my PDS-60ca, light 1 on port 1 is address 1. Light 1 on port 2 is address 151. So for this particular case you would not need specific port packets or multiple universes. However, if you have more channels on the strings than fit in one universe then you would need another method; in that case I think you have to use the port out commands.

So if you sent port out, would you expect that offset to apply relative to the two ports (i.e. it does it itself)?

Can you have more than (512/3) pixels on one physical port, if so what happens there, or can the PSUs not support that number of lights?

Does anyone use the KiNet universe? Other stuff seems to just set it to 0xffffffff which I assume is some sort of broadcast option?

On the PDS-60ca I have in front of me at the moment, I can set a universe for the whole device but it applies to both ports. This means that in order to control each port separately, you need to use the port out commands or have few enough lights to fit on one universe. The OLA that currently ships with Debian 11 writes to the single universe no matter which universe number is assigned in OLA or on the PDS.

Ah okay, so OLA is already using some sort of unicast send to the PSU, but sending to the "any universe" universe on the device, so it always receives the data on the PSU regardless of which universe that's set to. OLA certainly currently ignores the OLA universe when sending KiNet data.

What range of universes can you set on the device? Does it start at 0 or 1?

The universes I think come in handy for fixtures that are combination KiNet+fixture all-in-one, like a ColorBlaze TRX. In this configuration KiNet functions not that different from an individual sACN fixture where each fixture has a universe and a starting address and I am pretty sure packets are sent multicast. I should note that the TRX is more of a live lighting/live event light and not a device targeted at installation like the PDS-60ca.

A packet capture of that data would be invaluable, to find out where it multicasts to. Does it always multicast to one IP, and just change the universe data in the packet, or is there an IP per universe to multicast to too?

  1. The current (I think unicast) method where the PDS appears as one universe with as many lights as will fit in one universe. Basically the PDS-60ca appears as a single device that consumes many channels in the KiNet universe. Each device appears separate in OLA (because it is unicast) and the universe is determined by the mapping in OLA, not by the universe number the device is configured to.

Yes, that OLA send is currently unicast to the "broadcast universe" at that IP address.

  1. The same as above but broadcast multicast, which is how the live lighting KiNet lights work and is similar to sACN. This would function identically to the way that sACN shows up where the number of the universe in OLA aligns with the KiNet universe. This method is definitely needed to realistically use devices like the TRX and is the simplest way for PDS devices if your lights fit in one universe.

Okay, aside from pinning down some of the details above that would be a fairly easy change to add.

  1. Port out to each individual port. This method is required to realistically use PDS installation devices. In this method, each physical port of strips of fixtures is controlled individually, I believe irrespective of the universe.

Yeah that's what this new PR currently implements.

  1. Port out to all ports simultaneously (each port gets the same data). I'm sure that someone somewhere uses this, but I think it would be pretty rare that you would actually want to send the same signal to both ports/chains of lights on one device.

I assume some PSUs could have even more than just two ports?

So methods 1 & 2 are more traditional universe and 3 & 4 are by port. I believe methods 1 & 2 are mutually exclusive from 3 & 4, but I don't think this needs to be represented as mutually exclusive in the UI. I'm thinking that instead one should show up as "KiNet universes" and one as "KiNet Port Out." So by default simply "enabling KiNet" causes options similar to sACN to show up. If you add in a specific IP address, then it would additionally show methods 1 and 3 (and possibly 4).

I think logically config wise, 2 should be separate from 1 and 3 (and 4), I was more leaning towards with 1 and 3 you have to set an IP, and when you've set one you can configure to send to that IP by DMX or by port out; currently if you picked portout it would generate a device with 16 OLA ports, where each corresponds to a different KiNet port on that device.

For method 3, it should show each port separately. I think you can encompass method 4 by simply checking both method 3 ports into one universe in OLA.

Agreed, that will definitely work.

Would polling/discovery of the remote devices be useful where they support it (a bit like some of our ArtNet stuff)?

Definitely. This is what Quick Play Pro does. It detects the IPs of devices. It can also initiate a fixture scan on the PDS where the PDS figures out how many lights are plugged into it, but that is less necessary. The most important parts are finding the IPs and how many ports are available on each IP.

Ah I didn't realise you could find the port count too. Do you have two different devices with different port counts you could test with? We'll also need to allow manual config too, if anyone wants to send to an unofficial device or one which doesn't respond.

Does anyone want KiNet input (or is just output enough)?

I can only see this being necessary if you are interfacing with a CK or other brand of playback device. I suppose it would be very useful for debugging but probably not as much as a missing feature as the above parts. You would probably only want to implement methods 1 & 2 as input or you would have to add some strange port logic I suppose to OLA.

Okay, definitely a lower priority then, and easier to do at least some testing with Quickplay Pro of my own accord. Well we could make an OLA dummy CK device which just has 16 ports say, and they'd appear as OLA ports so you could patch those if you needed to receive port out data.

I am personally happy to just fire data at it as if it was sACN gateway (I suppose without RDM). You definitely need to know how many ports the PDS has and be able to assign them to separate universes, but I don't see a use for scanning the fixtures on the PDS or other configuration parameters. I feel like that will be a constant back-and-forth of reverse engineering as new devices come to market.

As far as OLA appearing in Quick Ply Pro, personally I can't see a need for it. Quick Play has a dropdown for specific devices from CK, so I could see that breaking with OLA pretty easily. I think it would be strange to go to the trouble to install a whole system and use a CK playback unit and need it to talk to OLA.

How does Quick Play Pro work with other manufacturer then?

@DaAwesomeP
Copy link
Member

OK, I built the new version, but I'm not seeing the new ports:
image

ola-kinet.conf:

enabled = true
power_supply = 192.168.1.250

I guess maybe I misunderstood your code note above, do I need to change that line and then this listed KiNet device will output to that fixed port?

@DaAwesomeP
Copy link
Member

DaAwesomeP commented Sep 14, 2022

So if you sent port out, would you expect that offset to apply relative to the two ports (i.e. it does it itself)?

Since this is a device with two physical ports but I am connecting in DMXOUT mode, it places the fixtures from both ports in one universe. In PORTOUT mode, each physical port would be completely separate with the devices starting at address 1.

Can you have more than (512/3) pixels on one physical port, if so what happens there, or can the PSUs not support that number of lights?

I would assume that no, you cannot have more than 512 addresses on a port. The datasheet for the largest PDS that I know of, the PDS-480ca, says that it can support no more than 75 fixtures on a port (225 addresses for 3-channel fixtures): https://www.docs.colorkinetics.com/support/datasheets/sPDS-480ca_ProductGuide.pdf

Ah okay, so OLA is already using some sort of unicast send to the PSU, but sending to the "any universe" universe on the device, so it always receives the data on the PSU regardless of which universe that's set to. OLA certainly currently ignores the OLA universe when sending KiNet data.

That matches my current experience with OLA.

What range of universes can you set on the device? Does it start at 0 or 1?

The textbox in QuickPlay Pro allows me to set from 0 to 65535 inclusive on the PDS-60ca.

A packet capture of that data would be invaluable, to find out where it multicasts to. Does it always multicast to one IP, and just change the universe data in the packet, or is there an IP per universe to multicast to too?

I think it is broadcast (to the subnet broadcast address) and it changes the packet contents. I am not sure if QuickPlay Pro will produce this packet or not, but I can try. It appears that in the left-hand pane of the "Test" page that I can select all Ethernet devices at once and send data to them. It seems to being using the DMXOUT method to do this since I cannot select a port, but it also doesn't let me select a universe (so it is maybe either using PORTOUT to all devices or DMXOUT to all universes).

The old version of QuickPlay Pro might be able to do this. I have some older DMXOUT-only devices at home that I can try playing around with.

I will think about what devices I may have that output a broadcast packet. I think this is the only part of the DMXOUT part of OLA that is missing.

I assume some PSUs could have even more than just two ports?

Definitely. The PDS-480ca has 8 ports!

I think logically config wise, 2 should be separate from 1 and 3 (and 4), I was more leaning towards with 1 and 3 you have to set an IP, and when you've set one you can configure to send to that IP by DMX or by port out; currently if you picked portout it would generate a device with 16 OLA ports, where each corresponds to a different KiNet port on that device.

I see. This prevents you from simultaneously sending a unicast DMXOUT and unicast PORTOUT to the same device as only one would appear in the interface.

Maybe it could be something like (similar to OSC):

enable_broadcast = true
power_supplies = 2
power_supply_1_ip = ip
power_supply_1_mode = portout
power_supply_1_ports = 8
power_supply_2_ip = ip
power_supply_2_mode = dmxout
  • The enable_broadcast option would enable method 3 (on by default), which would be zero-configuration operation like similar to sACN.
  • Each power supply can be in either DMXOUT or PORTOUT mode, but not both.
  • The number of ports on a PORTOUT device can be configured. This is better than just rigidly listing 16 ports. This option possibly removed if we get detection working.

Ah I didn't realise you could find the port count too. Do you have two different devices with different port counts you could test with? We'll also need to allow manual config too, if anyone wants to send to an unofficial device or one which doesn't respond.

I'm not certain if QuickPlay Pro is doing a lookup of the device type or if the device says how many ports it has. QuickPlay Pro definitely can identify models from serial numbers, as you can add virtual devices by serial number or model. I do not have two different types of PORTOUT devices at the moment, but I will most likely in a few months.

Okay, definitely a lower priority then, and easier to do at least some testing with Quickplay Pro of my own accord. Well we could make an OLA dummy CK device which just has 16 ports say, and they'd appear as OLA ports so you could patch those if you needed to receive port out data.

Sounds good! I think the DMXOUT input would also probably be simple to implement and similarly lower priority.

How does Quick Play Pro work with other manufacturer then?

Ah, I didn't think about that. I do not have any non-CK devices. However, in QuickPlay Pro if you click add Ethernet device, it does not show a "generic" or other method of adding another manufacturer's device (or maybe some of the ones listed are from other manufacturers). You can add a "Generic 3-ch" or etc. fixture to an Ethernet device though. How does it work with your device? This also might be worth checking in the old version of QuickPlay Pro.

@DaAwesomeP
Copy link
Member

OK, I built the new version, but I'm not seeing the new ports

Ah, ok I seem to have been using the existing version of libolaserver.so.0 on my machine. Where is libolaserver.so.0 built to after running make all?

@peternewman
Copy link
Member Author

peternewman commented Sep 14, 2022

OK, I built the new version, but I'm not seeing the new ports:
I guess maybe I misunderstood your code note above, do I need to change that line and then this listed KiNet device will output to that fixed port?

Did you run it as ./olad/olad -l 4 i.e. the new code not something else on the machine?

Can you share olad -l 4 logs which will confirm that?

@DaAwesomeP
Copy link
Member

DaAwesomeP commented Sep 14, 2022

OK, I removed the OS-installed OLA and ran it with:

$ LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/home/myuser/GitHub/ola/olad/.libs ./olad -c ~/ola -d /home/myuser/GitHub/ola/olad/www -p 9091
/home/myuser/GitHub/ola/olad/.libs/olad: error while loading shared libraries: /home/myuser/GitHub/ola/olad/.libs/libolaserver.so.0: file too short

It seems that file is indeed smaller than it should be:

$ ls -al .libs/
total 97836
drwx------ 2 myuser myuser     4096 Sep 14 13:37 .
drwxr-xr-x 7 myuser myuser     4096 Sep 14 13:37 ..
-rw------- 1 myuser myuser 49428096 Sep 14 13:37 libolaserver.a
-rw------- 1 myuser myuser       18 Sep 14 13:37 libolaserver.la
-rw------- 1 myuser myuser  1273776 Sep 14 11:58 libolaserver_la-AvahiDiscoveryAgent.o
-rw------- 1 myuser myuser   435176 Sep 14 11:58 libolaserver_la-ClientBroker.o
-rw------- 1 myuser myuser   158216 Sep 14 11:58 libolaserver_la-DiscoveryAgent.o
-rw------- 1 myuser myuser   224048 Sep 14 11:58 libolaserver_la-DynamicPluginLoader.o
-rw------- 1 myuser myuser   293264 Sep 14 11:58 libolaserver_la-HttpServerActions.o
-rw------- 1 myuser myuser     1384 Sep 14 13:37 libolaserver.lai
-rw------- 1 myuser myuser  2013288 Sep 14 11:58 libolaserver_la-OlaDaemon.o
-rw------- 1 myuser myuser  6857328 Sep 14 13:36 libolaserver_la-OladHTTPServer.o
-rw------- 1 myuser myuser  3211256 Sep 14 11:58 libolaserver_la-OlaServer.o
-rw------- 1 myuser myuser  3197904 Sep 14 11:58 libolaserver_la-OlaServerServiceImpl.o
-rw------- 1 myuser myuser   733352 Sep 14 11:58 libolaserver_la-PluginManager.o
-rw------- 1 myuser myuser 11969600 Sep 14 11:59 libolaserver_la-RDMHTTPModule.o
-rw------- 1 myuser myuser       21 Sep 14 13:36 libolaserver.so
-rw------- 1 myuser myuser       21 Sep 14 13:36 libolaserver.so.0
-rw------- 1 myuser myuser 19436720 Sep 14 13:36 libolaserver.so.0.0.0
-rwx--x--x 1 myuser myuser   819232 Sep 14 13:37 olad

I configured with:

$ ./configure --disable-all-plugins  --enable-e131 --enable-kinet --enable-dummy --enable-osc --enable-artnet  --enable-shared --enable-static

And build with:

$ make all -j4

@DaAwesomeP
Copy link
Member

My apologies...this is some KVM filesystem shenanigans. I have it all listed now:
image

@DaAwesomeP
Copy link
Member

OK, so unfortunately it does not seem to work. Nothing happens when I manipulate the sliders for both universe/port 1 and 2.

image

image

image

image

When running with -l 4 the log has the patching as I patch the universes and then just a ton of polling messages:

olad/plugin_api/PortManager.cpp:119: Patched 16-192.168.1.250-O-2 to universe 2
common/io/EPoller.cpp:306: ss process time was 0.000003
common/io/SelectPoller.cpp:233: ss process time was 0.000020
common/io/EPoller.cpp:306: ss process time was 0.000001
common/io/SelectPoller.cpp:233: ss process time was 0.000071
common/io/SelectPoller.cpp:233: ss process time was 0.000022
common/io/SelectPoller.cpp:233: ss process time was 0.000174
common/io/EPoller.cpp:306: ss process time was 0.000003
common/io/SelectPoller.cpp:233: ss process time was 0.000016
common/io/EPoller.cpp:306: ss process time was 0.000003
common/io/SelectPoller.cpp:233: ss process time was 0.000040
common/io/SelectPoller.cpp:233: ss process time was 0.000008
common/io/SelectPoller.cpp:233: ss process time was 0.000092
common/io/EPoller.cpp:306: ss process time was 0.000003
common/io/SelectPoller.cpp:233: ss process time was 0.000024
common/io/SelectPoller.cpp:233: ss process time was 0.000008
common/io/SelectPoller.cpp:233: ss process time was 0.000127
common/io/EPoller.cpp:306: ss process time was 0.000004
common/io/SelectPoller.cpp:233: ss process time was 0.000026

@DaAwesomeP
Copy link
Member

If we are using C++17 then the fix is:

KiNetNode.h:

class KiNetNode {
    private:
        static constexpr uint16_t KINET_PORTOUT_MIN_BUFFER_SIZE = 24;
};

@peternewman
Copy link
Member Author

One thing I have noticed over the past week is that if a KiNET device goes offline, then OLA slows to a crawl and the output stutters/becomes choppy on all universes (even the ones that are still online).

This isn't a huge issue and I'm sure the same issue affects other protocols in OLA as well, but it would be nice to have some resiliency such that one failure for some universes won't affect everything else.

That doesn't sound great, would you mind making a new issue to track it, particularly because as you say it might impact other OLA plugins.

I think the linker is maybe struggling with this because we are using std::max(), which possibly causes the linker to leave the class's private namespace while evaluating the reference.

Spot on, removing that for this usage in this build here clears the problem:
https://github.com/peternewman/ola/actions/runs/4571667878

So at least we know what needs resolving now!

@DaAwesomeP
Copy link
Member

Aha! Great! I think we should leave in the std::max() and fix the private namespace issue with one of the solutions above. Simply removing calls that leave the namespace feels like an annoying bandaid to work around the next time this code is touched.

I'm sure something like clang-tidy or other more strict static analysis would find this sort of C++ non-compliance bug in the future if we want to go that route. However the number of changes across the repo would be drastic unless it was done in small parts.

@DaAwesomeP
Copy link
Member

Opened peternewman#44 to resolve the variable issue.

KiNET Fix KiNetNode Static Private Constant Variables
@DaAwesomeP
Copy link
Member

Lint fails as expected from a codespell typo not from this pull, but the Debian build now passes!

@DaAwesomeP DaAwesomeP mentioned this pull request Apr 19, 2023
@DaAwesomeP
Copy link
Member

@peternewman polite ping!

@peternewman peternewman marked this pull request as ready for review April 30, 2023 21:17
@peternewman
Copy link
Member Author

Aha! Great! I think we should leave in the std::max() and fix the private namespace issue with one of the solutions above. Simply removing calls that leave the namespace feels like an annoying bandaid to work around the next time this code is touched.

Thanks very much for the other PR. I wasn't proposing removing the std::max(), I just wanted to confirm we actually knew what the issue was, given we've used a similar structure successfully with other plugins previously.

@peternewman polite ping!

Sorry for the slow action, I've been away and also wanted to clean up a few outstanding todos. I think this is now good to merge if @kripton gets a chance to take a look too...

@DaAwesomeP
Copy link
Member

Sorry for the slow action, I've been away and also wanted to clean up a few outstanding todos. I think this is now good to merge if @kripton gets a chance to take a look too...

No worries!

I'm now running up to 1bc6ff4 on a large permanent installation!

@DaAwesomeP
Copy link
Member

Sorry for the slow action, I've been away and also wanted to clean up a few outstanding todos. I think this is now good to merge if @kripton gets a chance to take a look too...

Possibly ping @kripton to take a look!

Copy link
Member

@kripton kripton left a comment

Choose a reason for hiding this comment

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

Hi, sorry it took so long. Since I don't own such devices, I did a code-only review and didn't spot any obvious issues. Since you seem to be already running this code for some time in production, I'd say: got for it and thanks for the contribution!

@DaAwesomeP
Copy link
Member

@peternewman ready to merge?

@peternewman
Copy link
Member Author

@peternewman ready to merge?

I'd like the new stack of tests to run first, which will need a resync PR from 0.10 to master, which I can do in due course unless you're keen enough you want to generate one (in your own branch) for now?

@DaAwesomeP
Copy link
Member

@peternewman ready to merge?

I'd like the new stack of tests to run first, which will need a resync PR from 0.10 to master, which I can do in due course unless you're keen enough you want to generate one (in your own branch) for now?

Makes sense! I'll take a try at merge 0.10 into master, or at least my DaAwesomeP-GitHubActionsMakeChecks branch.

@DaAwesomeP DaAwesomeP mentioned this pull request Jun 18, 2023
@DaAwesomeP
Copy link
Member

@peternewman OK, that wasn't so hard: #1860

@DaAwesomeP
Copy link
Member

Amazing! Build is passing!

@peternewman peternewman merged commit e12b0a6 into OpenLightingProject:master Jun 21, 2023
20 checks passed
@peternewman
Copy link
Member Author

Thanks for doggedly working to get this in @DaAwesomeP , now onto the next challenge...!

@peternewman peternewman added this to the 0.11.0 milestone Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support KiNet Port Out Packets
3 participants