no dives are shown in the Dive List

Jef Driesen jefdriesen at telenet.be
Mon Jan 27 11:26:35 UTC 2014


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


More information about the subsurface mailing list