[PATCH] libdivecomputer - custom implementation for serial communication

[Dirk Hohndel]

On Fri, Jun 26, 2015 at 07:00:37PM +0300, Claudiu Olteanu wrote:
> > I believe this is a good direction. From the discussion earlier I'm
> > doubtful that Jef agrees, but maybe a successful implementation can sway
> > him :-)
>
> It would be good if he will give us some feedback.

Jef is very busy and has a long list of issues he is trying to stay on top
of. That said, Yes, I'd love to get his input here. Even if it is along
the lines of "how to make this least annoying to him".

> > That looks odd. You should have Jef's Copyright there (since it's
> > based on his code), but you should add your own Copyright as well
> > as you created this file.
>
> I didn't know what to do with the license and how to handle this
> problem :). If I will write my name at the end it would be enough?

One way to do this would be to copy Jef's copyright line and add yours
below:

 * Copyright (C) 2008 Jef Driesen
 * Copyright (C) 2015 Claudiu Olteanu

This is appropriate for files where you took one of his files and modified
it, or if what you are doing is heavily based on code copied from his
files.

If you write a file completely from scratch you can simply put in your own
Copyright. Or if there are some parts you copied from existing code you
can do this

 * Copyright (C) 2015 Claudiu Olteanu
 * based on code that is Copyright (C) 2008 Jef Driesen

> > > +typedef struct serial_t serial_t;
> > > +
> > > +typedef struct dc_serial_operations_t
> > > +{
> > > +     //TODO in the end serial_t should be replaced with  dc_serial_t
> >
> > Can you explain that comment in more detail?
> > serial_t is an opaque structure that is used internally in
> > libdivecomputer. It contains members that dc_serial_t doesn't contain
> > (e.g. baudrate, duplex settings, termios data, etc). Since the
> > libdivecomputer code expects to pass a serial_t to the API functions, I'm
> > not quite sure what you mean that you would like to pass a dc_serial_t
> > around instead...
> 
> I believe that the baudrate, duplex settings, etc. can be set
> using the *data field from dc_serial_t structure. These are specific to
> the native serial communication. On IRDA or Bluetooth communication
> we only need a file descriptor, a context and a timeout.
> So I wanted to create a generic structure which can hold minimal information
> needed for any serial implementation and to offer the possibility
> to add some specific data.
> Now I believe that this is a bad idea and the structure can be represented
> as
> 
>  typedef struct dc_serial_t {
>       dc_transport_t type;                    //the type of the transport (USB, SERIAL, IRDA, BLUETOOTH)
>       serial_t *port;                         //serial device port
>       void *data;                             //specific data for device
>       const dc_serial_operations_t *ops;      //reference to a custom set of operations
>  } dc_serial_t;

Yeah, I think I like that better. Now how about doing it this way:

  typedef 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 device
       const dc_serial_operations_t *ops;      //reference to a custom set of operations
  } dc_serial_t;

Note how I turned the serial_t into a member instead of a pointer.
Now you can hand a pointer to this structure to a function that expects a
pointer to a serial_t and it will just work. Would this simplify things?

> > There's clearly some overlap between the structures, but they are quite
> > different.
> 
> I think that I should remove the fd, timeout and context fields and
> to add a serial_t *port member

See what I wrote above. I'm fine with other. I seem to remember that Jef
in general prefers the structures with common parts in front that can
simply be passed around and then work as expected...

> > This feels like it should have been squashed into the first patch...
>
> Yes, you are right. I forgot to add it in the beginning.

As a general rule I do NOT enforce this. I am MUCH happier when people do
small patches and commit often. But since you were reworking the patches
anyway (and will be reworking them to address the data structure issues)
it would be nice to turn commits that clearly are nothing but fixups into
just that (git even has the cool "--fixup" feature in newer versions...

> > Won't that break things for OSTC devices that are USB (so all but the very
> > latest dive computers from HW - which are the two that you have)?
> > Maybe I'm missing what you are doing with the data pointer below. I need
> > to look at this not as a patch but in the final code to make sure I
> > understand it more completely.
> > Yeah, it seems that you are encapsulating all this. OK. But then why not
> > include a serial_t as member of dc_serial_t ?
> 
> As I said it the beginning, now I believe that it would be a better
> idea to add a serial_t member. :-)
> For the moment it will break things for OSTC devices but after we will
> decide how the dc_serial_t structure should look like, I will create
> a new dc_serial_usb_open method and a dc_serial_irda_open and use it.

Great.
Right now I can't merge this into the Subsurface-testing branch because
most of our testers have USB (or more correctly, serial) based devices.
But once this becomes transparent I can add this and we can get much more
testing.

> Thanks for the feedback. I will try to use it and improve the current
> implementation. Please let me know if you agree with the new proposed
> dc_serial_t representation.

See above. Let me know what you think. Given existing precedent in the
libdivecomputer code I believe that this would be more in line with the
design philosophy.

/D