Hi Dirk,
On Dienstag, 31. Oktober 2017 13:52:45 CET you wrote:
> > On Oct 31, 2017, at 4:43 AM, Jan Mulder <[email protected]> wrote:
> >
> > On 31-10-17 12:05, Jan Mulder wrote:
> > Sounds like a fundamental problem, with the new way of error reporting ...
> > but not sure ...
> Yes, that makes sense. Doing the showNotification there might fail. Instead
> we should just let the UI thread know that there is a new message to
> display. This may prevent 4.7.2 today as I won't have time to look at this
> until late this evening and then we might want to test it :-)
>
> Unless one of you can look at it while I'm on an airplane for the next two
> hours
After a quick look it seems that report_error(...) is used quite liberally in
core/qt-ble.cpp, core/downloadfromthread.cpp and core/libdivecomputer.c,
which are all executed in DownloadThread context. Detangling this seems hard
as part of it is in C, other parts are callbacks from libdivecomputer, etc.
As a quick stop-gap measure I replaced the report_error(...) calls by
report_error_thread(...) calls, which collect the error messages in a thread
local storage. The collected error messages are then passed to the main thread
via a member.
As a FYI, the patch attached, though without commit-message because it is
simply too ugly. It works for my particular test case, but...
Berthold
diff --git a/core/downloadfromdcthread.cpp b/core/downloadfromdcthread.cpp
index e01de288..3ce4d7ab 100644
--- a/core/downloadfromdcthread.cpp
+++ b/core/downloadfromdcthread.cpp
@@ -1,8 +1,10 @@
#include "downloadfromdcthread.h"
#include "core/libdivecomputer.h"
+#include "core/membuffer.h"
#include "core/subsurface-qt/SettingsObjectWrapper.h"
#include <QDebug>
#include <QRegularExpression>
+#include <QThreadStorage>
QStringList vendorList;
QHash<QString, QStringList> productList;
@@ -10,6 +12,24 @@ static QHash<QString, QStringList> mobileProductList; // BT, BLE or FTDI support
QMap<QString, dc_descriptor_t *> descriptorLookup;
ConnectionListModel connectionListModel;
+QThreadStorage<QStringList> errorList;
+
+#define VA_BUF(b, fmt) do { va_list args; va_start(args, fmt); put_vformat(b, fmt, args); va_end(args); } while (0)
+
+extern "C"
+int report_error_thread(const char *fmt, ...)
+{
+ struct membuffer buf = { 0 };
+
+ /* Previous unprinted errors? Add a newline in between */
+ VA_BUF(&buf, fmt);
+ mb_cstring(&buf);
+
+ if(buf.len)
+ errorList.localData().append(QString(buf.buffer));
+ return -1;
+}
+
static QString str_error(const char *fmt, ...)
{
va_list args;
@@ -27,6 +47,7 @@ DownloadThread::DownloadThread()
void DownloadThread::run()
{
+ errorList.setLocalData(QStringList());
auto internalData = m_data->internalData();
internalData->descriptor = descriptorLookup[m_data->vendor() + m_data->product()];
internalData->download_table = &downloadTable;
@@ -51,6 +72,8 @@ void DownloadThread::run()
if (errorText)
error = str_error(errorText, internalData->devname, internalData->vendor, internalData->product);
+ errors = std::move(errorList.localData());
+
qDebug() << "Finishing the thread" << errorText << "dives downloaded" << downloadTable.nr;
auto dcs = SettingsObjectWrapper::instance()->dive_computer_settings;
dcs->setVendor(internalData->vendor);
diff --git a/core/downloadfromdcthread.h b/core/downloadfromdcthread.h
index ff4b8f39..2dd134ff 100644
--- a/core/downloadfromdcthread.h
+++ b/core/downloadfromdcthread.h
@@ -79,6 +79,7 @@ public:
Q_INVOKABLE DCDeviceData *data();
QString error;
+ QStringList errors;
private:
DCDeviceData *m_data;
@@ -101,4 +102,10 @@ extern QStringList vendorList;
extern QHash<QString, QStringList> productList;
extern QMap<QString, dc_descriptor_t *> descriptorLookup;
extern ConnectionListModel connectionListModel;
+
+// Dirty hack: report_error(...) crashed when not called from main thread.
+// Thus, replace report_error(...) calls by report_error_thread(...)
+// calls, which collect error messages in thread local storage.
+extern "C" int report_error_thread(const char *fmt, ...);
+
#endif
diff --git a/core/libdivecomputer.c b/core/libdivecomputer.c
index d8fe54e8..86456751 100644
--- a/core/libdivecomputer.c
+++ b/core/libdivecomputer.c
@@ -18,6 +18,8 @@
#include "libdivecomputer.h"
#include "core/version.h"
+int report_error_thread(const char *fmt, ...);
+
#if !defined(SSRF_LIBDC_VERSION) || SSRF_LIBDC_VERSION < 2
#pragma message "Subsurface requires a reasonably current version of the Subsurface-branch"
#pragma message "of libdivecomputer (at least version 2 of our API)."
@@ -106,10 +108,10 @@ static int parse_gasmixes(device_data_t *devdata, struct dive *dive, dc_parser_t
if (rc == DC_STATUS_SUCCESS) {
if (ntanks && ntanks < ngases) {
shown_warning = true;
- report_error("Warning: different number of gases (%d) and cylinders (%d)", ngases, ntanks);
+ report_error_thread("Warning: different number of gases (%d) and cylinders (%d)", ngases, ntanks);
} else if (ntanks > ngases) {
shown_warning = true;
- report_error("Warning: smaller number of gases (%d) than cylinders (%d). Assuming air.", ngases, ntanks);
+ report_error_thread("Warning: smaller number of gases (%d) than cylinders (%d). Assuming air.", ngases, ntanks);
}
}
#endif
@@ -134,14 +136,14 @@ static int parse_gasmixes(device_data_t *devdata, struct dive *dive, dc_parser_t
if (o2 + he <= O2_IN_AIR || o2 > 1000) {
if (!shown_warning) {
shown_warning = true;
- report_error("unlikely dive gas data from libdivecomputer: o2 = %d he = %d", o2, he);
+ report_error_thread("unlikely dive gas data from libdivecomputer: o2 = %d he = %d", o2, he);
}
o2 = 0;
}
if (he < 0 || o2 + he > 1000) {
if (!shown_warning) {
shown_warning = true;
- report_error("unlikely dive gas data from libdivecomputer: o2 = %d he = %d", o2, he);
+ report_error_thread("unlikely dive gas data from libdivecomputer: o2 = %d he = %d", o2, he);
}
he = 0;
}
@@ -206,7 +208,7 @@ static int parse_gasmixes(device_data_t *devdata, struct dive *dive, dc_parser_t
}
if (tank.gasmix != i) { // we don't handle this, yet
shown_warning = true;
- report_error("gasmix %d for tank %d doesn't match", tank.gasmix, i);
+ report_error_thread("gasmix %d for tank %d doesn't match", tank.gasmix, i);
}
}
if (!IS_FP_SAME(tank.volume, 0.0))
@@ -392,7 +394,7 @@ sample_cb(dc_sample_type_t type, dc_sample_value_t value, void *userdata)
if (nsensor < 3)
sample->o2sensor[nsensor].mbar = lrint(value.ppo2 * 1000);
else
- report_error("%d is more o2 sensors than we can handle", nsensor);
+ report_error_thread("%d is more o2 sensors than we can handle", nsensor);
nsensor++;
// Set the amount of detected o2 sensors
if (nsensor > dc->no_o2sensors)
@@ -1108,7 +1110,7 @@ const char *do_libdivecomputer_import(device_data_t *data)
}
if (rc != DC_STATUS_SUCCESS) {
- report_error(errmsg(rc));
+ report_error_thread(errmsg(rc));
} else {
#else
{
@@ -1163,17 +1165,17 @@ dc_status_t libdc_buffer_parser(struct dive *dive, device_data_t *data, unsigned
rc = dc_parser_new2(&parser, data->context, data->descriptor, 0, 0);
break;
default:
- report_error("Device type not handled!");
+ report_error_thread("Device type not handled!");
return DC_STATUS_UNSUPPORTED;
}
if (rc != DC_STATUS_SUCCESS) {
- report_error("Error creating parser.");
+ report_error_thread("Error creating parser.");
dc_parser_destroy (parser);
return rc;
}
rc = dc_parser_set_data(parser, buffer, size);
if (rc != DC_STATUS_SUCCESS) {
- report_error("Error registering the data.");
+ report_error_thread("Error registering the data.");
dc_parser_destroy (parser);
return rc;
}
@@ -1182,12 +1184,12 @@ dc_status_t libdc_buffer_parser(struct dive *dive, device_data_t *data, unsigned
if (dc_descriptor_get_type(data->descriptor) != DC_FAMILY_UWATEC_ALADIN && dc_descriptor_get_type(data->descriptor) != DC_FAMILY_UWATEC_MEMOMOUSE) {
rc = libdc_header_parser (parser, data, dive);
if (rc != DC_STATUS_SUCCESS) {
- report_error("Error parsing the dive header data. Dive # %d\nStatus = %s", dive->number, errmsg(rc));
+ report_error_thread("Error parsing the dive header data. Dive # %d\nStatus = %s", dive->number, errmsg(rc));
}
}
rc = dc_parser_samples_foreach (parser, sample_cb, &dive->dc);
if (rc != DC_STATUS_SUCCESS) {
- report_error("Error parsing the sample data. Dive # %d\nStatus = %s", dive->number, errmsg(rc));
+ report_error_thread("Error parsing the sample data. Dive # %d\nStatus = %s", dive->number, errmsg(rc));
dc_parser_destroy (parser);
return rc;
}
diff --git a/core/qt-ble.cpp b/core/qt-ble.cpp
index a72736d4..277e44c6 100644
--- a/core/qt-ble.cpp
+++ b/core/qt-ble.cpp
@@ -18,6 +18,9 @@
#include "core/qt-ble.h"
#include "core/btdiscovery.h"
+// Hack! for report_error_thread()
+#include "core/downloadfromdcthread.h"
+
#if defined(SSRF_CUSTOM_IO)
#include <libdivecomputer/custom_io.h>
@@ -323,7 +326,7 @@ dc_status_t qt_ble_open(dc_custom_io_t *io, dc_context_t *context, const char *d
break;
default:
qDebug() << "failed to connect to the controller " << devaddr << "with error" << controller->errorString();
- report_error("Failed to connect to %s: '%s'", devaddr, controller->errorString().toUtf8().data());
+ report_error_thread("Failed to connect to %s: '%s'", devaddr, controller->errorString().toUtf8().data());
controller->disconnectFromDevice();
delete controller;
return DC_STATUS_IO;
@@ -346,7 +349,7 @@ dc_status_t qt_ble_open(dc_custom_io_t *io, dc_context_t *context, const char *d
qDebug() << " .. done discovering services";
if (ble->preferredService() == nullptr) {
qDebug() << "failed to find suitable service on" << devaddr;
- report_error("Failed to find suitable service on '%s'", devaddr);
+ report_error_thread("Failed to find suitable service on '%s'", devaddr);
controller->disconnectFromDevice();
delete controller;
return DC_STATUS_IO;
@@ -361,7 +364,7 @@ dc_status_t qt_ble_open(dc_custom_io_t *io, dc_context_t *context, const char *d
if (ble->preferredService()->state() != QLowEnergyService::ServiceDiscovered) {
qDebug() << "failed to find suitable service on" << devaddr;
- report_error("Failed to find suitable service on '%s'", devaddr);
+ report_error_thread("Failed to find suitable service on '%s'", devaddr);
controller->disconnectFromDevice();
delete controller;
return DC_STATUS_IO;
diff --git a/desktop-widgets/downloadfromdivecomputer.cpp b/desktop-widgets/downloadfromdivecomputer.cpp
index 637f6034..57f74f08 100644
--- a/desktop-widgets/downloadfromdivecomputer.cpp
+++ b/desktop-widgets/downloadfromdivecomputer.cpp
@@ -220,6 +220,9 @@ void DownloadFromDCWidget::updateState(states state)
// got an error
else if (state == ERROR) {
timer->stop();
+ foreach(const QString &s, thread.errors) {
+ report_message(s.toUtf8().data());
+ }
QMessageBox::critical(this, TITLE_OR_TEXT(tr("Error"), thread.error), QMessageBox::Ok);
markChildrenAsEnabled();
progress_bar_text = "";
_______________________________________________
subsurface mailing list
[email protected]
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
