First cut at EON Steel patches

[Linus Torvalds]

On Wed, Oct 22, 2014 at 1:52 AM, Jef Driesen <jef at libdivecomputer.org> wrote:
>
> Storing the firmware per-dive is actually the right thing! If the firmware
> versions is needed for parsing (e.g. firmware dependent data format), then
> the current firmware version is useless, because you need to know the
> version at the time the dive was recorded. This is not only true for the
> firmware versions, but also other things like settings.

So the Suunto generally dies the right thing in general. But it's not
even things like fw versions it storess per-dive, it does:
 - conservatism settings
 - start and end tissue loadings and otu etc
 - but also device serial number and hardware version

In other words, for the EON Steel, I'd really rather not do *anything*
for the "device" part. Because there are no really easily parseable
device fields: yes, the suunto has a whole configuration interface too
that is per-device, but it's at *least* as complicated as the dive
parsing to parse.

So:

> The primary purpose of the devinfo event is not an interface to pass the
> firmware version to the application. It has been designed for use with the
> fingerprint feature.

The thing is, the "fingerprint" is useless. We don't really use it,
and it's for a very good reason: it's simply not worth it.

But even if we want to use it, on the suunto, EVEN THE FINGERPRINT
would be better decoded per-dive.

Seriously. Each dive has the device serial number, the hardware
version, and even the device name encoded. I literally have:

  ..
  0100: 'sml.DeviceLog.Device.Name' 'utf8' ''
    0100: EON Steel
  0101: 'sml.DeviceLog.Device.SerialNumber' 'utf8' ''
    0101: #p1437019
  0102: 'sml.DeviceLog.Device.Info.SW' 'utf8' ''
    0102: 1.0.3
  0103: 'sml.DeviceLog.Device.Info.HW' 'utf8' ''
    0103: 71.2.0
  0104: 'sml.DeviceLog.Device.Info.BSL' 'utf8' ''
    0104: 1.0.3
  ..

(Those off numbers are the "type" for the decoded entry: this is all
decoded data, none of it is made up by me, and all of it is strings.
The "BSL" seems to be soem off Suunto "Binary Software Layer", I'm not
sure why it's separate from the SW version, and they indeed have the
same value. I *think* it's more of a "bootloader" version vs the "OS"
version in the "SW" field, but that's just a guess).

This is from the *dive* file. This is exactly the thing I do *not*
want to try to parse one  more time, just because the libdivecomputer
interfaces are cumbersome.

NONE of this is "easily available" before getting and parsing the
dives. Yes, as mentioned, I can get the per-device settings other ways
too, but those other ways are not easier than parsing the dive data.

>         When the application gets the devinfo event, it should
> lookup whether it has a fingerprint for that specific device, and register
> it with the dc_device_set_fingerprint function.

.. and there is no reason why we have to do it long before parsing.

The whole "do devinfo before parsing" does not add any actual value.
See? And it actually makes things more complex.

THIS is why I'd like to do the devinfo when I actually parse things.

Of course, on many other dive computers you can have this info
*before* parsing (since the serial number or whatever is at a fixed
point in the memory dump, and regardless of each dive), but this is
*exactly* why I'd like to just call "devinfo" multiple times (because
several of the fields can change between dives) and if that means that
the app would look up the (same) device multiple times, who cares?

> Actually, including the firmware version in the devinfo was in fact a
> mistake. It's not needed for the fingerprint feature, and that's also the
> reason why most backends don't even bother filling it in. In retrospect, I
> should have designed the fingerprint interface slightly different. Instead
> of hooking into the events (which are mainly used for progress events), I
> should have added a function to register a fingerprint callback function:
>
> typedef void (*dc_fingerprint_callback_t) (dc_device_t *device, unsigned int
> model, unsigned int serial, unsigned char data[], unsigned int size, void
> *userdata);

So that DOES NOT HELP.

What part of "this is encoded per-dive" don't you get?

A callback before parsing is not helpful. At the very least, this
needs to happen *after* the whole "set_data()" that has gotten the
actual dive data. See?

The above in no way changes the devinfo thing, it just makes a new
interface that does the same thing, but only gets the fingerprint. NOT
USEFUL.

> In the past you have been criticizing more than once, that I shouldn't break
> backwards compatibility. I got the message, and I no longer do this. But now
> you're complaining that we're stuck with a bad interface because of that?
> What do you expect me to do?

I want *new* interfaces. I want *sane* interfaces. I want interfaces
that I can call when it's *convenient*, instead of having to do one
part before I've parsed things (and don't even *have* the data), and
then another part when I have the dive data.

I'm perfectly happy just entirely never *ever* generating a DEVINFO
callback for the EON Steel (that is in fact what I do now, and
subsurface doesn't even _care_, because the devinfo data just isnt'
important enough). I'd like to have a *new* callback that:

 - I can call at the beginning of each dive
 - I can give _text_ data (not numbers, and not some random binary
blob for "fingerprint" that has no possible sane encoding)
 - I can update the devinfo data.

So an old-style setup would just use the old global devinfo thing at
dive loading time. No changes, applications wouldn't even see a
difference - even if they register the new callback, they'd just never
get it.

A mixed mode might use the old global devinfo thing to give some
things (like serial number, although you have to use the broken
numeric version), but might update things partially with the new
callback for each dive (for example, give the firmware version). Or
maybe they'd only update things on the *first* dive, or whatever. Old
applications that don't register the new callback would still work,
they'd just not get some of the data.

And the EON steel would not ever use the old global devinfo thing
(because absolutely *none* of the fields are useful _or_ available at
that point) and would _only_ use the new "update device info per
dive". Applications that don't register the new callback would still
work, but don't get any serial numbers or anythign like that.

Notice? It's completely backwards compatible. Sure, you'd not get all
the new information if you don't request it, but things still work
without the fingerprint and serial numbers etc. I'm using subsurface
that way right now. So it doesn't break anything.

So the thing I'm asking for is a *new* interface. More akin to the
"get_field()" model (and it might even just *be* "get_field()" wioth
new fields), in that

 - it gets called at *parse* time. Not before. Not at some global setup time.

 - it returns *strings*. Not "data,len" random blobs that cannot be
displayed sanely. Not numbers. Not hashes. utf8 strings. If you have a
number (like an old-style dive computer) the dive computer backend can
damn well just use snprintf(). If you have some binary blob, you can
return it as a hex-dump or something. Strings work for these things,
in ways that other formats do *not*.

It could literally be a DC_FIELD_SERIAL_NR  /
DC_FIELD_FIRMWARE_VERSION etc that returns strings, but I'd rather see
just one callback that returns all of it.

So for example, think of DC_FIELD_GASMIX that takes that "struct
gasmix" or whatever it is. We could have a new DC_FIELD_DEVINFO, and
old backends would just return DC_STATUS_UNSUPPORTED without even
making any changes to them (at least if they are well written). And
new backends could fill in something like this:

   struct dc_field_devinfo_t {
      const char *vendor;
      const char *model;
      const char *firmware;
      const char *serial;
      const char *dive_id;
   };

and they could leave the fields NULL if they are the same as the old
"dc_event_devinfo_t" fields.

The "dive_id" could be a per-dive ID. On the EON Steel, I could, for
example, use the "sml.DeviceLog.Header.DateTime" field.

  0106: 'sml.DeviceLog.Header.DateTime' 'utf8' ''
    0106: 2014-10-11T14:12:39.358

yes, that's a string with date/time of the dive. With *milliseconds*.
Suunto is a bit odd sometimes, all the sample times are actually
reported in milliseconds. It's clearly the internal time format of
this thing.

And the "fingerprint"? It's useless. If I want a fingerprint, I can
just generate it from hashing the model and serial number. Why would I
ever ask for a fingerprint?

Anyway. I'm not at all saying it should be a "dc_field". But that
would be the easiest extension, I suspect. And then, to make it
possible for the code that *uses* libdivecomputer to see if
libdivecomputer supports the new thing or not, do *not* do this by
version numbers (testing version numbers is always a wrong thing to
do), just do this with something like this:

    diff --git a/include/libdivecomputer/parser.h
b/include/libdivecomputer/parser.h
    index 65b18c9bb2fe..f70d4bbe46c8 100644
    --- a/include/libdivecomputer/parser.h
    +++ b/include/libdivecomputer/parser.h
    @@ -53,9 +53,13 @@ typedef enum dc_field_type_t {
            DC_FIELD_GASMIX_COUNT,
            DC_FIELD_GASMIX,
            DC_FIELD_SALINITY,
    -       DC_FIELD_ATMOSPHERIC
    +       DC_FIELD_ATMOSPHERIC,
    +       DC_FIELD_DEVINFO,
     } dc_field_type_t;

    +// Make it easy to test support compile-time with "#ifdef DC_FIELD_DEVINFO"
    +#define DC_FIELD_DEVINFO DC_FIELD_DEVINFO
    +
     typedef enum parser_sample_event_t {
            SAMPLE_EVENT_NONE,
            SAMPLE_EVENT_DECOSTOP,

See? Now users like subsurface can just do

    #ifdef DC_FIELD_DEVINFO
              rc = dc_parser_get_field(parser, DC_FIELD_SALINITY, 0, &salinity);
              if (rc == DC_STATUS_SUCCESS) {
                 ...
              }
    #endif

and everybody is happy. Old users continue to work - even with new
backends - although they obviously don't get all the data. Old
backends might even use this *despite* the fact that the old model
worked for them, to just return the serial numbers etc in a new
format. And dive computers that save (some _or_ all) of these fields
per-dive can mix things up. Leave "serial" NULL if you don't want to
override the old one, but if you actually get the serial number from
the dive file, you can fill it in each time. Or just the first time.
Or never. Up to the backend to decide if it wants to update things or
not.

Wouldn't this be easy? No breakage, new features, it all "just works".

                     Linus