RFC: support for deco algorith and parameter fields

[Jef Driesen]

On 2014-10-16 20:16, Dirk Hohndel wrote:
> On Thu, Oct 16, 2014 at 05:09:06PM +0200, Jef Driesen wrote:
>> 
>> I think this is nicely complementary with another improvement that I 
>> have
>> been thinking about: a new DC_FIELD_DIVEMODE, to differentiate between
>> freedive, gauge, open circuit and closed circuit modes.
> 
> Definitely

So'll add that to my todo list for v0.5 too, along with the 
DC_FIELD_TEMPERATURE and DC_FIELD_TANK stuff.

>>  +	DC_DECO_ALG_UNKNOWN,
>> 
>> I wonder if we should rename this to NONE? It looks like you used this 
>> for
>> gauge/freedive mode. But in that case, the decompression algorithm is
>> basically disabled. That's not the same as an unknown algorithm. Maybe 
>> the
>> DC_STATUS_UNSUPPORTED status code is a better choice to indicate an 
>> unknown
>> algorithm?
> 
> Hmm. I think these are all different.
> 
> - DC_DECO_ALG_NONE would be if the computer is in gauge or freedive or
>   apnoe mode, right?

Yes

> - DC_DECO_ALG_UNKNOWN if it does deco calculation, but we don't know 
> the
>   algoright
> - DC_STATUS_UNSUPPORTED means the backend doesn't have this function
>   implemented

The DC_STATUS_UNSUPPORTED is used in case a feature is not supported by 
the dive computer, or not implemented by the backend. The line between 
those two is very thin. If it's not implemented by the backend, that 
could be due to the fact that it's not supported by the dive computer, 
or because we don't know yet where and how it's stored. But usually we 
don't know that difference in advance.

The same logic applies here. Of course every dive computer will use some 
deco model (unless in gauge/freedive mode), but for libdivecomputer 
unsupported and unknown are basically the same thing.

>> +	DC_DECO_ALG_BZH,    /* Buehlmann ZH, no GF */
>> +	DC_DECO_ALG_BZHGF,  /* Buehlmann ZH, with GF */
>> 
>> The BZH name sounds a bit cryptic to me. How about naming it BUHLMANN
>> instead?
> 
> DC_DECO_BUHLMAN ?

Yes, something like that.

>> +	DC_DECO_ALG_VPMGFS, /* VPM with GF surface */
>> +	DC_DECO_ALG_DSAT,   /* Aeris DSAT */
>> 
>> I've never really heard of these before. Apparently VPMGFS is a hybrid
>> between VPM and Buhlmann GF used by Shearwater,
> 
> Yes, as I explained in the other email, this one has a parameter as 
> well -
> more or less the GFhigh for a Buhlmann algorithm that is run in 
> parallel
> to VPM (and the lower ceiling is picked).
> 
>> and DSAT is some Pelagic thing? Are there any others we are missing?
> 
> Reading the Aeris A300CS documentation it states that the "proprietary
> DSAT algorithm [...] has been used in all prior Aeris dive computers".

I see no reasons not to include those two, I was just wondering how they 
relate to the others. DSAT (Diving Science and Technology) appears to be 
a PADI related company, but that's all I can find about it.

>> +	DC_DECO_RGBM,       /* Suunto fused RGBM */
>> 
>> I assume you mean all variants of RGBM, and not just the Suunto 
>> variant? I
>> believe Atomics also uses some RGBM variant. I don't think the older
>> Suunto's (eon and solution) use RGBM. Most likely they use Buhlmann,
>> although I'm not sure. Did RGBM even exist already at that time?
> 
> Maybe it would be smart to return UNKNOWN for the oldest Suuntos. I 
> know
> that they have been advertizing RGBM since at least the Vyper 
> generation.

That's also what I found. According to wikipedia, RGBM was introduced in 
1999, but the solution and eon range are much older. I checked some 
manuals, but the algorithm isn't mentioned. There are references to 
tissue groups and half times, so that makes me believe they use some 
variant of the Buhlmann model. But yes, unknown/unsupported is probably 
more correct.

http://www.suunto.com/en-US/Support/Suunto-rgbm-dive-algorithms/
http://en.wikipedia.org/wiki/Suunto#Diving_computers_and_instruments

> And yes, we should use this for all RGBM - the "Suunto fused" was meant 
> as
> an example in the comment.

I already assumed that, but the "Suunto fused RGBM" comment might be 
confusing. Giving the full "Reduced gradient bubble model" name is maybe 
more descriptive?

>> You forgot the ALG_ prefix in the name here.
> 
> So you want DC_DECO_ALG_BUHLMANN, etc? Just making sure this is all
> consistent (and I double checked, it's two 'n').

The actual prefix will depend on how we name the enum, but yes we should 
be consistent and use the same prefix for all enum values.

>> >I'm calling this an RFC because I want feedback if this is the right
>> >API...
>> >this was straight forward to do, but I wonder if you would prefer a
>> >complex
>> >data structure and a single API entry point:
>> >
>> >DC_FIELD_DECO_INFO
>> >
>> >and
>> >
>> >struct dc_deco_information_t {
>> >	unsigned int algo;
>> >	unsigned int param1; /* e.g. GFhigh or GFS */
>> >	unsigned int param2; /* e.g. GFlow */
>> >	unsigned int param3; /* currently unused */
>> >};
>> 
>> I'm not really sure what would be the best solution here. Are there 
>> any
>> other algorithms besides the GF that have parameters that make sense 
>> to
>> support?
> 
> VPMGFS also has one GF. In the other mail the sat/desat factor was
> mentioned. I don't know what else is out there.

I guess only the GF's are a (defacto) standard. Everything is more or 
less some manufacturer specific tweak of the algorithm. So I think it's 
not unreasonable if we don't support those. If you really need to have 
that much detail, you should probably stick to the manufacturer's 
application.

For Buhlmann we could maybe also include the number of tissues and the 
A, B or C variant for the coefficients. It seems VPM also has a few 
variants, like VPM-A and VPM-B. But as you mention in your other email, 
this might already be overkill, because in most cases we don't have this 
kind of detailed info.

>> One concern is that with an integrated structure, we can't add new
>> parameters if that's ever necessary. I'm not really in love with 
>> placeholder
>> fields either. While with a separate DC_FIELD_DECOPARAMS type, we can
>> introduce different data structures for each deco algorithm. For 
>> example:
>> 
>> struct dc_decoparams_gf_t {
>> 	unsigned int low;  /* e.g. GFhigh or GFS */
>> 	unsigned int high; /* e.g. GFlow */
>> };
>> 
>> Of course an application will first have to check the algorithm, to 
>> know
>> which structure to use, but I think that's not unreasonable. If you 
>> care
>> about the params, you probably need the algo anyway.
> 
> Yes. In the patch series I have separate fields for GFlow and GFhigh. 
> That
> may be overkill. But combining them to a dc_decoparams_gf_t looks good 
> to
> me. And since the GFS is in fact a GFhigh, we could use the same 
> structure
> for VPMGFS and just leave the GFlow 0.

I'm fine with that too, otherwise I wouldn't have proposed it :-)

Jef