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
_______________________________________________
subsurface mailing list
[email protected]
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
