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