understanding the sensor values in our samples

[Linus Torvalds]

On Wed, Apr 27, 2022 at 2:27 PM Dirk Hohndel <dirk at hohndel.org> wrote:
>
> Well, turns out that both in load-git.c and parse.c we have essentially this code:
>
>                 sample->sensor[0] = sanitize_sensor_id(state->active_dive, !state->o2pressure_sensor);
>                 sample->sensor[1] = sanitize_sensor_id(state->active_dive, state->o2pressure_sensor);
>
> But, if we don't have an o2 pressure sensor, why would we put a valid cylinder number there?
> That sensor should say "NO_SENSOR" (or -1) unless we have actually identified an O2 sensor...

So Michael's answer to this was largely correct, but the real issue is
that the code is confused and has historical reasons for being so.

The historical reasons is that OC and CCR had a very different notion
of how sensors worked.

We *used* to have

 - one single pressure reading, and a separate index to say "this is
the sensor number for that reading"

 - CCR had *two* different pressure readings, one for diluent and one for o2.

and then we started having dive computers that could actually have
multiple concurrent sensor readings, and so instead we now have that

        pressure_t pressure[MAX_SENSORS];

and that *should* be all that matters. The whole "sensor[]" array
shouldn't exist at all, and "MAX_SENSORS" should be "MAX_CYLINDERS",
and we sh ould never have those odd sensoir indexes at all.

And there shouldn't be any difference for OC and CCR, because you'd be
able to see which pressure is the O2 pressure by just looking at which
cylinder you have defined as the O2 cylinder.

So that is what a perfect world looks like.

But that is *not* what we ended up with ;(

Instead, we ended up with a horrible mess where

 - sensor numbering is independent from cylinder numbering

 - the 'sensor[]' array allegedly maps one to the other

 - CCR dives say "this sensor is the o2 sensor" in the save format,
because that's how they always worked

and the important things to take away from this is:

 - it's all very confused and complex.

 - that "allegedly" is important, I think it gets confused in multiple places

 - it doesn't help that there's also three 'o2sensor' values, which
has nothing to do with the 'sensor[]' values, and nothing to do with
tank pressures, but are the O2 partial pressure sensor values for CCR.
I wish this was called something else.

So I really would like to get rid of that 'sensor[]' array entirely.
But the above hopefully gives you some initial background.

Then, to actually explain those two lines you quote, you need a few more hints:

 (1) when loading a dive from XML or git save, we start with the
default assumption that sensor 0 is diluent, and sensor 1 is O2. So
look at parse_dive_entry(), and you will find this line

        state->o2pressure_sensor = 1;

 (2) When er parse cylinders, and find a pure oxygen cylinder listed,
we will fix it up:

        if (cylinder.cylinder_use == OXYGEN)
                state->o2pressure_sensor = state->active_dive->cylinders.nr;

so now o2pressure_sensor will point to the oxygen cylinder.

And now, finally, you are ready to read those two lines:

        sample->sensor[0] = sanitize_sensor_id(state->active_dive,
!state->o2pressure_sensor);
        sample->sensor[1] = sanitize_sensor_id(state->active_dive,
state->o2pressure_sensor);

because they didn't really say what you thought they said.

What those two lines say is:

 - sensor 0 is for cylinder 0, *except* if cylinder zero is our O2
cylinder, in which case it's for cylinder 1

 - sensor 1 is for cylinder 1, *but* will be the O2 cylinder if we had one

(and then that sanitize_sensor_id() just says that we'll never make a
sensor ID point to a cylinder we don't even have,

So basically "!state->o2pressure_sensor" is the expression for that
"cylinder 0, except if zero was our O2 sensor".

And state->o2pressure_sensor is "1, or our O2 cylinder".

Is this confusing? It sure as hell is.

Do we have bugs in this area? I'm quite sure we do.

               Linus