[PATCH] libdivecomputer - custom implementation for serial communication

[Jef Driesen]

On 2015-06-29 23:11, Claudiu Olteanu wrote:
> Hello Jef,
> 
> Thanks for making time to let us know what you
> have in mind for the next release. More or less it is
> similar with the changes I made, except that I tried
> to avoid to modify the native serial implementation.
> 
> I will make you a short resume about the changes
> I did because I believe that there were too many
> posts and it is not easy to keep the track.

All your changes seems to be in your github repository, so I didn't had 
to read all emails. :-)

> The dc_serial_t structure is declared as:
> struct dc_serial_t {
>  serial_t *port; //serial device port
>  dc_transport_t type; //the type of the transport (USB, SERIAL, IRDA,
> BLUETOOTH)
>  void *data; //specific data for serial device
>  const dc_serial_operations_t *ops; //reference to a custom set of
> operations
> } dc_serial_t;
> 
> As you can see, the only difference is that I
> added a new member (dc_transport_type).

If you haven't noticed, there is another fundamental difference. You 
have the dependencies reversed. Your custom implementation (dc_serial_t) 
depends on the native implementation (serial_t). That makes no sense. In 
your custom implementation, you certainly don't want to use the native 
implementation, so why would you need the serial_t pointer there?

If you look at my proposal, you'll see that the dc_serial_t is just an 
abstract interface (e.g. an empty box with a public api). Both the 
native and custom backend provide an implementation of this interface. 
The main advantage is that the dive computer backends can use the 
abstract interface without any modifications, and fully independent of 
the actual implementation that is being used.

> I thought that this can be used to identify the
> type of the transport and to do some specific
> operations if they are needed.

See that's exactly what we don't want to do. We want to do:

dc_serial_read(serial,...);

and not some ugly constructs with if-else everywhere:

if (serial->type == DC_TRANSPORT_SERIAL) {
     dc_serial_read(serial,...);
} else {
     serial->ops->read(serial,...);
}

> Also I used a pointer member for serial_t.
> Dirk suggested to remove the pointer but
> it was easier for me to use it :).

The reason why I'm not using a pointer here, is a well known technique 
for implementing inheritance in C. Assume you define two structures as 
follows:

struct base_t {
    ...
};

struct derived_t {
    base_t base;
    ...
};

with the "base" field being the first member of the derived_t structure. 
The C standard guarantees that the address of a struct is the same as 
the address of its first member. That means you can safely cast a 
derived_t pointer to a base_t pointer.

derived_t foo;
base_t *p1 = &foo.base;
base_t *p2 = (base_t*)&foo;

This is used everywhere in libdivecomputer. When you call 
dc_device_open() you get back a dc_device_t pointer, but it's in fact a 
pointer to a vendor_model_device_t struct.

With a pointer inside the struct, this trick does not works.

> I choose to represent all functions which are
> used for the communication and may need
> a custom implementation, depending on the
> protocol used (Bluetooth, IRDA, etc).

Although the different communications protocols (e.g. serial, bluetooth, 
irda and usb) can share a common interface, there is still lots of 
functionality that will always remain different. For example serial 
communication requires to configure the line settings (baudrate, 
databits, etc), but there is no such concept for irda communication. 
Note that bluetooth is more similar to serial I/O, because rfcomm is 
basically serial emulation over bluetooth.

That's why I suggested the more generic dc_stream_t abstraction, with 
the common functionality like read, write, close, etc:

dc_stream_read(stream, data, size);
dc_stream_write(stream, data, size);
dc_stream_close(stream);

And then some specific functions for dealing with the communication 
specific features:

dc_serial_configure(stream, baudrate, stopbits, ...);
dc_irda_xxx(stream, ...);

Internally, each "stream" knows its type, so trying to use a serial 
specific function on an irda stream will result in an error. In the dive 
computer backends that's fine, because trying to pass an irda to a 
serial based dive computer is never going to work.

[BTW, what I describe here is exactly how backend specific functions 
like hw_ostc3_device_fwupdate already work today.]

But let's take this one step at a time and concentrate on the 
serial/bluetooth part for now :-)

> The dc_serial_operations_t is:
> struct dc_serial_operations_t
> {
>  int (*open) (serial_t **device, dc_context_t *context, const char
> *name);
>  int (*close) (serial_t *device);
>  int (*read) (serial_t *device, void* data, unsigned int size);
>  int (*write) (serial_t *device, const void* data, unsigned int size);
>  int (*flush) (serial_t *device, int queue);
>  int (*get_received) (serial_t *device);
>  int (*get_transmitted) (serial_t *device);
> } dc_serial_operations_t;
> 
> As I said, I tried to avoid modifications on
> native serial implementation. Therefore I used
> the same signatures for needed functions.

It's no problem if you need to modify the native serial code. I also did 
that in my proposal.

But in the dive computer backends we want to avoid conditional code 
which depends on the type of low-level I/O. That's the whole point of 
abstracting the I/O behind a common interface.

> I preferred to do that because I wanted to keep
> backwards compatibility and to do as few changes
> as possible. So I created a dc_device_custom_open
> method where the application  can pass a reference
> to a dc_serial_t structure.
> 
> Also I added a dc_serial_native_open which
> creates a dc_serial_t structure for the
> native serial implementation. In this way I could
> use it in the DEVICE_FAMILY_device_open
> method and maintain the backwards compatibility.
> 
> It is obvious that you have a clear idea about
> how the libdivecomputer API should look
> like and which are the things that need to be
> refactored/cleaned.
> 
> I would definitely like to help you (now or later).
> 
> Dirk, Thiago, do you think that I should refactor
> again the libdivecomputer? Should I change
> again the design? Or I should continue my current
> work, use the current design for all vendors
> which have Bluetooth support, implement the
> Bluetooth transfer on Windows platforms and finally
> start to modify the design as Jef suggested?
> 
> I ask this because in the end the Subsurface
> custom serial implementation doesn't need
> many changes to be integrated with a new
> version of libdivecomputer and with its design.
> Also, I already have a functional prototype.
> 
> I am not sure how to prioritize things. :)
> 
> So please let me know what you think.

I guess the answer depends on whether the GSOC tasks includes the 
libdivecomputer side, or whether the focus should be on the subsurface 
side. I'll leave that decision to Dirk and Thiago.

Note that libdivecomputer will need the changes I described anyway. So 
if they are not done as part of the GSOC, then I can easily take care of 
that myself (after the v0.5 release). Claudiu can keep using the 
libdivecomputer hacks for now, and concentrate on the subsurface parts. 
Proper integration can be taken care of afterwards. But on the other 
hand, I'm more than happy to assist Claudiu with the libdivecomputer 
related parts too.

Jef