[PATH] Add native Bluetooth support for Windows platforms

[Thiago Macieira]

On Friday 14 August 2015 00:13:40 Claudiu Olteanu wrote:

Hi Claudiu

Great work, this is a very good contribution. Thank you for getting to it!

Most of my comments below are suggestions for learning and other requests for 
clarification on my part. There are just two issues that need to be fixed (both 
on patch 5). Please see below.

> Subject: [PATCH 03/17] Use only the BTH address to check if the device was
>  already added
>
> There are moments when the name of the device cannot be obtained.
> Therefore we should use only the Bluetooth address to identify if
> the discovered device was already added to the list.

> -       QList<QListWidgetItem *> itemsWithSameSignature =
> ui->discoveredDevicesList->findItems(deviceLabel, Qt::MatchStartsWith);  
> +     QList<QListWidgetItem *> itemsWithSameSignature =
> ui->discoveredDevicesList->findItems(remoteDeviceInfo.address().toString(),
> Qt::MatchContains);

This is a little confusing. I understand that the device is added to the list 
first as a MAC address only and then replaced by the label. But I'm not sure if 
the fix is correct... where is the corresponding addition to the list in the 
first place? How can we be sure that the change to a label hasn't happened?

> Subject: [PATCH 05/17] Add skeleton for Bluetooth custom serial
> implementation
>  on Windows platforms

This patch, by itself, doesn't do anything, except:

>  BtDeviceSelectionDialog::~BtDeviceSelectionDialog()
>  {
>         delete ui;
> 
> +#if defined(Q_OS_WIN)
> +       // Terminate the use of Winsock 2 DLL
> +       WSACleanup();
> +#else

This will mess up the winsock DLL internal refcounting, so it could 
accidentally shut down all sockets in Subsurface... I know the matching 
WSAStartup is in patch 12, but this means that patches 5 through 11 are not 
testable as they (potentially) break Winsock2. I'm not talking about testing 
the Bluetooth code (the initializeDiscoveryAgent code isn't added until patch 
17). This is about the whole Subsurface. The WSACleanup bit should be moved to 
patch 12.

This error won't show up right now because Qt does one WSAStartup/WSACleanup 
per socket, so as long as you have two or more sockets running, you won't see 
an issue. Nor if the sockets are created afterward the dialog.

But that's not the case in the future: I've made Qt 5.6 call WSAStartup 
exactly once.

(Assuming these WSAStartup / WSACleanup aren't no-ops on any version of modern 
Windows -- the wine versions don't do anything useful: 
http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/ws2_32/socket.c#l1437 )

> +WinBluetoothDeviceDiscoveryAgent::WinBluetoothDeviceDiscoveryAgent(QObject
> *parent) : QThread(parent)

Please initialise the stopped and running variables. In patch 11, when you 
actually use them, the stopped variable is used uninitialised, in:

       while (result == SUCCESS && !stopped){

As for the running variable, the isActive function may race against the 
initialisation, so please initialise it too.

> Subject: [PATCH 06/17] Add implementation for BTH custom serial open method
> on
>  Windows platforms

> @@ -51,7 +51,49 @@ static int qt_serial_open(serial_t **out, dc_context_t
> *context, const char* dev
> +       char *address = strdup(devaddr);

There doesn't seem to be any need for strdup here.

> Subject: [PATCH 10/17] Add internal method which returns a pretty message 
>  about the last error on Windows

Hint: you could have used qt_error_string(). That does exactly what you did in 
your code.

> Subject: [PATCH 11/17] Add implementation for our custom BTH device
> discovery
>  service

> +       WSAQUERYSET *pResults = (WSAQUERYSET*)&buffer;
[...]
> +                       if (WSAAddressToStringA((LPSOCKADDR)
> socketBthAddress,
[...]
> +                       QString deviceName =
> QString(pResults->lpszServiceInstanceName);
> +                       QString deviceAddress = QString(addressAsString);

I'm not going to hold you to this, so this is just learning for next time. 

It's usually a good idea to use the W functions on Windows instead of the A 
ones. First of all, they are faster, since they are the real native functions 
-- the A functions need to convert from wchar_t to char back and forth. Not to 
mention that this conversion is lossy, so the A functions cannot represent all 
possibilities.

Second, they are supported on Windows CE, whereas the A functions aren't. Not 
really important to us, but it gets you in the right habit.

And third, you would avoid an UTF-8 decode in those two QString creations -- 
instead, you could have used QString::fromWCharArray or QString::fromUtf16, 
which are simple memcpy. Or, in the case of addressAsString, you could have 
used QString itself as your buffer:

	QString deviceAddress(BTH_ADDR_STR_LEN, Qt::Uninitialized);
	if (WSAAddressToString(....
			reinterpret_cast<wchar_t*>(deviceAddress.data()),
			...
	deviceAddress.truncate(addressSize);

> Subject: [PATCH 12/17] Initialize WinSock and hide the information about the
>  local device

Note that you still call WSACleanup even if WSAStartup failed. That should 
hopefully never be a problem and I'm not holding you to fixing this because... 
well, I've just done the same in Qt itself and I realise that we've done that 
for 15 years -- probably since QSocket was introduced in Qt 2.1.

> Subject: [PATCH 14/17] Add implementation for add remote Bluetooth device
>  handler

>  #if defined(Q_OS_WIN)
> -       // TODO add the remote device
> +       QString deviceLabel = QString("%1 
> (%2)").arg(remoteDeviceInfo.name()).arg(remoteDeviceInfo.address().toString
> ()); 
> +       QList<QListWidgetItem *> itemsWithSameSignature =
> ui->discoveredDevicesList->findItems(remoteDeviceInfo.address().toString(),
> Qt::MatchContains);
[...]
>  #else
>  
>         QString deviceLabel = QString("%1 
> (%2)").arg(remoteDeviceInfo.name()).arg(remoteDeviceInfo.address().
> toString()); 
>         QList<QListWidgetItem *> itemsWithSameSignature =
> ui->discoveredDevicesList->findItems(remoteDeviceInfo.address().toString(),
> Qt::MatchContains);

Please merge as much of the code as you can. The first two (or more) lines of 
the two branches of the #if are identical, so it's a good idea to keep them 
together to avoid code duplication.

Ditto on patch 15.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
      PGP/GPG: 0x6EF45358; fingerprint:
      E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358