I know that Jef is on the Subsurface mailing list as well, but let's copy him directly on things like this :-)
On Fri, Jun 26, 2015 at 02:33:27AM +0300, Claudiu Olteanu wrote: > Hi there, > > I attached some patches which can be used to make the *libdivecomputer * > project to accept callbacks to custom functions used for serial > communication. > > As I mentioned in my weekly report, I am not sure if I made the best > decisions on the implementation. 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 :-) > I tested the HW OSTC 3 family with my OSTCs device and the native serial > communication works. I also implemented a custom implementation using > the QtBluetooth API and I managed to transfer the dive logs without any > problems. That's excellent. > From: Claudiu Olteanu <[email protected]> > > diff --git a/include/libdivecomputer/custom_serial.h > b/include/libdivecomputer/custom_serial.h > new file mode 100644 > index 0000000..788a062 > --- /dev/null > +++ b/include/libdivecomputer/custom_serial.h > @@ -0,0 +1,58 @@ > +/* > + * libdivecomputer > + * > + * Copyright (C) 2008 Jef Driesen 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. > +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... > + int (*open) (serial_t **device, dc_context_t *context); > + 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); > +} dc_serial_operations_t; > + > +typedef struct dc_serial_t { > + dc_context_t *context; //context > + int fd; //file descriptor > + long timeout; //timeout > + 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; There's clearly some overlap between the structures, but they are quite different. > Subject: [PATCH 05/13] Add a new parameter for serial open method > > This parameter is temporary. We use it to respect the signature > of the native serial open method. In this way we don't need to > modify the old implementation of native serial communication. > > Signed-off-by: Claudiu Olteanu <[email protected]> > --- > include/libdivecomputer/custom_serial.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/libdivecomputer/custom_serial.h > b/include/libdivecomputer/custom_serial.h > index ae0143d..cf005f4 100644 > --- a/include/libdivecomputer/custom_serial.h > +++ b/include/libdivecomputer/custom_serial.h > @@ -36,7 +36,7 @@ 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 > - int (*open) (serial_t **device, dc_context_t *context); > + 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); > -- > 2.1.4 This feels like it should have been squashed into the first patch... > Subject: [PATCH 07/13] Add dc_ prefix to custom serial open method > > Signed-off-by: Claudiu Olteanu <[email protected]> > --- > include/libdivecomputer/custom_serial.h | 2 +- > src/custom_serial.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/libdivecomputer/custom_serial.h > b/include/libdivecomputer/custom_serial.h > index 856facd..f1c8c3f 100644 > --- a/include/libdivecomputer/custom_serial.h > +++ b/include/libdivecomputer/custom_serial.h > @@ -53,7 +53,7 @@ typedef struct dc_serial_t { > const dc_serial_operations_t *ops; //reference to a custom set of > operations > } dc_serial_t; > > -void serial_init(dc_serial_t *device, void *data, const > dc_serial_operations_t *ops); > +void dc_serial_init(dc_serial_t *device, void *data, const > dc_serial_operations_t *ops); > > dc_status_t dc_serial_native_open(dc_serial_t **serial, dc_context_t > *context, const char *devname); > > diff --git a/src/custom_serial.c b/src/custom_serial.c > index 56ac619..34f0936 100644 > --- a/src/custom_serial.c > +++ b/src/custom_serial.c > @@ -34,7 +34,7 @@ const dc_serial_operations_t native_serial_ops = { > > > void > -serial_init(dc_serial_t *device, void *data, const dc_serial_operations_t > *ops) > +dc_serial_init(dc_serial_t *device, void *data, const dc_serial_operations_t > *ops) > { > memset(device, 0, sizeof (*device)); > device->data = data; > @@ -67,7 +67,7 @@ dc_serial_native_open(dc_serial_t **out, dc_context_t > *context, const char *devn > } > > // Initialize data and function pointers > - serial_init(serial_device, port, &native_serial_ops); > + dc_serial_init(serial_device, port, &native_serial_ops); > > // Set the type of the device > serial_device->type = DC_TRANSPORT_SERIAL; > -- > 2.1.4 Again something that could easily be merged with the earlier commit > Subject: [PATCH 09/13] Use the dc_serial_t structure in HW OSTC family > > Open a native serial device and use it in the HW OSTC > implementation. > > This patch replaces the old serial structure with the > new one, used for custom implementations. > > Signed-off-by: Claudiu Olteanu <[email protected]> > --- > include/libdivecomputer/hw_ostc.h | 1 + > src/hw_ostc.c | 66 > ++++++++++++++++++++------------------- > 2 files changed, 35 insertions(+), 32 deletions(-) > > diff --git a/include/libdivecomputer/hw_ostc.h > b/include/libdivecomputer/hw_ostc.h > index 91fe714..52ef81b 100644 > --- a/include/libdivecomputer/hw_ostc.h > +++ b/include/libdivecomputer/hw_ostc.h > @@ -26,6 +26,7 @@ > #include "device.h" > #include "parser.h" > #include "buffer.h" > +#include "custom_serial.h" > > #ifdef __cplusplus > extern "C" { > diff --git a/src/hw_ostc.c b/src/hw_ostc.c > index d994d38..ee7e17e 100644 > --- a/src/hw_ostc.c > +++ b/src/hw_ostc.c > @@ -64,7 +64,7 @@ > > typedef struct hw_ostc_device_t { > dc_device_t base; > - serial_t *port; > + dc_serial_t *serial; > unsigned char fingerprint[5]; > } hw_ostc_device_t; 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 ? > @@ -96,7 +96,7 @@ hw_ostc_send (hw_ostc_device_t *device, unsigned char cmd, > unsigned int echo) > > // Send the command. > unsigned char command[1] = {cmd}; > - int n = serial_write (device->port, command, sizeof (command)); > + int n = serial_write (device->serial->data, command, sizeof (command)); > if (n != sizeof (command)) { > ERROR (abstract->context, "Failed to send the command."); > return EXITCODE (n); > @@ -105,7 +105,7 @@ hw_ostc_send (hw_ostc_device_t *device, unsigned char > cmd, unsigned int echo) > if (echo) { > // Read the echo. > unsigned char answer[1] = {0}; > - n = serial_read (device->port, answer, sizeof (answer)); > + n = serial_read (device->serial->data, answer, sizeof (answer)); > if (n != sizeof (answer)) { > ERROR (abstract->context, "Failed to receive the > echo."); > return EXITCODE (n); > @@ -139,11 +139,11 @@ hw_ostc_device_open (dc_device_t **out, dc_context_t > *context, const char *name) > device_init (&device->base, context, &hw_ostc_device_vtable); > > // Set the default values. > - device->port = NULL; > + device->serial = NULL; so if serial by default is NULL, then the device->serial->data dereferences above should first check if device->serial is valid I will test this a little later today but would love it if you could answer the questions that I posted. Conceptually I think the design will work. I need to spend some more time to make sure that I understand any side effects on "regular" users of libdivecomputer. We basically want to make sure that everything keeps working just "as is" for someone who doesn't want to use the new API. /D _______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
