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

Update receiver_socket_udp.py #51

Merged
merged 2 commits into from
Jun 27, 2024
Merged

Conversation

andrewyager
Copy link
Contributor

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.

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

Noting that I'm not clear that the Windows implementation is needed; but I don't have the ability to test that.
@Hundemeier
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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

self._socket.setsockopt(socket.IPPROTO_IP, socket.IP_ADD_MEMBERSHIP,

to socket.SOL_IP instead of socket.IPPROTO_IP. The same with
self._socket.setsockopt(socket.IPPROTO_IP, socket.IP_DROP_MEMBERSHIP,

@andrewyager
Copy link
Contributor Author

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
Copy link
Contributor Author

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

self._socket.setsockopt(socket.IPPROTO_IP, socket.IP_ADD_MEMBERSHIP,

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

self._socket.setsockopt(socket.IPPROTO_IP, socket.IP_DROP_MEMBERSHIP,

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
Copy link
Owner

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.

Update README to indicate that the bind_address parameter does not bind to the socket on Linux (but is still used to determine the correct interface to join the Multicast group) as per request from @Hundemeier
@andrewyager
Copy link
Contributor Author

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

@Hundemeier
Copy link
Owner

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.

@Hundemeier Hundemeier merged commit 186576c into Hundemeier:master Jun 27, 2024
0 of 10 checks passed
@andrewyager
Copy link
Contributor Author

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

@Hundemeier
Copy link
Owner

Yes, that particular flake8 error was added recently. I've changed

if not self.manual_flush and
(output._changed or abs(current_time - output._last_time_send) >= SEND_OUT_INTERVAL)]

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants