Patches to add basic support for the Suunto EON Steel

[Jef Driesen]

On 2014-11-04 01:06, Linus Torvalds wrote:
> 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).

This a mainly a matter of personal preference. You seem to prefer 
compact code, while that's not the highest priority for me. For example 
I also do like empty lines, because for *me*, the extra whitespace helps 
me to visually separate the code blocks better. But in the end it's all 
very subjective, and there is simply no right answer. We can keep 
arguing about this forever, but that'll leads us nowhere and only a 
waste of time.

This is getting even more off topic, but there is one last thing I'm 
just curious about. When you say that returning -errno is superior 
compared to returning -1 and a separate errno, then I absolutely agree. 
But isn't that somewhat contradictory with your preference for using the 
report_error() function? With the report_error() function, you can only 
return -1, and not some more descriptive errno value. Unless you also 
pass the errno as a parameter:

int report_error(int err,...)
{
     /* Actual logging goes here. */
     return err;
}

if (failure) {
     return report_error(-errno,...);
}

That's something I find confusing, because unless I know the 
implementation of the report_error function, it's not immediately 
obvious which value will be returned. With the return statement on an 
extra line there is no such confusion, because the return value is right 
there:

if (failure) {
     report_error(...);
     return -errno;
}

Jef