Roadmap

[Jef Driesen]

Hi,

I have a couple of additions to the previous email. As usual nothing is set in 
stone, and all feedback is welcome!

14. Enumeration of supported models

Currently there is no way to retrieve the list of supported models, and as a 
result every single developer has to hard-code such a list into their 
application. It would be much better if that list could be provided by the 
libdivecomputer library directly. An additional benefit is that you'll always 
have access to an up-to-date list.

For the actual api, I'm thinking in the direction of a function that returns a 
table, containing a mapping between models and backends:

typedef dc_model_t {
       const char *manufacturer;
       const char *name;
       device_type_t type;
       unsigned int model;
} dc_model_t;

const dc_model_t *
dc_get_supported_models (void)
{
       static const dc_model_t g_models[] = {
          {"Suunto", "Vyper", DEVICE_TYPE_SUUNTO_VYPER, 0},
          {"Suunto", "Cobra", DEVICE_TYPE_SUUNTO_VYPER, 0},
          ...
          {NULL, NULL, DEVICE_TYPE_NULL}
       };

       return models;
}

Or some variant with output parameters, rather than a sentinel element to
locate the end of the array, like this:

void
dc_get_supported_models (const dc_model_t **models, unsigned int *n)
{
       ...

       *models = g_models;
       *n = C_ARRAY_SIZE (g_models) - 1;
}

But better ideas for the enumeration api are always welcome! In particular it 
would be nice if we could turn the dc_model_t structure into an opaque data type.

One possibility would be an iterator style api, similar to this:

dc_iterator_t *iterator = dc_get_supported_models ();

while (dc_iterator_next (iterator)) {
     dc_model_t *model = dc_iterator_current (iterator);

     /* Do stuff here */
     dc_model_get_name (model);
     dc_model_get_manufacturer (model);
     ...
}

dc_iterator_free (iterator);

15. Generic dc_device_open function.

Once we have support for enumerating the list of supported devices, we can 
implement a new dc_device_open() convenience function, which would take care of 
the mapping from a particular model (as obtained from the enumeration) to the 
correct xxx_device_open() function. The main advantage would be that an 
application doesn't need any changes when a new backend is added. An application 
would simply query the list of supported devices, pass the model selected by the 
user to the new function, and the libdivecomputer library will automatically 
select the matching backend.

dc_status_t
dc_device_open (dc_device_t **device, const dc_model_t *model, const char *devname);

or passing the backend type and model number explicitly:

dc_status_t
dc_device_open (dc_device_t **device, device_type_t type, unsigned int
model, const char *devname);

The only issue remaining is the devname parameter. Currently, it's only used for 
those backends using serial communication. But it's not unlikely that backends 
using some other low-level communication channel (usb, infrared, file, etc) may 
need different arguments. At the moment that's not the case, because usb and 
infrared do perform autodetection (vid/pid for usb and device name for irda), 
but that may change in the future.

One possibility to address that would be to include some flag in the model 
descriptor, that indicates the type of low-level communication channel, with an 
associated data structure for the arguments, which is then passed to the 
dc_device_open() function:

typedef dc_params_serial_t {
       const char *devname;
} dc_params_serial_t;

typedef dc_params_usb_t {
       ...
} dc_params_usb_t;

dc_status_t
dc_device_open (..., const void *params);

This flag could then also be used by applications to show the correct UI element 
for those paramters.

16. Generic dc_parser_create function.

For the parser we can do something similar. Currently an application needs to 
connect to the devinfo and clock events, store the associated data, and pass it 
back again to the xxx_parser_create() function.

However if the device object would store the event data internally, we can 
implement a new dc_parser_create() convenience function that grabs the event 
data directly from the device handle, and automatically does the right thing.

dc_status_t
dc_parser_create (dc_parser_t **parser, dc_device_t *device);

You'll only have to be careful not to call this new function before the events 
have been emitted. In practice that means at earliest when you receive the first 
dive. But that's no difference compared to the xxx_parser_create() functions of 
course.

It may be possible to workaround this limitation, but it's not trivial due to 
thread-safety issues, and I'm not sure it's worth the effort.

17. A context object for global state

As I have written before, the logging support is problematic because it relies 
on global state, which is obviously not thread-safe. A possible solution would 
be to introduce a new context object to store the global state. This approach 
has several benefits, but it will also require changes on the application side. 
Basically you'll have to create a context object first, and then pass it down to 
all calls that create other objects (e.g. device and parser):

dc_context_t *context = NULL;
dc_device_t *device = NULL;

dc_context_create (&context);

dc_device_open (&device, context, ...);
dc_device_foreach (device, ...);
dc_device_close (device);

dc_context_free (context);

Full thread-safe logging could be obtained by requiring one context object per 
thread (e.g. similar to the device and parser objects), or making the context 
object itself thread-safe (but without having to deal with a global state now). 
The context object would also allow to implement more complex initializations, 
like reading loglevels from environment variables at runtime (just to give an 
example).

18. Timezone support

The dc_datetime_t struct doesn't have any timezone support. Good timezone 
support is certainly non-trivial, but since modern dive computers often support 
a timezone setting, I believe we won't be able to ignore it. Full timezone 
support is definitely out of scope, but some basic support using a utc offset 
field would be sufficient for our use case (e.g. similar to the BSD struct tm 
tm_gmtoff extension).

Therefore I propose to add such a field as a placeholder. Even if it won't be 
using it immediately, we won't have to break backwards compatibility when we 
actually start using it later on.

19. Support partial read/write.

In many cases, the dc_device_read() and dc_device_write() functions will split 
large read/write request into multiple packets. Currently these functions either 
succeed or fail, but there is no way to indicate a partial success. Especially 
for writes, it may be good to know how much data was written correctly before a 
failure occurred.

Therefore, I propose to add an (optional) output parameter that receives the 
amount of bytes successfully read or written. The return code would function 
exactly the same as before, and indicate the status of the entire operation. But 
in the case of a partial success you receive an error, together with the actual 
number of bytes.

20. Units

Subsurface uses an interesting approach for defining values with units. 
Basically they define a special struct that makes the units very explicit. Maybe 
we can consider this approach for libdivecomputer too?

https://github.com/torvalds/subsurface/blob/master/dive.h

Note that internally libdivecomputer will always use metric units, and that 
won't change. The only change I may consider is using degrees Kelvin for the 
temperature rather than degrees Celcius. It may be slightly less convenient (for 
both metric and imperial developers), but it has the advantage that for all 
units (depth, pressure, temperature) the values are never negative.

21. Improved naming consistency: open/create/new?

Right now there are xxx_open() functions for the device handle, xxx_create() 
functions for the parser handle and a dc_buffer_new() function, each with their 
matching close(), free() and dc_buffer_free() functions.

The main difference between open() and create(), is that the create() function 
only allocates some memory resource, while the open() function also opens a 
connection with the device. But other than that, it's only a different name.

For create() and new(), the difference is that create() returns an error code, 
while new() returns a pointer. Thus the only failure new() can return is an 
out-of-memory condition (e.g. a NULL pointer), which is of course sufficient for 
a simple memory buffer object. In theory the create() function may return other 
error codes too, although at the moment that is not the case. Validation of the 
parameters (like the model code) is postponed until parsing, and the validation 
of the dive data is done in the xxx_set_data() function.

Personally, I prefer the name "new" over "create". But since it's common 
practice a new() function returns a pointer, not a status code, that may be 
confusing? Should we also change to dc_buffer_new() function to return a status 
code, or just make an exception here? (Note that in the original roadmap there 
were already plans to make all dc_buffer_xxx() functions return a self pointer, 
rather than an integer to indicate success or failure. So adding back a status 
code for the new() function would be a little weird.)

22. Add a destructor for the dive data?

The parser_set_data() function currently doesn't copy the dive data internally, 
which means an application needs to make sure the date remains alive for the 
lifetime of the parser. The main reaons why this was done is to avoid extra 
memory copies. Typically you receive the data from the dive callback (and 
optionally copy it if you want to use it outside the callback function), parse 
it, and then destroy it again (if necessary). In this scenario an extra copy 
would be wasteful.

Maybe it would be a nice idea to pass a destructor function, that the parser can 
call when it's done with the data? Then the application doesn't have to care 
anymore about keeping it alive long enough. Or just don't care about the extra 
memory copy at all, and always make a copy?

In case we don't care about the additional memory copy, then we could also 
consider dropping the parser_set_data() function. The purpose of this function 
is to be able to re-use a single parser object for multiple dives. But if we 
already copy the dive data, which is usually much larger than the parser object 
itself, then there is little reason to not allocate a new parser for every 
single dive. In this scenario, the dive data would be passed directly to the 
xxx_parser_create() function. (Note that this would also get rid of the 
limitation in #16, because now you won't be able to construct a parser in advance.)

Jef