Re: [PATCH 2/2] Properly clear sensor pressure data for synthetic plotinfo entries

[Linus Torvalds]

On Fri, Oct 6, 2017 at 10:46 AM, Stefan Fuchs  wrote:
>
> Could you please have a quick look at this:
> https://github.com/Subsurface-divelog/subsurface/pull/628

That looks right.

Alternatively, if the planner really wants to set pressures by hand
(I'm not sure why it would want to, but whatever) maybe that

sample->pressure[0].mbar = cyl->sample_end.mbar;

line should do

add_sample_pressure(sample, dp->cylinderid, cyl->sample_end.mbar);

which at least gets the cylinder ID right. That may have been the
problem with the old code, it just set the pressure without ever
setting the cylinder ID.

NOTE! There's three of those

sample->pressure[0].mbar = ...

lines in the planner there, and you only removed one (the "middle
case"). The two others should probably also either be removed or made
to use that add_sample_pressure() helper.

I'm not sure why the planner adds those sample pressures at all, since
the profile logic should just "do the right thing" based on gas change
events and the beginning/ending pressures of the cylinders.

But there may be valid reasons why the planner does this all, I don't
know the planner code at all.

 Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: [PATCH 2/2] Properly clear sensor pressure data for synthetic plotinfo entries

[Stefan Fuchs]

Hi Linus,

Am 20.09.2017 um 10:16 schrieb Stefan Fuchs:
>> On Fri, Sep 15, 2017 at 5:31 AM, Stefan Fuchs  wrote:
>>> Sorry for the maybe stupid questions: This does already solve the issue with
>>> the spike I reported, isn't it?
>> Maybe. I'm not convinced it didn't exist before, though, although
>> maybe hidden. It definitely existed as far as the o2pressure was
>> concerned, since that wasn't cleared before either.
> I can still create one interesting situation where I finally will have
> the issue (all related files are attached):
> - I plan a new dive of the specific kind (deco gas used as first gas
> and then used later again) with the planner and save it to a new XML file
>   --> Everything ok, no vertical line in graph, in XML at time of gas
> change 43:35 there is no pressure value. This is as Linus said it
> should be.
> - I close Subsurface, restart, open XML
>   --> Everything ok, no vertical line in graph
> - Replan dive in planner
>   --> Vertical line at ~43min appears and if I now save this and look
> in the XML a pressure value was added at 43:35 which is incorrect.
>
> The question is now: Why does the planner (or whatever) does the thing
> correctly when creating the dive but then does a mistake when
> replanning the same dive?
Could you please have a quick look at this:
https://github.com/Subsurface-divelog/subsurface/pull/628

I think I identified the position in the code where the planner writes
this ambiguous and unnecessary pressure information which confuses the
plot code.
And I even think that we can simply fix this w/o side effects. At least
tests on my side show positive results.

What do you think?

Best regards
Stefan


-- 

Stefan Fuchs
E-Mail: sfu...@gmx.de 

___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: [PATCH 2/2] Properly clear sensor pressure data for synthetic plotinfo entries

[Robert Helling]

Stefan,

> On 20. Sep 2017, at 10:16, Stefan Fuchs  wrote:
> 
> The question is now: Why does the planner (or whatever) does the thing 
> correctly when creating the dive but then does a mistake when replanning the 
> same dive?

both these issues (this and the other mail) look like they require somewhat 
painful debugging. I will look into these as soon as I have a continuous hour 
for subsurface.

Best
Robert


signature.asc
Description: Message signed with OpenPGP
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: [PATCH 2/2] Properly clear sensor pressure data for synthetic plotinfo entries

[Stefan Fuchs]

Hi Linus, hallo Robert,

sorry, me again regarding the wrong pressure data graph or let's better
say wrong pressure data entry in the XML file where Linus did a pretty
extensive analysis.

Am 15.09.2017 um 20:22 schrieb Linus Torvalds:
> On Fri, Sep 15, 2017 at 5:31 AM, Stefan Fuchs  wrote:
>> Sorry for the maybe stupid questions: This does already solve the issue with
>> the spike I reported, isn't it?
> Maybe. I'm not convinced it didn't exist before, though, although
> maybe hidden. It definitely existed as far as the o2pressure was
> concerned, since that wasn't cleared before either.
I can still create one interesting situation where I finally will have
the issue (all related files are attached):
- I plan a new dive of the specific kind (deco gas used as first gas and
then used later again) with the planner and save it to a new XML file
  --> Everything ok, no vertical line in graph, in XML at time of gas
change 43:35 there is no pressure value. This is as Linus said it should be.
- I close Subsurface, restart, open XML
  --> Everything ok, no vertical line in graph
- Replan dive in planner
  --> Vertical line at ~43min appears and if I now save this and look in
the XML a pressure value was added at 43:35 which is incorrect.

The question is now: Why does the planner (or whatever) does the thing
correctly when creating the dive but then does a mistake when replanning
the same dive?


Best regards
Stefan

-- 

Stefan Fuchs
E-Mail: sfu...@gmx.de 








  divbSubsurface (4.6.4.842) Plan/b erstellt am 
20.09.17/bbrRuntime: 
75minbr/divtabletheadtrth/ththTiefe/thth
 style='padding-left: 10px;'Dauer/thth style='padding-left: 
10px;'Runtime/thth style='padding-left: 10px; float: 
left;'Gas/th/tr/theadtbody style='float: 
left;'trtd style='padding-left: 10px; float: 
right;'#10136;/tdtd style='padding-left: 10px; float: 
right;' 15m/tdtd style='padding-left: 10px; float: right;'  
5min/tdtd style='padding-left: 10px; float: right;'  
5min/tdtd style='padding-left: 10px; color: red; float: 
left;'bEAN50/b/td/trtrtd 
style='padding-left: 10px; float: right;'#10137;/tdtd 
style='padding-left: 10px; float: right;' 15m/tdtd 
style='padding-left: 10px; float: right;'  5min/tdtd 
style='padding-left: 10px; float: right;' 10min/tdtd 
style='padding-left: 10px; color: red; float: 
left;'b(21/35)/b/td/trtrtd 
style='padding-left: 10px; float: right;'#10136;/tdtd 
style='padding-left: 10px; float: right;' 45m/tdtd 
style='padding-left: 10px; float: right;'  5min/tdtd 
style='padding-left: 10px; float: right;' 
15min/tdtdnbsp;/td/trtrtd 
style='padding-left: 10px; float: right;'#10137;/tdtd 
style='padding-left: 10px; float: right;' 45m/tdtd 
style='padding-left: 10px; float: right;' 25min/tdtd 
style='padding-left: 10px; float: right;' 
40min/tdtdnbsp;/td/trtrtd 
style='padding-left: 10px; float: right;'#10138;/tdtd 
style='padding-left: 10px; float: right;' 21m/tdtd 
style='padding-left: 10px; float: right;'  4min/tdtd 
style='padding-left: 10px; float: right;' 
44min/tdtdnbsp;/td/trtrtd 
style='padding-left: 10px; float: right;'-/tdtd 
style='padding-left: 10px; float: right;' 21m/tdtd 
style='padding-left: 10px; float: right;'  5min/tdtd 
style='padding-left: 10px; float: right;' 49min/tdtd 
style='padding-left: 10px; color: red; float: 
left;'bEAN50/b/td/trtrtd 
style='padding-left: 10px; float: right;'-/tdtd 
style='padding-left: 10px; float: right;'  9m/tdtd 
style='padding-left: 10px; float: right;'  6min/tdtd 
style='padding-left: 10px; float: right;' 
55min/tdtdnbsp;/td/trtrtd 
style='padding-left: 10px; float: right;'-/tdtd 
style='padding-left: 10px; float: right;'  6m/tdtd 
style='padding-left: 10px; float: right;' 14min/tdtd 
style='padding-left: 10px; float: right;' 69min/tdtd 
style='padding-left: 10px; color: red; float: 
left;'bSauerstoff/b/td/trtrtd 
style='padding-left: 10px; float: right;'#10138;/tdtd 
style='padding-left: 10px; float: right;'  0m/tdtd 
style='padding-left: 10px; float: right;'  6min/tdtd 
style='padding-left: 10px; float: right;' 
75min/tdtdnbsp;/td/tr/tbody/tablebrdivCNS:
 65%brOTU: 112br/divdivDekomodell: VPM-B mit 
Konservatismus +2, effektive GF=75/75brOberflächendruck: 1013mbar 
(0m)br/divdivGasverbrauch (basierend auf AMV 
18|15ℓ/min):br854ℓ/77bar von span style='color: 
red;'bEAN50/b/span (474ℓ/44bar im berechneten 
Aufstieg)br3017ℓ/131bar von span style='color: 
red;'b(21/35)/b/span (211ℓ/9bar im berechneten 
Aufstieg)brnbsp;mdash; span style='color: 
red;'Minimum gas/span (based on 3.3xSAC/+4min@45m): 
2008ℓ/84barbr432ℓ/59bar von span style='color: 
red;'bSauerstoff/b/span (432ℓ/61bar im berechneten 
Aufstieg)br/div
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  
  










  divbSubsurface (4.6.4.842) Plan/b erstellt am 
20.09.17/bbrRuntime: 
75minbr/divtabletheadtrth/ththTiefe/thth
 style='padding-left: 10px;'Dauer/thth style='padding-left: 
10px;'Runtime/thth style='padding-left: 10px; float: 

Re: [PATCH 2/2] Properly clear sensor pressure data for synthetic plotinfo entries

[Linus Torvalds]

On Fri, Sep 15, 2017 at 5:31 AM, Stefan Fuchs  wrote:
>
> Sorry for the maybe stupid questions: This does already solve the issue with
> the spike I reported, isn't it?

Maybe. I'm not convinced it didn't exist before, though, although
maybe hidden. It definitely existed as far as the o2pressure was
concerned, since that wasn't cleared before either.

> What is clear is that the fix only helps for any newly planned dives after
> applying the fix.

Hmm. Does the planner read back the sample from the plot-info to read
the interpolated values to fill in the samples? I don't think it does,
so it *shoulnd't* be affected by the bug except possibly as it shows
it.

Or maybe it does fill in the planned dive samples with the intepolated
pressures from the plot into, and I'm just not seeing where it does
it. In that case, then yes, the bug would affect a few interpolated
pressure points, and then feed back into the samples that the planner
creates for the planned dive.

 Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: [PATCH 2/2] Properly clear sensor pressure data for synthetic plotinfo entries

[Stefan Fuchs]

Hi Linus,


Am 14.09.2017 um 21:20 schrieb Linus Torvalds:
> From: Linus Torvalds <torva...@linux-foundation.org>
> Date: Thu, 14 Sep 2017 12:14:33 -0700
> Subject: [PATCH 2/2] Properly clear sensor pressure data for synthetic 
> plotinfo entries
>
> We only cleared the first sensor data when we created new synthetic plot
> info entries, because we only used to have one (well, we had the o2
> data, but apparently nobody ever noticed that it didn't get properly
> interpolated, probably because people who have CCR dives with o2
> pressures are few, and the pressure drops are gradual anyway).
>
> Clear all the pressure data, so that the interpolation code doesn't
> think we have some existing real sensor data for the plot info entries
> in between proper sample entries.
>
> Signed-off-by: Linus Torvalds <torva...@linux-foundation.org>
> ---
>
> This fixes the very odd "sometimes interpolation doesn't work". 
Sorry for the maybe stupid questions: This does already solve the issue
with the spike I reported, isn't it?

I first was thinking no, this now is only taking care about s.th.
additional you discovered during your investigations.
But after reading everything again and testing the patch I now believe
this is already the final fix?!

What is clear is that the fix only helps for any newly planned dives
after applying the fix. Any old dive from the planner with the specific
setup will still show the spike because there is the "incorrect"
pressure info in the XML. To fix old dives I need to remove the pressure
info manually in the XML.

Please correct me if I'm wrong!
Thanks!

Best regards
Stefan


-- 

Stefan Fuchs
E-Mail: sfu...@gmx.de <mailto:sfu...@gmx.de>

___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

[PATCH 2/2] Properly clear sensor pressure data for synthetic plotinfo entries

[Linus Torvalds]

From: Linus Torvalds <torva...@linux-foundation.org>
Date: Thu, 14 Sep 2017 12:14:33 -0700
Subject: [PATCH 2/2] Properly clear sensor pressure data for synthetic plotinfo 
entries

We only cleared the first sensor data when we created new synthetic plot
info entries, because we only used to have one (well, we had the o2
data, but apparently nobody ever noticed that it didn't get properly
interpolated, probably because people who have CCR dives with o2
pressures are few, and the pressure drops are gradual anyway).

Clear all the pressure data, so that the interpolation code doesn't
think we have some existing real sensor data for the plot info entries
in between proper sample entries.

Signed-off-by: Linus Torvalds <torva...@linux-foundation.org>
---

This fixes the very odd "sometimes interpolation doesn't work". 

It's a one-liner trivial fix, but figuring it out was annoyingly 
difficult.

 core/profile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/core/profile.c b/core/profile.c
index af9ebff2..4ff27fcd 100644
--- a/core/profile.c
+++ b/core/profile.c
@@ -494,7 +494,7 @@ struct plot_info calculate_max_limits_new(struct dive 
*dive, struct divecomputer
entry->sec = _time; \
entry->depth = _depth;  \
entry->running_sum = (entry - 1)->running_sum + (_time - (entry - 
1)->sec) * (_depth + (entry - 1)->depth) / 2; \
-   SENSOR_PRESSURE(entry, 0) = 0; \
+   memset(entry->pressure, 0, sizeof(entry->pressure)); \
entry->sac = _sac;  \
entry++;\
idx++
-- 
2.14.1.538.gc8f31819a

___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface