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