Patches to add basic support for the Suunto EON Steel

[Linus Torvalds]

On Mon, Nov 3, 2014 at 3:18 PM, Jef Driesen <jef at libdivecomputer.org> wrote:
>
> And other then being a bit more verbose with one line extra, how's that
> different compared to yours:

One extra line? It's more than that. It's the difference between

   if (something_failed)
      return -1;

with no messages at all, vs

   if (something_failed)
       return error("Describe the failure"):

that looks fairly similar but is now _descriptive_ which is generally
a bonus (maybe it even means that you can avoid a comment), and
finally

   if (something_failed) {
     ERROR("Describe the failure");
     return -1;
   }

which I actually think is *worse* than even the first case, because
now error handling actually ends up being bigger and more visually
distracting than the actual real code.

See the difference?

So it's not just two lines more, and "more verbose". It actually ends
up visually noticeably more distracting from the important part of the
code itself, which is not the error handling, but the *normal* case
code.

No, it's not a big deal in individual cases. If you have one of these
in your project, who cares? Nobody. But you don't have one of them.

Look at that "receive_data()" function, for example. My
"error_report()" function makes that small loop much more readable,
exactly because the error handling doesn't end up blowing up all that
code, and the whole loop easily fits in one screenful, even if you
have a traditional 80x25 terminal.

THAT is important. Having error handling that doesn't detract (too
much) from the actual real code.

If I'd have your "ERROR()" function that returns the completely
useless SUCCESS enum, I'd literally have to remove all the messages
entirely, and just go back to the simple "return -1" to get readable
code. Or I'd have to make more than half of that simple loop be just
error handling noise, and not fit on a screen any more.

As it is, now that that code "just works", removing the error messages
entirely is probably fine. Of course, then if Suunto changes some
protocol thing that I assumed, or there's some odd USB issue, you'd
now be screwed.

So really. Look at that one "receive_data()" function. It's 30 lines
total, of which two are that debugging comment for when you want to do
per-packet dumps (yeah, hopefully never, but I ended up enabling that
quite a lot while doing early development). And it shows exactly why I
want that "report_error()" interface, because it has four very
different error conditions in that inner loop. The actual "work" of
the inner loop (ignoring errors) is already just seven lines (five
lines of "work", and two lines for the exit condition).

Those *seven* lines are what is important. The rest is annoying error
handling that right now is already 8 lines. But at least it is *only*
eight lines. It's not sixteen. The error handling as it is is already
bigger than the "real work", but hey, at least they are minimally
bigger, and you can certainly argue that the error handling adds
value.

Sixteen lines of error handling, for seven lines of actual working
code? That's not a good tradeoff. I understand that some CS people
think that "the more error handling, the better", but that's just
insane.

Code is better if you can make the error handling *less* intrusive,
not more. Yes, you want to handle error cases, but you want to handle
them without swamping the real work and making it harder to read.

That one simple function also shows *why* that "-1" is special, in
ways that an enum is not. Because a positive return value is actually
a meaningful return value. It's not just 'success vs failure'. It's
'return data vs failure'. The enum simply cannot do that.

Side note: receive_data() is not the only example. Look at
"send_receive()". Exactly the same issue. Do you want to fill your
screen with real code, or with useless noise just because your code
interfaces were bad? It has six different things it validates, and
like receive_data(), it actually returns real information on success,
so again an enum of failure reasons wouldn't work.

And yes, I realize that the whole "return negative on error" is
somewhat idiomatic traditional C and UNIX interface. If you come from
another background, it isn't *nearly* as natural. But there's a reason
why idiomatic C and UNIX has been a very successful model. It's a
*good* model. It's not just me, it's not just libdivecomputer, it's
been a very successful model in a ton of big projects. There's a
reason DOS and VMS died. They were horrible horrible models.

It is *not* a random accident that you have UNIX system calls return
negative error codes, and have 0/positive be success. It's actually
fundamentally good design. Those original UNIX people knew what they
were doing.

(Of course, I actually think that the Linux model of not just
returning "-1", but returning "-errno" is even more superior, because
unlike "errno" it is also trivially thread safe and denser from an
information standpoint. So a "negative enum for error cases" is
actually a perfectly fine model, and imnsho actively superior. That
freaks some people out just because they are used to the whole "-1
means error" model, though).

                Linus