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 _______________________________________________ subsurface mailing list subsurface@hohndel.org http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface