bpo-36590: Add Bluetooth RFCOMM support for Windows.#12767
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
|
I believe unit tests are certainly needed for such a change. |
zooba
left a comment
zooba
left a comment
There was a problem hiding this comment.
Agreed it needs some kind of test (at least to validate the address parsing/rendering, if not actual functionality).
Also a couple of comments about testing for macro existence rather than platform. I didn't put comments on every instance, but it applies to all of them.
There was a problem hiding this comment.
Looks like AF_BTH is defined in all supported versions of Windows, so no need for the second part of the test.
There was a problem hiding this comment.
Done -- only checks for MS_WINDOWS now.
There was a problem hiding this comment.
Can we test whether macros are defined rather than checking for platform? (Also, is BTHPROTO_L2CAP the same as BTPROTO_L2CAP here?)
There was a problem hiding this comment.
I changed all of the ifndef MS_WINDOWS tests to ifdef BTPROTO_X and added/changed such tests in other cases (e.g. ifdef BTPROTO_SCO instead of checking for not FreeBSD).
BTHPROTO_L2CAP is the same as BTPROTO_L2CAP. Windows defines the constant, but winsock does not implement L2CAP support. I was able to build with this, but creating a socket with BTPROTO_L2CAP with winsock fails.
4faa1eb to
cae1d59
Compare
|
Added some basic unittests that check for some constants and try to create sockets with various protocols. I did not add any tests for the address parsing/rendering as I wasn't sure if there is a good way to do so. The only way to hit |
cae1d59 to
a02c83a
Compare
The change was pretty straightforward. The only difficulty was that Windows' SOCKADDR_BTH contains member names incompatible with the _BT_RC_MEMB() macros. To workaround this, a new struct, SOCKADDR_BTH_REDEF was defined and contains unions with both the Windows and Linux member names. Support for RFCOMM, L2CAP, HCI, SCO is based on the BTPROTO_* macros being defined. Winsock only supports RFCOMM, even though it has a BTHPROTO_L2CAP macro. L2CAP support would build on windows, but not necessarily work. This also adds some basic unittests for constants (all of which existed prior to this commit, just not on windows) and creating sockets. pair: Nate Duarte <slacknate@gmail.com>
a02c83a to
eb475d2
Compare
|
LGTM. Sorry for the delay in getting it merged |
) Support for RFCOMM, L2CAP, HCI, SCO is based on the BTPROTO_* macros being defined. Winsock only supports RFCOMM, even though it has a BTHPROTO_L2CAP macro. L2CAP support would build on windows, but not necessarily work. This also adds some basic unittests for constants (all of which existed prior to this commit, just not on windows) and creating sockets. pair: Nate Duarte <slacknate@gmail.com>
) Support for RFCOMM, L2CAP, HCI, SCO is based on the BTPROTO_* macros being defined. Winsock only supports RFCOMM, even though it has a BTHPROTO_L2CAP macro. L2CAP support would build on windows, but not necessarily work. This also adds some basic unittests for constants (all of which existed prior to this commit, just not on windows) and creating sockets. pair: Nate Duarte <slacknate@gmail.com>
) Support for RFCOMM, L2CAP, HCI, SCO is based on the BTPROTO_* macros being defined. Winsock only supports RFCOMM, even though it has a BTHPROTO_L2CAP macro. L2CAP support would build on windows, but not necessarily work. This also adds some basic unittests for constants (all of which existed prior to this commit, just not on windows) and creating sockets. pair: Nate Duarte <slacknate@gmail.com>
5 participants
The change was pretty straightforward. The only difficulty was that
Windows' SOCKADDR_BTH contains member names incompatible with the
_BT_RC_MEMB() macros. To workaround this, a new struct,
SOCKADDR_BTH_REDEF was defined and contains unions with both the Windows
and Linux member names.
pair: Nate Duarte slacknate@gmail.com
https://bugs.python.org/issue36590