[PATH] Add native Bluetooth support for Windows platforms

[Claudiu Olteanu]

Hi Thiago,

Thanks for making time to look over the patches!

> 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?
>

Sorry for the confusion but after some discussions with Dirk I removed that
commit.
All the new patches were included in my latest e-mail from this thread.
Also you can
find them in this branch [1].

The only difference is that patch number 3 was replaced with this one [2]
and
I added a new one [3].

Now I clear the list with the discovered devices when a new device lookup
is
initiated.


> > 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.
>

Ok, I will move the WSACleanup call in the same patch where WSAStartup is
used.


> 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.
>

I had that in mind but I totally forgot :). I will do the initialization.


> > 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.
>

Yes, you are right.


> > 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.
>

Thanks for the hint! I will remove the internal function and use the one
you
suggested.


> > 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);
>

Thank you for your detailed explanation! I didn't know about the difference
between the W functions on Windows instead of the A ones.
Since I will create a new set of patches I will try to add all the things
you
suggested.

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.
>

I know that there is code duplication but I thought that it would be easier
to follow the implementation :).

Have a nice day,
Claudiu

[1] - https://github.com/claudiuolteanu/subsurface/commits/bth_windows
[2] -
https://github.com/claudiuolteanu/subsurface/commit/9ae338fdfe28c6707e98442ebe587a4b22d6809c
[3] -
https://github.com/claudiuolteanu/subsurface/commit/08d3c3a6ebfa69c41810d2af158fb2808a0c0a11
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150818/d2b46f76/attachment.html>