Real support for multiple concurrent sensors..

[Dirk Hohndel]

> On Jul 27, 2017, at 12:43 PM, Linus Torvalds <torvalds at linux-foundation.org> wrote:
> 
> 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.

Then what's the path forward?
Further down you say

> 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.

In my experience the best way to get things tested is if I include them in
test binaries - but that would require a signed off patch that you are 
comfortable to have me include in master... which of course would also
impact the beta for Subsurface-mobile that is crawling along in parallel.

But given that there clearly is more work to be done before I'll be 
comfortable to release Subsurface-mobile 2.0, I think having it in master
and getting it tested that way may be the best course of action.

> 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.

Those are the cases that I remember - we had a ton of problems there,
especially (but not only) when the order of tanks was different between
the dive computers. I know that we were at the Yellow House doing
tech dives (I'm guessing this was the trimix class) where I had the gas
order differently between the different computers and that caused all
kinds of issues.

> 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.

Great.

> 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.

That would look odd

> 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.

But couldn't we use the gas mix to get this right?
Because even if an analysis of the gases gave you slightly different
(or, frankly, radically different) gases, doing a "no gas switch" side
mount session only makes sense if we assume that the gasses
between cylinder 1 and 2 (in your example) is the same.

> 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".

That's what I mean - but I actually would much rather have this
explicit. Because I just KNOW that we'll get a diver who will try to
do things right by having their two side mount tanks marked as
EAN31 and EAN32, hoping somehow that this gives us "better"
data, using consumption or what-not.

> 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.

What would be the advantage of this over the idea of marking
cylinders in the equipment list as "side mount set"?

> 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.

Please just resend it with the commit message of your choice
and an SOB and I'll create new builds

/D