Patches to add basic support for the Suunto EON Steel

[Linus Torvalds]

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++).

> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-parser-add-DC_FIELD_DEVINFO-field-type-for-parse-tim.patch
Type: text/x-patch
Size: 2138 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141027/e0be7276/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Suunto-EON-Steel-support-downloading-of-core-dive-pr.patch
Type: text/x-patch
Size: 37910 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141027/e0be7276/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Suunto-EON-Steel-populate-dive-surface-pressure-and-.patch
Type: text/x-patch
Size: 6557 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141027/e0be7276/attachment-0005.bin>