Scubapro Galileo 2 (G2)

[Jef Driesen]

On 2017-06-14 23:19, Linus Torvalds wrote:
> 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.

That table is used for selecting the correct device descriptor when 
using the dctool --family option. It's used for both downloading and 
parsing. With the wrong type in the table, it will pick the IrDA smart 
backend for downloading, which obviously won't work for the G2. Parsing 
will still work because the G2 uses the same smart backend.

>> 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.

The dive computer is indeed named Scubapro G2. But the underlying 
protocol and data format is the Uwatec Smart protocol. That means that 
for libdivecomputer, it's part of the existing Uwatec family. If it was 
a completely different protocol, then a new family would make sense, but 
that's not the case here.

I already used that naming scheme for the Scubapro Meridian, and for 
consistency I would like to continue the existing practice.

BTW, the reason why the family type has two parts (vendor and product) 
and not just the product part alone, is simply because I wanted to group 
related stuff together. Having all the suunto, mares, uwatec, ... files 
and functions sitting nicely next to each other (due to the common 
prefix) is very handy. You may consider that a silly reason, but it does 
work very well for me!

>> 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.

You are right that the USB HID packets are vendor specific, and we can't 
incorporate that in the usbhid code. But that's not really what the I/O 
refactoring is about. It only handles how those packets are being 
send/received, not their content.

The I/O layer abstraction will let you send those USB HID packets over 
something else then USB HID. In this particular cases, that does not 
make much sense, but there are cases where it does. For example classic 
bluetooth (rfcomm) and their emulated serial ports. That's basically 
just a difference in the underlying I/O API that is being used.

Note: if we're lucky and the Bluetooth Low Energy (BLE) uses the same 
type of packetization as their USB HID implementation, then we just need 
to swap the I/O layer, and we're done.

>>> +       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.

The Uwatec communication protocol (like many others), exchanges the 
exact model number during the download. In that case, we simply ignore 
the model number from the descriptor and always use the detected model 
number. That's more reliable because end-users sometimes select the 
wrong device (especially for devices with very similar names). With the 
autodetection everything will just work, as long as the end-users picks 
a device from the right family. But of course that fails if you hardcode 
the model number.

If the G2 model number is different from 0x11 (I haven't seen any G2 
data yet, so I simply don't know), then the new model number should be 
added to the parser.

Note that the hardcoded number will also cause trouble if some future 
model is introduced with a different model number.

>> 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.

I'm only trying to maintain a consistent style throughout the entire 
codebase. If I let one backend do things completely different, then we 
soon end up with an inconsistent mess that is difficult to maintain. You 
may disagree with the current conventions, but that's another 
discussion.

>> 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.

If LogTRAK does the handshaking, then I think it's indeed a good idea to 
add it back. We don't really know its purpose, but it might be something 
relevant after all.

>> 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).

I wasn't referring to error reporting, but to the normal 
DC_EVENT_PROGRESS events. Now you jump from 0% at the start of the 
download to 100% at the end of the download, without any updates in 
between. For just one or a few dives that may not be a big deal, but for 
larger downloads that's not very nice. (I don't know how fast the 
download is.)

> 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.

I never said you should remove the more detailed logging. On the 
contrary, I'm perfectly fine with that! They are absolutely a great aid 
during debugging and troubleshooting!

If you ask me, returning error codes is a common practice in many C 
libraries. So I don't think they are useless.

> 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.

The logging is enabled by default. Thus, unless you explicitly disable 
it, libdivecomputer is *always* build with logging enabled.

Jef