[PATCH] libdivecomputer - custom implementation for serial communication

Dirk Hohndel dirk at hohndel.org
Fri Jun 26 07:27:58 PDT 2015


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 <olteanu.claudiu at ymail.com>
> 
> 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 <olteanu.claudiu at ymail.com>
> ---
>  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 <olteanu.claudiu at ymail.com>
> ---
>  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 <olteanu.claudiu at ymail.com>
> ---
>  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


More information about the subsurface mailing list