Changes to dive sample structure

[Dirk Hohndel]

Willem,

PLEASE keep the mailing list copied.
I am a strong believer in not having a Reply-To set to the mailing list
(as with that it's way too easy to send personal information to a
mailing list by mistake), but that means that when responding to emails
on the list you need to hit Reply-All...

Everyone else - I'm including all of Willem's email to me so context is
maintained

On Tue, 2014-04-22 at 22:28 +0200, Willem Ferguson wrote:
> On 22/04/2014 16:23, Dirk Hohndel wrote:
> > Hi Willem
> >
> > A few questions...
> >
> > On Tue, 2014-04-22 at 12:05 +0200, Willem Ferguson wrote:
> >> Here are suggested additions to the existing sample structure in order
> >> to accommodate dive logs from rebreather systems.
> >>
> >> struct sample {
> >> 	duration_t time;
> >> 	depth_t depth;
> >> 	temperature_t temperature;
> >> 	pressure_t cylinderpressure;
> >> 	int sensor;   /* pressure of primary cylinder */
> >>           int pdiluent; /* pressure of diluent cylinder in CCR dive log*/
> > so that would be
> > 	pressure_t pdiluent;
> > :-)
> >
> >> 	duration_t ndl;
> >> 	duration_t stoptime;
> >> 	depth_t stopdepth;
> >> 	bool in_deco;
> >> 	int cns;
> >> 	int po2		// O2 reading from 1st O2 sensor in CCR divelog
> >> 	int o2sensor2;  // O2 reading from 2nd O2 sensor in CCR divelog
> >> 	int o2sensor3;  // O2 reading from 3rd O2 sensor in CCR divelog
> > IIRC the readings from these sensors will be between 0.10 (because if
> > it's below that, you won't come back to download the data), and 2.5
> > (because above that, you won't come back to download the data, either).
> > What's the resolution? 0.01? So this would easily fit in
> >
> > 	uint8_t o2sensor[3];
> >
> >> 	int ccrsetpoint;	// CCR O2 setpoint
> > and then we could nicely pack the setpoint into it as well.
> >
> > struct __attribute__((__packed__)) {
> > 	uint8_t setpoint;
> > 	uint8_t o2sensor[3];
> > }
> >
> > and instead of 4 32 bit values we added just one 32 bit value.
> > Or, if you are worried about higher resolution (unlikely, but possible)
> > or people who want to do some hyper saturation shit (hey, they dive a
> > rebreather, we KNOW that they are crazy and/or suicidal) and who want
> > pO₂ > 2.55... pack 16bit integers with mbar resolution. That gives you
> > 0.001 -> 65.535 as range for your pO₂ - and still only adds 64 bit.
> >
> >> 	int heartbeat;
> >> 	int bearing;
> >> };
> >>
> >> The above changes add four integers (i.e. 16 bytes of int32) to the
> >> sample structure in order to accomodate rebreather dive logs. These are
> >> necessitated because of the large amount of information logged by
> >> rebreather dive computers and which most rebreather divers would
> >> consider as pretty important because it allows evaluation of the oxygen
> >> sensors as well as the efficiency of the oxygen management system
> >> during a dive. There is talk that some of the new rebreather systems
> >> will include an inline CO2 sensor as well.
> > Yep, makes sense.
> >
> >> The concern is about bloat in the size of dive logs if several fields
> >> are added to the existing sample structure. I assume that the int
> >> variables in the structure are int32. There is clearly no need for a
> >> signed integer. Would it be helpful at all if the int variables are
> >> changed to uint16? That would decrease the size of the above structure
> >> by 18 bytes.
> > I think making the partial pressures uint8_t values gives us the range
> > and resolution we need (and I have yet to see a dive computer that gives
> > us more) and would nicely balance bloat and functionality.
> >
> >> I have a suspicion that such a change (to uint16) would have repercussions in many places within the existing code base.
> > cns certainly doesn't need to be an int :-)
> >
> >
> 
> If my counting is correct (pressure_t, depth_t etc are all defined as 
> int) then the existing version of sample is 49 bytes.

It's actually 52 bytes since the compilers will pad it to 4byte
granularity.

> Let us, for the moment and following Dirk's suggestion above, propose a modified version:
> 
> 
> struct __attribute__((__packed__)) {
> 	uint16_t setpoint;
> 	uint16_t o2sensor[3];	
> } o2ccr;

That's the someone more generous version that honestly I'm not sure we'd
need, but sure.

> struct sample {
>      duration_t time;
>      depth_t depth;
>      temperature_t temperature;
>      pressure_t cylinderpressure;
>      pressure_t pdiluent; /* pressure of diluent cylinder in CCR dive 
> log */
>      pressure_t sensor;   /* Cylinder pressure sensor index */
>      pressure_t po2;     // Oxygen partial pressure
>      duration_t ndl;
>      duration_t stoptime;
>      depth_t stopdepth;
>      int bearing;
>      o2ccr ccrdata;    // packed structure containing sensor and 
> setpoint data
>      short int cns;
>      short int heartbeat;
>      bool in_deco;
> };
> 
> This is 57 bytes. A few questions:

In reality 60 bytes

> 1) Two fields are now short int. How portable is short int to Windows 
> and Mac environments? Or briefly, is using short int here a good idea at 
> all?

Doesn't matter, this is an in memory data structure. Which issues do you
think there could be? But let's not use short. Let's use uint16_t. I
think all platforms have 16bit shorts, but I'd rather just specify
this...

> 2) Strictly speaking po2 is possibly not used if o2sensor data are 
> stored in the packed structure. But if po2 remains, it surely should 
> also conform to
> the pressure_t characteristic? So this point has 2 subquestions: a) 
> should po2 be kept?; b) if it's kept, should it remain as int, as at 
> present?

I think it should be of the same type as the other partial pressure
related members. You could create partpressure_t as a struct (just like
pressure_t) only with mbar as uint16_t (funnily enough, we could
actually simply convert pressure_t to be 16 bits - see below).

> 3) All of the declared types in this structure (i.e. duration_t, 
> depth_t, temp_t, pressure_t) are defined as int. Is there not a case to 
> make that pressure_t,
> temperature_t and possibly depth_t should be defined as short int? This 
> will make the sample structure potentially weighing in at below 40 
> bytes, but
> this would depend on the efficiency of short int memory allocation.

And on the required range.
temperature_t is in mK - so uint16_t allows for temperatures up to 65K -
that would be a little rough to dive in
depth_t is in mm - up to 65m - not quite enough
but pressure_t is a good candidate: 65000mbar is enough for depths up to
650m that should be enough even for our rebreather divers :-)

So unless Linus sees an issue that I'm missing... that might work and
make the whole pO₂ situation easier.

> As an academic aside: Does the bool variable incur some padding bytes at 
> the end of the structure?

Yes it does.

/D