Ok, this is very much incomplete, and it doesn't send things like the DC_EVENT_VENDOR or DC_EVENT_DEVINFO stuff at all. Which means that you don't get serial numbers etc.
Part of that is that I'm *really* frustrated with the libdivecomputer crap wrt firmware versions and serial numbers. This goes back *years*: the fact that libdivecomputer still seems to think that they are unsigned integer values. They are *not*. They are strings. But libdivecomputer doesn't have a sane interface to report strings.
The firmware version I have is 1.0.7. The serial number I have is "#p1437019". Neither of them is a number (and yes, I'm serious, the characters '#' and 'p' are part of the serial number: both as the dive computer reports it, and printed on the dive computer itself).
But apart from the fact that this is *still* completely broken in libdivecomputer, these two patches get at least a usable profile out of the Eon Steel.
Let me know if you find something fundamentally broken here,
Linus
So I have a few more patches pending for the EON steel, and am parsing a fair amount more data. In particular, I'm parsing things like device ID's now, but the sad part is that I have no really good way to pass the data back.
Why?
Part of it is an EON Steel detail: the firmware version number etc is actually encoded per-dive: yes, you can find a "current firmware version" for the device itself, but I'd *much* rather use the per-dive one, which is stable (ie if I upgrade the firmware, the device firmware version updates, but the dive info still has the firmware version that was active for the dive). That means that the dive data doesn't change if the firmware is updated.
But the
device_event_emit(device, DC_EVENT_DEVINFO, &devinfo);
seems to be meant to be called just once, not once per dive. Whatever - I guess I could just violate that unwritten rule and just do multiple devinfo callbacks, once before each dive.
HOWEVER.
The other issue is that all of this is encoded in the same format that all the sample data is encoded in, so it's really only available when actively *parsing* the file. So I'd *really* like to call that device_event_emit() when I get the set_data() in the parser - but by then I no longer have the "device" struct to send the info to.
Can I parse the damn thing multiple times? Sure. Right now I already parse it twice: once at parser setup time (to get the "fields" data - dive duration, surface pressure etc), and then once for the samples_foreach() case. Can I make the parser and the dive data loading IO side be incestuous and parse it a *third* time early? Yes I can, but yes I'd really prefer not to. It negates the whole principle of separating out the dive parsing from the dive loading that libdivecomputer seems to have.
Jef, comments?
Linus
On 2014-10-21 23:55, Linus Torvalds wrote:
So I have a few more patches pending for the EON steel, and am parsing a fair amount more data. In particular, I'm parsing things like device ID's now, but the sad part is that I have no really good way to pass the data back.
Why?
Part of it is an EON Steel detail: the firmware version number etc is actually encoded per-dive: yes, you can find a "current firmware version" for the device itself, but I'd *much* rather use the per-dive one, which is stable (ie if I upgrade the firmware, the device firmware version updates, but the dive info still has the firmware version that was active for the dive). That means that the dive data doesn't change if the firmware is updated.
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.
But the
device_event_emit(device, DC_EVENT_DEVINFO, &devinfo);
seems to be meant to be called just once, not once per dive. Whatever
- I guess I could just violate that unwritten rule and just do
multiple devinfo callbacks, once before each dive.
HOWEVER.
The other issue is that all of this is encoded in the same format that all the sample data is encoded in, so it's really only available when actively *parsing* the file. So I'd *really* like to call that device_event_emit() when I get the set_data() in the parser - but by then I no longer have the "device" struct to send the info to.
Can I parse the damn thing multiple times? Sure. Right now I already parse it twice: once at parser setup time (to get the "fields" data - dive duration, surface pressure etc), and then once for the samples_foreach() case. Can I make the parser and the dive data loading IO side be incestuous and parse it a *third* time early? Yes I can, but yes I'd really prefer not to. It negates the whole principle of separating out the dive parsing from the dive loading that libdivecomputer seems to have.
You certainly should not emit the devinfo event more than once. Let me explain why.
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. 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. To be able to distinguish different devices, I needed something that's unique for each device. And that's where the serial number comes into play, because serial numbers are supposed to be unique. However for this purpose, the serial number as used by libdivecomputer doesn't have to match exactly with the real serial number. That's why decoding it as a number is fine for this purpose. In theory we could even calculate a hash here.
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);
dc_status_t dc_device_set_fingerprint (dc_device_t *device, dc_fingerprint_callback_t callback, void *userdata);
If libdivecomputer calls your callback function, you lookup the fingerprint for that specific (device,model,serial) triplet, and write it directly to the provided buffer in the data parameter. With this interface, the intended purpose would have been much more clear. And it's probably also a bit easier to use this way too. And then the devinfo event is available for other purposes...
I agree with you, that for communicating the real serial number and firmware version to the application we'll need some different interface. And yes, strings will be more appropriate here. No need to convince me about that. But right now, we can't change that without breaking backwards compatibility.
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?
Anyway, my advice is that for now you encode the serial and firmware version as a number for the fingerprint feature, and at the same try to keep it at close as possible to the real fingerprint. Once we fix this design mistake after v0.5, we can return the real values.
Jef
On Wed, Oct 22, 2014 at 10:52:42AM +0200, Jef Driesen 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.
We already have other backends where we figure out the firmware version while parsing a dive (OSTC comes to mind, but I think there are others). There it's much more that we switch between two known layouts, but that's fundamentally the same thing.
You certainly should not emit the devinfo event more than once. Let me explain why.
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. 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. To be able to distinguish different devices, I needed something that's unique for each device. And that's where the serial number comes into play, because serial numbers are supposed to be unique. However for this purpose, the serial number as used by libdivecomputer doesn't have to match exactly with the real serial number. That's why decoding it as a number is fine for this purpose. In theory we could even calculate a hash here.
Which reminds me. I STILL need to redesign our libdivecomputer code to correctly use fingerprints. So many things to do, so little time.
I agree with you, that for communicating the real serial number and firmware version to the application we'll need some different interface. And yes, strings will be more appropriate here. No need to convince me about that. But right now, we can't change that without breaking backwards compatibility.
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?
Anyway, my advice is that for now you encode the serial and firmware version as a number for the fingerprint feature, and at the same try to keep it at close as possible to the real fingerprint. Once we fix this design mistake after v0.5, we can return the real values.
Here I'm confused. So we are already adding a few new things to 0.5 compared to 0.4. What is stopping us from adding two new optional APIs?
DC_EVENT_SERIAL which returns a string with the serial number DC_FIELD_VERSION which returns a string with the firmware version for this dive
If the backend doesn't support them the app will need to fall back to the existing means of discovering these data - but for backends which support this, we can indeed return appropriate strings...
Am I missing something?
/D
On Wed, Oct 22, 2014 at 1:52 AM, Jef Driesen jef@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 shouldlookup 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
-
Dirk Hohndel -
Jef Driesen -
Linus Torvalds