Patches to add basic support for the Suunto EON Steel

[Jef Driesen]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-naming-conventions.patch
Type: text/x-patch
Size: 14526 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141031/dbf2e2ee/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Fix-windows-build-issues.patch
Type: text/x-patch
Size: 1489 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141031/dbf2e2ee/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Fix-pointer-casts.patch
Type: text/x-patch
Size: 8794 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141031/dbf2e2ee/attachment-0005.bin>