On 27-01-14 19:10, Dirk Hohndel wrote:
On Mon, 2014-01-27 at 10:53 +0100, Jef Driesen wrote:
I think the real problem is something else. Jun Song's dives fail to
parse. Not sure why yet. But that's just one part of the problem. When I
try download the memory dump into subsurface, I can confirm no dives are
imported. No error message is shown. So it seems subsurface is silently
dropping dives that are failing to parse. That's the second part of the
problem.
I quickly checked the subsurface code. The dive_cb function exits
immediately in case of a parsing error, and as a result the dive never
gets added anywhere.
Which seems reasonable. If we can't parse it, we clearly can't add it.
Depends. If there is one small parsing error that does not necessary mean you
didn't get any useful info? It's not always black and white. I also realize this
is tricky.
This is one of the reasons why I recommend keeping the raw data around. Because
then you can re-parse the dive again when the bug is fixed, without having to
download again. If it's a nasty bug that takes a while to fix (or you dive very
often) then by the time the bug is fixed, those dives may already have been
pushed out of the dive computer's memory. If you still have the raw data, that's
not a problem.
But the error is also never reported back to the
user. There is a call to the dev_info function to show an error message
in the progress bar, but I assume that because the download dialog is
closed almost immediately, the user doesn't have a chance to notice this
error message.
I'll fix that.
I think this will already be a great improvement. Silent data loss is nasty and
confusing.
The libdivecomputer error code is used as the exit code for the dive_cb
function. But this is not used to communicate errors back to the caller.
The return value of the callback function is used to tell
libdivecomputer whether it should proceed with the next dive or not. It
should be either 0 (abort) or 1 (continue). Note that the (negative)
dc_status_t error codes happens to work too, because all non-zero values
evaluate to "true" in the boolean expression that is used internally.
Fun. So a long time ago Linus misunderstood the libdivecomputer API
(clearly documented in http://libdivecomputer.org/manual.html) and ever
since we didn't deal with errors correctly. We faithfully returned the
error codes, clearly assuming that they would be passed on to the
device_foreach function. But reading the source that clearly isn't the
case. And that's why we never respond to the error - it never gets
passed up the stack!
If I read the sources correctly, then Subsurface needs to pass the error
code to the calling function some other way as an error in the parser
that we find in the dive_cb cannot be passed back up to the actual
caller (do_device_import() in libdivecomputer.c) as dc_device_foreach()
will report DC_STATUS_SUCCESS even if the parsing of a dive failed and
caused us to abort the operation.
Am I reading this correctly?
Yes you are reading this correct. Sorry for the lack of documentation. Another
long time item on my todo list.
The return code from the dc_device_foreach function indicates whether the
download succeeded or not. So it makes no sense to abuse it for return an error
from application code. You need to pass the error back in some other way. This
is one of the drawbacks of the callback interface. An iterator based interface
is also on my todo list :-)
Jef
_______________________________________________
subsurface mailing list
[email protected]
http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
