[PATCH] libdivecomputer - custom implementation for serial communication

[Claudiu Olteanu]

> 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.


> 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?


> > +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;


> 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


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



>
> 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.

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.

Cheers,
Claudiu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150626/476e1b51/attachment.html>