Real support for multiple concurrent sensors..

[Linus Torvalds]

On Thu, Jul 27, 2017 at 11:31 AM, Dirk Hohndel <dirk at hohndel.org> wrote:
>
> So far I haven't seen any substantial feedback on your code changes,
> I'm hoping to get a few more people to actually read through the patch...

I'd like more people to look at it, but at the same time I suspect they won't.

Some of the code looks fairly straightforward, and is much more subtle
than you'd think.

For an example of the kinds of issues I hit with that code:

 - let's say that you do *not* have a pressure sensor on your
cylinder, so all you have are the manual beginning/ending pressures
and the gas change events for that cylinder

 - Fine, we populate just the beginning and ending pressure with the
manual pressures (at the first and last plot entry matching the gas
change events), and then interpolate.

 - but the interpolation code also looks at the gas changes to decide
the range on when to *not* interpolate, and now that we actually track
multiple pressures, we do *not* want to show the cylinder pressure of
a cylinder that we have explicitly switched away from.

 - There's a nasty border condition wrt gas changes: when you switch
from one gas to another, the point of the switch actually wants to
contain *both* pressures. You don't just clear the old and set the new
- you set the new one and then you clear the old one in the *next*
plot entry.

I had a couple off-by-one errors like that, and I think they are all
correct in that patch I sent out, but those kinds of issues are really
hard to see by just reading the patch.

So even more than patch review, it would be really good for people to
just test it, particularly on interesting dives with more than one
cylinder.

Of course, those are also the dives that we had various hacks in place
for before (like the "set sensor ID based on gas switches in dive
fixup").

So I'm a bit worried that some of those hacks ended up giving the
right end result for bad data, and now that we actually *trust* (and
use) the sensor index information we get, we end up showing something
incorrectly, simply because the old data was bogus and depended on the
fixups.

I *think* the craziest dive fix-ups were all for the nasty cases where
we merged dives from multiple dive computers, and didn't do the
cylinder indexing right.

And I think past fixups should then have resulted in the final dive
being saved with the right data, and we're all good now.

But that's actually the thing that worried me most: the new code
fundamentally depends on the sensor information in a way that the old
code did not. In many ways, the old code largely ignored the sensor
number entirely, and then "fixed things up" to look right.

And that's kind of fundamental. When you only track one single
pressure (and the o2pressure was really entirely different), the
sensor index really ends up not so much being about the *pressure*, as
it's about *currently selected cylinder". Which is what that TankItem
thing used it as - it didn't even care about the pressure, and it
didn't care about the fact that the sensor index was meaningless (and
basically ignored by other code) when there was no pressure data.

So we had all that hacky code that took the stuff we loaded from the
sample data, and changed the sensor index, and cleared the old
pressure reading if it did so, and just generally had all these hacks
that worked exactly because it really didn't matter that much.

The TankItem thing was just the most obvious one. There might be some
other things like that lurking.

Also, there are a few things that never worked and that I'd like to
make work, like just making the "Info window" show all the pressures
that we're tracking at that point. Right now it only shows the first
cylinder in the info window.

So there's a few more changes to expose things we can now do and we
previously just couldn't. The code isn't "done" - it's just that now I
think all the big lifting is done, and it really is "oh, we can show
more/better information".

Some things probably also do need actual new UI. For example, let's
say that you really *are* diving side-mount, and you have a deco
bottle. So you have three cylinders: two for travel gas, one for deco.
Let's say that your sidemount is cylinders 1/2 and your deco is
cylinder 3.

Right now, it would show up as "you start out breathing the first
cylinder, and then you finish by switching to the third one". What
about the second one? Because it's not mentioned in the gas changes at
all, right now the profile will show the pressure information kind of
oddly:

 - cylinder 1 pressure is shown from beginning to the deco gas switch

 - cylinder 3 pressure is shown from the deco gas switch to the end

 - cylinder 2 pressure is shown for the whole dive, because there are
no gas changes for it.

It's not *wrong* - the sensor data will be correct, and cylinder 2
will just flatten out at the end. But if you didn't have sensors, and
instead just filled in the beginning/ending pressures by hand? Then
we'll *interpolate* the cylinder 2 pressures from beginning to end
too, so it won't even be flattening out, the code just thinks that
it's being breathed the whole time.

No, it's not the end of the world, but at some point we probably would
want a "these two cylinders work together" model. I could hack
something up that just says "there are no gas switches for this
cylinder, but it has the same gas mix as that other cylinder, so I'll
just assume it's a sidemount and shares the same gas switches".

Or maybe we should just make our own internal gas switch model be a
bitmap ("switch to this _set_ of cylinders"), and you can just edit
that instead. That might be the right thing to do.

None of this affects the current patch, I'm just explaining that while
the core code now handles multiple concurrent gases, there might be
old hacks that people relied on, and there will probably be new issues
that people will want to handle that just weren't an issue before.

I have sprivately igned off on that patch in my local git tree,
because I do think it's ok, but I repeat: testing on real data would
be good.

                  Linus