On Thu, Jul 17, 2014 at 7:45 PM, Anton Lundin <[email protected]> wrote:
> On 17 July, 2014 - Miika Turkia wrote: > > > On Thu, Jul 17, 2014 at 10:05 AM, Anton Lundin <[email protected]> > wrote: > > > > > On 12 July, 2014 - Miika Turkia wrote: > > > > > > > On Sat, Jul 12, 2014 at 3:21 PM, Anton Lundin <[email protected]> > wrote: > > > > > > > > > On 12 July, 2014 - Miika Turkia wrote: > > > > > > > > > > > On Sat, Jul 12, 2014 at 2:24 PM, Anton Lundin <[email protected] > > > > > wrote: > > > > > > > > > > > > > On 12 July, 2014 - Miika Turkia wrote: > > > > > > > > > > > > > > > Subsurface has saved gas change events without type > attribute at > > > some > > > > > > > > point. Thus we need to add the type when reading in log > files, > > > if it > > > > > is > > > > > > > > missing. (Gas change logic relies on the type field > nowadays.) > > > > > > > > > > > > > > > > Fixes #617 > > > > > > > > Fixes #600 > > > > > > > > > > > > > > > > Signed-off-by: Miika Turkia <[email protected]> > > > > > > > > --- > > > > > > > > parse-xml.c | 6 ++++++ > > > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > > > > > diff --git a/parse-xml.c b/parse-xml.c > > > > > > > > index 5375e32..606e251 100644 > > > > > > > > --- a/parse-xml.c > > > > > > > > +++ b/parse-xml.c > > > > > > > > @@ -1337,6 +1337,12 @@ static void event_end(void) > > > > > > > > pic->offset.seconds = > > > > > > > cur_event.time.seconds; > > > > > > > > dive_add_picture(cur_dive, > pic); > > > > > > > > } else { > > > > > > > > + /* At some point gas change > events > > > did > > > > > not > > > > > > > have any type. Thus we need to add > > > > > > > > + * one on import, if we > encounter > > > the > > > > > type > > > > > > > one missing. > > > > > > > > + */ > > > > > > > > + if (cur_event.type == 0 && > strcmp( > > > > > > > cur_event.name, "gaschange") == 0) > > > > > > > > + cur_event.type = 25; > > > > > > > > > > > > > > I would prefer if we used SAMPLE_EVENT_GASCHANGE2 instead of > the > > > enum > > > > > > > number. > > > > > > > > > > > > > > Are you sure they are of type GASCHANGE2, aka with He? I would > > > guess > > > > > > > they where of type GASCHANGE , aka the ones without any He > info. > > > > > > > > > > > > > > > > > > > All the test dives use type 25 even though there is no helium in > > > them and > > > > > > manually added gas change event uses hard coded 25 as well, so I > > > didn't > > > > > > realize it should change based on gas. I'll send a fix shortly... > > > > > > > > > > > > > > > > It depends on what format the source of the event was talking. > There > > > are > > > > > DC's that only tell us the o2 content, even if the gas had a > he-part. > > > > > > > > > > If you look at the get_cylinder_index function, if its a > > > > > SAMPLE_EVENT_GASCHANGE, just don't care about any he-parts when > were > > > > > looking for the matching gas, eg: > > > > > > > > > > If a dive had a gaslist like: > > > > > * 21/35 > > > > > * AIR > > > > > > > > > > Then a SAMPLE_EVENT_GASCHANGE with o2 = 21% would return the > 21/35, its > > > > > the fist best match, but a SAMPLE_EVENT_GASCHANGE2 with o2 = 21% > he = > > > 0% > > > > > would return the air because the 21/35 has helium in it and the > event > > > > > says that the gas shouldn't have any helium in it. > > > > > > > > > > > > > > > I would say in the unknown event type case, if the event data has > a he > > > > > part then its definitely a SAMPLE_EVENT_GASCHANGE2, otherwise i > would > > > > > guess at a SAMPLE_EVENT_GASCHANGE, and let get_cylinder_index > figure > > > the > > > > > rest out. > > > > > > > > > > > > > This patch should take care of adding proper type attribute for both > XML > > > > import and manually added gas change events. > > > > > > > > miika > > > > > > > From e60b73cf33f79f144fc15705ac4f11645fed68e9 Mon Sep 17 00:00:00 > 2001 > > > > From: Miika Turkia <[email protected]> > > > > Date: Sat, 12 Jul 2014 15:51:03 +0300 > > > > Subject: [PATCH] Detect proper event type based on helium content > > > > > > > > Select proper SAMPLE_EVENT_GASCHANGE "version" based on helium > content > > > > on the mix. > > > > > > > > Signed-off-by: Miika Turkia <[email protected]> > > > > --- > > > > parse-xml.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/parse-xml.c b/parse-xml.c > > > > index 606e251..20eafc3 100644 > > > > --- a/parse-xml.c > > > > +++ b/parse-xml.c > > > > @@ -11,6 +11,7 @@ > > > > #include <libxml/parserInternals.h> > > > > #include <libxml/tree.h> > > > > #include <libxslt/transform.h> > > > > +#include <libdivecomputer/parser.h> > > > > > > > > #include "gettext.h" > > > > > > > > @@ -712,7 +713,7 @@ void add_gas_switch_event(struct dive *dive, > struct > > > divecomputer *dc, int second > > > > he = (he + 5) / 10; > > > > value = o2 + (he << 16); > > > > > > > > - add_event(dc, seconds, 25, 0, value, "gaschange"); /* > > > SAMPLE_EVENT_GASCHANGE2 */ > > > > + add_event(dc, seconds, he ? SAMPLE_EVENT_GASCHANGE2 : > > > SAMPLE_EVENT_GASCHANGE, 0, value, "gaschange"); > > > > } > > > > > > > > static void get_cylinderindex(char *buffer, uint8_t *i) > > > > @@ -1341,7 +1342,7 @@ static void event_end(void) > > > > * one on import, if we encounter the > type > > > one missing. > > > > */ > > > > if (cur_event.type == 0 && strcmp( > > > cur_event.name, "gaschange") == 0) > > > > - cur_event.type = 25; > > > > + cur_event.type = > cur_event.value > > > >> 16 > 0 ? SAMPLE_EVENT_GASCHANGE2 : SAMPLE_EVENT_GASCHANGE; > > > > > > > > add_event(dc, cur_event.time.seconds, > > > > cur_event.type, > cur_event.flags, > > > > > > I would say that this code is wrong. > > > > > > You can't choose between SAMPLE_EVENT_GASCHANGE and > > > SAMPLE_EVENT_GASCHANGE2 just based on if there is helium in the gas or > > > not. > > > > > > You should use SAMPLE_EVENT_GASCHANGE if the gaschange event only > > > contains the oxygen part of a gasmix, and SAMPLE_EVENT_GASCHANGE2 if > the > > > event describes the whole gasmix. > > > > > > > So in Subsurface we should always use SAMPLE_EVENT_GASCHANGE2? Unless > > libdivecomputer tells us otherwise? If I am finally understanding you... > > > > I hope i didn't mess things up too badly for you. > > Yes, if we added the event, we should always use > SAMPLE_EVENT_GASCHANGE2, but we can't assume that all events we read > without a event type will be SAMPLE_EVENT_GASCHANGE2. Well, this has been surprisingly difficult concept to grasp, even though it is actually quite simple. Here is a patch that switches to SAMPLE_EVENT_GASCHANGE2 for the manually added gas change events, as then we do know the full mix contents. But for input from saved files we still need to do our best guess. I suppose we need an Ack from Anton for this one even though I think I have understood the concept after the lengthy correspondence... miika
From 21cf8ce46ae104f1900f65aa7961b8228c9cf7de Mon Sep 17 00:00:00 2001 From: Miika Turkia <[email protected]> Date: Mon, 21 Jul 2014 07:45:10 +0300 Subject: [PATCH] Use SAMPLE_EVENT_GASCHANGE2 for manually added changes We should use SAMPLE_EVENT_GASCHANGE2 when the full gas mix is described. This includes both trimix and nitrox mixes. As we know the full mix when setting a gaschange event manually, we need to use the CHANGE2. Signed-off-by: Miika Turkia <[email protected]> --- parse-xml.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-xml.c b/parse-xml.c index 8e0099d..185fc7c 100644 --- a/parse-xml.c +++ b/parse-xml.c @@ -713,7 +713,7 @@ void add_gas_switch_event(struct dive *dive, struct divecomputer *dc, int second he = (he + 5) / 10; value = o2 + (he << 16); - add_event(dc, seconds, he ? SAMPLE_EVENT_GASCHANGE2 : SAMPLE_EVENT_GASCHANGE, 0, value, "gaschange"); + add_event(dc, seconds, SAMPLE_EVENT_GASCHANGE2, 0, value, "gaschange"); } static void get_cylinderindex(char *buffer, uint8_t *i) -- 1.9.1
_______________________________________________ subsurface mailing list [email protected] http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
