Ok, I re-organized my patches a bit, and here are four patches to support the basic dive log download of the new Suunto EON Steel dive computer. I combined some of my basic support patches into one, since I didn't feel like I needed to show the odd history of having implemented some things in stages (ie the second attachment was originally four patches that didn't do anything useful unless combined).. Three of the patches are for libdivecomputer: - patch 1/3: the preparatory parser support patch - patch 2/3: the basic core dive data downloader for the EON Steel - patch 3/3: the small addition to parse more than the basic data Patch 2/3 works on its own, but means that you *just* get the core dive data (with full dive profiles and cylinder pressures). Patch 3/3 gives you the serial number and firmware version etc, but that relies on 1/3. The fourth patch (1/1) is the patch to subsurface to actually use the new parser support in libdivecomputer, so that you can get the EON Steel serial numbers etc. This makes your Suunto EON Steel quite usable with subsurface. There is more we could do, but it's all fairly secondary stuff. I'd like to get this all accepted before I even bother starting to look at the other things I can do with the dive computer. Of course, since apparently the EON Steel won't really be *available* until Nov 15th or so, right now these patches are useful mainly to people with preproduction models like me. But wouldn't it be nice if we could just say that we support the thing before it's even available for sale? Linus
On 23-10-14 00:34, Linus Torvalds wrote:
Ok, I re-organized my patches a bit, and here are four patches to support the basic dive log download of the new Suunto EON Steel dive computer. I combined some of my basic support patches into one, since I didn't feel like I needed to show the odd history of having implemented some things in stages (ie the second attachment was originally four patches that didn't do anything useful unless combined)..
Three of the patches are for libdivecomputer:
- patch 1/3: the preparatory parser support patch
- patch 2/3: the basic core dive data downloader for the EON Steel
- patch 3/3: the small addition to parse more than the basic data
Patch 2/3 works on its own, but means that you *just* get the core dive data (with full dive profiles and cylinder pressures). Patch 3/3 gives you the serial number and firmware version etc, but that relies on 1/3.
The fourth patch (1/1) is the patch to subsurface to actually use the new parser support in libdivecomputer, so that you can get the EON Steel serial numbers etc.
This makes your Suunto EON Steel quite usable with subsurface. There is more we could do, but it's all fairly secondary stuff. I'd like to get this all accepted before I even bother starting to look at the other things I can do with the dive computer.
Of course, since apparently the EON Steel won't really be *available* until Nov 15th or so, right now these patches are useful mainly to people with preproduction models like me. But wouldn't it be nice if we could just say that we support the thing before it's even available for sale?
I did a first review of your Eon Steel backend. Here are my comments: Both the windows and non-libusb builds are broken. The msvc build it's a bit tricky. Because msvc doesn't support C99, I compile the code as C++. All the C99 features I use in libdivecomputer are supported in C++, so this mostly works. But there are also some extras necessary, like explicit casts from void pointer to other pointers. But even the mingw (gcc) build fails. It fails on the ERROR macros with "error: called object '0' is not a function". Not sure why. The directory_entry structure with it's zero length array is most likely also not very portable. C99 has flexible arrays, but I'm not sure what msvc supports. Better integration with the existing libdivecomputer infrastructure. We already have logging functions, array_uint{16,24,32}_{be,le} helper functions for dealing with endianness, dynamic memory buffers (dc_buffer_t), etc. There is no need to re-implement any of those. Out of memory error handling. I know the arguments for calling exit very well, but for libdivecomputer I made the choice to handle out of memory differently. Please stick to this pattern. Many functions return -1 in the case of an error. Can you rework this as dc_status_t values? BTW, if you want to use goto style error handling, that's fine. The remaining ones are mainly style issues: For the name of the backend, I prefer a single word like "eonsteel" or just "steel" instead of eon_steel. This is just to follow the existing practice in libdivecomputer were backends are named DC_FAMILY_VENDOR_PRODUCT. So no underscores in the product part. Main structures (struct eon_steel, eon_parser) and all entry point functions (public or through the vtable) get the same backend prefix. I also prefer to see all structures and enums typedef'ed, so you can use them without the struct and enum keywords. That's it for now. The parser logic is a bit hard to follow without having some example data to look at. Can you share some data files? Jef
On Mon, Oct 27, 2014 at 3:57 PM, Jef Driesen <jef@libdivecomputer.org> wrote:
On 23-10-14 00:34, Linus Torvalds wrote:
Both the windows and non-libusb builds are broken.
I have no sane way to test the windows build issues. But I fixed the non-libusb build by moving the ifdef around a bit, and I renamed "error()" to "report_error()" to hopefully get the Windows build fixed. Can you mention the actual broken casts? This is my #1 "C++ is a broken piece opf shit" moment, though, since I really hate how *wrong* C++ got the whole "void *" thing (not just NULL, absolutely _everything_ is broken wrt type handling of "void *" in C++).
But even the mingw (gcc) build fails. It fails on the ERROR macros with "error: called object '0' is not a function". Not sure why.
It sounds like the mingw header files use some odd kind of "error" #define. Worked around by making it be "report_error()"
The directory_entry structure with it's zero length array is most likely also not very portable. C99 has flexible arrays, but I'm not sure what msvc supports.
Changed to a one-byte array - it's going to waste a few bytes of memory per entry, but it's not a big deal. (That's not due to the "1" - that could be easily fixed - but due to then also aligning the thing up for "sizeof()").
Better integration with the existing libdivecomputer infrastructure. We already have logging functions, array_uint{16,24,32}_{be,le} helper functions for dealing with endianness, dynamic memory buffers (dc_buffer_t), etc. There is no need to re-implement any of those.
Quite frankly, your logging functions suck, because they don't return any value. So you can't use "return ERROR()", which is the sane common use. Plus they need that "context" thing which makes no sense and isn't even available except at "open" time. It's not just open that wants to log things. So no, I can't use that horrible thing. For example, even ignoring the brokenness of the "context" thing, something like: if (badness) return report_error("badness %d happened", badness); compared to: if (badness) { ERROR(context, "Badness %d happened", badness); return DC_STATUS_WHATEVER; } is the difference between "I can bother to make reasonable error reporting" and "f*ck this, I'll not bother logging the reason at all". So then I'd just do if (badness) return DC_STATUS_FAIL; so now there's no logging at all - just because your interface is bad. But I can't even use that DC_STATUS_FAIL kind of thing. See later. As to the "array_uint()" functions - you have no writing functions, and you're lacking a "u8". And your types are wrong, not allowing "void *". And they are insanely verbose. But whatever. I converted the ones you had.
Out of memory error handling. I know the arguments for calling exit very well, but for libdivecomputer I made the choice to handle out of memory differently. Please stick to this pattern.
I made it use the dc_buffer_t, and just ignore out-of-memory cases. They don't happen, and if they did, they'd just result it empty buffers now.
Many functions return -1 in the case of an error. Can you rework this as dc_status_t values? BTW, if you want to use goto style error handling, that's fine.
No, I cannot use dc_status_t values, because they make it impossible to return an error *and* a value. There is a very good reason why UNIX calling conventions are "return -1 on error". That reason is the *non-error* case. Think of a simple function like "read()". Ask yourself what it returns on error, and what it returns on success. Yes, I have used things like DC_STATUS_SUCCESS. I used VMS for some time. It's horrible. Suddenly you have to first check for success, and then have another interface to return the result size etc. The whole "negative means error, zero or positive is success" is really powerful. It means that you can return how many bytes you read, and easily test whether there was something wrong, *without* having to have some "pass return size value as a pointer" model. So please realize that my "send_receive()" function works like "read()" - it returns negative on error, and the number of bytes received on success. I can't do that with DC_STATUS_XYZ.
For the name of the backend, I prefer a single word like "eonsteel" or just "steel" instead of eon_steel. This is just to follow the existing practice in libdivecomputer were backends are named DC_FAMILY_VENDOR_PRODUCT. So no underscores in the product part.
Did it for DC_FAMILY_SUUNTO_EONSTEEL.
Main structures (struct eon_steel, eon_parser) and all entry point functions (public or through the vtable) get the same backend prefix.
I *really* don't want to do this, though. The public ones are "suunto_eon_steel()" because they are visible to others. But I really object to writing that long name out for internal helper functions that are static. Why write that horribly long name for no gain?
The parser logic is a bit hard to follow without having some example data to look at. Can you share some data files?
I don't have any data files, I just run it on the dive computer. Remember: this is not a "memory dump" style device. So the dc_device_dump() approach doesn't work, because on this dive computer you'd have to do it per-dive. It would be easy to dump the parse data, but I don't think there is any such functionality: you expect the dump to be a memory dump of the whole device. New versions attached. Linus
On Mon, Oct 27, 2014 at 5:40 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
New versions attached.
Side note: I baked in two other changes that weren't explicitly asked for: - the new version also honors the "cancelled" flag in between reading dive files - the new version improves on the parsing-past-the-end case because your dc_buffer_t wasn't really amenable to the kinds of games I played to pad the buffer with zeroes. but other than that it's identical to the old version, modulo the whole asked-for-changes (at least the ones that were reasonably changeable). Linus
On 28-10-14 01:40, Linus Torvalds wrote:
On Mon, Oct 27, 2014 at 3:57 PM, Jef Driesen <jef@libdivecomputer.org> wrote:
On 23-10-14 00:34, Linus Torvalds wrote:
Both the windows and non-libusb builds are broken.
I have no sane way to test the windows build issues. But I fixed the non-libusb build by moving the ifdef around a bit, and I renamed "error()" to "report_error()" to hopefully get the Windows build fixed.
Can you mention the actual broken casts? This is my #1 "C++ is a broken piece opf shit" moment, though, since I really hate how *wrong* C++ got the whole "void *" thing (not just NULL, absolutely _everything_ is broken wrt type handling of "void *" in C++).
Basically every cast from void* to something else. In C++ you need an explicit cast, while in C this isn't necessary. I admit that this whole C++ hack is annoying, and I would rather ignore it. But on Windows supporting msvc is pretty much a must. I don't use it at all myself (I cross compile on Linux), other than checking once in a while whether it still compiles. But there are people using it. And if a few extra casts are necessary to make this work, then that's a price I'm willing to pay. I don't expect you to fix all Windows issues, but if you can already keep this in mind, that means less work afterwards. There are also some header files like unistd.h which don't exist on windows. Several of the headers you include are not necessary at all, and can be removed. There are other things like the snprintf function which doesn't implement the C99 standard and returns a different value if the buffer is too short.
But even the mingw (gcc) build fails. It fails on the ERROR macros with "error: called object '0' is not a function". Not sure why.
It sounds like the mingw header files use some odd kind of "error" #define. Worked around by making it be "report_error()"
I remember again what's the problem. The libusb header pulls in the windows.h header. And somewhere deep in there, there is a ERROR macro defined. I worked around that in the past by doing: #ifdef HAVE_LIBUSB #ifdef _WIN32 #define NOGDI #endif #include <libusb-1.0/libusb.h> #endif We don't need GDI at all, so it's no problem to disable it.
The directory_entry structure with it's zero length array is most likely also not very portable. C99 has flexible arrays, but I'm not sure what msvc supports.
Changed to a one-byte array - it's going to waste a few bytes of memory per entry, but it's not a big deal. (That's not due to the "1" - that could be easily fixed - but due to then also aligning the thing up for "sizeof()").
Better integration with the existing libdivecomputer infrastructure. We already have logging functions, array_uint{16,24,32}_{be,le} helper functions for dealing with endianness, dynamic memory buffers (dc_buffer_t), etc. There is no need to re-implement any of those.
Quite frankly, your logging functions suck, because they don't return any value. So you can't use "return ERROR()", which is the sane common use.
Plus they need that "context" thing which makes no sense and isn't even available except at "open" time.
It's not just open that wants to log things.
So no, I can't use that horrible thing. For example, even ignoring the brokenness of the "context" thing, something like:
if (badness) return report_error("badness %d happened", badness);
compared to:
if (badness) { ERROR(context, "Badness %d happened", badness); return DC_STATUS_WHATEVER; }
is the difference between "I can bother to make reasonable error reporting" and "f*ck this, I'll not bother logging the reason at all". So then I'd just do
if (badness) return DC_STATUS_FAIL;
so now there's no logging at all - just because your interface is bad.
After the open call, the context pointer is stored in the device/parser structure, and you can get it from there. Libdivecomputer doesn't log anything on its own. The actual logging is done by the application. That's on purpose. That's for example how subsurface is able to generate a libdivecomputer log directly from the GUI. I believe that's a very useful feature, which certainly doesn't sucks :-) It may require a bit more verbosity in our code, but it saves me *a lot* of time when dealing with support issues. So it's worth the effort. Trust me, you don't want to explain some Windows user how to redirect stderr to a file. In a Windows GUI app you don't even have stderr! So this is not something I'm willing to change.
But I can't even use that DC_STATUS_FAIL kind of thing. See later.
As to the "array_uint()" functions - you have no writing functions, and you're lacking a "u8". And your types are wrong, not allowing "void *". And they are insanely verbose. But whatever. I converted the ones you had.
Well, nobody said you may not add new functions. Why do you need void pointers here? You're parsing a byte array, which is by definition an unsigned char array. So where do you get void pointers from?
Out of memory error handling. I know the arguments for calling exit very well, but for libdivecomputer I made the choice to handle out of memory differently. Please stick to this pattern.
I made it use the dc_buffer_t, and just ignore out-of-memory cases. They don't happen, and if they did, they'd just result it empty buffers now.
That's fine. The dc_buffer_t functions handle out of memory internally.
Many functions return -1 in the case of an error. Can you rework this as dc_status_t values? BTW, if you want to use goto style error handling, that's fine.
No, I cannot use dc_status_t values, because they make it impossible to return an error *and* a value.
There is a very good reason why UNIX calling conventions are "return -1 on error".
That reason is the *non-error* case. Think of a simple function like "read()". Ask yourself what it returns on error, and what it returns on success.
Yes, I have used things like DC_STATUS_SUCCESS. I used VMS for some time. It's horrible. Suddenly you have to first check for success, and then have another interface to return the result size etc.
The whole "negative means error, zero or positive is success" is really powerful. It means that you can return how many bytes you read, and easily test whether there was something wrong, *without* having to have some "pass return size value as a pointer" model.
So please realize that my "send_receive()" function works like "read()" - it returns negative on error, and the number of bytes received on success.
I can't do that with DC_STATUS_XYZ.
I'm not going to argue which style is better or not. There are valid arguments for both styles, and you can get things wrong anyway. If you forget to return value from read() for negative values, things can go very wrong, and so on. The point is that I deliberately choose to not mix error codes and values in libdivecomputer. I have nothing against your implementation on its own. I'm only trying to maintain a consistent style throughout the entire codebase. If every backend starts to do things in a completely different way, then the end result becomes a mess. That's all. Nothing more, nothing less. But the main reason why I mention the -1 in the first place is that in most places you just return -1 to indicate a failure, while in some case you could give a more meaningful error. And then we're back at the dc_status_t values, because those are the "native" libdivecomputer error codes. BTW, one of the nice things about enums, is that whenever I see dc_status_t I immediately know it contains some status value. With plain int I need some more context. Is it success/failure (0 or -1), true/false (1 vs 0) or some value? Also in a debugger you get to see the symbolic name, not some random number.
For the name of the backend, I prefer a single word like "eonsteel" or just "steel" instead of eon_steel. This is just to follow the existing practice in libdivecomputer were backends are named DC_FAMILY_VENDOR_PRODUCT. So no underscores in the product part.
Did it for DC_FAMILY_SUUNTO_EONSTEEL.
Main structures (struct eon_steel, eon_parser) and all entry point functions (public or through the vtable) get the same backend prefix.
I *really* don't want to do this, though. The public ones are "suunto_eon_steel()" because they are visible to others. But I really object to writing that long name out for internal helper functions that are static. Why write that horribly long name for no gain?
I don't insist on it for all functions, only the entry point functions (e.g. the ones that end up in the vtable). The reason behind that is when running in the debugger it makes it easy to set breakpoints if the functions have a standard naming pattern. For the main structures, they extend the base structure, so I indicate that in the name. For example suunto_eonsteel_device_t extends the base dc_device_t structure.
The parser logic is a bit hard to follow without having some example data to look at. Can you share some data files?
I don't have any data files, I just run it on the dive computer. Remember: this is not a "memory dump" style device.
So the dc_device_dump() approach doesn't work, because on this dive computer you'd have to do it per-dive.
It would be easy to dump the parse data, but I don't think there is any such functionality: you expect the dump to be a memory dump of the whole device.
I know it doesn't support a standard memory dump. But a dump of one dive is fine too. I can use the parser standalone. That's actually something I want to address with the new dctool application. There are other devices that don't support memory dumps. And for debugging the parsing code it would be very convenient to be able to work with single dives. That's even easier than the simulator. Jef
On Tue, Oct 28, 2014 at 04:43:22PM +0100, Jef Driesen wrote:
I don't expect you to fix all Windows issues, but if you can already keep this in mind, that means less work afterwards.
I'll be happy to help with the Windows issues - as long as they relate to cross compiling from Linux. Maybe we can ask Lubomir to help with native compile questions...
There are also some header files like unistd.h which don't exist on windows.
I had no issues with header files when cross compiling Linus' code.
Several of the headers you include are not necessary at all, and can be removed. There are other things like the snprintf function which doesn't implement the C99 standard and returns a different value if the buffer is too short.
So what do you want done there? Is this something you will address?
I remember again what's the problem. The libusb header pulls in the windows.h header. And somewhere deep in there, there is a ERROR macro defined. I worked around that in the past by doing:
#ifdef HAVE_LIBUSB #ifdef _WIN32 #define NOGDI #endif #include <libusb-1.0/libusb.h> #endif
We don't need GDI at all, so it's no problem to disable it.
I did this for my tests and it works.
After the open call, the context pointer is stored in the device/parser structure, and you can get it from there.
Libdivecomputer doesn't log anything on its own. The actual logging is done by the application. That's on purpose. That's for example how subsurface is able to generate a libdivecomputer log directly from the GUI. I believe that's a very useful feature, which certainly doesn't sucks :-)
It may require a bit more verbosity in our code, but it saves me *a lot* of time when dealing with support issues. So it's worth the effort. Trust me, you don't want to explain some Windows user how to redirect stderr to a file. In a Windows GUI app you don't even have stderr! So this is not something I'm willing to change.
I'll be happy to rework the logging code :-)
But I can't even use that DC_STATUS_FAIL kind of thing. See later.
As to the "array_uint()" functions - you have no writing functions, and you're lacking a "u8". And your types are wrong, not allowing "void *". And they are insanely verbose. But whatever. I converted the ones you had.
Well, nobody said you may not add new functions.
Why do you need void pointers here? You're parsing a byte array, which is by definition an unsigned char array. So where do you get void pointers from?
Habit of a C developer would be my guess.
I'm not going to argue which style is better or not. There are valid arguments for both styles, and you can get things wrong anyway. If you forget to return value from read() for negative values, things can go very wrong, and so on.
The point is that I deliberately choose to not mix error codes and values in libdivecomputer. I have nothing against your implementation on its own. I'm only trying to maintain a consistent style throughout the entire codebase. If every backend starts to do things in a completely different way, then the end result becomes a mess. That's all. Nothing more, nothing less.
But the main reason why I mention the -1 in the first place is that in most places you just return -1 to indicate a failure, while in some case you could give a more meaningful error. And then we're back at the dc_status_t values, because those are the "native" libdivecomputer error codes.
BTW, one of the nice things about enums, is that whenever I see dc_status_t I immediately know it contains some status value. With plain int I need some more context. Is it success/failure (0 or -1), true/false (1 vs 0) or some value? Also in a debugger you get to see the symbolic name, not some random number.
And the horrible thing about enums is that you can't test for there existance. Anyway, so this needs to be rewritten to comply with the libdivecomputer conventions. I can do that if Linus is too annoyed with it. Fundamentally I'd like to know "what's the next step", "who is doing what". I really want to see this backend in libdivecomputer (stating the obvious here), so I'm motivated to help ;-) /D
participants (3)
-
Dirk Hohndel -
Jef Driesen -
Linus Torvalds