Hi Claudiu,
Thanks for sending the patches...
On Fri, Jun 26, 2015 at 05:00:48AM +0300, Claudiu Olteanu wrote:
> I attached some patches for the QtBluetooth serial implementation.
>
> The UI exposes the following functionalities:
> - read information about the local device and its state
> - scan for remote devices
Just to make sure we get the terminology straight... the "local device" is
your computer or tablet or whatever Subsurface is running on, the "remote
device" is the BT dive computer, correct?
> - pair/unpair with a remote device (it doesn't work for custom PIN codes)
> - turn on/off the local device
I assume that means turn on/off BT support on your computer?
> - logging
> - data transfer (dive logs import)
>
> For the moment the *qt_serial_bluetooth* implementation works
> only with *hw_ostc3* and *hw_ostc* (hopefully :-) ).
OK, I can test those two.
> After you agree with
> the modifications and the design of libdivecomputer project I will
> start to add support for other vendors.
>
> Known issues:
> - during the installation of libdivecomputer, the *custom_serial* header
> is not copied
> - the pair command doesn't work with devices which require a specific PIN
> code
> - the design of the UI is ugly :-)
Those seem all acceptable issues at this stage of the project.
> Subject: [PATCH 1/9] Add checkbox and button for Bluetooth download mode
>
> The checkbox will be used to enable the Bluetooth
> downloading mode. The button will be used to create
> a dialog selection where the user will be able to
> scan and select remote devices.
Good commit message. Concise, yet describes what the patch does.
> Subject: [PATCH 2/9] Implement temporaty UI design for Bluetooth selection
> dialog
>
> Implement a temporary UI design for Bluetooth selection dialog.
> Functionalities of the widget:
> - expose information about the local device
> - scan for remote devices
> - pair/unpair with a remote device
> - turn on/off the local device
> - logging
> - save the selected device
>
> --- /dev/null
> +++ b/qt-ui/btdeviceselectiondialog.cpp
> @@ -0,0 +1,257 @@
> +#include <QShortcut>
> +#include <QDebug>
> +#include <QMessageBox>
> +#include <QMenu>
> +
> +#include "ui_btdeviceselectiondialog.h"
> +#include "btdeviceselectiondialog.h"
> +
> +BtDeviceSelectionDialog::BtDeviceSelectionDialog(QWidget *parent) :
> + QDialog(parent),
> + localDevice(new QBluetoothLocalDevice),
> + ui(new Ui::BtDeviceSelectionDialog)
> +{
> + /* Check if Bluetooth is available on this device */
> + if (!localDevice->isValid()) {
> + QMessageBox::warning(this, tr("Warning"),
> + "Your local Bluetooth device cannot be
> accessed. Please check if you have installed qtconnectivity library.");
> + return;
This message seems off as a user facing message. The QtConnectivity
libraries would have come with the install - either as dependency or Linux
or bundled with the executable on Windows / Mac. What other reasons could
there be for this test failing? Bluetooth turned off maybe?
Think of this as something you tell the user so she can fix it...
> + /* Set UI information about the local device */
> + ui->deviceAddress->setText(localDevice->address().toString());
> + ui->deviceName->setText(localDevice->name());
Many BT UIs do that... I always wonder if it wouldn't be better NOT to
show the hex address if you have a name...
I'll ask our resident Qt experts to comment on the Qt code :-)
> Subject: [PATCH 5/9] Add a skelet for Bluetooth serial communication
skeleton :-)
> Subject: [PATCH 6/9] Implement the Qt Bluetooth serial communication
>
> diff --git a/qtserialbluetooth.cpp b/qtserialbluetooth.cpp
> static int qt_serial_close(serial_t *device)
> {
> - return 0;
> + if (device == NULL || device->socket == NULL)
> + return DC_STATUS_SUCCESS;
> +
> + device->socket->close();
> +
> + delete device->socket;
> + free(device);
> +
> + return DC_STATUS_SUCCESS;
> }
>
> static int qt_serial_read(serial_t *device, void* data, unsigned int size)
> {
> - return 0;
> + if (device == NULL || device->socket == NULL)
> + return DC_STATUS_INVALIDARGS;
So the close function above frees the device when it closes it.
Is the caller required to set it to NULL? Because if someone
passes a device to close() (and the memory gets freed) and then
to read() (admittedly a bug) then you dereference freed memory
here.
I guess that's just how the API works?
> static int qt_serial_get_transmitted(serial_t *device)
> {
> - return 0;
> + if (device == NULL || device->socket == NULL)
> + return DC_STATUS_INVALIDARGS;
> +
> + return device->socket->bytesToWrite();
Not having looked at the API in enough detail... but to me
"get_transmitted()" sounds like how much has been transmitted,
whereas "bytesToWrite()" sounds like a count of data in a queue...
> Subject: [PATCH 7/9] Make the qt_serial_open method callable from C
>
> Signed-off-by: Claudiu Olteanu <[email protected]>
> ---
> qtserialbluetooth.cpp | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/qtserialbluetooth.cpp b/qtserialbluetooth.cpp
> index 5842c2a..5a2b3be 100644
> --- a/qtserialbluetooth.cpp
> +++ b/qtserialbluetooth.cpp
> @@ -6,6 +6,7 @@
>
> #include <libdivecomputer/custom_serial.h>
>
> +extern "C" {
> typedef struct serial_t {
> /* Library context. */
> dc_context_t *context;
> @@ -211,3 +212,4 @@ dc_status_t dc_serial_qt_open(dc_serial_t **out,
> dc_context_t *context, const ch
>
> return DC_STATUS_SUCCESS;
> }
> +}
I guess this commit could have reasonably squashed into an earlier one,
but it's fine.
I haven't tested this, yet, but reading through the code I'm quite happy
with what I see.
/D
_______________________________________________
subsurface mailing list
[email protected]
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
