Scubapro Galileo 2 (G2)

[Jef Driesen]

On 2017-06-22 02:49, Linus Torvalds wrote:
> On Tue, Jun 20, 2017 at 11:59 PM, Jef Driesen <jef at libdivecomputer.org> 
> wrote:
>> 
>> 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.
> 
> Ok. That will work. In fact, it's what I'm doing for the BLE support
> to make USB HID and BLE be real choices.
> 
> But it almost certainly has to be integrated with the "custom serial"
> kind of model that allows the *outside* to set the packet sending
> details, rather than being something internal to libdivecomputer.
> 
> And that's because libdivecomputer almost certainly will never be able
> to do BLE sanely. Sending packets over GATT a very complex and nasty
> protocol, not anything like the fairly straightforward "serial line
> over bluetooth" that rfcomm is.
> 
> We're using QtConnectivity in subsurface for BLE (well, in my
> experimental patches), and while I'm making some progress, we've
> actually found two major issues in QtConnectivity already exactly
> because BLE is very complex. And that was after I found some issues in
> Bluez earlier just to pair the Suunto EON Steel in the first place.
> 
> So I do have a set of three patches that does this, and turns the
> "custom_serial" thing into a "custom_io" thing (that allows both a
> serial _and_ a packet interface), and lets devices use that.
> 
> I then made a simpe helper to say "if no custom IO has been defined,
> create a custom IO definition for this USB HID device", and converted
> both the Suunto EON Steel and the Scubapro G2 profile over to that
> helper.
> 
> That actually allows me to feed in a custom packet handling set of
> routines, and I'm now experimenting with BLE, but am hitting
> QtConnectivity issues.
> 
> You can see my libdvicecomputer work at
> 
>    git://github.com/torvalds/libdc-for-dirk.git Subsurface-branch
> 
> where the top three commits do that packet interface (it needs a
> subsurface patch to then work with subsurface, which is why those
> three patches haven't been pushed out to the real repo yet - I'll need
> to synchronize with Dirk).
> 
> NOTE! It should be *trivial* to do the same kind of thing for a serial
> line as I did for USB HID, ie a dive computer backend that sends
> "packets" over a serial line could do a very similar "open serial line
> with these settings" and then create a packet-sending custom_io
> handler for that. So long-term, I'd actually like to remove the
> "serial" part of the custom_io struct entirely. But I left it alone
> for now, just to minimize the churn, and because I really just want to
> work on the BLE side.

This all sounds very similar to the I/O refactoring I proposed. Minus 
the packetization stuff of course (because that wasn't necessary yet at 
that time). But that's trivial to add. We only need to expose the packet 
size (just like you did), so you can use the read/write functions with 
the right size.

I don't think it's that easy to completely get rid of the serial 
communication stuff. There are several backends that need to change the 
serial line settings and DTR/RTS lines during the communication. That's 
why those functions are present in the abstract I/O interface, even if 
they only apply to serial communication. My reasoning was those 
functions can be implemented as stubs for I/O implementations where they 
don't make sense.

>> 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.
> 
> From what I've seen, they are very similar at least for both the EON
> Steel and the Scubapro G2.
> 
> But there is one big difference: the packet sizes are different for
> BLE than from USB HID.
> 
> USB HID uses 64-byte packets, while BLE uses 32-byte packets but has
> various other BLE headers.
> 
> In fact, I think there's only really 20 bytes of payload per packet in
> GATT, and that the packetization is entirely different - it looks like
> BLE over GATT acts more like a "stream" than the very obvious HID
> packetization that at least the Suunto EON Steel is very obviously
> very aware of.
> 
> So at a minimum, the divecomputer-specific download code needs to know
> the packetization boundaries, in order to handle that correctly. Which
> is why my custom_io definition has five fields: one field for the
> packet size, and four fields for open/close/read/write functions
> respectively.
> 
> But that *may* be the only real difference. Due to my lack of success
> in getting BLE working yet, I can't guarantee there aren't other
> issues.

If the differences are bigger than just a difference in the packet size, 
then we can probably still deal with that with a minimal amount of 
transport specific knowledge in the backend.

Take for example again the three uwatec variants: smart (IrDA), meridian 
(usb-serial with some additional framing) and g2 (USB HID and BLE GATT 
with their own packetization). Since the high level parts of the 
protocol are the same, we could implement this as a single backend with 
some function pointer for the transfer function:

typedef dc_status_t (*uwatec_smart_transfer_t) (uwatec_smart_device_t 
*device, const unsigned char command[], unsigned int csize, unsigned 
char answer[], unsigned int asize);

typedef struct uwatec_smart_device_t {
     ...
     uwatec_smart_transfer_t transfer;
} uwatec_smart_device_t;


dc_status_t
uwatec_smart_device_open (dc_device_t **out, dc_context_t *context, 
dc_iostream_t *iostream)
{
     uwatec_smart_device_t *device;

     ...

     switch (dc_iostream_get_type(iostream)) {
     case DC_TRANSPORT_IRDA:
         device->transport = irda_transfer;
         break;
     case DC_TRANSPORT_SERIAL:
         device->transport = serial_transfer;
         break;
     case DC_TRANSPORT_USBHID:
         device->transport = usbhid_transfer;
         break;
     case DC_TRANSPORT_BLE:
         device->transport = ble_transfer;
         break;
     }

     ...
}

This way, the dive computer backend needs to be aware how the data 
packets are structured, but it doesn't need to know how they are send 
out. That's still up to the I/O implementation.

Thus if you implement a custom transport, you just need to indicate the 
correct transport type, and you'll get the right type of packets. So for 
testing purpose you can easily implement a transport that pretends to do 
BLE, and then send out the data over a serial line (or a tcp/ip socket, 
etc). (That's in fact how I test the smart backend against the simulator 
without having to deal with IrDA.)

Jef