First cut at EON Steel patches

[Jef Driesen]

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