[PATCH] Re: Minor planner bugs

Wed Oct 21 00:59:59 PDT 2015

Hi,

I have to admit, I am totally lost here.

Here is what I guess I figured out:

First of all: The error message “Too many gas mixes” is totally misleading. It is triggered in the planner when verify_gas_exists() fails. Having too many gases is one possibility but that is not what happens here. It also fails when it cannot find the gas in the gas list and when searching it is ignoring cylinders if cylinder_nodata() returns true (i.e. uninitialized cylinders. One trivial (but I guess in the end wrong) way to get rid of the message would be delete the test for cylinder_nodata() and proceed like elsewhere interpreting a totally zeroed out cylinder as an air cylinder of unspecified size. But that would be dealing with symptoms.

In any case, the wording of the meassage needs to be changed.

> On 21.10.2015, at 08:32, K. pestophagous Heller <pestophagous at gmail.com> wrote:
> 
> On Tue, Oct 20, 2015 at 1:04 AM, Robert Helling <helling at atdotde.de <mailto:helling at atdotde.de>> wrote:
>> Hi,
>> 
>> On 19.10.2015, at 08:11, Martin Měřinský <mermar at centrum.cz> wrote:
>> 
>> Hi.
>> I have found a few minor bugs in 4.5.0 planner. I mentioned it in
>> "Proposal on new planner layout" thread, but was asked to send it
>> separately.
>> 
>> Start Subsurface (no log file).
>> Ctrl+l (Log -> Plan dive).
>> I get warning "Too many gas mixes".
>> Warning text is there twice.
>> It doesn't make sense, because I have nothing in Available gases.

Martin, I see from your video that in your case (as I said, not for me and this is why I cannot debug this) the planner comes up without any gas in the gas list. That simply should not happen and void DivePlannerPointsModel::setupCylinders() should make sure this does not happen.

It does so by first trying to copy the gases from the current dive (when invoking the planner). Since you start from an empty dive list, this should not do anything, as current_dive should be NULL.

Then it checks if you have a default cylinder and if yes it adds that.

Could you please check in the preferences if you have configured a default cylinder. If you do, what is it? Could you try if changing it to some other value from the list solves this problem?

If there is no default cylinder, it should add an “unknown” cylinder of 11,1 liter size and 207 bar of air.

In any case, after that, there should be a cylinder.

Could you use a debugger to set a breakpoint at the start of that function and step through it to figure out how you can end up without a gas? If you don’t have access to a debugger or don’t know how to use it, I could send you a patch that adds printf’s in all kinds of places to determine the code path in your case, so let me know.

Besides that, I have no idea what is going on.

>> 
>> Close the warning.
>> Hit plus button, to add gas.
>> I get warning "Too many gas mixes" again.
>> 
>> When going to plan dive, there is automatically one dive planner point,
>> but no gas. I would suppose to have one gas as well.
>> 
>> 
>> I could not reproduce that behavior. But looking at the code, there might be
>> the case that there is a current dive but that that dive does not have any
>> cylinder. Here is a patch that is supposed to catch that case and add a
>> default cylinder instead. Could you please test that this patch solves this
>> problem?
> 
> I can provoke the "Too many gas mixes" warning, but for me it doesn't
> happen right away.
> 
> What I am doing to trigger the "Too many gas mixes" warning is
> definitely different than what Martin is doing.  Ideally i would
> prefer to reproduce Martin's bug report just as it was originally
> reported.  Meanwhile I do not want to "steal" this thread and make it
> about my own crazy steps to reproduce. Hopefully some of what follows
> will be of use...
> 
> I have a partial diagnosis for what is broken when I see "Too many gas mixes".
> 
> Perhaps some of my partial diagnosis will end up helping us solve
> Martin's case, too.
> 
> The steps that led me to "Too many gas mixes" happened while I was
> trying to reproduce this other bug:
> http://trac.subsurface-divelog.org/ticket/745 <http://trac.subsurface-divelog.org/ticket/745>

K Pestophagus, again, I tried to reproduce what you did in the video. I can confirm that the waypoints behave strangely if they get moved past each other (they change identity) and you can end up with two waypoints of different depth at the same time but I could not make them do anything more strange. In particular I always ended up with a more or less reasonable dive (possibly with instant ascents or descents). I never got the warning about too many gases.

> 
> When I see "Too many gas mixes", there are two problems:
> 
>   1. violated assumptions about 'divedatapoint::time' of 0 having
> special meaning.
>   2. an apparently unintended cascade of Qt signals/slots leading to
> ProfileWidget2::plotDive getting called while "in the middle of
> switching dives"
> 
> Concretely, you can see which assumptions are violated by looking at
> this small commit:
> 
>   https://github.com/pestophagous/subsurface/commit/6f2532fa8bf7215bba6250d64e13f16bbb7bb8e5 <https://github.com/pestophagous/subsurface/commit/6f2532fa8bf7215bba6250d64e13f16bbb7bb8e5>
>   (^^ code is clearer than my prose. look. ^^)
> 
> All four places in that commit where i added FASSERT (in my local
> codebase) show where things are going wrong leading up to the "Too
> many gas mixes" warning.  Note the wording i used for one assert in
> verify_gas_exists: "totally null cylinder list. displayed_dive seems
> problematic."  That matches Robert's hypothesis exactly.
> 
> For my path through the code (inspired by the videos by Poltsi at Trac
> ticket 745), Robert's patch to
> 'DivePlannerPointsModel::setupCylinders' does not make any difference.
> The reason it makes no difference has to do with the "cascade of Qt
> signals/slots" that I mentioned.
> 
> When I click "Apply changes", i end up in MainTab::acceptChanges.
>> From there, the following (and more) happens:
> 
>   clone_dive
>   record_dive
>   MainWindow::instance()->dive_list()->unselectDives();
>   MainWindow::instance()->dive_list()->reload
> 
> (MainTab::acceptChanges is a big function, and those four lines are
> only a small fraction of what it does.)
> 
> The clone_dive and record_dive calls result in the correct dive being
> added to the log.
> 
> PROBLEM: dive_list()->unselectDives() triggers slots, and makes
> ProfileWidget2::plotDive get called, which calls create_dive_from_plan
> .... but "displayed_dive" is a totally zeroed-out dive. (it just got
> zeroed by clone_dive).

I think this is a valid analysis. We had problems at other places when a replot was triggered while in the middle of some operation. For this there is

	MainWindow::instance()->graphics()->setReplot(false);

which prevents replotting until called again with true as argument. Could you try to add that around the code you identified in MainTab::AcceptChanges to see if it prevents the warning?

Best
Robert

--
.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oO
Robert C. Helling     Elite Master Course Theoretical and Mathematical Physics
                      Scientific Coordinator
                      Ludwig Maximilians Universitaet Muenchen, Dept. Physik
                      Phone: +49 89 2180-4523  Theresienstr. 39, rm. B339
                      http://www.atdotde.de

Enhance your privacy, use cryptography! My PGP keys have fingerprints
A9D1 A01D 13A5 31FA 6515  BB44 0820 367C 36BC 0C1D    and
DCED 37B6 251C 7861 270D  5613 95C7 9D32 9A8D 9B8F

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20151021/dff1c856/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20151021/dff1c856/attachment-0001.sig>