Linus,
Could you do me a favor and provide some high level overview of the api design that you have in mind? I'm trying to understand your point of view, but it's not always crystal clear to me. If you could explain your api design with some pseudo code, call sequences, etc I think that would help a lot.
Some more comments below.
On 2012-06-24 20:32, Linus Torvalds wrote:
On Sat, Jun 23, 2012 at 1:45 PM, Jef Driesen jefdriesen@telenet.be wrote:
Creating the parser in the callback is convenient. In some cases you can't create a parser upfront (at the time of calling dc_device_open, or even immediately after it) because the actual model number is required for parsing the data.
Jef, this is exactly the kind of totally broken thinking that makes libdivecomputer interfaces so nasty to use.
The user SHOULD NOT CARE about these kinds of internal libdivecomputer issues. The whole *point* of libdivecomputer should be to hide all these internal implementation issues, and as long as you continue to think that your internal details should matter for the interface, the interface will be crap.
Why the hell should I care that you need some model number for the parser? I do not even *KNOW* the model number, for chissake! The only thing that knows (and cares) is libdivecomputer itself.
So according to your absolutely insane logic, I cannot create the parser at device open time, because I don't know if that particular device needs the model number. But that means that I should magically decide to create the dive parser after I get the DC_EVENT_DEVINFO callback. Can you not see that THAT MAKES NO SENSE! From an interface standpoint, that is absolutely insane. I don't even *care* about the DC_EVENT_DEVINFO - the fact that I actually catch it and print out the model number on the progress bar is simply because that (to me) is an indicator that libdivecomputer is working at all.
Why would you want to create a parser in advance? You can't really parse anything until you get the first dive in the dive callback function. And in that case you can equally well create the parser in the dive callback. So I don't understand why this is a problem.
If you call dc_parser_new() from inside the dive callback, you don't need to care about the model number at all. If a parser needs it, it's guaranteed to be available in the device handle (regardless of whether you catch the events or not). If it's not required, then no problem either. That's what I meant when I said it's convenient: you don't need any device specific knowledge, and it will always just work.
The only real issue I see here is that the api doesn't prevent you from calling dc_parser_new() immediately after dc_device_open(), in which case you may end up with a non functional parser.
Why you use DC_EVENT_DEVINFO as an indicator is a mystery to me. We have DC_EVENT_{PROGRESS,WAITING} for that purpose. The primary purpose for DEVINFO is for use with the fingerprint feature, which subsurface isn't using.
You fixed the interfaces to the top-level open code, and to the top-level parser create code. Please just fix this too.
The fix is damn easy: get rid of the insane and useless "dc_parser_new()" and "dc_parser_destroy()" interfaces entirely. Create the parser inside of libdivecomputer (whenever it makes sense) and don't expose that totally idiotic and pointless interface to the user at all. Seriously. There is absolutely *ZERO* upside to ask me to create a parser, since I don't care, and I cannot possibly do it at a sane time anyway, because for the user, no sane time exists at all.
Just create the parser internally, and hide that information in the "dc_device_t" structure that is totally internal to libdivecomputer, and is just an opaque object to the users.
Please. Stop these insane interfaces that only complicate the use of libdivecomputer, and don't help anybody.
I'm not sure what you propose here. Do you mean to integrate the parser object into the device object? How is that supposed to work? For example a device can contain more than one dive, so how do you decide which dive to parse? Or do you mean the dive callback should be changed to receive a parser object directly, pre-populated with the dive data, model numbers, and whatever else is necessary. Something like:
int callback (dc_parser_t *parser, void *userdata) { ... dc_parser_get_datetime (parser, ...); dc_parser_samples_foreach (parser, ...); ... }
Essentially turning the dc_parser_t into a dc_dive_t object. I considered doing this in the early days of the libdivecomputer project, but there are a couple of problems with this approach.
With this api, your have no other option than to do the parsing directly in the callback function. However some devices (e.g. the sensusultra) have very strict timing requirements and you are very limited in the amount of processing you can do in the callback without messing up the downloading. In this cases it's usually better to save the data and process it once the download has finished, or offload it to a separate thread. But that's impossible because the lifetime of the parser is now limited to the duration of callback function.
Assume we introduce some method to extend the lifetime of the parser to remain valid outside the callback (e.g. a deep copy or something). We still can't transfer this parser to another thread because it may share some state with the device handle. Remember that in the roadmap emails [1], I announced that I'm about to introduce a new library context object to fix the problems related to this shared state. With the context object, you could create one context for the device thread and another one for the parser thread. If you then call:
dc_context_new (&ctx_device); dc_context_new (&ctx_parser);
dc_device_open (&device, ctx_device, ...);
callback (...) { dc_parser_new (&parser, ctx_parser, device); }
The device and parser won't share any state and everything is fine. But if the parser is created directly by the device, there is no way to use another context object. The sensusultra timing problem can be fixed with internal caching, but the threading problem is a more fundamental one. How would you solve this?
[1] http://www.divesoftware.org/libdc/roadmap.html#17
You also have to take into account the use-case where you want to parse data without a device connection available.
No you don't. You can expose damn well any interface you want for that case, but that's a totally separate case. That in *no* way means that when we have the device available, the user should somehow magically know when to create the parser. There is zero point.
BTW, for this situation I want to introduce some variant of the new dc_parser_new function that doesn't take a device handle. I have been thinking of something like this:
dc_status_t dc_parser_new_for_data (dc_parser_t **parser, const dc_event_devinfo_t *devinfo, const dc_event_clock_t *clock);
Congratulations on making the interface EVEN MORE BROKEN.
Don't do this. Seriously. Just f*cking don't. Any sane user of the libdivecomputer stuff will never ever care. If I opened a device through libdivecomputer, you already have ALL that information. It's totally pointless to give it to be as a callback, and then expect me to just give it back to you. Can't you see how STUPID that is?
Make that interface what you do *internally*. Use it for the pointless case where somebody cares. Don't force it for the normal case.
I think you misunderstood me here. This new dc_parser_new_for_data() function is supposed to co-exist with the dc_parser_new() function. You would only use it for the "offline" parsing use-case, so it's not intended as a replacement for the dc_parser_new() function. Isn't that what you meant when you wrote I can "expose damn well any interface you want" above?
Even if I were to want to parse a pre-downloaded dump from a file, I WOULD NEVER EVER WANT TO USE THAT INTERFACE. I would want to use the "open dive computer" interface, and just give you the filename. And then libdivecomputer can again internally do the above.
So feel free to do those kinds of changes for your internal flow, but if you really think that it's a good idea to expose that kind of internal detail to the user, I don't know what to say. Except to say that you're wrong. It's a really *bad* idea to make these kinds of interfaces. Don't make things worse, make them better.
I'm not convinced a file import interface is the right solution here. What if the application doesn't store the data in a raw file (for example in a database), or uses some different format (for example the individual dives instead of a memory dump)? If an application wants to support offline parsing, then it will need some specific knowledge anyway. For example in the Uwatec Smarttrak scenario, the data isn't stored in the original format, and you have to combine several database fields to reconstruct it. You can't do this without any knowledge of the format. So the internals are already exposed.
- The event handling is still horrible. There does not seem to be any sane generic way to just get the event type and meaning.
Which events are you referring to? The device events (e.g. DC_EVENT_*), or the parser events (e.g. DC_SAMPLE_EVENT)? Can you explain in some more detail?
The DC_SAMPLE_EVENT interface is just crazy.
Why do I have to have this kind of code in there:
static const char *events[] = { "none", "deco", "rbt", "ascent", "ceiling","workload", "transmitter", "violation", "bookmark", "surface", "safety stop", "gaschange", "safety stop (voluntary)", "safety stop (mandatory)", "deepstop", "ceiling (safety stop)", "unknown", "divetime", "maxdepth", "OLF", "PO2", "airtime", "rgbm", "heading", "tissue level warning" }; const int nr_events = sizeof(events) / sizeof(const char *); ... type = value.event.type; name = "invalid event number"; if (type < nr_events) name = events[type];
where that set of strings is something I had to just mindlessly copy from your "example.c".
The 'enum parser_sample_event_t' numbers are totally meaningless to me. They don't tell me anything. Same goes for 'enum parser_sample_flags_t'. And what does the even 'value' actually do? None of this makes any sense, and more importantly, as far as I can tell, I *cannot* make sense of them.
Yes, the 'flags' things seems to possibly make sense as a "this begins the event" and "this ends it". That's not documented, but at least it's *somehow* sensible. But if that is all it does, why is it some bitfield thing? What else can it possibly be? What should I do as a user to protect myself from you changing the flags?
And the 'value' field? What does it do? It makes no sense to me, and I can't tell what it would do. Does it have any meaning? Is it in degrees for the heading event? Is it minutes for the airtime event? What is it?
The sample events are indeed a big mess. I'm aware of this, and they will be removed from the api. I don't know yet how to implement an appropriate replacement, so at the moment the idea is to introduce a vendor extension type and just pass the raw binary data. Applications that insist on using this type of data can still get it with some additional effort. But at least we won't be stuck forever with a broken interface.
Jef