From b7057c414fc43bb81fc0d01bc07f32d18bce8ab0 Mon Sep 17 00:00:00 2001 From: Jan Mulder Date: Wed, 5 Jul 2017 18:37:21 +0200 Subject: OSTC over BLE: take care of credits Handle credits. Do not just ask for maximum credits all the time as this will stop the download. Also do not let the credits go back to 0 (while this might work, this is not tested). Getting back the 0 credits stops the download, and even when it can be restarted, it is less efficient (and not needed). Notice also that it takes some time before a grant request is honoured. During testing I saw reception of up to 25 packets between request and grant. So a lower bound for the request of 32 packets seems resonable. One aspect the Telit/Stollmann TIO puzzeled me. Sections 4.1 and 4.2 both talk about credits, but my hyphothesis is that there are two credits counters in play. One for traffic either way. This commit only deals with credits granted by Subsurface to the OSTC to send data. Credits granted by the OSTC to allow Subsurface to send new commands is NOT part of this commit, and is seemingly not needed in our scenario. As we only send new commands to the OSTC when a previous one is finished (per HW's interface spec), the OSTC does not run out of credits to receive commands. Signed-off-by: Jan Mulder --- core/qt-ble.cpp | 66 +++++++++++++++++++++++++++++++++++---------------------- core/qt-ble.h | 2 ++ 2 files changed, 43 insertions(+), 25 deletions(-) diff --git a/core/qt-ble.cpp b/core/qt-ble.cpp index 0ce52bc02..7afdf484b 100644 --- a/core/qt-ble.cpp +++ b/core/qt-ble.cpp @@ -30,6 +30,9 @@ static int debugCounter; #define IS_EON_STEEL(_d) same_string((_d)->product, "EON Steel") #define IS_G2(_d) same_string((_d)->product, "G2") +#define MAXIMAL_HW_CREDIT 255 +#define MINIMAL_HW_CREDIT 32 + extern "C" { void waitFor(int ms) { @@ -65,21 +68,24 @@ void BLEObject::characteristcStateChanged(const QLowEnergyCharacteristic &c, con { if (IS_HW(device)) { if (c.uuid() == hwAllCharacteristics[HW_OSTC_BLE_DATA_TX]) { + hw_credit--; receivedPackets.append(value); + if (hw_credit == MINIMAL_HW_CREDIT) + setHwCredit(MAXIMAL_HW_CREDIT - hw_credit); } else { qDebug() << "ignore packet from" << c.uuid() << value.toHex(); } } else { receivedPackets.append(value); } - //qDebug() << ".. incoming packet count" << receivedPackets.length(); } void BLEObject::characteristicWritten(const QLowEnergyCharacteristic &c, const QByteArray &value) { if (IS_HW(device)) { if (c.uuid() == hwAllCharacteristics[HW_OSTC_BLE_CREDITS_RX]) { - qDebug() << "HW_OSTC_BLE_CREDITS_RX confirmed" << c.uuid() << value.toHex(); + bool ok; + hw_credit += value.toHex().toInt(&ok, 16); isCharacteristicWritten = true; } } else { @@ -90,8 +96,6 @@ void BLEObject::characteristicWritten(const QLowEnergyCharacteristic &c, const Q void BLEObject::writeCompleted(const QLowEnergyDescriptor &d, const QByteArray &value) { - Q_UNUSED(value) - qDebug() << "BLE write completed on" << d.name() << d.value(); } @@ -188,7 +192,7 @@ dc_status_t BLEObject::read(void *data, size_t size, size_t *actual) int offset = 0; while (!receivedPackets.isEmpty()) { /* - * Yes, to while loops with same condition seems strange. The inner one + * Yes, two while loops with same condition seems strange. The inner one * does the real work, but it prevents the QtEventloop to do its thing. * As the incoming packets arrive based on signals and slots, that * stuff is not handeled during the inner loop. So, add a short waitFor @@ -220,6 +224,36 @@ we_are_done: return DC_STATUS_SUCCESS; } +dc_status_t BLEObject::setHwCredit(unsigned int c) +{ + /* The Terminal I/O client transmits initial UART credits to the server (see 6.5). + * + * Notice that we have to write to the characteristic here, and not to its + * descriptor as for the enabeling of notifications or indications. + * + * Futher notice that this function has the implicit effect of processing the + * event loop (due to waiting for the confirmation of the credit request). + * So, as characteristcStateChanged will be triggered, while receiving + * data from the OSTC, these are processed too. + */ + + QList list = preferredService()->characteristics(); + isCharacteristicWritten = false; + preferredService()->writeCharacteristic(list[HW_OSTC_BLE_CREDITS_RX], + QByteArray(1, c), + QLowEnergyService::WriteWithResponse); + + /* And wait for the answer*/ + int msec = BLE_TIMEOUT; + while (msec > 0 && !isCharacteristicWritten) { + waitFor(100); + msec -= 100; + }; + if (!isCharacteristicWritten) + return DC_STATUS_TIMEOUT; + return DC_STATUS_SUCCESS; +} + dc_status_t BLEObject::setupHwTerminalIo(QList allC) { /* This initalizes the Terminal I/O client as described in * http://www.telit.com/fileadmin/user_upload/products/Downloads/sr-rf/BlueMod/TIO_Implementation_Guide_r04.pdf @@ -252,26 +286,8 @@ dc_status_t BLEObject::setupHwTerminalIo(QList allC) d = allC[HW_OSTC_BLE_DATA_TX].descriptors().first(); preferredService()->writeDescriptor(d, QByteArray::fromHex("0100")); - /* The Terminal I/O client transmits initial UART credits to the server (see 6.5). - * - * Notice that we have to write to the characteristic here, and not to its - * descriptor as for the enabeling of notifications or indications. - */ - isCharacteristicWritten = false; - preferredService()->writeCharacteristic(allC[HW_OSTC_BLE_CREDITS_RX], - QByteArray(1, 255), - QLowEnergyService::WriteWithResponse); - - /* And give to OSTC some time to get initialized */ - int msec = BLE_TIMEOUT; - while (msec > 0 && !isCharacteristicWritten) { - waitFor(100); - msec -= 100; - }; - if (!isCharacteristicWritten) - return DC_STATUS_TIMEOUT; - - return DC_STATUS_SUCCESS; + /* The Terminal I/O client transmits initial UART credits to the server (see 6.5). */ + return setHwCredit(MAXIMAL_HW_CREDIT); } dc_status_t qt_ble_open(dc_custom_io_t *io, dc_context_t *context, const char *devaddr) diff --git a/core/qt-ble.h b/core/qt-ble.h index 7368b71cd..ad5dfda2b 100644 --- a/core/qt-ble.h +++ b/core/qt-ble.h @@ -32,6 +32,7 @@ public slots: void characteristicWritten(const QLowEnergyCharacteristic &c, const QByteArray &value); void writeCompleted(const QLowEnergyDescriptor &d, const QByteArray &value); dc_status_t setupHwTerminalIo(QList); + dc_status_t setHwCredit(unsigned int c); private: QVector services; @@ -39,6 +40,7 @@ private: QList receivedPackets; bool isCharacteristicWritten; dc_user_device_t *device; + unsigned int hw_credit = 0; QList hwAllCharacteristics = { "{00000001-0000-1000-8000-008025000000}", // HW_OSTC_BLE_DATA_RX -- cgit v1.2.3-70-g09d2 From b409e9fc91d87bbd5f88c53cf937cf73a66821f4 Mon Sep 17 00:00:00 2001 From: Jan Mulder Date: Tue, 11 Jul 2017 13:29:41 +0200 Subject: BLE read: remove aggressive read Commit 709c1df2af4b87 introduced a hard blocking read for BLE devices. This did break BLE reads from multiple DCs, and (in hindsight) was not a correct implementation. It would require, for example, dynamic read buffers as especially profile data grows with dive time, and in addition, and more importantly, also the OSTC libdc parser cannot process the entire profile of a dive at once (but likes to receive it in 1K blocks). So, basically, it introduced issues, and did not solve the OSTC read. This commit reverts this hard blocking read (and as such will break OSTC BLE reads). But it enables removal of the special cases for the EON Steel and G2. A next commit will solve OSTC BLE reads. Signed-off-by: Jan Mulder --- core/qt-ble.cpp | 37 +++++++------------------------------ 1 file changed, 7 insertions(+), 30 deletions(-) diff --git a/core/qt-ble.cpp b/core/qt-ble.cpp index 7afdf484b..a8002b49b 100644 --- a/core/qt-ble.cpp +++ b/core/qt-ble.cpp @@ -27,8 +27,6 @@ static int debugCounter; #define IS_HW(_d) same_string((_d)->vendor, "Heinrichs Weikamp") #define IS_SHEARWATER(_d) same_string((_d)->vendor, "Shearwater") -#define IS_EON_STEEL(_d) same_string((_d)->product, "EON Steel") -#define IS_G2(_d) same_string((_d)->product, "G2") #define MAXIMAL_HW_CREDIT 255 #define MINIMAL_HW_CREDIT 32 @@ -189,38 +187,17 @@ dc_status_t BLEObject::read(void *data, size_t size, size_t *actual) if (receivedPackets.isEmpty()) return DC_STATUS_IO; - int offset = 0; - while (!receivedPackets.isEmpty()) { - /* - * Yes, two while loops with same condition seems strange. The inner one - * does the real work, but it prevents the QtEventloop to do its thing. - * As the incoming packets arrive based on signals and slots, that - * stuff is not handeled during the inner loop. So, add a short waitFor - * between the inner and outer while loop. - */ - while (!receivedPackets.isEmpty()) { - QByteArray packet = receivedPackets.takeFirst(); + QByteArray packet = receivedPackets.takeFirst(); - if (IS_SHEARWATER(device)) - packet.remove(0,2); + if (IS_SHEARWATER(device)) + packet.remove(0,2); - //qDebug() << ".. read (packet.length, contents, size)" << packet.size() << packet.toHex() << size; + if (packet.size() > size) + return DC_STATUS_NOMEMORY; - if ((offset + packet.size()) > size) { - qDebug() << "BLE read trouble, receive buffer too small"; - return DC_STATUS_NOMEMORY; - } + memcpy((char *)data, packet.data(), packet.size()); + *actual += packet.size(); - memcpy((char *)data + offset, packet.data(), packet.size()); - offset += packet.size(); - *actual += packet.size(); - // EON Steel wants to read only one packet at a time - if (IS_EON_STEEL(device) || IS_G2(device)) - goto we_are_done; - } - waitFor(50); // and process some Qt events to see if there is more data coming in. - } -we_are_done: return DC_STATUS_SUCCESS; } -- cgit v1.2.3-70-g09d2 From 891128159352621f6694d6a62b0af6a451b32ec7 Mon Sep 17 00:00:00 2001 From: Jan Mulder Date: Tue, 11 Jul 2017 13:41:21 +0200 Subject: OSTC over BLE: read a long as needed See also b409e9fc91d87bbd5 and 709c1df2af4b87. The OSTC parser cannot handle reads of single 20 byte BLE packages in serial mode. Instead of doing a deeper down agressive read, we can read on the serial level more subtile. As the parser is requesting a specific number of bytes, we just read that number of bytes and return them. As the 20 byte BLE packets do (obviously) not align with the reading requirement of the libdc parser, a little housekeeing needs to be done in between individual reads. CAVEAT 1: In contradiction to 709c1df2af4b87, this is supposed to work for all parsers that properly specify the needed bytes to fetch. CAVEAT 2: All above tested on Linux Desktop with bluez stack. Subsurface mobile is step 2. Signed-off-by: Jan Mulder --- core/qtserialbluetooth.cpp | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/core/qtserialbluetooth.cpp b/core/qtserialbluetooth.cpp index 8b5779ce5..3a031e1ae 100644 --- a/core/qtserialbluetooth.cpp +++ b/core/qtserialbluetooth.cpp @@ -126,19 +126,34 @@ static dc_status_t ble_serial_read(dc_custom_io_t *io, void* data, size_t size, { Q_UNUSED(io) size_t len; + size_t received = 0; if (buffer.in_pos >= buffer.in_bytes) { + ble_serial_flush_write(); + } + + /* There is still unused/unread data in the input steam. + * So preseve it at the start of a new read. + */ + if (buffer.in_pos > 0) { + len = buffer.in_bytes - buffer.in_pos; + memcpy(buffer.in, buffer.in + buffer.in_pos, len); + buffer.in_pos = 0; + buffer.in_bytes = len; + } + + /* Read a long as requested in the size parameter */ + while ((buffer.in_bytes - buffer.in_pos) < size) { dc_status_t rc; - size_t received; - ble_serial_flush_write(); - rc = ble_serial_ops.packet_read(&ble_serial_ops, buffer.in, sizeof(buffer.in), &received); + rc = ble_serial_ops.packet_read(&ble_serial_ops, buffer.in + buffer.in_bytes, + sizeof(buffer.in) - buffer.in_bytes, &received); if (rc != DC_STATUS_SUCCESS) return rc; if (!received) return DC_STATUS_IO; - buffer.in_pos = 0; - buffer.in_bytes = received; + + buffer.in_bytes += received; } len = buffer.in_bytes - buffer.in_pos; -- cgit v1.2.3-70-g09d2 From fbaaa64a4a3cf3266b8afe62af578105ec32a374 Mon Sep 17 00:00:00 2001 From: Jan Mulder Date: Tue, 11 Jul 2017 17:11:49 +0200 Subject: Trivial code cleanup Signed-off-by: Jan Mulder --- core/qt-ble.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/qt-ble.cpp b/core/qt-ble.cpp index a8002b49b..967827db1 100644 --- a/core/qt-ble.cpp +++ b/core/qt-ble.cpp @@ -69,7 +69,7 @@ void BLEObject::characteristcStateChanged(const QLowEnergyCharacteristic &c, con hw_credit--; receivedPackets.append(value); if (hw_credit == MINIMAL_HW_CREDIT) - setHwCredit(MAXIMAL_HW_CREDIT - hw_credit); + setHwCredit(MAXIMAL_HW_CREDIT - MINIMAL_HW_CREDIT); } else { qDebug() << "ignore packet from" << c.uuid() << value.toHex(); } @@ -94,7 +94,9 @@ void BLEObject::characteristicWritten(const QLowEnergyCharacteristic &c, const Q void BLEObject::writeCompleted(const QLowEnergyDescriptor &d, const QByteArray &value) { - qDebug() << "BLE write completed on" << d.name() << d.value(); + Q_UNUSED(value) + Q_UNUSED(d) + qDebug() << "BLE write completed"; } void BLEObject::addService(const QBluetoothUuid &newService) -- cgit v1.2.3-70-g09d2