Re: Patches to add basic support for the Suunto EON Steel
Linus, According to Dirk the plan is to have a subsurface beta ready before the eonsteel is available on the market. And thus is would be convenient to have a first version included in libdivecomputer. I'll do my best to meet that deadline. There has been quite some discussion on the new string based api for libdivecomputer. I'll need some more time to go through this whole discussion and give its some more thoughts. Therefore, I propose we target for a first version of the eonsteel backend without any of those string based api changes. So that's basically your 0001 patch. Those are not critical features, and trying to rush things is always a bad idea. So what I consider mandatory to fix before I'll commit your patches are: Naming conventions for entry point functions and the private device and parser structures. I know you don't like these long names, but that's the convention in the libdivecomputer codebase, and I'll insist on following existing practice. See the 0001 patch. Working build on Windows. I have attached a patch that does the necessary casts (and a few other things) to get the mingw and msvc builds to work. In most places, I simply added casts wherever the compiler complained, without checking whether that was the right thing to do. Many of those casts can probably avoided by using the correct type (unsigned instead of signed). Even the gcc compilers spits out tons of warnings, so you don't even need msvc to locate the type mismatches. BTW, if the goal is to demo a build to Suunto, then I think we should not only compile test on Windows, but actually try a Windows build too. They are probably using Windows. Proper integration with the libdivecomputer logging functions. The other ones I can live with and/or fix later. (more inline comments below) On 28-10-14 17:51, Linus Torvalds wrote:
On Tue, Oct 28, 2014 at 8:43 AM, Jef Driesen <jef@libdivecomputer.org> wrote:
There are also some header files like unistd.h which don't exist on windows.
Hmm. I wonder why I included them.
Part of it is probably that the whole back-end started out as this "suunto hacking tool" that did a lot of "print hex information of the packets". That's also why there was no libdivecomputer infrastructure, this thing was entirely stand-alone for just trying to gather traces and recreate them, and figure out what the things did.
No idea why you needed sys/types.h, fcntl.h, etc. Most of the other ones are also no longer necessary if the custom logging is removed.
I got no actual information from Suunto, but the fact that many things are just text helped. Seeing filenames etc in the dumps etc.. And the actual dive dumps have this interesting "each data has its type associated with it, and each type has a human-readable type descriptor". Which meant that I never needed to guess what the actual data means - the problem was figuring out the odd marshaling of the data, and the odd packet protocol.
I guess everyone does printf logging at some point. Me too :-)
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 :-)
So the logging I have is for protocol errors, or surprising things. Not useful to users. It's more of a protocol verification, I would *not* want it to show up in applications.
The logged info doesn't need to be useful for the user. The user only needs some super easy way to get the log and then send it back to someone that does understand what's inside. There are also multiple loglevels, which the application can control. ERROR and WARNING are self explanatory. INFO is used for logging the communication protocol (e.g. raw data packets), and DEBUG is for whatever else you would like to log.
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?
"void *" is the "random byte area" type. It's one of the things C++ completely screwed up. It's very useful when you use the helper function on random data (that may be embedded in structures etc, or give pointers to "uint32_t" etc when you convert from little-endian to big-endian etc.
But if there are no cases of that left over, maybe you can convert them all to "unsigned char *" ..
I very rarely need void pointers. Probably only for very generic functions like memcpy and friends. For byte arrays unsigned char's are perfect, and they also avoid the whole void pointer arithmetic problem.
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.
I hacked up subsurface to just dump the per-dive data to a "dive-%08x.log" file. That's *fairly* close to what the computer does internally, although this dumped data contains the date in the first 32-bit word, which is what the parser expects. On the dive computer, the date is encoded in the filename - see the
dc_buffer_append(file, buf, 4);
in eon_steel_device_foreach() where I append it to the data buffer instead, since the parser doesn't have access to the filename.
That looks very reasonable. Is it always a 32bit number? If it could be a 64bit timestamp, then it might be safer to reserve 8 bytes here. One Y2038 problem less :-) This timestamp is also a good candidate for use as the libdivecomputer fingerprint!
(The dive date is *also* encoded in the dive data, but in a very inconvenient format with a dynamic type, so I wanted the data from the filename for simplicity).
So here are the dumps of the dives I have on that dive computer so far, in that "parser format".
That first dive is just me lowering the dive computer into the neighbors pool. The other 21 dives are the ones we did in Bonaire.
Your data files where too large for the list. I got them, but if you want I can still manually approve your mail.
Dirk has one of the Intel engineers create a contraption to lower the thing into the pool multiple times, so that I can see what happens to the directory listing when it no longer fits in one "readdir" reply (the packets may be 64 bytes each, but they are chained up to be replies that can apparently be up to 1kB in size - at least I haven't seen anything bigger than that yet. And I don't know what happens when the directory listing is bigger than that, because the 22 dives I have so far create only about 460 bytes of "readdir reply").
A small pressure chamber might be even more convenient: no need to have a pool around. Jef
On Fri, Oct 31, 2014 at 3:25 PM, Jef Driesen <jef@libdivecomputer.org> wrote:
According to Dirk the plan is to have a subsurface beta ready before the eonsteel is available on the market. And thus is would be convenient to have a first version included in libdivecomputer. I'll do my best to meet that deadline.
It's not a big deal. I'd love to be able to say that subsurface supports the EON Steel, but if it's just a preliminary build of subsurface with a temporary hack for the dive computer, that's fine. I'd prefer for there to be device ID's (and that needs the string patches, even if it might then be implemented another way later), because without that the dive matching is much less reliable. Anyway, my point being that it certainly doesn't need to be some "final released form". More of a "Suunto gave me access to an EON Steel, so here's a random build of subsurface that you can use if you want to".
Naming conventions for entry point functions and the private device and parser structures. I know you don't like these long names, but that's the convention in the libdivecomputer codebase, and I'll insist on following existing practice. See the 0001 patch.
All patches look fine to me. Although I think Dirk had another patch for some Windows issue. I doubt Dirk cares one way or another.
BTW, if the goal is to demo a build to Suunto, then I think we should not only compile test on Windows, but actually try a Windows build too. They are probably using Windows.
Dirk had that working already, afaik. Although he uses gcc and cross-compiles, so he didn't hit some of the issues your build would have.
Proper integration with the libdivecomputer logging functions.
I'll take a look, today has been Linux work for me (and will be halloween later().
No idea why you needed sys/types.h, fcntl.h, etc. Most of the other ones are also no longer necessary if the custom logging is removed.
The sys/types.h and fcntl.h etc are for "open()". I suspect unistd.h was for similar direct access. Remember, my EON steel debug application was stand-alone.
I very rarely need void pointers.
It's not about "need". It's about "the right thing". The fact that C++ screwed up "void *" is a sad commentary on the crap that is C++. "void *" is simply the right type to use when the type is some opaque generic byte stream. Exactly because it doesn't need pointless casts.
For byte arrays unsigned char's are perfect
The thing is, it's not always a "byte array". Sometimes it's a constant string (which is *not* "unsigned char"). Sometimes it might be a packed structure. Etc etc. "void *" does that right, in ways that "unsigned char *" does not. Of course, if you have to use C++ because of Windows breakage, you don't ever see this, because you're fighting language bugs.
I hacked up subsurface to just dump the per-dive data to a "dive-%08x.log" file. That's *fairly* close to what the computer does internally, although this dumped data contains the date in the first 32-bit word, which is what the parser expects. On the dive computer, the date is encoded in the filename - see the
dc_buffer_append(file, buf, 4);
in eon_steel_device_foreach() where I append it to the data buffer instead, since the parser doesn't have access to the filename.
That looks very reasonable. Is it always a 32bit number? If it could be a 64bit timestamp, then it might be safer to reserve 8 bytes here. One Y2038 problem less :-)
It's actually always a 32-bit number, and it's encoded on the computer as a 8-byte hex string. The Suunto EON Steel filesystem looks suspiciously like a FAT filesystems. Names are case-insensitive and of type "8.3" (even if they just look like strings in the protocol). The dive directory is "0:/dives", but I checked, and "a:/dives" actually also works ;) So the dive files on the device are actually named "%08x.LOG". And yes, I realize that this means that Suunto expects to redesign something before 2038 rolls around. We'll see how they do it. It may be that by then, we'll have to actually parse the string-format date. Or maybe they'll change their filesystem (the packet-based path interface is *not* limited to 8.3 names, so they could do it without breaking things). I suspect one reason for the 32-bit timestamp is just Suunto's own DM5 application too. It shows some limitations in other forms: it shows temperature in whole degrees Celsius, even though the EON Steel actually returns things in tenth of a degree. I suspect there's a lot of legacy code there for the older Suunto dive computers, which (if I recall correctly) also do a 32-bit timestamp in seconds since 1970.
This timestamp is also a good candidate for use as the libdivecomputer fingerprint!
I actually use not the timestamp, but the time *string* as the fingerprint. You should be able to see it from the log-files I sent you: the 4-byte timestamp is what I use for time, but therre;'s actually this (inconvenient) "Header.DateTime" thing that gives date and time as a string with *milliseconds*. I give that as the "Dive ID" string, and then generate the dive fingerprint by hashing that string. And that better fingerprint is not available until after parse time, once more. The parsing is a bit complex, as you probably saw, with embedded dynamic type information (well, _some_ of it is dynamic, including the DateTime string, while some of the simpler fields seem to be encoded in a static form - the type is a two-byte entity, and it looks like the upper byte is "static vs dynamic". So a type of "0x0010" is a static type and has had the same meaning in all the dives I've seen, while a type of 0x0110 is dynamic and seems to depend on the dive - so the types really have to be parsed on a dive-by-dive basis). I actually really like the Suunto marshaling of data, but simple it is not. Nothing is at fixed offsets etc.
Your data files where too large for the list. I got them, but if you want I can still manually approve your mail.
No, I don't think anybody else really needs them. If somebody asks, I'll forward them. Or you can, if somebody asks you for test-cases. It's not like they are secret or contain sensitive information, but no need to put them on the list if nobody asks for them.
A small pressure chamber might be even more convenient: no need to have a pool around.
Oh agreed. I don't have one, though. I'd love to, but I suspect anything that would be automatically controllable runs in the thousands of dollars. The "dip into the pool" is an arduino and a motor, and can be built by crazy Intel engineers without any fear of explosions. Linus
On Fri, Oct 31, 2014 at 4:57 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
On Fri, Oct 31, 2014 at 3:25 PM, Jef Driesen <jef@libdivecomputer.org> wrote:
Proper integration with the libdivecomputer logging functions.
I'll take a look, today has been Linux work for me (and will be halloween later).
Ok, here's a re-send of the libdivecomputer patches (no change to the subsurface part, so I'm not resending that patch). The two DC_FIELD_STRING patches haven't really changed, but they aren't identical either, due to changes around them. This - once again - squashed all the changes into one "EON Steel support patch": your three patches and the change to use the libdivecomputer ERROR() function. It's still wrapped with the "report_error()" function due to return value issues (returning DC_STATUS_SUCCESS on error? That's just odd, but regardless, the internal EON steel functions return number of bytes transmitted etc, so the enums just wouldn't work even if they were actually error numbers),. If you'd prefer an incremental diff instead, just holler. But I rebased on top of your tree, and you hadn't applied the patches, so I was assuming you want a clean resend. Linus
On 01-11-14 19:29, Linus Torvalds wrote:
On Fri, Oct 31, 2014 at 4:57 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
On Fri, Oct 31, 2014 at 3:25 PM, Jef Driesen <jef@libdivecomputer.org> wrote:
Proper integration with the libdivecomputer logging functions.
I'll take a look, today has been Linux work for me (and will be halloween later).
Ok, here's a re-send of the libdivecomputer patches (no change to the subsurface part, so I'm not resending that patch). The two DC_FIELD_STRING patches haven't really changed, but they aren't identical either, due to changes around them.
This - once again - squashed all the changes into one "EON Steel support patch": your three patches and the change to use the libdivecomputer ERROR() function. It's still wrapped with the "report_error()" function due to return value issues (returning DC_STATUS_SUCCESS on error? That's just odd, but regardless, the internal EON steel functions return number of bytes transmitted etc, so the enums just wouldn't work even if they were actually error numbers),.
Just a quick comment, because I'm running out of time right now. I still prefer to have proper integration of the logging functions. Calling ERROR from your report_error is an ugly hack that throws away several of the nice features of the ERROR macro. For example the line numbers and function names will always be those of your report_error() function, hiding the real source. And when building with all logging disabled, your report_error function is still being called. Your debug() function is still there too. Basically I want all printf's (directly or through a wrapper function) replaced. Also, my patch with the casts was merely to indicate where the problems were, and not supposed to be applied as is. Ideally I would rather see the root problem "fixed" instead of fixing the symptoms. For example by changing dive_directory to an array: -static const char *dive_directory = "0:/dives"; +static const char dive_directory[] = "0:/dives"; and then calling memcpy instead of strcpy: - strcpy((char *)cmd+4, dive_directory); - cmdlen = 4+strlen(dive_directory)+1; + memcpy(cmd + 4, dive_directory, sizeof(dive_directory)); + cmdlen = 4 + sizeof(dive_directory); Saves a strlen call too :-) This is just an example, but I think you'll understand what I mean.
If you'd prefer an incremental diff instead, just holler. But I rebased on top of your tree, and you hadn't applied the patches, so I was assuming you want a clean resend.
A clean resend is what I prefer as well. These incremental changes don't add much value to the git history. Jef
On Nov 3, 2014 8:12 AM, "Jef Driesen" <jef@libdivecomputer.org> wrote:
I still prefer to have proper integration of the logging functions.
Calling ERROR from your report_error is an ugly hack that throws away several of the nice features of the ERROR macro. By it fixes one huge big mistake in that macro namely the broken return value. Even if I could use the enums (which are not appropriate), the enum you return is useless, since it's "success*. But i can make my wrapper a macro, that _just_ fixes the return value. That ok? Linus
This does the "use a macro to make the ERROR() return value work for the code". But I refuse to use "memcpy()" on strings, just because you have a broken shit-for-brains compiler that doesn't even compile the language the file is in, and that you care about more than about code sanity. I added the cast to shut it up, even though I disagreed with it. But I'm not going to fight that insanity any more. This is my final submission. I'm not going to bother any more, really. I can download from my Suunto EON Steel, and by now I no longer care one whit whether anybody else can. It's your project Jef, you run with it if you want to. But I'm done. Linus
On 03-11-14 17:23, Linus Torvalds wrote:
On Nov 3, 2014 8:12 AM, "Jef Driesen" <jef@libdivecomputer.org <mailto:jef@libdivecomputer.org>> wrote:
I still prefer to have proper integration of the logging functions. Calling
ERROR from your report_error is an ugly hack that throws away several of the nice features of the ERROR macro.
By it fixes one huge big mistake in that macro namely the broken return value. Even if I could use the enums (which are not appropriate), the enum you return is useless, since it's "success*.
Now I realize why you have been complaining about the logging functions and the dc_status_t enum. It's about the return value of the dc_context_log() function itself, which indicates whether the logging was successful or not. Of course that's not what you want to return from the backend code. The return value of the logging function is nearly alway irrelevant, and you typically just ignore it: if (failure) { ERROR(context, "Some error message."); return -1; /* or some DC_STATUS_XXX value depending on the function */ } That's completely independent from the question about returning -1 or an enum value. The above construct works for both. And other then being a bit more verbose with one line extra, how's that different compared to yours: if (failure) { return report_error("Some error message."); /* Returns -1 */ } Jef
On Mon, Nov 3, 2014 at 3:18 PM, Jef Driesen <jef@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
On 2014-11-04 01:06, Linus Torvalds wrote:
On Mon, Nov 3, 2014 at 3:18 PM, Jef Driesen <jef@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
On Wed, Nov 5, 2014 at 8:00 AM, Jef Driesen <jef@libdivecomputer.org> wrote:
This a mainly a matter of personal preference.
That's one way of looking at it. It's an odd way. Compact code without unnecessary syntactic overhead is generally considered universally a good thing. Not "personal preference". Sure, if your employer ends up giving bonuses by number of lines written, it's a bad thing, so I guess you can call it "personal preference" at some level, but people actually design whole programming languages on the principle that concise representation of the problem is a good thing. No, C is not one of those concise languages. With great power comes great number of lines, paraphrasing uncle Ben. C has all that syntactic noise for doing almost everything manually and describing the solution in sometimes very tedious detail indeed. So conciseness is not the only - or even the primary - issue, but when you have two solutions that are otherwise completely semantically identical, the concise solution without unnecessary syntactic sugar is generally considered the superior one. That is *not* some subjective thing. That's a very objective statement. It's also - despite all the manual work you have to do in C - how the C language and its base runtime is designed. Compare C to contemporary languages that did similar things, and notice how C comes from the background of being fairly information-dense. You can use assignments as part of expressions, you have a lot of library functions that return redundant information because it often helps write expressions involving them more dense etc etc. So even C - despite being a very verbose language in requiring the programmer to specify every little detail manually - is actually trying to be fairly dense in other ways. Dense, information-rich code is better code. Sure, you can take it a bit *too* far, and make it so dense that it's unreadable (APL, traditional dense "write-only" perl code, C obfuscation contest etc), but my examples certainly didn't do that. I didn't try to cram things onto a single line, for example. But I definitely stand by my point: "designing things so that they are unnecessarily verbose" is universally considered a *bad* thing.
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:
Correct. But if I return "-errno", I do it because I expect the *caller* to report the error. So I expect the caller to do something like if (rc < 0) fprintf(stderr, "Could not open file %s: %s", filename, strerror(-rc)); (the above ignores the fact that there may be several layers of functions that just return the error code back up - only the "top-most" caller generally does the actual error reporting, and the "direct" caller often just returns the error number without any commentary on it). So that is the common interface for library routines. They return error codes so that the callers ("the real code") can decide how - and whether - reporting an error is appropriate at all. Maybe the caller doesn't even consider the "error" to be an error at all - the caller just wanted to first *try* to do something, and if that fails, it has a different fallback. See? That's the normal thing a library would do: not enforce any logging model at all, because not all errors should necessarily even be logged in the first place. So you can do two models: (a) return an error code that contains the error information (it is a *negative* error, because positive or zero contains information about the successful case) This is the normal C library model. (b) do the whole logging thing, and then just return the fact that errors happened (again, negative error, because the non-error case wants to return data too) This is obviously what you do but doing *both* (a) and (b) is just redundant. If you log an error message, you migth as well just say "-1". You've already done the descriptive thing, much more so than some individual error code. Also, quite frankly, while the whole "-errno" thing is how we do things inside the kernel, and while I think it's the superior model, in user space I would generally do the whole "-1" thing, just because it's what people are more used to. That's especially true since you would tend to have to mix your library code with *other* library code, and the -1 model is the norm. In the kernel, we don't have that issue. We don't use random libraries. We _can't_ use random libraries. Even when we end up using interfaces that *look* like standard libraries, we have to reimplement them by hand (or rely on the compiler just doing it automatically for us). So the kernel can use the more efficient "-errno" model without confusion. Linus
On Nov 3, 2014 8:12 AM, "Jef Driesen" <jef@libdivecomputer.org> wrote:
Also, my patch with the casts was merely to indicate where the problems
were, and not supposed to be applied as is. Ideally I would rather see the root problem "fixed" instead of fixing the symptoms. But the root problem is that you're compiler is wrong. Anything else is just papering over that issue. The code is C, and better idiomatic C at that. How the heck can I fix the "root" problem that you aren't even using a proper C compiler, and then make up your own rules about what code is supposed to look like, but those rules aren't even the patches you send me? Jef, that's just crazy. If you have random personal rules that don't even make sense in the context of the language, then make those changes yourself. Don't make me have to guess what your are thinking. I don't have ESP and unlike you I don't have a broken compiler, or think that I should care about the known-broken warning messages from it. So now you tell me that the patch y patch sent me want even what you wanted. Really, I'm not willing to play some clown to your crazy rules. Dirk, please take over. I'm done. Linus
On 03-11-14 17:31, Linus Torvalds wrote:
On Nov 3, 2014 8:12 AM, "Jef Driesen" <jef@libdivecomputer.org <mailto:jef@libdivecomputer.org>> wrote:
Also, my patch with the casts was merely to indicate where the problems were,
and not supposed to be applied as is. Ideally I would rather see the root problem "fixed" instead of fixing the symptoms.
But the root problem is that you're compiler is wrong. Anything else is just papering over that issue. The code is C, and better idiomatic C at that.
How the heck can I fix the "root" problem that you aren't even using a proper C compiler, and then make up your own rules about what code is supposed to look like, but those rules aren't even the patches you send me?
Jef, that's just crazy. If you have random personal rules that don't even make sense in the context of the language, then make those changes yourself. Don't make me have to guess what your are thinking. I don't have ESP and unlike you I don't have a broken compiler, or think that I should care about the known-broken warning messages from it.
So now you tell me that the patch y patch sent me want even what you wanted. Really, I'm not willing to play some clown to your crazy rules.
Dirk, please take over. I'm done.
Well, I can't really do much about the lack of C99 support in the msvc compiler. I'm the first to admit that compiling as C++ is one ugly hack. But I'm willing to pay that price because msvc support matters for many Windows developers (and no, I'm not one of those). Windows happens to be one of the officially supported platforms. It's fine if *you* don't care about that, but libdivecomputer does... and thus every new backend too. Anyway, I was just trying to be helpful and point out where the issues with msvc where, in order to get the eonsteel backend ready as soon as possible. I don't think I ever said you have to fix those issues all by yourself. If you told me "Hey Jef, Here's what I have already. It works fine on Linux, but I don't want to deal with all those crazy msvc workarounds. Just take the code and do whatever you need to do to get it working with msvc." or something like that, then that would have been fine too. Then me, or someone else, can take care of that. It's a public mailinglist after all. Granted, it would likely take a bit longer before I would be able to work on that. But... and correct me if I wrong, I had the impression that we wanted to have this ready as fast as possible. So I did an effort to provide feedback as quick as possible and try to clearly explain the kind of changes I wanted. And that included those extra msvc tricks. So I just fired up my Windows VM to check what the msvc compiler didn't like and send you a quick "patch". Maybe I should have been a bit more explicit there. Do you really consider the changes I asked unreasonable? The only changes I really insist on are proper integration into the existing infrastructure, and a working build on all supported platforms. That doesn't sound crazy to me. And here I'm not even talking about those pointer casts (which are strictly speaking also required for proper C code, except for void pointers). A small analogy. What would you say if I submit a linux kernel patch and say "Hey, I didn't like the existing printk function, so I just used my own custom logging function." Won't your response be something along the lines of "Hey, whether you like it or not, but the kernel uses printk, so please use that."? Because that's what I have been asking. So how do we proceed next? Do I take your code and integrate the logging and msvc stuff myself? Jef
On Tue, Nov 04, 2014 at 12:18:20AM +0100, Jef Driesen wrote:
So how do we proceed next? Do I take your code and integrate the logging and msvc stuff myself?
Yep, I think that sounds like a reasonable next step. /D
On 2014-11-04 00:22, Dirk Hohndel wrote:
On Tue, Nov 04, 2014 at 12:18:20AM +0100, Jef Driesen wrote:
So how do we proceed next? Do I take your code and integrate the logging and msvc stuff myself?
Yep, I think that sounds like a reasonable next step.
I have finished the necessary changes. I have kept everything as separate patches. So if Linus doesn't like them, they'll they have my name attached :-) The last three patches are bugfixes, and are not related to some windows or msvc problem. The last one is still a bit work-in-progress. The memory leaks are gone, but I still need to check whether I didn't break anything else by accident. @Linus: Can you have a look? You know the code better than I do. Jef
On 2014-11-13 16:33, Jef Driesen wrote:
On 2014-11-04 00:22, Dirk Hohndel wrote:
On Tue, Nov 04, 2014 at 12:18:20AM +0100, Jef Driesen wrote:
So how do we proceed next? Do I take your code and integrate the logging and msvc stuff myself?
Yep, I think that sounds like a reasonable next step.
I have finished the necessary changes. I have kept everything as separate patches. So if Linus doesn't like them, they'll they have my name attached :-)
The last three patches are bugfixes, and are not related to some windows or msvc problem. The last one is still a bit work-in-progress. The memory leaks are gone, but I still need to check whether I didn't break anything else by accident.
@Linus: Can you have a look? You know the code better than I do.
I have one specific question about the type_desc array. It seems it's only populated, but never used for anything. Is this some leftover from earlier prototyping, or is this something that should be kept? The reason why I'm asking is that instead of trying to fix the memory leaks, I could also just remove this code (see attached patch). Once that's clarified and fixed, I'll push the eonsteel backend to the repository. There are still a few features missing, like the fingerprint support, but that's something we can always address later. Jef
On Fri, Nov 21, 2014 at 2:39 AM, Jef Driesen <jef@libdivecomputer.org> wrote:
I have one specific question about the type_desc array. It seems it's only populated, but never used for anything. Is this some leftover from earlier prototyping, or is this something that should be kept?
It's used. See the whole entry traversal, traverse_dynamic_fields() in particular. This is not prototyping code. It isn't needed for the really basic sample data (which uses the types that always seem to be static), but all the events etc need it. Maybe you're still working with just the basic model because you haven't done the DC_FIELD_STRING parts yet. Linus
On 2014-11-21 20:51, Linus Torvalds wrote:
On Fri, Nov 21, 2014 at 2:39 AM, Jef Driesen <jef@libdivecomputer.org> wrote:
I have one specific question about the type_desc array. It seems it's only populated, but never used for anything. Is this some leftover from earlier prototyping, or is this something that should be kept?
It's used. See the whole entry traversal, traverse_dynamic_fields() in particular.
This is not prototyping code.
It isn't needed for the really basic sample data (which uses the types that always seem to be static), but all the events etc need it.
Maybe you're still working with just the basic model because you haven't done the DC_FIELD_STRING parts yet.
You're right, I'm only looking at the basic patch without the new string stuff. So I left the code, and just fixed the memory leak. The eonsteel backend is now on the master branch. Jef
On Mon, Nov 24, 2014 at 5:40 AM, Jef Driesen <jef@libdivecomputer.org> wrote:
You're right, I'm only looking at the basic patch without the new string stuff. So I left the code, and just fixed the memory leak.
The eonsteel backend is now on the master branch.
.. and here are the updated patches to support the extended information. Things still seem to work, although subsurface now has a somewhat misleading debug message: string field "FW Version": 1.0.3 -- overwriting the existing firware of (null) string field "Serial": #p1437019 -- overwriting the existing serial of (null) which might even SIGSEGV on non-Linux targets due to that "%s" with NULL. But that's purely a subsurface debug message issue. Linus
participants (3)
-
Dirk Hohndel -
Jef Driesen -
Linus Torvalds