-
Notifications
You must be signed in to change notification settings - Fork 659
udp_multicast interface: support windows #1914
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
udp_multicast interface: support windows #1914
Conversation
Neither the SO_TIMESTAMPNS / via recvmsg() method, nor the SIOCGSTAMP / ioctl() method for obtaining the packet timestamp is supported on Windows. This code adds a fallback to datetime, and switches to recvfrom() so that the udp_multicast bus becomes usable on windows.
on windows, the example would otherwise cause an _enter_buffered_busy fatal error, notifier shutdown and bus shutdown are racing eachother...
|
@thseiler There are two Multicast tests in Is there a reason to use datetime intead of |
|
@zariiii9003 Thanks a lot for the guidance. The |
can/interfaces/udp_multicast/bus.py
Outdated
| if ( | ||
| error.errno == errno.ENOPROTOOPT | ||
| or error.errno == errno.EINVAL | ||
| or error.errno == errno.WSAEINVAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could cause an AttributeError on Linux. You could redefine WSAEINVAL as a module constant.
test/back2back_test.py
Outdated
| @unittest.skipUnless( | ||
| IS_UNIX and not (IS_CI and IS_OSX), | ||
| "only supported on Unix systems (but not on macOS at Travis CI and GitHub Actions)", | ||
| (IS_UNIX and not (IS_CI and IS_OSX)) or IS_WINDOWS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can replace this with @unittest.skipIf(IS_CI and IS_OSX, ...
test/back2back_test.py
Outdated
| @unittest.skipUnless( | ||
| IS_UNIX and not (IS_TRAVIS or (IS_CI and IS_OSX)), | ||
| "only supported on Unix systems (but not on Travis CI; and not on macOS at GitHub Actions)", | ||
| (IS_UNIX and not (IS_TRAVIS or (IS_CI and IS_OSX))) or IS_WINDOWS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, we're not using Travis anymore anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment suggests skipping the UdpMulticast tests on macOS was necessary because of an issue with Travis CI. However, the tests may well work in the .github/workflow macOS environment, and skipping them is no longer necessary. Would you agree this is worth a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that might work. There's still a formatting issue, use black to fix it.
zariiii9003
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you 👍
Neither the recvmsg() / SO_TIMESTAMPNS method, nor the SIOCGSTAMP / ioctl() method for obtaining the packet timestamp is supported on Windows. This code switches from recvmsg to recvfrom() so that the udp_multicast works on Windows.
To support precise timestamps on Windows, Python would need to expose the WSArecvmsg() function first; see here. This PR is falling back to time.time() for now...