Patches to add basic support for the Suunto EON Steel

[Jef Driesen]

On 28-10-14 01:40, Linus Torvalds wrote:
> On Mon, Oct 27, 2014 at 3:57 PM, Jef Driesen <jef at 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