Downloading G2 via bluetooth after scubapro 2.0 firmware upgrade

[jme]

Have been using, and loving, Subsurface for several years.   It was downloading from my Scubapro G2, perfectly, via bluetooth until I upgraded to Scubapro's latest (2.0) firmware (Macos 10.15.7, Subsurface 5.0.10).   Now it connects and seems to go through most (according to the progress bar) of the download until it quits with an error.   Sorry I didn't grab a screen shot of the exact error message, but I can get it after the next dive if that helps.   It works fine with the USB cable.   Anyone else run into this?

thanks,

jim

[Linus Torvalds]

On Mon, Oct 31, 2022 at 3:52 PM jme <jmej...@gmail.com> wrote:
>
> It was downloading from my Scubapro G2, perfectly, via bluetooth until
> I upgraded to Scubapro's latest (2.0) firmware (Macos 10.15.7,
> Subsurface 5.0.10).

Hmm. I guess I will have to try to find my G2 and try to upgrade the firmware.

And I wouldn't have been surprised by some odd handshake issue
(there's a special little back-and-forth that has changed with
different versions of the Scubapro BLE computers), but the fact that
you say it connects and seems to start the download implies that isn't
it either.

Yet the fact that it apparently still works over USB implies that the
protocol hasn't changed.

So it all sounds a bit odd. But hopefully solvable.

Linus

[Linus Torvalds]

On Mon, Oct 31, 2022 at 5:13 PM Linus Torvalds
<torv...@linuxfoundation.org> wrote:
>
> Hmm. I guess I will have to try to find my G2 and try to upgrade the firmware.

Ok, Dirk has it. I'll try to pick it up at some point tomorrow.

At least I'm not in the merge window, and the recent kernel TLB
discussion seems to have sorted itself out, so I _should_ have time to
look into this.

That said, I see that Jef has looked at the G2 TEK lately, so maybe he
has some ideas about the new firmware on the regular G2 too. Jef?

The "What's new" file with that G2 firmware update does say

For compatibility reasons update to latest
SCUBAPRO LogTRAK for PC/Mac/iOS/Android.

which does seem to imply something changed.

Linus

[jme]

Thanks Linus/Dirk - really appreciate you looking at this.   It definitely connects (both the G2 and Subsurface say "connected") and the progress bar goes almost to the end before it dies.    If it helps, I can enable logging and try again after I dive tomorrow.   At the moment it connects and reports "no new dives", as it should as I downloaded via USB.   I would downgrade the firmware, but haven't been able to find the previous version anywhere.    I guess the good news is that I didn't brick my computer during a dive trip and the USB still works.

cheers,

jim

[Linus Torvalds]

On Mon, Oct 31, 2022 at 6:09 PM jme <jmej...@gmail.com> wrote:
>
> If it helps, I can enable logging and try again after I dive tomorrow.

Yes, please do. It won't hurt at least,

Linus

[Jef Driesen]

I remember receiving a very similar report recently. But I don't remember from
where, and I'm unable to find it again right now. Anyway, I didn't include a
log, so at the moment I have no idea where it fails. The fact that the progress
bar goes near the end, seems to imply the protocol hasn't changed.

@Jim: If you can enable the logging, that would be a good start. The log file
will give us some more info on where and how the download fails. And since you
say USB is still working, can you also do a full memory dump? That way, we also
know what data we were supposed to receive, and compare the two. (For the full
memory dump, also enable the "Force download of all dives".)

Jef

[jme]

Thanks Jef.   I tried downloading all dives through usb and bluetooth.   Enabled logged and and dumpfile for both.   USB worked fine, but I didn't get a dumpfile for bluetooth.   Please find logfiles and screen shot attached.

best,

jim

[Linus Torvalds]

On Tue, Nov 1, 2022 at 5:28 AM jme <jmej...@gmail.com> wrote:
>
> I tried downloading all dives through usb and bluetooth. Enabled logged and and dumpfile for both.

Hmm. libdivecomputer literally just does *one* "please send me
everything" command.

And it works perfectly fine - until it doesn't.

So this isn't a data parsing bug - we never even get to that point.
This is a "we asked for data, and we got a lot of it, but we don't
think we got all of it".

Interestingly, the last data in the BT dump is a final packet with 28
bytes of data:

0000006A6BAD217805A0004C64FB04168000FB081A595900000000C0

and the last data in the USB dump is a 51-byte packet (the
packetization is different for USB vs BT), but the data is identical.

The beginning packet data is also identical between the two dump files.

So it looks like we've lost some data in the *middle*.

Linus

[Linus Torvalds]

On Tue, Nov 1, 2022 at 11:10 AM Linus Torvalds
<torv...@linuxfoundation.org> wrote:
>
> So it looks like we've lost some data in the *middle*.

Hmm. I think I see the problem.

The dump file for a good packet looks like this:

INFO: Read: size=64, data=C8B1C17F08C4[...]
DEBUG: rcv: size=63, data=B1C17F08C4[..]

because that first byte (C8) is a magic thing. We have this big
comment about it:

* Something changed in the G2 firmware between
versions 1.2 and 1.4.
*
* The first byte of a packet always used to be the
length of the
* packet data. That's still true for simple
single-packet replies,
* but multi-packet replies seem to have some other
data in it, at
* least for BLE.
*
* The new pattern *seems* to be:
*
* - simple one-packet reply: the byte remains the
size of the reply
*
* - otherwise, it's an endlessly repeating sequence of
*
* 0xf7 247
* 0x14 20
* 0x27 39
* 0x3a 58
* 0x4d 77
* 0x60 96
* 0x73 115
* 0x86 134
* 0x99 153
* 0xac 172
* 0xbf 191
* 0xd2 210
* 0xe5 229
* 0xf7 247
* .. repeats ..
*
* which is basically "increase by 19" except for that
last one (229->247
* is an increase by 18).
*
* The number 19 is the real payload size for BLE GATT
(20 bytes minus the
* one-byte magic size-that-isn't-size-any-more-byte).
*
* It may be just an oddly implemented sequence
number. Whatever.

so what we end up doing is this:

unsigned int len = buf[0];
if (len + 1 > transferred)
len = transferred-1;

and that's actually important for the USB download, because the packet
size is always fixed (you always get 64-byte packets). So the first
byte is basically a "how much of that packet is data".

So we got a 64-byte packet, and turn it into "one byte of length
and/or sequence number" plus "63 bytes of data".

And that also worked for BT, even though BT packets can be any size
(up to a maximum).

HOWEVER, with the new G2 firmware, on the BT side we now end up seeing
packets like this:

INFO: Read: size=37, data=0000001901[..]
DEBUG: rcv: size=0, data=

where we got a 37-byte packet, and the first character was zero, and
we decide to turn that 37-byte packet into "one byte of length, and no
data".

So it *looks* like what changed is that the "length" byte is now only
length for USB transfers, and for BLE transfers it's just random data.

Statistics for the first byte data on the USB side:

1 INFO: Read: size=64, data=33
3 INFO: Read: size=64, data=01
4 INFO: Read: size=64, data=04
4 INFO: Read: size=64, data=10
4639 INFO: Read: size=64, data=3F

ie we had a couple of single-byte replies, 4-byte replies and 16-byte
replies, and then we had a *lot* of "full packet" replies (63 bytes of
data in a 64-byte packet) plus that one single final partial packet
(51 bytes of data).

In contrast, the same statistics on the BLE side make no sense at all.
The first byte seems pretty much random now:

20 INFO: Read: size=37, data=0D
21 INFO: Read: size=37, data=0F
21 INFO: Read: size=37, data=74
21 INFO: Read: size=37, data=76
21 INFO: Read: size=37, data=77
23 INFO: Read: size=37, data=0B
25 INFO: Read: size=37, data=0A
25 INFO: Read: size=37, data=65
25 INFO: Read: size=37, data=75
30 INFO: Read: size=37, data=09
30 INFO: Read: size=37, data=78
32 INFO: Read: size=37, data=79
33 INFO: Read: size=37, data=7A
34 INFO: Read: size=37, data=7C
37 INFO: Read: size=37, data=07
38 INFO: Read: size=37, data=04
38 INFO: Read: size=37, data=05
40 INFO: Read: size=37, data=08
42 INFO: Read: size=37, data=AE
44 INFO: Read: size=37, data=06
44 INFO: Read: size=37, data=9E
45 INFO: Read: size=37, data=7D
48 INFO: Read: size=37, data=03
53 INFO: Read: size=37, data=7B
59 INFO: Read: size=37, data=02
65 INFO: Read: size=37, data=7E
72 INFO: Read: size=37, data=C1
76 INFO: Read: size=37, data=00
78 INFO: Read: size=37, data=7F
93 INFO: Read: size=37, data=01
350 INFO: Read: size=37, data=9F
366 INFO: Read: size=37, data=81
534 INFO: Read: size=37, data=AF
1459 INFO: Read: size=64, data=65
1459 INFO: Read: size=64, data=C8

and I don't see much of a pattern, except that we have a lot of
37-byte BLE packets, but most of them are 64-byte. And the 64-byte
packets seem to still have a clear pattern in the first byte (ie for
64-byte packets it's always either 0x65 or 0xc8).

So let's just ignore the first byte for the BLE packet. I'll send a patch.

Linus

[Linus Torvalds]

On Tue, Nov 1, 2022 at 11:35 AM Linus Torvalds
<torv...@linuxfoundation.org> wrote:
>
> So let's just ignore the first byte for the BLE packet. I'll send a patch.

Something like this should do the trick.

Entirely untested. Jef, comments?

Dirk - let me know when you're around, and I can pick up the G2 for
testing. Or, you could just test this directly and commit it yourself
if it works. Whatever is easiest.

Linus

[jme]

Thanks for tracking this down.    Really odd that they would change something  as seemingly basic as the data length.   Hopefully the entire packet is valid data, both for the 2.0 and 1.4 firmware.   Speaking of, has the firmware version always been reported as 0?    I've never paid attention to it, but I see it shows up a 0 in the UI when downloading.   Looking at the log file I see:

Event: model=50 (0x00000032), firmware=0 (0x00000000), serial=70019135 (0x042c683f)

When I look on the computer (8.1 Device information), it shows:

ID: 7001913526 (2 more digits than the log file serial)

HW Version: 0.0

SW version: 2.0

I can't imagine what a pain it is trying to make Subsurface work with so many computers and various download modes - you guys are amazing.

thanks,

jim

[Jef Driesen]

At first sight, the code looks fine, but when I try to replay the BLE log, it
doesn't work. I'll have to take a closer look, but I think the problem is a bit
more complex. Take a look at the first few packets:

USB:

INFO: Read: size=64,
data=3FA5A55A5A1A0B0000EE84D24BFFFFFFFFE801000600023808CC0345003C011001480100000000000005000040000019000300B464DD0F3C00FC31AF149A1900
DEBUG: rcv: size=63,
data=A5A55A5A1A0B0000EE84D24BFFFFFFFFE801000600023808CC0345003C011001480100000000000005000040000019000300B464DD0F3C00FC31AF149A1900
INFO: Read: size=64,
data=3F000000190100C05492C0C1025C01220000FF7FFFBDFB5C1000000000000000000000000000000000000000000000000000250000650B0000650B0000650B00
DEBUG: rcv: size=63,
data=000000190100C05492C0C1025C01220000FF7FFFBDFB5C1000000000000000000000000000000000000000000000000000250000650B0000650B0000650B00

BLE:

INFO: Read: size=64,
data=C8A5A55A5A1A0B0000EE84D24BFFFFFFFFE801000600023808CC0345003C011001480100000000000005000040000019000300B464DD0F3C00FC31AF149A1900
DEBUG: rcv: size=63,
data=A5A55A5A1A0B0000EE84D24BFFFFFFFFE801000600023808CC0345003C011001480100000000000005000040000019000300B464DD0F3C00FC31AF149A1900
INFO: Read: size=37,
data=000000190100C05492C0C1025C01220000FF7FFFBDFB5C1000000000000000000000000000
DEBUG: rcv: size=0, data=

Look carefully at the first 4 bytes, after ignoring the first byte. For USB we see:

A5A55A5A
00000019

and for BLE:

A5A55A5A
00001901 <- Wrong

For BLE we're missing the first zero byte in the second packet. That's the one
we just dropped as the "unknown" byte. So for this second packet, this byte is
part of the payload. But for the first packet that's not the case.

I guess we're seeing some kind of packet with a 1 byte header, that is split
over multiple BLE packets. Most BLE packets appear to be a sequence of a 64 byte
packet followed by a 37 byte packet. And the first byte in those 64 byte packets
alternates between 0xC8 and 0x65. The latter corresponds to 64 + 37 bytes. Maybe
not a coincidence? No idea what that 0xC8 means.

That's what I discovered for now.

Jef

[Linus Torvalds]

On Tue, Nov 1, 2022 at 4:13 PM Jef Driesen <j...@libdivecomputer.org> wrote:
>
> At first sight, the code looks fine, but when I try to replay the BLE log, it
> doesn't work. I'll have to take a closer look, but I think the problem is a bit
> more complex.

Yeah, that does seem to be the case. It looks like that unknown byte
does end up being the data byte.

> I guess we're seeing some kind of packet with a 1 byte header, that is split
> over multiple BLE packets.

No, I think I have a simpler explanation.

I suspect that what has happened is that the new BLE code simply uses
larger packets.

But then, because libdivecomputer only asks for 64 bytes of data at a
time, it will get that larger packet as first a 64-byte read, and then
a "rest of it" read.

So it was never a 64-byte packet followed by a 37-byte packet. It was
a 101-byte packet. Which is actually a believable round number,
because that's "100 data bytes".

IOW, I think the one-liner fix for all this is just to not limit our
packet size fo that USB HID packet size, simply doing somehting like
this instead:

- unsigned char buf[PACKETSIZE_USBHID_RX];
+ unsigned char buf[256];

which should make it all work.

Knock wood.

I have my G2 now, will do the firmware update and try it out.

Linus

[Linus Torvalds]

On Tue, Nov 1, 2022 at 5:27 PM Linus Torvalds
<torv...@linuxfoundation.org> wrote:
>
> I have my G2 now, will do the firmware update and try it out.

Bah. The firmware update was easy.

But I'm not actually able to connect over bluetooth to that thing at
all. So it doesn't even begin any IO.

I don't think I've done BLE on my desktop machine in years (pretty
much since my last "debug BLE issue"). I have this dim memory of using
a BLE dongle because my desktop BLE chipset was garbage.

I'll have to continue on my old laptop, which I know had working BLE.

Linus

[Linus Torvalds]

On Tue, Nov 1, 2022 at 5:48 PM Linus Torvalds
<torv...@linuxfoundation.org> wrote:
>
> I'll have to continue on my old laptop, which I know had working BLE.

.. same behavior.

I wonder if I've ever tested BLE with whatever Qt updates that have
happened since I did this last.

It fails connecting for me, and I can see the dive computer go to
"connected" and then immediately back to "ready" or whatever it says
in BT mode.

Very annoying.

I suspect that "just allow bigger packets at receive" will fix the
problem, but right now I have something else wrong that causes it to
fail long before it gets to talk to the thing.

Linus

[Linus Torvalds]

On Tue, Nov 1, 2022 at 6:08 PM Linus Torvalds
<torv...@linuxfoundation.org> wrote:
>
> I wonder if I've ever tested BLE with whatever Qt updates that have
> happened since I did this last.

Yeah, I get

.. service state changed to QLowEnergyService::InvalidService

and then it disconnects. Something in current Qt bluetooth, it seems.

Not the first time we've had problems with Qt changing how it does
bluetooth, and particularly with odd devices. Nobody else tests
devices like this that have vendor-specific odd services.

I guess I'll have to go back into BLE debugging mode and talk to the
Qt BLE people, but not going to bother right now.

Linus

[jme]

Any chance you have the old firmware?    I've tried to find it, but no joy.   

thanks,

jim

[jme]

Adric kindly supplied the 1.9 firmware, but the g2 doesn't seem to like downgrading.    I followed the upgrade steps to copy it to the g2, but 8.8 (software upgrade) only shows 2.0.    Interesting approach to software development - optimism is a nice quality, but shouldn't be a strategy...

[Jef Driesen]

Yes, that appears to be problem!

Note that the libdivecomputer I/O layer always expects to retrieve a full
packet. If the buffer is too small, subsurface should return an error instead of
returning a partial packet. That would have resulted in an immediate failure,
instead of the silent data loss we're seeing now.

As long as we don't really know how to interpret that first byte, I think your
first patch still makes sense. For USB, that first byte is indeed the size of
the payload only (excluding the first byte itself). For BLE, if the first byte
is 0x65 (101), that would be the size of the entire packet, and not just the
payload.

Pity you're not able to test at the moment.

@Jim: Unfortunately I don't have a build environment to produce Mac binaries. Do
you happen to have access to a Windows system to try a build with the fixes? In
the Windows build of subsurface it's easy to replace the libdivecomputer dll. I
can also provide a build of the libdivecomputer dctool command-line application
with BLE enabled.

Jef

[Linus Torvalds]

On Wed, Nov 2, 2022 at 9:09 AM Jef Driesen <j...@libdivecomputer.org> wrote:
>
> Note that the libdivecomputer I/O layer always expects to retrieve a full
> packet. If the buffer is too small, subsurface should return an error instead of
> returning a partial packet. That would have resulted in an immediate failure,
> instead of the silent data loss we're seeing now.

The libdivecomputer IO layer is very confused and wrong, Jef.

And no, not everything is packetized, and your suggestion of just
dropping data doesn't work.

For example, if I just do the "change size to be large
unconditionally", it looks like the usbhid case will just break,
because it will happily try to read more than 64 bytes and die of a
timeout because it won't get it.

So to make libdivecomputers *own* src/usbhid.c implementation happy, I
need to do only 64-byte reads in uwatec_smart.c for the USBHID case.

And no, for the BLE stream case, the whole "I/O layer expects to
retrieve a full packet" is also completely wrong, and that's simply
not true, because some of them aren't even packetized but treat the
BLE side as just a stream. For example, the broken Mares BlueLink
thing is literally just a "serial to BLE" converter, and doesn't have
any packetization.

The Subsurface commit comment for that was

However, the Mares BlueLink backend treats it as just a basic "serial
over BLE" transport, and for historical reasons reads the reply packets
in smaller chunks.

This allows that kind of IO behavior, where if the divecomputer backend
reads just a part of a packet, we'll split the packet, return the part
the user asked for, and push back the leftover packet onto the received
packet queue.

IOW, the BLE "return the rest of the packet for the next IO" is
_required_, and you simply don't know your own I/O layer.

So what subsurface does (and what _does_ work) is:

(a) when the buffer is large enough, and the transport has some kind
of packetization, the subsurface iostream will stop at packet
boundaries (even if multiple packets may be pending).

(b) when the buffer is too small for one packet, we'll just return as
much as we can, and the next read will return the remaining data

and those are the semantics that the iostream interface had better
have, because they are the only *valid* semantics. They allow for both
the streaming and the packet-based parsing, unlike your "it's always
packets" model that is completely broken.

So please fix your mental model, because it's not only unworkable, it
already doesn't match reality.

Sadly, it really looks like src/usbhid.c doesn't work with "give me
any size packet", and you literally have to ask for a 64-byte one.

The reason seems to be that at least the libusb case uses
libusb_interrupt_transfer(), and if you ask for 'size' bytes, it will
wait for 'size' bytes. So it then times out when you give a large
buffer size, because it only got 64 bytes.

Some googling finds other complaining:

https://github.com/libusb/libusb/issues/1178

and this *may* be limited to just the libusb case (ie maybe
"hid_read_timeout()" doesn't have the same problem.

Possibly the dc_usbhid_read() could have a

if (size > 64) size = 64;

kind of thing, but I made the uwatec smart driver just do "if HIDUSB,
ask for 64-byte packets, otherwise, ask for maximum sized ones".

Not pretty, but at least the USB side works.

I still haven't figured out what broke on the Qt side. Oh well.

Linus

[jme]

I can try - I have Windows running in a VM, but I don't do much with it and have never tried sharing the blue tooth connection.   It should work...   is it easy to put together a Windows binary or should I start by installing the current Subsurface version and seeing if I can reproduce the problem first?

[jme]

Unfortunately it doesn't look like I can run the current binary on my vm.

[Linus Torvalds]

On Tue, Nov 1, 2022 at 6:27 PM Linus Torvalds
<torv...@linuxfoundation.org> wrote:
>
> Yeah, I get
>
> .. service state changed to QLowEnergyService::InvalidService

It turns out that this was purely because the device wants me to do a
proper secure pairing with a new host key, almost certainly because
it's been paired with another machine in the meantime.

So this was all just because it *had* been paired with my machines
before, and they remembered it, but the pairing information was just
stale.

Silly. And partly because the Fedora bluetooth settings GUI "helper"
thing is a nasty broken useless pos, so it doesn't make that at all
obvious, and I stupidly thought it would DTRT.

Using 'bluetoothctl' just worked, and then everything connects fine.

And yes, my libdivecomputer fix at

https://github.com/subsurface/libdc/commit/28c27e23921e61264538ebba37baedc27af0a0f6

seems to work just fine, and happily downloaded all the dives from my G2.

I already asked Dirk to create a daily build of subsurface with this
because I thought I'd have more trouble debugging this than I did.

But now I guess I'll ask Dirk to do a daily build just so that people
can use subsurface with the new firmware version.

Dirk?

And I need to remember in the future that the gnome bluetooth settings
thing is a piece of s**t.

I've had this issue before where that thing doesn't realize that it
actually needs to do a secure pairing with proper pin codes.

Linus

[Linus Torvalds]

Oh, and looking closer, I realize that the packet size seems to be
about BLE version, because now that I look at my dump, I'm seeing not
101-byte packets, but 20-byte packets.

That's the default old legacy BLE packet size (well, 27 bytes of data,
but 4 bytes for the L2CAP header, and 3 bytes for the GATT header, so
20 bytes remaining for the payload).

BLE 4.2 apparently upped that maximum native BLE packet size to 244
bytes (251 with the headers), but from what I can tell you need BLE
5.0 to get 100+ bytes of data for GATT transfers.

So what seems to be going on with firmare 2.0 is simply that the G2
can now do BLE 5.0 packets.

My download didn't actually test that, because my desktop simply isn't
BLE 5.0.

So my testing is kind of half-arsed, and I can't actually trigger the
bigger packet size that broke for jme, but it is what it is, and I'm
pretty sure that patch is the right fix.

Linus

[jme]

Have been looking for a daily build where I can test this fix, but don't see one beyond 30-Oct.   Is there a mac binary I can test for you?   I'm not in any hurry, but if I can help by testing a fix, please let me know.

cheers,

jim

[Jef Driesen]

On 2/11/2022 18:04, Linus Torvalds wrote:
> On Wed, Nov 2, 2022 at 9:09 AM Jef Driesen <j...@libdivecomputer.org> wrote:
>>
>> Note that the libdivecomputer I/O layer always expects to retrieve a full
>> packet. If the buffer is too small, subsurface should return an error instead of
>> returning a partial packet. That would have resulted in an immediate failure,
>> instead of the silent data loss we're seeing now.
>
> The libdivecomputer IO layer is very confused and wrong, Jef.
>
> And no, not everything is packetized, and your suggestion of just
> dropping data doesn't work.

I never suggested to drop data. I suggested to return an error if the buffer
size is not large enough to hold a complete packet. That way the problem can be
handled gracefully by allocating a larger buffer and trying again (which is
something we don't do due to the use of stack buffers), or by aborting the
download. Sure, the latter doesn't really help the end-user, but at least it's
very clear there is some kind of problem, and we can fix it.

> For example, if I just do the "change size to be large
> unconditionally", it looks like the usbhid case will just break,
> because it will happily try to read more than 64 bytes and die of a
> timeout because it won't get it.
>
> So to make libdivecomputers *own* src/usbhid.c implementation happy, I
> need to do only 64-byte reads in uwatec_smart.c for the USBHID case.

This is actually a bug. Some more on that below.

> And no, for the BLE stream case, the whole "I/O layer expects to
> retrieve a full packet" is also completely wrong, and that's simply
> not true, because some of them aren't even packetized but treat the
> BLE side as just a stream. For example, the broken Mares BlueLink
> thing is literally just a "serial to BLE" converter, and doesn't have
> any packetization.
>
> The Subsurface commit comment for that was
>
> However, the Mares BlueLink backend treats it as just a basic "serial
> over BLE" transport, and for historical reasons reads the reply packets
> in smaller chunks.
>
> This allows that kind of IO behavior, where if the divecomputer backend
> reads just a part of a packet, we'll split the packet, return the part
> the user asked for, and push back the leftover packet onto the received
> packet queue.
>
> IOW, the BLE "return the rest of the packet for the next IO" is
> _required_, and you simply don't know your own I/O layer.

For protocols that are simply a serial stream over BLE (e.g. iconhd, ostc3,
etc), returning a partial packet and keeping the remainder for the next call
will indeed work fine. It even makes things easier in the upper layers, because
you don't have to deal with the packet sizes. But for the protocols that are
really packet oriented (e.g. eonsteel, atom2, smart, petrel, etc) that's not the
case. If the packet has some kind of structure with a header that's not part of
the data and needs to processed in a special way, the upper layer needs access
to those packet boundaries. If you return a partial packet, those boundaries are
lost and the upper layer won't be able to process the packet correctly. That's
exactly the problem we're seeing here: we're incorrectly treating payload data
as the packet header, because we think we're processing a new packet when it's
actually the remainder of the previous packet. And of course that goes
completely wrong.

Once the packet boundaries are lost, you can't reconstruct them. At least not
without some extra protocol on top that preservers those boundaries. But that's
something we don't have here.

And that's the reason why libdivecomputer expects to receive a complete packet
from the I/O layer. For the protocols that don't need it, it's trivial to hide
this packet nature, and provide an stream like interface on top of the packets.
The iconhd and ostc3 backends do exactly that. They cache the last BLE packet,
and serve partial reads from this cache. So no, the Mares Bluelink code doesn't
need that behavior from the I/O layer at all. It's handled in the dive computer
backends directly.

You could also move all that logic into the I/O layer, but then it's no longer
generic.

> So what subsurface does (and what _does_ work) is:
>
> (a) when the buffer is large enough, and the transport has some kind
> of packetization, the subsurface iostream will stop at packet
> boundaries (even if multiple packets may be pending).
>
> (b) when the buffer is too small for one packet, we'll just return as
> much as we can, and the next read will return the remaining data
>
> and those are the semantics that the iostream interface had better
> have, because they are the only *valid* semantics. They allow for both
> the streaming and the packet-based parsing, unlike your "it's always
> packets" model that is completely broken.

The first part is fine, but as explained above, the second part causes trouble
for the true packet based communication protocols.

Note that in libdivecomputer the packet semantics only applies to the packet
based transports (e.g. BLE, USB, USBHID). For the stream based transports
(serial, irda, bluetooth) nothing changed: a read operation returns when either
the requested number of bytes is returned or the timeout expires.

> So please fix your mental model, because it's not only unworkable, it
> already doesn't match reality.

The libusb and hidapi library seem to disagree with you. If you pass a buffer
that is too small to hold a single packet you get either a LIBUSB_ERROR_OVERFLOW
error (libusb) or a truncated packet with the remainder dropped (hidapi). (The
latter is also not great, but that's another story.)

> Sadly, it really looks like src/usbhid.c doesn't work with "give me
> any size packet", and you literally have to ask for a 64-byte one.
>
> The reason seems to be that at least the libusb case uses
> libusb_interrupt_transfer(), and if you ask for 'size' bytes, it will
> wait for 'size' bytes. So it then times out when you give a large
> buffer size, because it only got 64 bytes.
>
> Some googling finds other complaining:
>
> https://github.com/libusb/libusb/issues/1178
>
> and this *may* be limited to just the libusb case (ie maybe
> "hid_read_timeout()" doesn't have the same problem.
>
> Possibly the dc_usbhid_read() could have a
>
> if (size > 64) size = 64;
>
> kind of thing, but I made the uwatec smart driver just do "if HIDUSB,
> ask for 64-byte packets, otherwise, ask for maximum sized ones".
>
> Not pretty, but at least the USB side works.

Yes, that's indeed a bug in the libusb based implementation. The hidapi based
implementation does this correctly (confirmed on linux with both the
hidapi-libusb and hidapi-hidraw variant, and on windows). But this is easily
fixed. The attached patch takes care of that.

With this fixed, it should no longer be a problem to use a larger buffer.

Jef

[Linus Torvalds]

On Mon, Nov 7, 2022 at 8:08 AM Jef Driesen <j...@libdivecomputer.org> wrote:
>
> I never suggested to drop data. I suggested to return an error if the buffer
> size is not large enough to hold a complete packet.

That's the same thing, Jef.

The problem - that you ignored - is that the stream users *DO NOT WANT
PACKET BOUNDARIES*.

> For protocols that are simply a serial stream over BLE (e.g. iconhd, ostc3,
> etc), returning a partial packet and keeping the remainder for the next call
> will indeed work fine. It even makes things easier in the upper layers, because
> you don't have to deal with the packet sizes.

You don't get it.

The BLE code is packetized. End of story. There is no "stream over BLE".

Really. No such thing exists

BLE simply does not *have* the equivalent of 'rfcomm'. Now, I don't
like that, and I think it was a mistake by the bluetooth association
to make that decision, but there we are.

All you can do is send packets, and then you can decide to treat the
resulting data as a stream.

Yes, there are vendor-specific specifications for that packet data
that is intended to be used basically as an rfcomm stream replacement
and be treated like a serial stream.

For example, Nordic Semiconductor has a vendor-specific protocol on
top of BLE GATT that they specified for a "serial port emulation over
BLE", exactly because BLE doesn't *have* that natively. And we even
have at least one dive computer that uses that version of "treat BLE
packets as a stream": the Cosmiq+.

But that Nordic Semi vendor "standard" is really just standardizing
the particular BLE GATT service UUID, and the practice of sending and
receiving the data (as BLE GATT packets) from particular descriptors
from that service. It's actually *technically* 100% the same as other
vendor standards (like the Suunto one), which do the exact same thing,
just with their own service UUID's and slightly different GATT service
descriptor organization.

And that - basically identical - Suunto vendor standard? It's packetized.

> But for the protocols that are really packet oriented [..]

Jef, let me be brutally honest: you don't know what you are talking about.

Give me some credit here. As far as I know, I'm the one who has done
EVERY SINGLE BLE downloader, because nobody else has stepped up to
even try to understand the unholy mess that is BLE.

So how about we just agree that I know what I'm saying, and you don't?

> Once the packet boundaries are lost, you can't reconstruct them. At least not
> without some extra protocol on top that preservers those boundaries. But that's
> something we don't have here.

Right.

Which is why the solution is simple:

- BLE is always packetized

- dive computers that need to see full packets need to have a big
enough buffer that they will always receive a full packet. In the case
of BLE, "big enough buffer" for one packet is basically 243 bytes iirc
(round up to 256), although a dive computer manufacturer could decide
to do it's packetization on different scales.

- dive computers that *don't* care about packet boundaries can - and
do - use smaller buffers because they literally do things like "I want
the next byte in the stream", and the BLE code will give them partial
packets.

But no, the solution is *NOT* that "small buffers result in errors".
Because that DOES NOT WORK for the streaming case.

Because that is just how the world works, and I'm not in the least
interested in having to make even more special cases than we already
have in the BLE code for your idea of how the world *should* work.

Linus

[Linus Torvalds]

On Mon, Nov 7, 2022 at 1:00 AM jme <jmej...@gmail.com> wrote:
>
> Have been looking for a daily build where I can test this fix, but
> don't see one beyond 30-Oct. Is there a mac binary I can test for you?
> I'm not in any hurry, but if I can help by testing a fix, please let
> me know.

Dirk started a new job last week, so I think he may be busy with
things that actually pay the bills (which is very much *not*
subsurface - apparently donations now cover the actual server side
costs).

I'm sure he'll get around to it, but there are distractions.

Linus

[Dirk Hohndel]

BLE almost never works in VMs. So usually that's not a usable path.

/D

[Dirk Hohndel]

Indeed - distractions like having a job that keeps me quite busy :)

New daily binaries are up that include the G2 fix from Linus.

/D

[jme]

Hope the new job is going well Dirk.   Just tried the daily build - clicked download all files, create log file, and create dump file.    I chugged on for quite some time (I have about 150 dives on my g2) and then threw up a "error - no new logs to download" so I think it's fixed!   

big thanks to you and Linus for all the help,

jim

[jme]

When I ran into this problem I noticed that the reported firmware version is 0, not 2.0.   This isn't ruining my day in any way, but, being curious, I tried to figure out why.   I can see in uwatec_smart.c where it's set to 0, and I can see where the model and serial number are read.    I'm wondering if it's possible to read the firmware version the same way the model and serial number are read.   My first question is, where do these constants come from:

#define CMD_MODEL      0x10
#define CMD_SERIAL     0x14

Apologies if this is a really lame question.   This is all new to me and google queries for ble gatt don't seem to be helping. 

thanks,

jim

[Jef Driesen]

On 2022-11-17 10:40, jme wrote:
> When I ran into this problem I noticed that the reported firmware
> version
> is 0, not 2.0. This isn't ruining my day in any way, but, being
> curious,
> I tried to figure out why. I can see in uwatec_smart.c where it's set
> to
> 0, and I can see where the model and serial number are read. I'm
> wondering if it's possible to read the firmware version the same way
> the
> model and serial number are read. My first question is, where do
> these
> constants come from:
>
> #define CMD_MODEL 0x10
> #define CMD_SERIAL 0x14
>
> Apologies if this is a really lame question. This is all new to me
> and
> google queries for ble gatt don't seem to be helping.

This information is obtained from reverse engineering the communication
protocol. We know the commands for reading the model and serial number,
because that information is also read by the Scubapro application. But
because it doesn't try to read the firmware version, we also don't have
that info.

Now, I do know there are indeed commands for reading out the hardware
and software versions, but I've never tried them. Mainly because I don't
have direct access to a Scubapro/Uwatec dive computer to try it, and
also because so far the info wasn't really needed for anything
important.

Jef

[jme]

Thanks Jeff.   Both impressive and sad that so much was done via reverse engineering.  If you happen to know what the commands are, I can give them a try.   I know that sending a 0x13 and requesting a byte returned 0x20, and my firmware is 2.0, but that's probably just be a coincidence.   Agreed that it's not needed for anything - I was just surprised to see it as 0 in the UI and thought it would be interesting to see why.    Logtrak must read the firmware version because it suggested the  2.0 firmware upgrade.   I don't usually use Logtrak, but it's the only way I know for setting personalization data on the G2 and there are so many G2's around now I wanted my name on the home screen.   

thanks,

jim

[jme]

A bit like a dog with a bone, but it's interesting trying to sort this out.   I think I found the command to get the G2 firmware version, but I'm not sure as I can't downgrade my G2 to test it.   Any chance someone has a mac and a G2 with firmware other than 2.0 they would be willing to use to run a test for me?   I have a mac binary with an extra debug line showing what I hope is the version number - if someone could use it to do blue tooth upload with the option to save the log file and send me the log file I'd really appreciate it.

thanks,

jim

[wfg...@gmail.com]

Did this ever get resolved? Do you still need someone to run the test? I have a Mac and a G2 which I have not updated to 2.0 yet.

I would be willing to run the test.

[jim]

Thanks for the follow up. Jef Driesen put together a patch with a fix for this, but I don’t know if he ever submitted it or not. After seeing that he had done the same fix, I bowed out.

cheers,
jim

> --
> You received this message because you are subscribed to the Google Groups "Subsurface Divelog" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to subsurface-dive...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/subsurface-divelog/2075c0c0-d0c3-472e-b260-f993802ea440n%40googlegroups.com.

[Linus Torvalds]

On Mon, Jan 30, 2023 at 7:40 AM wfg...@gmail.com <wfg...@gmail.com> wrote:
>
> Did this ever get resolved? Do you still need someone to run the test? I have a Mac and a G2 which I have not updated to 2.0 yet.
> I would be willing to run the test.

It should work fine with current subsurface - it's been fixed in the
subsurface branch of libdivecomputer since November.

I didn't check when Dirk did the latest release, but at least the
daily builds have that.

Linus

[Jim E]

Different issue.  You fixed the G2 download, but this was about the reported firmware version.  My fault - I should have started a new thread.  Jeff had a patch for it, but I don't think it was submitted.

--
You received this message because you are subscribed to the Google Groups "Subsurface Divelog" group.
To unsubscribe from this group and stop receiving emails from it, send an email to subsurface-dive...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/subsurface-divelog/CAHk-%3DwhiSq%2B7niJrt%3DOqJ4iz1MXbcRF6kqnZmx6O_1AebwbKcA%40mail.gmail.com.

[fetzy...@gmail.com]

Hi everyone,

I have the same problem with the Aladin Sport Matrix after updating the firmware. I have SubS. latest version and Macos 10.15.7.  Any news about it? Thanks a lot for your great work!

Federico

[Jef Driesen]

On 2023-03-14 15:05, fetzy...@gmail.com wrote:
> Hi everyone,
> I have the same problem with the Aladin Sport Matrix after updating
> the firmware. I have SubS. latest version and Macos 10.15.7. Any news
> about it? Thanks a lot for your great work!
> Federico

Try to download with the "Save libdivecomputer logfile" checkbox
enabled. Afterwards send the log file, so we can have a look. Without
this information we have no clue what went wrong.

Jef

[wfg...@gmail.com]

Hello. Based on this information, I updated my FW on the G2 to 2.0. I am also running V3.4.7 of Subsurface-Mobile on iOS 16.3.1.

Subsurface-Mobile connects to the G2 and attempts to download, but comes back with “No new dives”. I have two new dives on the G2 that I was able to successfully download using LogTRAK 2.0.

I am also able download via the desktop version of Subsurface.

The logs from the Subsurface-Mobile app are attached.

[Jef Driesen]

On 2023-03-28 00:43, wfg...@gmail.com wrote:
> Hello. Based on this information, I updated my FW on the G2 to 2.0. I
> am
> also running V3.4.7 of Subsurface-Mobile on iOS 16.3.1.
> Subsurface-Mobile connects to the G2 and attempts to download, but
> comes
> back with “No new dives”. I have two new dives on the G2 that I was
> able to
> successfully download using LogTRAK 2.0.
> I am also able download via the desktop version of Subsurface.
>
> The logs from the Subsurface-Mobile app are attached.

Your log shows you're using a build with a libdivecomputer version which
doesn't include the necessary G2 fixes:

Subsurface: vv5.0.9-33-g5c13da5ba0a7, built with libdivecomputer
v0.8.0-devel-Subsurface-NG (a17e466bd1d2e675666e20862182d618cf6d7190)

That's why the download doesn't work for you.

Jef

[wfg...@gmail.com]

Jef -

Thanks for the quick response.

So it hasn’t been released to the App Store yet? 

[Dietrich Schwanenhorst]

Unfortunately, only an old version is available in the Google Playstore. The test and daily versions cannot be installed for me.
Is there a version with the bug fixes available somewhere?

[Damien GOUJU]

Hello,

Is there a plan for a new build? I'm blocked on all my devices: iPhone, Android, MacOS (probably because of the BLE issue too on recent MacOS releases, daily builds don't work either) :-(

Any alternative?
Thanks a lot for your help!