Scubapro Galileo 2 (G2)

[Linus Torvalds]

() (),

On Wed, Jun 14, 2017 at 8:14 PM, Jef Driesen <jef at 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