Rebreathers in Subsurface - new branch
Dirk Hohndel
dirk at hohndel.org
Mon Jun 2 10:27:07 PDT 2014
Ok, here we go... I'm on a plane, nothing else useful to do.
[edited later: I'm finishing this on the bus which is really making me
sick... but I didn't want you to have to wait for two more hours)
/D
]
On Mon, Jun 02, 2014 at 11:11:16AM +0200, Willem Ferguson wrote:
> ---
> developernotes | 101 ++++
> dive.c | 67 ++-
> dive.h | 39 +-
> divelist.c | 10 +-
> file.c | 2 +-
> gas0.dat | 1081 +++++++++++++++++++++++++++++++++
> gas1.dat | 1712 +++++++++++++++++++++++++++++++++++++++++++++++++++++
Why are these data files included? If they are sample files, they at least
should be in dives/gas[01].dat
> libdivecomputer.c | 10 +-
> load-git.c | 8 +-
> parse-xml.c | 49 +-
> planner.c | 10 +-
> profile.c | 328 +++++++++-
> profile.c.bak | 1463 +++++++++++++++++++++++++++++++++++++++++++++
Hm???
> profile.h | 6 +-
> save-git.c | 10 +-
> save-xml.c | 10 +-
> uemis.c | 2 +-
> units.h | 20 +-
> 18 files changed, 4829 insertions(+), 99 deletions(-)
> +++ b/developernotes
> @@ -0,0 +1,101 @@
> +Changes to the Subsurface code to implement rendering of rebreather dive logs.
> +
> +Three steps are involved:
> +
> +1)
> +In units.h, creating two new typedefs:
> + o2pressure_t
> + bearing_t
> +In units.h, changing the types of the unit variables to int32_t or int16_t.
> +This was for duration_t, depth_t, pressure_t, temperature_t.
> +
> +Changing the types of the sample structure in dive.h so that all variables are
> +strongly typed and that all variables are defined in term of units. This included:
> +In dive.h, add the following variables to store rebreather data:
> + o2setpoint
> + o2sensor[3]
> + Change cylinderpressure to cylinderpressure[2]
That I don't like at all. It clutters the code with unintuitive names and
I don't think I ever see you calculate the index - so it's always just a
direct access to one of the three values.
So this should be
main_cylinder_pressure
dilutent_cylinder_pressure
o2_cylinder_pressure
> +This precipitated many small changes throughout the code base in order to work
> +with different data types.
And that should have been one commit, all by itself. No other change, just
change the existing code to use the new types / variables
> +
> +In dive.h, add two variables to the divecomputer structure:
> + dc->dctype (to indicate what type of computer, rebreather in this case)
> + dc->No2sensors, the number of O2 sensors in the rebreather system
> +
> +Rebreather dives have a dctype="rebreather" in the xml dive log. This identifies
> +the dc as a rebreather and activates the appropriate code to handle rebreather dives.
> +For open circuit dives, at this time, dctype would not be defined. I test for the
> +dctype by examining only the first character of dctype ('r').
There are different types of rebreathers, which need to be handled
differently. Why not dctype="CCR" and test for that.
Again - this alone, the new dctype and its handling in parse-xml,
load-git, save-git, save-xml should be its own commit.
> +2)
> +Diluent gas pressure:
> +Including infrastructure to handle two rebreather cylinders (oxygen and diluent).
> +Normally, the gas pressures for all cylinders are calculated over the total period
> +for which they are measured during the dive. However, only a subset of the pressure
> +measurements are used if there are gas changes during the dive: pressure values
> +while a cylinder is not selected are (sensibly) dropped. When there are two
> +cylinders that need to be simultaneously considered, one needs to keep information
> +for more than one cylinder. This was performed by adding a new array, diluentpressure[2],
> +to the plot_data structure. This array is used in exactly the same way, and in addition
> +to, the pressure[2] array that has always been used.
> +
> +Three new functions were created within profile.c:
> + struct get_pr_dil_interpolate_data
> + fill_missing_diluent_pressure
> + populate_diluent_information
> +These are direct copies of existing functions in profile.c that handle other gasses.
If they are direct copies, then why aren't you reusing the existing
functions on the different values?
> +As indicated above, the diluent pressures need to be stores separately (in
> +diluentpressure[2], mentioned above) and outside of the normal storage of gas
> +pressures. A sophisticated approach would have made slight adaptations to the existing
> +functions in in order to add diluent handling. The approach above has the advantage
> +that it keeps the code for rebreathers separate from the original code. This promotes
> +stability of the overall code base.
Yes, but it also bloats the code and risks that bugs are fixed on one side
and not the other.
But this is fine, we can always reintegrate them again after your patch
lands on master.
> +For the diluent gas, the diluent data are taken from the sample structures and
> +transferred to the plot_data structures. The interpolation is performed as for the
> +other gases and the pressures are available separately from the other gases. The
> +oxygen cylinder pressure, on the other hand, is dealt with in the normal way. The oxygen
> +cylinder is cylinder[0]. If the diver changed to a baleout cylinder during the dive,
> +the pressures of the baleout gas would be stored in the plot_data structures
> +(in pressure[2]) because it would be a normal change of gas.
This confuses me. So O2 is pressure[0], dilutent is pressure[1] and bailout is
pressure[2]? But when you bail out bailout becomes pressure[0], because we
are now in OC mode? But since earlier rebreather mode was a property of
the dive computer, how does that work?
> +The above results in both the oxygen as well as the diluent pressures being available
> +in the plot_data structures within plot_info, ready for the graphic display of pressures
> +on the dive profile UI.
That should be easy. I think it will be visually challenging, but I'm sure
we can figure that out.
> +3)
> +Oxygen partial pressures
> +Rebreathers have integrates oxygen pressure sensors. Some rebreathers (e.g. Poseidon
> +MkVI have 2 sensors, others (e.g. APD Inspriation) have three sensors.
> +In order to analyse the oxygen management system of the rebreather, it is necessary
> +to store the data from thesre sensors. The following variables were created:
> +Wihin sample:
> + o2setpoint
> + o2sensor[3]
> +Within plot_data:
> + o2setpoint
> + o2sensor[3]
are we sure that there are no rebreathers with more than 3 sensors?
> +Within fixup_dive_dc in dive.c, redundant setpoint data and oxygen sensor data are
> +removed from sample structures.
> +
> +The following function was written:
> + fill_o2_values
> +This does the opposite of fixup_dive_dc: it takes the o2 sensor values that have been
> +transfered from sample to plot_data and fills in the zero values to create a full set
> +of setpoint and oxygen sensor data within the plot_data structures.
> +
> +Within calculate_gas_information, the po2 variable is calculated from the raw sensor data.
> +For 2-sensor systems, the mean of the 2 sensor values is used, for 3-sensor systems,
> +the mean of the two sensors with most similar values is used. the fo2 is also calculated
> +based on the po2 value.
Can you explain why "mean of the two most similar values" is the correct
way to do this? What if we have one sensor show a stable 1.3, one show a
stable 1.0 and the third oscillates between 1.1 and 1.2? Then the "most
similar" pair will keep changing and the actual reading will jump between
1.05 and 1.25 - amplifying the oscillation.
> diff --git a/dive.c b/dive.c
> index ae07d15..596e7e7 100644
> --- a/dive.c
> +++ b/dive.c
> @@ -454,10 +454,10 @@ void per_cylinder_mean_depth(struct dive *dive, struct divecomputer *dc, int *me
>
> static void fixup_pressure(struct dive *dive, struct sample *sample)
> {
> - unsigned int pressure, index;
> + int32_t pressure, index;
why switch to a signed value?
> cylinder_t *cyl;
>
> - pressure = sample->cylinderpressure.mbar;
> + pressure = sample->cylinderpressure[0].mbar;
See my comment earlier, I really don't like this.
> if (!pressure)
> return;
> index = sample->sensor;
> @@ -848,6 +848,7 @@ static void fixup_dc_events(struct divecomputer *dc)
>
> static void fixup_dive_dc(struct dive *dive, struct divecomputer *dc)
> {
> + FILE *f1;
???
> int i, j;
> double depthtime = 0;
> int lasttime = 0;
> @@ -855,25 +856,52 @@ static void fixup_dive_dc(struct dive *dive, struct divecomputer *dc)
> int maxdepth = dc->maxdepth.mm;
> int mintemp = 0;
> int lastdepth = 0;
> - int lasttemp = 0, lastpressure = 0;
> + int lasttemp = 0, lastpressure = 0, lastdiluentp = 0;
> + int lasto2[3] = { 0, 0, 0 }, lastsetpt = 0;
Whitespace. Actually, there are whitespace issues EVERYWHERE. Please at
least run scripts/whitespace.pl against your newly written code.
On an unrelated note: I'm ok with o2[3] - these are three o2 sensors and
you do things like create averages with them, so it makes sense to
calculate the index or have the index in a variable.
What I don't like is cylinderpressure[3] - different cylinders with
different use, treated as if they were 3 sensors.
> int pressure_delta[MAX_CYLINDERS] = { INT_MAX, };
>
> /* Fixup duration and mean depth */
> fixup_dc_duration(dc);
>
> update_min_max_temperatures(dive, dc->watertemp);
> +
> +
> +//f1 = fopen("gas0.dat","w");
> +//if(f1 == NULL)
> +// printf("File open error\n");
> +//fprintf(f1,"id t1 gas t2 t3 dil t4 t5 depth t6 cylinderid t7 setpoint sensor1 sensor2 senor3 \n");
> +
> +
> +//int s1,s2,s3,s4;
I understand the need for debug code. But don't comment it out like this,
please. Put it between #if / #endif pairs.
> int time = sample->time.seconds;
> int depth = sample->depth.mm;
> int temp = sample->temperature.mkelvin;
> - int pressure = sample->cylinderpressure.mbar;
> + int pressure = sample->cylinderpressure[0].mbar; // primary cylinder pressure
> + int diluentp = sample->cylinderpressure[1].mbar; // rebreather diluent cylinder pressure
Here you see what I mean - on the left side you have reasonable names, on
the right you have these hard coded indices.
I think I'd hate it less if it was cylinderpressure[MAINTANK] and
cylinderpressure[DILUTENT] - but I still prefer actual variable names.
> + if (setpt == lastsetpt)
> + sample->o2setpoint.mbar = 0;
> + for (j=0; j < dc->No2sensors; j++)
variables / structure or class members start with a lower case letter.
Always.
> + for (j=0; j < dc->No2sensors; j++)
> + lasto2[j] = o2value[j];
> +
> +//s1 = (int)sample->o2sensor[0].mbar;
> +//s2 = (int)sample->o2sensor[1].mbar;
> +//s3 = (int)sample->o2sensor[2].mbar;
> +//s4 = (int)sample->o2setpoint.mbar;
> +
> +//fprintf(f1,"%d gas=%8d ; dil=%8d ; depth=%8d idx=%2d sp=%8d %8d %8d %8d\n",i, sample->cylinderpressure[0].mbar, sample->cylinderpressure[1].mbar, sample->depth.mm, sample->sensor,s4, s1,s2,s3,sample->cylinderpressure[0].mbar);
> +
> +//fprintf(f1,"id t1 gas t2 t3 dil t4 t5 depth t6 cylinderid t7 setpoint sensor1 sensor2 senor3 \n");
Again, this needs to go inside #if / #endif
> diff --git a/dive.h b/dive.h
> index 50bd2df..cf0568a 100644
> --- a/dive.h
> +++ b/dive.h
> @@ -113,21 +113,24 @@ static inline depth_t gas_mod(struct gasmix *mix, pressure_t po2_limit) {
> return depth;
> +struct sample // BASE TYPE BYTES UNITS RANGE DESCRIPTION
> +{ // --------- ----- ----- ----- -----------
> + duration_t time; // int32_t 4 seconds (0-68 yrs) elapsed dive time up to this sample
> + duration_t stoptime; // uint32_t 2 seconds (0-18 h) time duration of next deco stop
> + duration_t ndl; // uint32_t 2 seconds (0-18 h) time duration before no-deco limit
Read the last three lines and tell me a few things that you notice.
Like all three members have duration_t, but allegedly one is signed 32bit
and 4 bytes, the others unsigned 32 bits but only 2 bytes.
Ummm -- what?
> + depth_t depth; // int32_t 4 mm (0-2000 km) dive depth of this sample
> + depth_t stopdepth; // int32_t 4 mm (0-2000 km) depth of next deco stop
> + temperature_t temperature; // int32_t 4 mdegrK (0-2 MdegrK) ambient temperature
> + pressure_t cylinderpressure[2];// int32_t 8 mbar (0-2 Mbar) cylinder pressure
> + uint8_t sensor; // uint8_t 1 sensorID (0-255) ID of cylinder pressure sensor
> + o2pressure_t po2; // uint16_t 2 mbar (0-65 bar) O2 partial pressure
> + o2pressure_t o2setpoint; // uint16_t 2 mbar (0-65 bar) O2 setpoint (rebreather)
> + o2pressure_t o2sensor[3]; // uint16_t 6 mbar (0-65 bar) Up to 3 PO2 sensor values (rebreather)
> + bearing_t bearing; // int16_t 2 degrees (-32k to 32k deg) compass bearing
> + uint8_t cns; // uint8_t 1 % (0-255 %) cns% accumulated
> + uint8_t heartbeat; // uint8_t 1 beats/m (0-255) heart rate measurement
> + bool in_deco; // bool 1 y/n y/n this sample is part of deco
> +}; // Total size of structure: 48 bytes, excluding padding at end
>
> struct divetag {
> /*
> @@ -201,7 +204,9 @@ struct divecomputer {
> depth_t maxdepth, meandepth;
> temperature_t airtemp, watertemp;
> pressure_t surface_pressure;
> - int salinity; // kg per 10000 l
> + const char *dctype; // Type of dive computer: 'rebreather'=rebreather
Why would this be a string in the structure? That's silly.
Have an enum with the types that we support and then translate that
to/from string in the file handling
> + int No2sensors; // rebreathers: number of O2 sensors used
See above, num_o2_sensors
> @@ -215,6 +220,8 @@ struct divecomputer {
> #define W_IDX_PRIMARY 0
> #define W_IDX_SECONDARY 1
>
> +#define REBREATHER 1
That's just pointless - defining REBREATHER here and then conditionally
compiling on this later... please remove that
> diff --git a/profile.c b/profile.c
> index 26b267d..152c850 100644
> --- a/profile.c
> +++ b/profile.c
> @@ -14,10 +14,12 @@
> #include "libdivecomputer/parser.h"
> #include "libdivecomputer/version.h"
> #include "membuffer.h"
> +#include "stdint.h"
>
> int selected_dive = -1; /* careful: 0 is a valid value */
> unsigned int dc_number = 0;
>
> +//#define DEBUG_PR_TRACK
Not something you should leave in a submission
> @@ -396,12 +398,12 @@ static void fill_missing_segment_pressures(pr_track_t *list)
> pr_track_t *tmp = list;
> int pt_sum = 0, pt = 0;
>
> - for (;;) {
> - pt_sum += tmp->pressure_time;
> + for (;;) { // Move down temp until an end pressure vaue is found
> + pt_sum += tmp->pressure_time; // Sum pressure_time
> end = tmp->end;
> - if (end)
> + if (end) // end pressure found. break
> break;
> - end = start;
> + end = start; // inspect next segment
> if (!tmp->next)
> break;
> tmp = tmp->next;
commenting existing algorithms without any code changes. Great for its own
commit
> @@ -460,10 +462,68 @@ static inline int pressure_time(struct dive *dive, struct divecomputer *dc, stru
> return depth_to_mbar(depth, dive) * time;
> }
>
> -static struct pr_interpolate_struct get_pr_interpolate_data(pr_track_t *segment, struct plot_info *pi, int cur)
> +static struct pr_interpolate_struct get_pr_dil_interpolate_data(pr_track_t *segment, struct plot_info *pi, int cur)
> +/* For rebreather dives:
> + * This function is a copy of the function get_pr_interpolate_data that has been adapted to deal with the diluent gas
> + * pressures. Parameters: cur = index to pi->entry corresponding to t_end of segment.
> + * It should be rudimentary to write a common function that deals with both diluent and the other gases by
> + * merging this function with get_pr_interpolate_data, but at present, such a common function causes
> + * interaction between the different gases. Therefore a separate function.
> + * Called by: populate_diluent_information */
Comments are above the function, not between its declaration and the body.
We definitely need to combine those, but that can happen later.
> +static void fill_missing_diluent_pressures(struct dive *dive, struct plot_info *pi, pr_track_t *track_pr)
> +/* For rebreather dives:
> + * This function is a copy of the function fill_missing_tank_pressures of which the code has been
> + * adapted to process the gas pressure in the diluent tank, fills up the pressure data for the
> + * diluent, given a pr_track_t set of structures,then calls the function that creates an interpolate
> + * structure and then does the interpolation for the empty pressures for this cylinder.
> + * Called by: populate_diluent_information */
ditto
> +static void populate_diluent_information(struct dive *dive, struct divecomputer *dc, struct plot_info *pi)
> +/* For rebreather dives:
> + * This function is a copy of the function populate_pressure_information. The code in this function has been
> + * adapted to process the gas pressure in the diluent tank. In contrast with the other cylinders whose
> + * pressure are only shown when that particular cylinder is selected (and the full set of pressures for that
> + * tank is therefore only transient), the diluent gas pressure is stored for the whole dive and displayed,
> + * with the EAN100 cylinder pressure, when the EAN100 cylinder is selected.
> + * Called by: create_plot_info_new */
> +{
ditto
> @@ -982,15 +1155,15 @@ static void populate_pressure_information(struct dive *dive, struct divecomputer
> }
>
> if (!pressure) {
> - missing_pr = 1;
> + missing_pr = 1;
> continue;
> }
> -
> current->end = pressure;
>
> /* Was it continuous? */
> - if (SENSOR_PRESSURE(entry - 1))
> + if (SENSOR_PRESSURE(entry - 1)) {
> continue;
> + }
???
> @@ -1133,11 +1308,50 @@ static void calculate_gas_information_new(struct dive *dive, struct plot_info *p
> fo2 = get_o2(&dive->cylinder[cylinderindex].gasmix);
> fhe = get_he(&dive->cylinder[cylinderindex].gasmix);
>
> +
> +#ifdef REBREATHER
you hard coded this to be 1 - so the #if is pointless
> + if ((dc->dctype) && (dc->dctype[0] == 'r')) { // for rebreathers..
NO. This needs to be a flag value with an enum type
And a nice macro IS_CCR(dc)
> + // Estimate the most reliable PO2, given the different oxygen sensor values
> + switch(dc->No2sensors) {
> + case 2: { // for 2 sensors, take the mean value of the two sensors.
> + entry->po2 = (entry->o2sensor[0] + entry->o2sensor[1])*0.5;
> + break;
> + }
> + case 3: { // For 3 sensors, take the mean of the two values which are..
> + // .. the most similar and disregard the third value.
> + // This is a brute force approach. There must be a more sophisticated implementation..
> + sensordifference[0] = abs(entry->o2sensor[0] - entry->o2sensor[1]);
> + sensordifference[1] = abs(entry->o2sensor[1] - entry->o2sensor[2]);
> + sensordifference[2] = abs(entry->o2sensor[0] - entry->o2sensor[2]);
See my comment above - this algorithm is very suspect to me.
> void create_plot_info_new(struct dive *dive, struct divecomputer *dc, struct plot_info *pi)
> {
> - int o2, he, o2low;
> + FILE *f1;
Please clean up all these leftovers from debugging sessions
> +#ifdef DEBUG_GAS
> +if(!(f1 = fopen("gas1.dat","w")) printf("File open error\n");
> +else {
> + fprintf(f1,"id t1 gas gasint t2 t3 dil dilint t4 t5 setpoint sensor1 sensor2 sensor3 t6 po2 fo2\n");
> + for(i=0; i< pi->nr; i++) {
> + entry = pi->entry + i;
> + fprintf(f1,"%d gas=%8d %8d ; dil=%8d %8d ; o2_sp= %f %f %f %f PO2= %f %f\n",i,SENSOR_PRESSURE(entry),INTERPOLATED_PRESSURE(entry),DILUENT_PRESSURE(entry),
> + INTERPOLATED_DILUENT_PRESSURE(entry),entry->o2setpoint,entry->o2sensor[0],entry->o2sensor[1],entry->o2sensor[2],entry->po2,entry->fo2);
> + }
> + fclose(f1);
> + }
> +#endif
ditto
> diff --git a/profile.h b/profile.h
> index 54d250a..d3e9022 100644
> --- a/profile.h
> +++ b/profile.h
> @@ -83,7 +85,9 @@ int get_maxdepth(struct plot_info *pi);
> #define SENSOR_PR 0
> #define INTERPOLATED_PR 1
> #define SENSOR_PRESSURE(_entry) (_entry)->pressure[SENSOR_PR]
> +#define DILUENT_PRESSURE(_entry) (_entry)->diluentpressure[SENSOR_PR]
> #define INTERPOLATED_PRESSURE(_entry) (_entry)->pressure[INTERPOLATED_PR]
> +#define INTERPOLATED_DILUENT_PRESSURE(_entry) (_entry)->diluentpressure[INTERPOLATED_PR]
catchy - and so easy to type
> diff --git a/units.h b/units.h
> index 0a9e44b..f2aeebe 100644
> --- a/units.h
> +++ b/units.h
> @@ -59,22 +59,32 @@ typedef int64_t timestamp_t;
>
> typedef struct
> {
> - int seconds;
> + int32_t seconds; // durations up to 68 yrs
> } duration_t;
>
> typedef struct
> {
> - int mm;
> + int32_t mm;
> } depth_t;
>
> typedef struct
> {
> - int mbar;
> + int32_t mbar; // pressure up to 2000 bar
> } pressure_t;
>
> typedef struct
> {
> - int mkelvin;
> + uint32_t mbar;
> +} o2pressure_t; // pressure up to 65 bar
No, that's a uint32, did you want 16 bits?
My suggestion:
Go through these comments. They give you a pretty clear roadmap to a
handful of things that really ought to be their own commits. And it seems
like that would be fairly easy to do.
Some of the other changes suggested will be more work, but they are
required for this to get accepted into master
Thanks
/D
More information about the subsurface
mailing list