RFC: New api for gas changes

[Dirk Hohndel]

On Fri, May 22, 2015 at 04:34:45PM +0200, Jef Driesen wrote:
> 
> I have been working on a new api for gas changes. The idea is that old gas
> change events (SAMPLE_EVENT_GASCHANGE and SAMPLE_EVENT_GASCHANGE2) will be
> replaced with a new DC_SAMPLE_GASMIX sample. The corresponding sample data
> is an integer with the index of the gas mix. The actual oxygen and helium
> percentages can be retrieved with the DC_FIELD_GASMIX api. The main
> advantage over the old events, is that the gas mix index allows to
> distinguish two identical gas mixes.

Yes, we'd love to see that.

> I have attached two patches. The first patch contains the new api, and with
> the second patch all backends are ported to the new api.

Comments on the patches below.

> Please note that applying these patches will break existing applications.

Life could be so much easier if you did the one simple thing that Linus
and I have suggested quite a few time which would allow applications to
detect which features are in the version of libdivecomputer that they are
building against and which aren't. Your vision of doing this has always
been in the context of "releases", assuming that the consumers of
libdivecomputer won't be building against master - but that just isn't how
many people appear to be using libdivecomputer. And because all the
constants are enums and not defines, there is no easy way to test what's
there and what isn't.

In the Subsurface branch we have trivial things like this:

diff --git a/include/libdivecomputer/parser.h b/include/libdivecomputer/parser.h
index 774e5e3242bb..dbd9d2d81cf5 100644
--- a/include/libdivecomputer/parser.h
+++ b/include/libdivecomputer/parser.h
@@ -59,9 +59,13 @@ typedef enum dc_field_type_t {
 	DC_FIELD_TEMPERATURE_MAXIMUM,
 	DC_FIELD_TANK_COUNT,
 	DC_FIELD_TANK,
-	DC_FIELD_DIVEMODE
+	DC_FIELD_DIVEMODE,
+	DC_FIELD_STRING,
 } dc_field_type_t;
 
+// Make it easy to test support compile-time with "#ifdef DC_FIELD_STRING"
+#define DC_FIELD_STRING DC_FIELD_STRING
+

Redundant? Ugly? You tell me.
But most importantly exceptionally useful. Because code that requires
DC_FIELD_STRING can now simply do

#ifdef DC_FIELD_STRING
	// ah, we have that feature, let's use it...
	...
#endif

> They will still build, but they will no longer get any gas change events. In
> order to maintain backwards compatibility, I'm planning to introduce the new
> api as follows: The old events will get marked as deprecated, but not
> removed yet. That ensures existing applications will continue to work with
> no changes. In the meantime, applications can start to use the new api. And
> once v0.5 is out, the old events will be removed.

But that's not what the patches below do, is it? They just remove support
for the old event from the backends.

I'd rather have you do what you describe here - support both for a while,
but make it possible to detect if the new SAMPLE type is available (which
then allows the app to ignore the deprecated events).

> PS: I'm leaving tonight for a diving trip, so I probably won't be able to
> respond to questions or comments during the next week.

We all feel very sad for your pain and suffering...

> diff --git a/include/libdivecomputer/parser.h b/include/libdivecomputer/parser.h
> index 774e5e3..c22d26e 100644
> --- a/include/libdivecomputer/parser.h
> +++ b/include/libdivecomputer/parser.h
> @@ -43,7 +43,8 @@ typedef enum dc_sample_type_t {
>  	DC_SAMPLE_SETPOINT,
>  	DC_SAMPLE_PPO2,
>  	DC_SAMPLE_CNS,
> -	DC_SAMPLE_DECO
> +	DC_SAMPLE_DECO,
> +	DC_SAMPLE_GASMIX
>  } dc_sample_type_t;

Yeah, I'd REALLY want a

#define DC_SAMPLE_GASMIX DC_SAMPLE_GASMIX

here. Because then the application could KNOW that the new sample type is
available.

> diff --git a/src/atomics_cobalt_parser.c b/src/atomics_cobalt_parser.c
> index 83bcfc7..925e05a 100644
> --- a/src/atomics_cobalt_parser.c
> +++ b/src/atomics_cobalt_parser.c
> @@ -306,13 +306,8 @@ atomics_cobalt_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback
>  				ERROR (abstract->context, "Invalid gas mix index.");
>  				return DC_STATUS_DATAFORMAT;
>  			}
> -			unsigned int o2 = data[SZ_HEADER + SZ_GASMIX * idx + 4];
> -			unsigned int he = data[SZ_HEADER + SZ_GASMIX * idx + 5];
> -			sample.event.type = SAMPLE_EVENT_GASCHANGE2;
> -			sample.event.time = 0;
> -			sample.event.flags = 0;
> -			sample.event.value = o2 | (he << 16);
> -			if (callback) callback (DC_SAMPLE_EVENT, sample, userdata);
> +			sample.gasmix = idx;
> +			if (callback) callback (DC_SAMPLE_GASMIX, sample, userdata);
>  			gasmix_previous = gasmix;
>  		}
>  

So you are no longer sending the event.

I would much rather see you send the event AND provide the
DC_SAMPLE_GASMIX. This way (assuming you took my previous suggestion), the
application can simply say "if DC_SAMPLE_GASMIX isn't defined, respond to
the two GASCHANGE events, but if it is defined then ignore the GASCHANGE
events and instead use the DC_SAMPLE_GASMIX".

Now old software works with the new library. And updated software works
with both the old and the new library.

Ta-Da!

/D