[PATH] Add native Bluetooth support for Windows platforms
These patches will add Bluetooth support for Windows platforms
and will fix some potential issues.
Since we cannot use the QtBluetooth framework I needed to
implement our custom Bluetooth device discovery agent.
I decided to create a separated class and raise the same signals
as QtBluetoothDeviceDiscoveryAgent class.
I am not sure if this is the best idea or the most elegant one but it works.
Claudiu
From 3d2e55d597aed27515650792f8bd77bf742f6576 Mon Sep 17 00:00:00 2001
From: Claudiu Olteanu
Date: Wed, 12 Aug 2015 22:46:31 +0300
Subject: [PATCH 01/17] Cleanup Bluetooth local device and the discovery agent
on exit
Do some extra cleanup when the BtDeviceSelectionDialog is destroyed.
Signed-off-by: Claudiu Olteanu
---
qt-ui/btdeviceselectiondialog.cpp | 9 +
1 file changed, 9 insertions(+)
diff --git a/qt-ui/btdeviceselectiondialog.cpp b/qt-ui/btdeviceselectiondialog.cpp
index 007fe94..ce759cc 100644
--- a/qt-ui/btdeviceselectiondialog.cpp
+++ b/qt-ui/btdeviceselectiondialog.cpp
@@ -61,6 +61,15 @@ BtDeviceSelectionDialog::BtDeviceSelectionDialog(QWidget *parent) :
BtDeviceSelectionDialog::~BtDeviceSelectionDialog()
{
delete ui;
+
+ // Clean the local device
+ delete localDevice;
+
+ // Clean the device discovery agent
+ if (remoteDeviceDiscoveryAgent->isActive())
+ remoteDeviceDiscoveryAgent->stop();
+
+ delete remoteDeviceDiscoveryAgent;
}
void BtDeviceSelectionDialog::on_changeDeviceState_clicked()
--
2.4.3
From aaa885948c1847bf766d3f0f6e546e4651100cca Mon Sep 17 00:00:00 2001
From: Claudiu Olteanu
Date: Wed, 12 Aug 2015 22:46:56 +0300
Subject: [PATCH 02/17] Check the last error when the BTH device scanning is
finished
If there is no error reported when the device scanning is finished
then report to the dialog status that the scanning finished
successfully. Otherwise report the last error.
Signed-off-by: Claudiu Olteanu
---
qt-ui/btdeviceselectiondialog.cpp | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/qt-ui/btdeviceselectiondialog.cpp b/qt-ui/btdeviceselectiondialog.cpp
index ce759cc..3af2501 100644
--- a/qt-ui/btdeviceselectiondialog.cpp
+++ b/qt-ui/btdeviceselectiondialog.cpp
@@ -124,7 +124,12 @@ void BtDeviceSelectionDialog::on_scan_clicked()
void BtDeviceSelectionDialog::remoteDeviceScanFinished()
{
- ui->dialogStatus->setText("Scanning finished.");
+ if (remoteDeviceDiscoveryAgent->error() == QBluetoothDeviceDiscoveryAgent::NoError) {
+ ui->dialogStatus->setText("Scanning finished successfully.");
+ } else {
+ deviceDiscoveryError(remoteDeviceDiscoveryAgent->error());
+ }
+
ui->scan->setEnabled(true);
}
--
2.4.3
From f21c7c9f9a9a135faed50b66b57223f6165658b3 Mon Sep 17 00:00:00 2001
From: Claudiu Olteanu
Date: Wed, 12 Aug 2015 22:48:54 +0300
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.
Signed-off-by: Claudiu Olteanu
---
qt-ui/btdeviceselectiondialog.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qt-ui/btdeviceselectiondialog.cpp b/qt-ui/btdeviceselectiondialog.cpp
index 3af2501..7fd2c7c 100644
--- a/qt-ui/btdeviceselectiondialog.cpp
+++ b/qt-ui/btdeviceselectiondialog.cpp
@@ -146,7 +146,7 @@ void BtDeviceSelectionDialog::hostModeStateChanged(QBluetoothLocalDevice::HostMo
void BtDeviceSelectionDialog::addRemoteDevice(const QBluetoothDeviceInfo &remoteDeviceInfo)
{
QString deviceLabel = QString("%1 (%2)").arg(remoteDeviceInfo.name()).arg(remoteDeviceInfo.address().toString());
- QList itemsWithSameSignature = ui->discoveredDevicesList->findItems(deviceLabel, Qt::MatchStartsWith);
+ QList itemsWithSameSignature = ui->discoveredDevicesList->findItems(remoteDeviceInfo.address().toString(), Qt::MatchContains);
// Check if the remote device is already in the list
if (itemsWithSameSignature.empty()) {
--
2.4.3
From d24aa8f4eeb6182750d79c78ff093a4043b3fad2 Mon Sep 17 00:00:00 2001
From: Claudiu Olteanu
Date: Wed, 12 Aug 2015 22:56:46 +0300
Subject: [PATCH 04/17] Add set_timeout callback for Bluetooth custom serial
implementation
The new callback will be usefull when we will implement the support
for Windows. The implementation of native serial set_timeout method
uses a HANDLER on Windows and we will use the WinSock2 API which has
a socket descriptor.
Signed-off-by: Claudiu Olteanu
---
qtserialbluetooth.cpp | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/qtserialbluetooth.cpp b/qtserialbluetooth.cpp
index 5a982d6..378f330 100644
--- a/qtserialbluetooth.cpp
+++ b/qtserialbluetooth.cpp
@@ -224,6 +224,15 @@ static int qt_serial_get_transmitted(serial_t *device)
return device->socket->bytesToWrite();
}
+static int qt_serial_set_timeout(serial_t *device, long timeout)
+{
+ if (device == NULL)
+ return DC_STATUS_INVALIDARGS;
+
+ de
Re: [PATH] Add native Bluetooth support for Windows platforms
On Fri, Aug 14, 2015 at 12:13:40AM +0300, Claudiu Olteanu wrote: > These patches will add Bluetooth support for Windows platforms > and will fix some potential issues. > > Since we cannot use the QtBluetooth framework I needed to > implement our custom Bluetooth device discovery agent. > I decided to create a separated class and raise the same signals > as QtBluetoothDeviceDiscoveryAgent class. > > I am not sure if this is the best idea or the most elegant one but it works. I have pushed the changes to the libdc/Subsurface-testing branch, but I'd like to hear from Thiago before merging this into Subsurface... > 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. Can you explain this a bit more? Should we not prefer the deviceLabel IF we can get it? For the rest of the patches... a) I really like how you broke things down into reasonable commits, first providing the placeholders and then implementing things one at a time. That makes it much easier to digest what you did there. b) did you write all this from scratch or was this inspired by some existing BT code for Windows from some other project? This is a non-trivial chunk of code in an area I'm not very familiar with... (and just to avoid email communication issues - no, I'm not doubting that you wrote this code... I just want to make sure that if there is a source we should credit that we do). Thanks /D ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATH] Add native Bluetooth support for Windows platforms
> > Can you explain this a bit more? Should we not prefer the deviceLabel IF > we can get it? > When I created that patch I thought that it would be better to use only the device address to identify if the device is already in the list. This is due the fact that currently the pattern of the deviceLabel is *$deviceName ($deviceAddress) $pairingStatus *. There are moments when the device name is missing and moments when the pairing status is different. Now if I think more clearer if we don't update the devices state it would lead to some inconsistent states. Probably it would be a better idea to clear the list with the discovered devices when a new device lookup is initiated. In this way we will show only the devices that are in range during the last scanning. If you agree with this I can send you another patch. > > b) did you write all this from scratch or was this inspired by some > existing BT code for Windows from some other project? This is a > non-trivial chunk of code in an area I'm not very familiar with... > (and just to avoid email communication issues - no, I'm not doubting that > you wrote this code... I just want to make sure that if there is a source > we should credit that we do). > The code is written from scratch but I used two sources of inspiration: - for the implementation of our custom serial callbacks I looked over the Native bluetooth communication sample created by Jef - for the device lookup I looked over a sample[1] provided by Microsoft Corporation to demonstrate how to use Winsock 2.2 Api and RFCOMM protocol I didn't know where I should mention them :) . While Jef is probably already mentioned for his help I am not sure if Microsoft should be mentioned when you get inspiration from one of their samples. Also you should know that our custom serial implementation is based on the native serial implementation provided by Jef (the design). I thought that it is obvious but I just wanted to be clear :). Claudiu [1] - https://code.msdn.microsoft.com/windowsdesktop/Bluetooth-Connection-e3263296 ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATH] Add native Bluetooth support for Windows platforms
On Sat, Aug 15, 2015 at 04:15:54PM +0300, Claudiu Olteanu wrote: > > > > Can you explain this a bit more? Should we not prefer the deviceLabel IF > > we can get it? > > When I created that patch I thought that it would be better to use only > the device address to identify if the device is already in the list. This is > due the fact that currently the pattern of the deviceLabel is > *$deviceName ($deviceAddress) $pairingStatus *. There are moments > when the device name is missing and moments when the pairing status is > different. > Now if I think more clearer if we don't update the devices state it would > lead > to some inconsistent states. > > Probably it would be a better idea to clear the list with the discovered > devices > when a new device lookup is initiated. In this way we will show only the > devices > that are in range during the last scanning. Yes, I think that's the way to go. > If you agree with this I can send you another patch. Please. > > b) did you write all this from scratch or was this inspired by some > > existing BT code for Windows from some other project? This is a > > non-trivial chunk of code in an area I'm not very familiar with... > > (and just to avoid email communication issues - no, I'm not doubting that > > you wrote this code... I just want to make sure that if there is a source > > we should credit that we do). > > > > The code is written from scratch but I used two sources of inspiration: > - for the implementation of our custom serial callbacks I looked over the > Native bluetooth communication sample created by Jef > - for the device lookup I looked over a sample[1] provided by Microsoft > Corporation > to demonstrate how to use Winsock 2.2 Api and RFCOMM protocol OK, this code is available under the MS-LPL (Microsoft Limited Public License). I assume that you didn't do any "cut and paste" but that you read the code and then reimplemented it based on the logic you understood from studying the code? I'm not a lawyer but from my reading of the license that is perfectly fine (and in many ways the intention of that code). If you did cut and paste the code then we need to rewrite things as the MS-LPL is not GPL compatible. > I didn't know where I should mention them :) . While Jef is probably already > mentioned for his help I am not sure if Microsoft should be mentioned when > you get inspiration from one of their samples. Absolutely. Whenever you either copy code or learn from code it is important to give credit. So in each commit message where you commit code that uses existing code either as inspiration (or for better understanding of API use) you should give credit. > Also you should know that our custom serial implementation is based on the > native serial implementation provided by Jef (the design). I thought that > it is > obvious but I just wanted to be clear :). Yes, that is obvious and was discussed back then. It's the MSFT code that I want to make sure we give proper credit to. Can you redo the commits and mention that you looked at this sample code in order to understand the use of the API? Something like this: "This code was written with the help of the sample code documenting the relevant APIs provided by Microsoft Corporation at https://code.msdn.microsoft.com/windowsdesktop/Bluetooth-Connection-e3263296 which is under the MS-LPL. No code from the samples was copied and the code in this commit is covered by the GPL and not the MS-LPL" Thanks /D ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATH] Add native Bluetooth support for Windows platforms
No I didn't do any copy and paste. I will send you tomorrow the patches with the modification discussed (I will clear the discovered devices list after each update) and I will update the commit message for the patch with the implementation of a device discovery agent. Claudiu ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATH] Add native Bluetooth support for Windows platforms
Hi,
I did the modifications we discussed and I added two extra
improvements (patch number 4 and 19).
The modifications are available on *bth_windows* branch [1].
I thought that it would be easier for Thiago to review the new code
if I will create a separate branch.
Claudiu
[1] - https://github.com/claudiuolteanu/subsurface/tree/bth_windows
From b5225ce0a5d94e70253d0a97b3f894234c7f93df Mon Sep 17 00:00:00 2001
From: Claudiu Olteanu
Date: Sun, 16 Aug 2015 15:51:32 +0300
Subject: [PATCH 01/19] Cleanup Bluetooth local device and the discovery agent
on exit
Do some extra cleanup when the BtDeviceSelectionDialog is destroyed.
Signed-off-by: Claudiu Olteanu
---
qt-ui/btdeviceselectiondialog.cpp | 9 +
1 file changed, 9 insertions(+)
diff --git a/qt-ui/btdeviceselectiondialog.cpp b/qt-ui/btdeviceselectiondialog.cpp
index 007fe94..ce759cc 100644
--- a/qt-ui/btdeviceselectiondialog.cpp
+++ b/qt-ui/btdeviceselectiondialog.cpp
@@ -61,6 +61,15 @@ BtDeviceSelectionDialog::BtDeviceSelectionDialog(QWidget *parent) :
BtDeviceSelectionDialog::~BtDeviceSelectionDialog()
{
delete ui;
+
+ // Clean the local device
+ delete localDevice;
+
+ // Clean the device discovery agent
+ if (remoteDeviceDiscoveryAgent->isActive())
+ remoteDeviceDiscoveryAgent->stop();
+
+ delete remoteDeviceDiscoveryAgent;
}
void BtDeviceSelectionDialog::on_changeDeviceState_clicked()
--
2.1.4
From 546184eb3d8e246ed4dd34b31d43bd37b2312e44 Mon Sep 17 00:00:00 2001
From: Claudiu Olteanu
Date: Sun, 16 Aug 2015 15:52:31 +0300
Subject: [PATCH 02/19] Check the last error when the BTH device scanning is
finished
If there is no error reported when the device scanning is finished
then report to the dialog status that the scanning finished
successfully. Otherwise report the last error.
Signed-off-by: Claudiu Olteanu
---
qt-ui/btdeviceselectiondialog.cpp | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/qt-ui/btdeviceselectiondialog.cpp b/qt-ui/btdeviceselectiondialog.cpp
index ce759cc..3af2501 100644
--- a/qt-ui/btdeviceselectiondialog.cpp
+++ b/qt-ui/btdeviceselectiondialog.cpp
@@ -124,7 +124,12 @@ void BtDeviceSelectionDialog::on_scan_clicked()
void BtDeviceSelectionDialog::remoteDeviceScanFinished()
{
- ui->dialogStatus->setText("Scanning finished.");
+ if (remoteDeviceDiscoveryAgent->error() == QBluetoothDeviceDiscoveryAgent::NoError) {
+ ui->dialogStatus->setText("Scanning finished successfully.");
+ } else {
+ deviceDiscoveryError(remoteDeviceDiscoveryAgent->error());
+ }
+
ui->scan->setEnabled(true);
}
--
2.1.4
From 70ad7b8a83d3f594ec0dc5ba089aa4944e5a8a1d Mon Sep 17 00:00:00 2001
From: Claudiu Olteanu
Date: Sun, 16 Aug 2015 15:56:31 +0300
Subject: [PATCH 03/19] Clear the BTH discovered devices list on each search
Clear the Bluetooth discovered devices list on each search.
In this way we will show only the devices that are in range
and active during the last scannning. Also if we clear the
list before each call we don't need to check anymore if the
discovered device is already in the list.
Signed-off-by: Claudiu Olteanu
---
qt-ui/btdeviceselectiondialog.cpp | 38 +++---
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/qt-ui/btdeviceselectiondialog.cpp b/qt-ui/btdeviceselectiondialog.cpp
index 3af2501..cbc247d 100644
--- a/qt-ui/btdeviceselectiondialog.cpp
+++ b/qt-ui/btdeviceselectiondialog.cpp
@@ -118,6 +118,7 @@ void BtDeviceSelectionDialog::on_clear_clicked()
void BtDeviceSelectionDialog::on_scan_clicked()
{
ui->dialogStatus->setText("Scanning for remote devices...");
+ ui->discoveredDevicesList->clear();
remoteDeviceDiscoveryAgent->start();
ui->scan->setEnabled(false);
}
@@ -145,28 +146,27 @@ void BtDeviceSelectionDialog::hostModeStateChanged(QBluetoothLocalDevice::HostMo
void BtDeviceSelectionDialog::addRemoteDevice(const QBluetoothDeviceInfo &remoteDeviceInfo)
{
- QString deviceLabel = QString("%1 (%2)").arg(remoteDeviceInfo.name()).arg(remoteDeviceInfo.address().toString());
- QList itemsWithSameSignature = ui->discoveredDevicesList->findItems(deviceLabel, Qt::MatchStartsWith);
+ QColor pairingColor = QColor(Qt::red);
+ QString pairingStatusLabel = QString("UNPAIRED");
+ QBluetoothLocalDevice::Pairing pairingStatus = localDevice->pairingStatus(remoteDeviceInfo.address());
- // Check if the remote device is already in the list
- if (itemsWithSameSignature.empty()) {
- QListWidgetItem *item = new QListWidgetItem(deviceLabel);
- QBluetoothLocalDevice::Pairing pairingStatus = localDevice->pairingStatus(remoteDeviceInfo.address());
- item->setData(Qt::UserRole, QVariant::fromValue(remoteDeviceInfo));
+ if (pairingStatus == QBluetoothLocalDevice::Paired) {
+ pairingStatusLabel = QString("PAIRED");
+ pairingColor = QColor(Qt::gray);
+ } else if (pairingStatus == QBluetoothLocalDevice::AuthorizedPaired) {
+ pairingStatusLabel = QString("AUTHORIZED_PAIRED");
+ pairingColor = QColor(Qt::blue);
+ }
- if (pairingS
Re: [PATH] Add native Bluetooth support for Windows platforms
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 itemsWithSameSignature =
> ui->discoveredDevicesList->findItems(deviceLabel, Qt::MatchStartsWith);
> + QList 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(deviceAddress.data()),
...
deviceAddress.truncate(addressSize);
> Subject: [PATCH 12/17] Initialize WinSock and hide t
Re: [PATH] Add native Bluetooth support for Windows platforms
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 itemsWithSameSignature =
> > ui->discoveredDevicesList->findItems(deviceLabel, Qt::MatchStartsWith);
> > + QList 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(deviceAddress.data()),
> ...
> deviceAddress.truncate(addressSize);
>
Thank you for your detailed explanation! I didn't know about the difference
between the W functions on Windows inst
Re: [PATH] Add native Bluetooth support for Windows platforms
On Tue, Aug 18, 2015 at 12:31:53PM +0300, Claudiu Olteanu wrote: > > Thanks for making time to look over the patches! Yes, let me repeat that. Excellent review, Thiago. Thank you. [...] > Since I will create a new set of patches I will try to add all the things > you suggested. Thank you. I look forward to that updated patch set. BTW for things like this I'm also happy to accept pull requests (as I have done for Grace, Gehad and Jan). /D ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATH] Add native Bluetooth support for Windows platforms
On Tuesday 18 August 2015 12:31:53 Claudiu Olteanu wrote: > 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. Hi Claudiu Please do this only if you can do it quickly. Your code is correct, just a little inefficient. If it isn't straightforward, please keep it as-is and provide a patch later to fix it. -- 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 ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
