() (),
On Wed, Jun 14, 2017 at 8:14 PM, Jef Driesen jef@libdivecomputer.org wrote:
On 2017-06-09 07:28, Linus Torvalds wrote:
diff --git a/examples/common.c b/examples/common.c index 3c4561b..5ebdd77 100644 --- a/examples/common.c +++ b/examples/common.c @@ -54,6 +54,7 @@ static const backend_table_t g_backends[] = { {"smart", DC_FAMILY_UWATEC_SMART, 0x10},
{"g2", DC_FAMILY_UWATEC_SMART, 0x10}, {"meridian", DC_FAMILY_UWATEC_MERIDIAN, 0x20},I think you meant DC_FAMILY_SCUBAPRO_G2 here.
I actually did that one on purpose, since it talked about "backends", and I think this just does the second stage parsing thing, which is all UWATEC smart, no?
But it shouldn't matter, and I guess if there does end up being any differences, it's better to call it the SCUBAPRO_G2.
For consistency with the other uwatec/scubapro backends, replace DC_FAMILY_SCUBAPRO_G2 with DC_FAMILY_UWATEC_G2. Same remark for the filenames.
Please, let's absolutely *not* do that.
This thing is known as the "Scubapro G2". As far as I know, it's not sold anywhere as "Uwatec". Uwtec isn't mentioned in any place that I can find, so it's purely an internal detail, and calling it a UWATEC_G2 is actively misleading.
For end-users the fact that the internal name is uwatec instead of scubapro doesn't really matter, because they will only see the name "Scubapro G2".
This isn't aboue end-users. This is about developers. And for any *developers*, calling it "UWATEC_G2" is actively wrong.
There is no such thing afaik. I tried to google it in case it's perhaps sold as something else in Europe, but that doesn't seem to be the case.
There's just a Scubapro G2.
BTW, we now have 3 backends (smart, meridian and g2) using the exact same communication protocol, but with a different transport (irda, usb-serial and usbhid). I wonder if we can merge this back into a single backend some day? Because once the I/O refactoring lands, the difference between the transports will disappear and those backends will even look more similar.
I really don't think the IO refactoring will work.
The packetization rules are dive-computer-specific. There is no generic "USB HID" transport, for example: the actual HID packets have different rules depending on the dive computer.
For example, the EON Steel always sends full 64-byte HID packets, so the first byte (the packet payload size) is 0x3f (63). But then the Suunto EON Steel does its *own* packetization within that, and the second byte is the length of the actual payload data as far as the EON Steel is concerned.
And the Scubapro G2 also uses USB HID, but it doesn't have that two-level packet size thing, so for the G2, it just uses the HID size directly for the payload.
So you really cannot do some generic transport, because the actual rules of the transport will depend on the low-level dive computer anyway. The best you can do is to just have generic helper routines for one particular transport, the way libdc already does the USB HID helpers. But each dive computer really *does* need to then look at the USB HID packets independently.
The main difference is the transfer function. For the smart it's a plain IrDA read/write, for the meridian there is some additional framing added, and for the G2 there is the USB HID packet stuff.
Yes, the transfer function is the only thing different, but they are different in ways that are also different between different dive computer vendors (see above).
So you can do a "transport layer for smart/meridian/G2" abstraction, but you can *not* do a generic USBHID transport layer one.
At which point almost all of the point of the transport layer goes away, since it just ends up being internal to a particular dive computer family anyway, not truly generic.
case DC_FAMILY_SCUBAPRO_G2:rc = uwatec_smart_parser_create (&parser, context, 0x11,devtime, systime);
Is there a reason why you have hardcoded the model number to 0x11? Does the data contain a different model number? Because in that case we should update the parser to support the new model number.
I wasn't sure which number if would be, so I just picked the one that matched the galileo, but I thoyght t might change. And then it just worked, and I left it as that.
Also, even the galileo *itself* doesn't use the GALILEO define in the descriptor tables. See src/descriptor.c, it's all 0x11 there.In fact, the whole define only exists within the uwatec_smart_parser.c file, so I'm not sure what you really expect people to do.
+typedef struct scubapro_g2_device_t {
...unsigned int address;...+} scubapro_g2_device_t;
The address member is a left-over from the irda code and can be removed.
Will do.
There are a two issues with this code:
I'm going to ignore your nitpicking as long as you ignore my sane string interfaecs.
Jef, the reason I no longer send you patches, and just push stuff directly to the subsurface libdc repository is that I can't work with you.
I'll take your input, but I'm done fighting your oddities. Integrate *our* code that actually matters, and I can work with you. In the meantime, I'm not interested in nitpicking details that don't matter one whit.
Is the handshaking no longer necessary for the G2?
It didn't seem to make any difference for me, so I removed it.
But since some people seem to have download issues, I'm actually considering putting it back. My download trace from the Scubapro LogTRAK does have
out: > 01 1b in : < 01 01
out: > 05 1c10270000 in : < 01 01
as the first packet sequence (that is just a random cleaned-up packet thing: the numbers are packet length followed by packet data). Ad that matches the handshake sequence. So LogTRAK does do that handshake, but the G2 didn't seem to care.
I suspect the handshake is more important over some random serial protocol that doesn't have device ID's etc.
But as mentioned, because people clearly have some issues that I can't reproduce, I'm thinking of just doing that handshake anyway, just because it also doesn't seem to hurt.
I've always wondered why it's there for the smart and meridian because it doesn't really return any useful info. It's certainly possible that it can be omitted, but I never really tried that. So I'm just curious why you left it out.
I really just tried to minimize the data sent/receviced, but I don't have any other good reason for it.
This will need some improvements to report more fine-grained progress events.
The thing is, it already reports *better* progress events for errors thanks to the actual error handling in receive_data(), which gives descriptive error STRINGS for what went wrong, and that subsurface can actually show to users in sanen ways (in the error status at the bottom).
In contrast, the random crazy libdivecomputer error codes are completely useless.
This continues to be a contention point for me: your insistence on those useless random integer codes, when giving useful human-readable data with actual error descriptions and details on *what* went wrong is so much superior.
So all libdivecomputer really needs to return is "an error happened". The error *description* is done by the dc_context_log() interface that is able to give real information.
This is also why we use - and *NEED* - that string interface for other random dive computer data. There is absolutely no sane way to return info like "transmitter battery level at end of dive" with random codes. Or serial numbers, or deco algorithms. They are all *strings*.
So the scubapro G2 downloader returns error *strings* that can be displayed. Of course, you have ended up using macros that often get disabled, so if ENABLE_LOGGING isn't on, nothing gets logged. I'm wondering if that's in fact part of the problem with the current lack of any error output for people.
Linus