dive planner fixes [was: Re: source code may be getting a bit intimidating]

[Linus Torvalds]

On Mon, Jan 7, 2013 at 1:41 PM, Dirk Hohndel <dirk at hohndel.org> wrote:
>
> So I'll do it hear with just cut and paste copies of your patch:
>
>       -         for (i = t; i < dp->time; i += 10) {
>       -                 depth = lastdepth + (i - t) * (dp->depth - lastdepth) / (dp->time - t);
>       -                 sample = prepare_sample(dc);
>       -                 sample->time.seconds = i;
>       -                 sample->depth.mm = depth;
>       -                 sample->sensor = gasused;
>       -                 dc->samples++;
>
> I assume you are removing the intermediate samples because with your
> change to the plot_info this isn't needed for things to look good...
>
> But this has an interesting side effect. We do the deco calculation
> based on the samples here, while profile.c does it based on the
> plot_info. While I think that shouldn't matter, I haven't fully
> convinced myself that this doesn't cause issues.

Hmm. I guess we can add it back.

I actually removed it mainly because the code was doing bad things,
like that silly gasused thing (what does the sensor have to do with
anything?) and the dc->samples++. And I was thinking that the
sample->sensor setting might have had something to do with the gas
confusion, so removing code that looked redundant was a priority.

But putting it back as just

    for (i = t; i < dp->time; i += 10) {
        depth = lastdepth + (i - t) * (dp->depth - lastdepth) / (dp->time - t);
        sample = prepare_sample(dc);
        sample->time.seconds = i;
        sample->depth.mm = depth;
        finish_sample(sample);
    }

shouldn't hurt.

That said, I wonder if maybe the deco code could just use the plot
samples. That has some other advantages, like the fact that the plot
samples have all the correct gas switches already calculated etc.

And it really *would* be nice if the planned dive actually was just
the minimally required samples, because then we could just print out
the result by looking at the samples. Or, even more importantly,
*modify* an existing plan by re-populating it from the samples etc.

So I do think that "samples == waypoints" would be really good in the
end. But if the deco code needs more samples, then I guess we don't
have much choice.

> This calls show_planned_dive even if the gas didn't validate with a
> default of AIR.

So showing the planned dive even if something doesn't validate is
actually nice. In *particular* for the gas entries, because it means
that you can just leave them empty, and it will use the previous gas.

>Let's say someone has 20/30
> in the diveplan, then edits it to read "20^30" by mistake. Should that
> switch to AIR and recalculate and redisplay the dive? Or should this
> simply change the color of the cell but keep the old value and not
> redraw the profile?

So what it does now is to use the last valid gas, and complain about
it, so that the user can fix it. I don't think you can conceptually do
any better.

But the "compain about it", of course, could be better and a proper
GUI thing (rather than the printf it is now - that's not new, though).
I think turning the background a different color would be lovely, but
that requires gtk knowledge again ;)

And it doesn't actually default to air at all. It most specifically
defaults to "last gas", and only then if there are no gases at all
will that happen to be air.

> So any feedback on the above? Can I get a signed-off patch with a commit
> message?

The message could be something like:

  Simplify dive planning code

  This simplifies the dive planning code by:

   - allowing empty gas mixes (which means "pick previous gas")

   - avoiding unnecessary strdup/free calls (this requires us to
handle "const char *" in the parsers, but that was already true from a
code standpoint, just not a type one)

   - re-use the "plan()" function for a successful dive plan, rather
than open-coding the dive plan segment handling.

  Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>

Hmm?

             Linus