Send ArtnetPollReply for Art-Net proxy universe #3892
Conversation
|
@mxklb @troyhacks @netmindz as you have more DMX experience can you verify/review this PR, please? |
|
Is there an associated issue detailing what the issue is that this PR seeks to resolve? It's hard to review a solution without a clear understanding of the intention |
| } | ||
|
|
||
| #ifdef WLED_ENABLE_DMX |
There was a problem hiding this comment.
As far as I can tell, there's already code that is allowing Art-Net to be translated/proxied to DMX512, like some of the hardware boxes I've been using (Pknight, for example).
This (and the previous Art-Net PR) seems to extend this network-to-wired functionality. I think. 😁
There was a problem hiding this comment.
The variable e131ProxyUniverse used in the ifdef block is only available if WLED_ENABLE_DMX is defined (here)
There was a problem hiding this comment.
yeah easier to see from my laptop than looking on my phone. I can see now that you are not moving any existing code into the ifdef, just adding extra which does depend on a value only present based on existing ifdef
|
I think it's a feature enhancement. With some of the existing WLED code we can automatically detect Art-Net endpoints, and this appears to extend this to the Art-Net proxy system. I haven't used this functionality but I don't think there's any risk in adding it. Art-Net users at this level are few, I think. I'm one of them and I'm still kinda like ... whut? 😁 |
Yes, that's correct. This is a nice feature to get a quick overview (for example in QLC) of the available Art-Net nodes in the network and it would be useful if it also worked for proxied DMX outputs. |
|
I'm good with adding this. Thanks, @askask |
| @@ -346,7 +346,6 @@ void handleArtnetPollReply(IPAddress ipAddress) { | |||
|
|
|||
| switch (DMXMode) { | |||
| case DMX_MODE_DISABLED: | |||
| return; // nothing to do | |||
There was a problem hiding this comment.
while this code may well be redundant, code cleanup in a PR that is meant to be adding new features muddies the waters a bit. Better to keep the PR to only the changes required for the task
There was a problem hiding this comment.
I replaced the return with an if condition because otherwise the code I added wouldn't be called if DMX mode is set to disabled
There was a problem hiding this comment.
ah, yes I follow what you are doing now
|
Looking good. I'm not using this artnet poll feature, so can't test it, but looks like an improvement for DMX proxy. Save to merge |
| @@ -346,7 +346,6 @@ void handleArtnetPollReply(IPAddress ipAddress) { | |||
|
|
|||
| switch (DMXMode) { | |||
| case DMX_MODE_DISABLED: | |||
| return; // nothing to do | |||
There was a problem hiding this comment.
ah, yes I follow what you are doing now
This commit adds sending an ArtnetPollReply for the e131ProxyUniverse, which is currently not implemented.
It also checks that this universe doesn't overlap with the regular e131Universe, so no duplicate replies are sent.