Hundemeier / sacn

A simple ANSI E1.31 (aka sACN) module for python.
MIT License
47 stars 21 forks source link

Update receiver_socket_udp.py #51

Closed andrewyager closed 1 month ago

andrewyager commented 2 months ago

Proposed fix for #47 - allowing for differences between Windows and Linux.

Noting that I'm not clear that the difference between the Linux and Windows implementation is needed; but I don't have the ability to test that; so I've added use of the platform module to behave differently between the two operating systems.

Hundemeier commented 2 months ago

Thanks for the PR!

However, it might be sufficient to simply use the bind_address parameter of the sACNreceiver on Linux. Can you confirm that the following has the same effect?

sacn.sACNreceiver(bind_address='')

You PR would disable the bind_address parameter on Linux completely. This would require at least a big note in the Readme. If the above code snippet works, this might be more of a documentation issue or the platform OS needs to be read by sACNreceiver in addition to checking whether or not the bind_address parameter was set.

andrewyager commented 2 months ago

This definitely does not work!

There are a few reasons. Specifically, the join multicast socket code requires the correct interface IP to be specified. Even specifying 0.0.0.0 will cause the multicast group membership to be listed on the first interface on the box and not on any others.

Secondly, the aton call requires an IP - and a blank string doesn't meet the requirements for this so it will fail.

It's questionable, to be honest, if the bind address works at all on Linux in the current form. It may indeed work for unicast sACN, but this is not technically within standard. Certainly - on a single interface box it works because you are binding to the first interface and the multicast group membership is there; but if your receive interface is not the first on the box it definitely does not work in its current form.

andrewyager commented 2 months ago

But for clarity - this was one of the first things I tried, because a fix that did not require a patch or a different library was ideal.

The change that I've proposed also still works on single IP interfaces; but I don't know the impact of it on unicast scenarios as I do not have any equipment that will send a unicast sACN stream.

That said, I believe that is the only circumstance where this fix wouldn't be ideal; because in all other cases the multicast group join socket option binds the IP.

Hundemeier commented 2 months ago

After reading #47 as well I understand the issue a bit better. Maybe this whole issue is a regression from #42 , that changed something different. What if you try your specific IP-address

sacn.sACNreceiver(bind_address="1.2.3.4")

combined with a reverted #42? Change https://github.com/Hundemeier/sacn/blob/1bc53abc677c97ba4f8a40e13b341de49ffd18c4/sacn/receiving/receiver_socket_udp.py#L80 to socket.SOL_IP instead of socket.IPPROTO_IP. The same with https://github.com/Hundemeier/sacn/blob/1bc53abc677c97ba4f8a40e13b341de49ffd18c4/sacn/receiving/receiver_socket_udp.py#L91

andrewyager commented 2 months ago

Sorry for the multiple messages - one more comment is that the bind_address is still used; in the building of the struct that is passed with the multicast address.

In the scenario with multiple interfaces it is necessary to bind the listener to the correct interface by passing the correct IP there. It is only when the socket is also created bound to that IP explicitly that it fails.

andrewyager commented 2 months ago

After reading #47 as well I understand the issue a bit better. Maybe this whole issue is a regression from #42 , that changed something different.

What if you try your specific IP-address


sacn.sACNreceiver(bind_address="1.2.3.4")

combined with a reverted #42?

Change

https://github.com/Hundemeier/sacn/blob/1bc53abc677c97ba4f8a40e13b341de49ffd18c4/sacn/receiving/receiver_socket_udp.py#L80

to socket.SOL_IP instead of socket.IPPROTO_IP. The same with

https://github.com/Hundemeier/sacn/blob/1bc53abc677c97ba4f8a40e13b341de49ffd18c4/sacn/receiving/receiver_socket_udp.py#L91

I tried this change in an earlier attempt to fix it/understand the problem and it behaves identically to the current non-working behaviour; as in it still does not work.

Hundemeier commented 2 months ago

It seems that your tests and this PR are currently the best fix for this behavior. Could you please add a note along the lines of "On Linux this parameter is ignored when binding the socket. However, it is still used when joining or leaving a multicast universe." to the bind_address parameter in the Readme.md?

Then I will merge this PR, create a new version, and upload it to PyPI.

andrewyager commented 1 month ago

Apologies for taking some time to get back to this - but I've just done this today!

Hundemeier commented 1 month ago

Looks good to me, even if the checks fail with

./sacn/sending/sender_handler.py:47:42: E502 the backslash is redundant between brackets
            if not self.manual_flush and \
            (output._changed or abs(current_time - output._last_time_send) >= SEND_OUT_INTERVAL)]
                                         ^

Which is unrelated to this PR and I don't understand why it gets flagged.

Apologies for taking some time to get back to this - but I've just done this today!

No worries, I'm not one to respond fast either.

andrewyager commented 1 month ago

I think the linting error is because the if statement is inside square brackets, so the line formatting doesn't need the \ escape.

Hundemeier commented 1 month ago

Yes, that particular flake8 error was added recently. I've changed https://github.com/Hundemeier/sacn/blob/8d807be73ba816b6e037993f4754d2dab118f226/sacn/sending/sender_handler.py#L47-L48

So the new version v0.10.0 does contain this fix.