Real support for multiple concurrent sensors..

[Dirk Hohndel]

On Wed, Jul 26, 2017 at 09:57:27PM -0700, Linus Torvalds wrote:
> 
> Ok, I've actually spent a fair amount of time on this patch, and it 
> actually seems to work.

Always a plus.

> It really handles multiple cylinder pressures, both overlapping and 
> consecutive, and it seems to work on the nasty cases I've thrown at it. 
> Want to just track five different cylinders all at once, without any 
> pesky gas switch events? Sure, you can do that. It will show five 
> different gas pressures for your five cylinders, and they will go down as 
> you breathe down the cylinders.

That seems extremely useful

>  - actually make the "struct plot_info" have all the cylinder pressures 
>    (so no "sensor index and pressure" - every cylinder has a pressure for
>    every plot info entry)
> 
>    This obviously makes the plot_info much bigger. We used to have 
>    MAX_CYLINDERS be a fairly generous 8, which seems sane. The planning 
>    code made that 8 be 20. That seems questionable. But whatever.

It did so for a really good reason. I didn't necessarily like that change
and it's consequences, but otherwise replanning wouldn't have worked in
some cases that were relevant to our tech divers.

>    The good news is that the plot-info should hopefully get freed, and 
>    only be allocated one dive at a time, so the fact that it is big and 
>    nasty shouldn't be a scaling issue, though.

So, technically, on Subsurface-mobile we can have more than one plot in
flight. I need to make sure that these are indeed freed the moment we are
finished drawing the profile - but with the cashing of dives on "either
side" of the current dive, we tend to have at least three (and I have seen
five) profiles in parallel.

>  - the "populate_pressure_information()" function had to be rewritten 
>    quite a bit. The good news is that it's actually simpler now, although 
>    I would not go so far as to really call it simple. It's still 
>    complicated and suble, but now it explicitly just does one cylinder at 
>    a time.
> 
>    It *used* to have this insanely complicated "keep track of the pressure 
>    ranges for every cylinder at once". I just couldn't stand that model 
>    and keep my sanity, so it now just tracks one cylinder at a time, and 
>    doesn't have an array of live data, instead the caller will just call 
>    it for each cylinder.

Oh good. I think that old code was fairly impenetrable...

>  - there's no sane way to do this as a series of small changes. 
> 
>    The change to make the plot_info take an array of cylinder pressures 
>    rather than the sensor+pressure model really isn't amenable to "fix up 
>    one use at a time". When you switch over to the new data structure 
>    model, you have to switch over to the new way of populating the 
>    pressure ranges. The two just go hand in hand.

OK. Big nasty patch. Got it.

>  - Some of our code *depended* on the "sensor+pressure" model. I fixed all 
>    the ones I could sanely fix. There was one particular case that I just 
>    couldn't sanely fix, and I didn't care enough about it to do something
>    insane.
> 
>    So the only _known_ breakage is the "TankItem" profile widget. That's 
>    the bar at the bottom of the profile that shows which cylinder is in
>    use right now. You'd think that would be trivial to fix up, and yes it 
>    would be - I could just use the regular model of
> 
>      firstcyl = explicit_first_cylinder(dive, dc)
>      .. then iterate over the gas change events to see the others ..
> 
>    but the problem with the "TankItem" widget is that it does its own 
>    model, and it has thrown away the dive and the dive computer 
>    information. It just doesn't even know. It only knows what cylinders 
>    there are, and the plot_info. And it just used to look at the sensor 
>    number in the plot_info, and be done with that. That number no longer 
>    exists.

That's a pity. I love that bar. I think I implemented it. It would seem to
me that the dive computer (for its calculations) still has to know which
gas you are currently breathing. i.e., it would be invalid to do the
"random switching between cylidners" with cylinders of different gas. And
that makes me think that we should still be able to draw that bar. But
I'll have to stare at your changes a bit more to figure out why that
doesn't work.

>  - I have tested it, and I think the code is better, but hey, it's a 
>    fairly large patch to some of the more complex code in our code base. 
>    That "interpolate missing pressure fields" code really isn't pretty. It 
>    may be prettier, but..
> 
> Anyway, without further ado, here's the patch. No sign-off yet, because I 
> do think people should look and comment. But I think the patch is fine, 
> and I'll fix anythign that anybody can find, *except* for that TankItem 
> thing that I will refuse to touch. That class is ugly. It needs to have 
> access to the actual dive.

It may have to be redesigned. Let me take a look at that.

> Note how it actually does remove more lines than it adds, and that's 
> despite added comments etc. The code really is simpler, but there may be 
> cases in there that need more work.

With this pesky day job of mine I'm not sure how quickly I'll be able to
do a solid review of this code. I'll add it to the top of my non-work,
non-buying-a-house priority list.

/D