Next round of api improvements.

[Jef Driesen]

On 2012-11-15 17:56, Mikko Rasa wrote:
> On 15.11.2012 13:16, Jef Driesen wrote:
>> Note that this new dc_dive_t interface will also simplify the
>> transition from a callback based interface to an iterator based
>> interface, because the dc_iterator_next() function can now return 
>> the
>> dc_dive_t object:
>>
>> dc_dive_t *dive;
>> dc_iterator_t *iterator;
>> dc_device_iterator (device,&iterator);
>> while (dc_iterator_next (iterator,&dive) == DC_STATUS_SUCCESS) {
>>      /* Contents of the callback function goes here. */
>> }
>> dc_iterator_free(iterator);
>
> dc_device_iterator sounds like it would iterate devices.  Perhaps it
> could be called dc_device_iterate_dives, or dc_device_dive_iterator, 
> or
> some variant thereof?

It's just a tentative name and can be changed.

>> 1. Object ownership, lifetime and memory management.
>> =====================================================
>>
>> How should we deal with the ownership of the new objects?
>>
>> With the current api, the application has to call dc_parser_new()
>> function explicitly, and their is no doubt that the application is 
>> the
>> owner of the returned object. However, because the new dive and 
>> sample
>> objects will be allocated internally, their ownership is not 
>> immediately
>> obvious. It all depends on the internal implementation. There are a
>> couple of options, each with their own advantages and disadvantages:
>
> What about utilizing the object hierarchy?  Make the device object 
> own
> the dives, and each dive object own its samples.  This gives the
> application control over their lifetime, but doesn't require freeing
> everything separately.
>
> The downside is a larger memory footprint while parsing, since all 
> dives
> would be in memory simultaneously.  I don't think it's an issue, but 
> if
> it is, you could provide a dc_device_discard_dives function to get 
> rid
> of unwanted dives.
>
> On the other hand, this would save some work when iterating over the
> same dives a second time.

This is a bad idea. Some devices consume significantly more power when 
connected to the host system, and therefore the connection should be 
closed as soon as possible. But if the device object owns the dives, the 
connection can't be closed until the application is done with the dives.

>> 2. Querying available sample values?
>> =====================================
>>
>> I think the simplest solution is to use the accessor function itself 
>> to
>> probe for the presence of the info. You just call the accessor 
>> function
>> and if the info isn't available, a special error code like 
>> UNSUPPORTED
>> is returned:
>>
>> if (dc_sample_get_depth (sample,&depth) == DC_STATUS_SUCCESS) {
>>      /* Depth retrieved successfully. */
>> } else {
>>      /* No depth available (or an error occurred). */
>> }
>
> Are there any legitimate error conditions other than that piece of 
> data
> not being present in the sample?  I try to avoid out parameters due 
> to
> the greater amount of scaffolding required in calling code when 
> compared
> to using the return value, but then again I mostly write C++ which
> provides out-of-band error handling with exceptions.

Real errors are rare, but they can happen. For the majority of the 
devices our knowledge is based on reverse engineering, so there is 
always a possibility that their are errors in our knowledge. Thus the 
most common parsing error is that we encounter some data that doesn't 
match our expectations (e.g. DC_STATUS_DATAFORMAT).

In theory we could catch some of those errors when constructing the 
dive/sample object, but then the error is pretty much fatal. I mean if 
the dc_xxx_foreach (or dc_iterator_next) function fails, there is no way 
to proceed anymore with the downloading or parsing. But in many cases 
it's still possible to move to the next dive or sample.

Take for example the Suunto data format. The dives are stored in a 
linked list, so you need zero knowledge of the actual dive format to 
locate and download the dives. Even if some of the dives will fail to 
parse correctly (for example when the data format has been changed to 
support a new feature), that has no impact on the ability to download 
the dives. If this data format error is detected by the parser while 
constructing the dive object, we have two options:

1. Fail to construct the dive object. This leaves the foreach() 
function no other option than to abort the downloading. The result is 
that a minor problem with just one dive will cause the entire download 
to fail.

2. Construct the dive object anyway in an error state. The application 
will still get the dive, and downloading will continue. It may not be 
able to parse this dive completely, but depending on the severity of the 
error you may still be able to get some info. For example the raw dive 
data will always be available. Some of the fields may still be 
parse-able too. But this only works if the individual accessor functions 
can return an error.

Note that in the current design this is less of a problem, because 
downloading the dives and creating a parser are completely decoupled. A 
failure in creating a parser will never affect the downloading.

>> But then the caller somehow has to know the maximum value for n, and
>> the api also has to communicate back the actual number of values
>> returned. The second part is easy with an extra output parameter, 
>> but I
>> don't know a nice solution for the first problem (other than a 
>> second
>> dc_sample_get_pressure_count function).
>
> A typical solution is to allow passing a null pointer for the array, 
> in
> which case no data is written to it but the count is still returned.
>
>> Alternatives for the probing approach outlined above, are an 
>> iterator
>> like interface to iterator over the available sample values, or a
>> function to retrieve a bitmap with the available sample values.
>
> I went with the bitmap approach in a similar scenario.  It works well
> enough and produces more compact code.
>
>> 3. Supported dive and sample data
>> ==================================
>>
>>    * airtime
>
> Does some computer actually report this?  If tank pressure is also
> reported, airtime can be calculated from that.  In my experience it's
> not too stable over the course of the dive, so it would be of little
> value, but perhaps others disagree.

There are several devices which support it. Uwatec, Oceanic, and 
probably a few other too. It may not be the most critical piece of 
information, but I do get requests for it. So while it's not on my 
must-have list, it's something we can consider adding.

You would be surprised what all those dive computers support. We 
definitely won't support everything, but we want to make a good 
trade-off between what we consider important and what is available.

>> For the events, I'm not sure yet how the querying (see previous 
>> item)
>> should be done. The normal samples are continuous values, and will
>> usually appear at least every X sample, or not at all. But the 
>> events
>> are discrete in nature and will only appear occasionally. Should 
>> this be
>> done differently in the api?
>
> I'd add a separate API for iterating events.  They're different 
> enough
> from normal samples that trying to force them into one API is only 
> going
> to cause headache down the road.

Do you have a suggestion for the api?

>> We'll also have to agree on a data structure and the exact semantics
>> for each of those pieces of data. Some won't need much discussion, 
>> but
>> others will. For example take the temperature. A device can record a
>> single value which could be the minimum, maximum or average 
>> temperature.
>> It can have all three, or a full temperature profile. How do we deal
>> with that? Things are not always as easy as they may look.
>
> Minimum, maximum and average values belong in the dive, not in the
> samples.  I dunno what to do if it's not known which of those a
> particular computer provides, though.

I was referring to the dive temperature here, not the sample 
temperature. It makes indeed no sense to have min/max/avg for the 
samples.

Jef