On Wed, Jun 7, 2017 at 8:17 AM, CZS charles.z.stover@gmail.com wrote:
Thanks for the reply, it's never a bad thing when the developer is diving the same computer ;) Great to hear you're working towards a USB download first, as frankly I find the BLE support a mere luxury!
So it turns out that the Scubapro G2 support looks really straightforward, helped by the fact that the USB HID part of the thing is very similar to what the Suunto EON Steel does (and I did the reverse engineering for that one, and wrote the downloader), and the actual commands and data parsing appears pretty much exactly the same as the Galileo Sol.
So if you actually build your own, here's a patch to libdivecomputer that should get things into testable territory.
I *have* actually tested it myself, but right now my only "dive" is me dropping the dive computer into the neighbors pool for four minutes. So my test data is very very limited.
I'll have more test data in a week, but for now this might be good enough to do at least some initial trial downloads.
Linus
Several folks from the shop are diving them in Cozumel this week. If you need tests run against a variety of dives (all recreational) I can borrow them and run whatever is necessary from mac or Linux.
-bill
On Fri, Jun 9, 2017 at 1:29 AM Linus Torvalds torvalds@linux-foundation.org wrote:
On Wed, Jun 7, 2017 at 8:17 AM, CZS charles.z.stover@gmail.com wrote:
Thanks for the reply, it's never a bad thing when the developer is diving the same computer ;) Great to hear you're working towards a USB download first, as frankly I find the BLE support a mere luxury!
So it turns out that the Scubapro G2 support looks really straightforward, helped by the fact that the USB HID part of the thing is very similar to what the Suunto EON Steel does (and I did the reverse engineering for that one, and wrote the downloader), and the actual commands and data parsing appears pretty much exactly the same as the Galileo Sol.
So if you actually build your own, here's a patch to libdivecomputer that should get things into testable territory.
I *have* actually tested it myself, but right now my only "dive" is me dropping the dive computer into the neighbors pool for four minutes. So my test data is very very limited.
I'll have more test data in a week, but for now this might be good enough to do at least some initial trial downloads.
Linus
devel mailing list devel@libdivecomputer.org http://libdivecomputer.org/cgi-bin/mailman/listinfo/devel
On Fri, Jun 9, 2017 at 4:58 AM, William Perry wmperry@gmail.com wrote:
Several folks from the shop are diving them in Cozumel this week. If you need tests run against a variety of dives (all recreational) I can borrow them and run whatever is necessary from mac or Linux.
The more testing, the merrier.
My pool dive only tested the very basics: it does have depth and temperature data, but no events, no cylinder pressure, no heartrate monitor etc.
That said, just the fact that all of that showed up on the very first try (ok, blush, *second* try, because the first try I had entirely missed filling in any model number at all) does imply that it very much is the same protocol as the Galileo Sol, except just with a different transport layer. So it probably gets most things right.
There might be real differences that show up much later - the Uwatec Smart backend in libdivecomputer has a "GALILEO" vs "GALILEOTRIMIX" model number and I might have expected the G2 to be the trimix version because it supports it, but I'm not seeing any actual differences in parsing between the two.
An maybe it has new events or warnings or whatever. And maybe I just screwed up the downloading and it just happens to work for one dive but will completely break immediately when there are more dives. But on the whole I think that it should JustWork(tm).
But testing it is the only way.
I don't think that I'm going to actually push out my patch to the official Subsurface libdivecomputer branch until it gets some more testing than that one trivial pool-dive-on-a-string, so you do have to build your own for now for testing.
After next week, I should have much more confidence whether it actually *really* works, and I'll push it out and maybe we'll have daily builds that just work with the Scubapro G2 out of the box. But in the meantime it's a "please test if you can".
Linus
Linus,
This is fantastic news! Thank you for the hard work.
On Friday, June 9, 2017 at 12:28:53 AM UTC-5, Linus Torvalds wrote:
On Wed, Jun 7, 2017 at 8:17 AM, CZS <charles....@gmail.com javascript:> wrote:
Thanks for the reply, it's never a bad thing when the developer is
diving
the same computer ;) Great to hear you're working towards a USB
download
first, as frankly I find the BLE support a mere luxury!
So it turns out that the Scubapro G2 support looks really straightforward, helped by the fact that the USB HID part of the thing is very similar to what the Suunto EON Steel does (and I did the reverse engineering for that one, and wrote the downloader), and the actual commands and data parsing appears pretty much exactly the same as the Galileo Sol.
So if you actually build your own, here's a patch to libdivecomputer that should get things into testable territory.
I *have* actually tested it myself, but right now my only "dive" is me dropping the dive computer into the neighbors pool for four minutes. So my test data is very very limited.
I'll have more test data in a week, but for now this might be good enough to do at least some initial trial downloads.
Linus
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.
diff --git a/include/libdivecomputer/common.h b/include/libdivecomputer/common.h index 67398d1..fdaaa1b 100644 --- a/include/libdivecomputer/common.h +++ b/include/libdivecomputer/common.h @@ -59,6 +59,8 @@ typedef enum dc_family_t { DC_FAMILY_UWATEC_MEMOMOUSE, DC_FAMILY_UWATEC_SMART, DC_FAMILY_UWATEC_MERIDIAN,
- /* We'll enumerate the Scubapro G2 under Uwatec */
- DC_FAMILY_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. 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".
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.
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.
diff --git a/src/parser.c b/src/parser.c index ebbc6a6..fea1454 100644 --- a/src/parser.c +++ b/src/parser.c @@ -96,6 +96,9 @@ dc_parser_new_internal (dc_parser_t **out, dc_context_t *context, dc_family_t fa case DC_FAMILY_UWATEC_MEMOMOUSE: rc = uwatec_memomouse_parser_create (&parser, context, devtime, systime); break;
- case DC_FAMILY_SCUBAPRO_G2:
rc = uwatec_smart_parser_create (&parser, context, 0x11, devtime,systime);
break;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.
+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.
+#define PACKET_SIZE 64 +static int receive_data(scubapro_g2_device_t *g2, unsigned char *buffer, int size) +{
- while (size) {
unsigned char buf[PACKET_SIZE];size_t transferred = 0;dc_status_t rc;int len;rc = dc_usbhid_read(g2->usbhid, buf, PACKET_SIZE, &transferred);if (rc != DC_STATUS_SUCCESS) {ERROR(g2->base.context, "read interrupt transfer failed");return -1;}if (transferred != PACKET_SIZE) {ERROR(g2->base.context, "incomplete read interrupt transfer (got%zu, expected %d)", transferred, PACKET_SIZE);
return -1;}len = buf[0];if (len >= PACKET_SIZE) {ERROR(g2->base.context, "read interrupt transfer returns impossiblepacket size (%d)", len);
return -1;}HEXDUMP (g2->base.context, DC_LOGLEVEL_DEBUG, "rcv", buf+1, len);if (len > size) {ERROR(g2->base.context, "receive result buffer too small -truncating");
len = size;}memcpy(buffer, buf+1, len);size -= len;buffer += len;- }
- return 0;
+}
There are a two issues with this code:
The error code from dc_usbhid_read is dropped and replaced with a generic -1, which is translated again to DC_STATUS_IO in the caller. It would be much better to pass the actual error code. The difference between for example TIMEOUT, IO and PROTOCOL errors is valuable info. (Maybe not for the end-user, but certainly for the developer debugging the problem.) Those other -1 should be translated into DC_STATUS_PROTOCOL.
Truncating the data if the buffer is too small is a bad idea because you'll return corrupt data. Just bail out with error DC_STATUS_PROTOCOL. If this ever happens, then it's most likely caused by a bug in the code or a change in the communication protocol. In both cases the code should be fixed.
+dc_status_t +scubapro_g2_device_open(dc_device_t **out, dc_context_t *context) +{
- dc_status_t status = DC_STATUS_SUCCESS;
- scubapro_g2_device_t *device = NULL;
- ...
- // Open the irda socket.
- status = dc_usbhid_open(&device->usbhid, context, 0x2e6c, 0x3201);
- if (status != DC_STATUS_SUCCESS) {
ERROR (context, "Failed to open USB device");goto error_free;- }
- *out = (dc_device_t*) device;
- return DC_STATUS_SUCCESS;
+error_free:
- dc_device_deallocate ((dc_device_t *) device);
- return status;
+}
Is the handshaking no longer necessary for the G2? 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.
+static dc_status_t +scubapro_g2_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) +{
- scubapro_g2_device_t *device = (scubapro_g2_device_t*) abstract;
- dc_status_t rc = DC_STATUS_SUCCESS;
- ...
- if (receive_data(device, data, length)) {
ERROR (abstract->context, "Received an unexpected size.");return DC_STATUS_IO;- }
- ...
- return DC_STATUS_SUCCESS;
+}
This will need some improvements to report more fine-grained progress events.
The rest looks good to me. Will you send an updated patch, or do I make the changes myself?
Jef
() (),
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
On Thu, Jun 15, 2017 at 6:19 AM, Linus Torvalds torvalds@linux-foundation.org wrote:
() (),
That isn't some kind of odd googly-eyed emoticon, that's just the web editor in gmail being odd with mixed touchpad input and cursor keys, and I missed that garbage before sending.
Just in case somebody wondered why there were two eyes with a tear down one cheek or whatever odd thing that could have been ;)
Linus
On 2017-06-14 23:19, Linus Torvalds wrote:
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.
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
On Tue, Jun 20, 2017 at 11:59 PM, Jef Driesen jef@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.
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.
Linus
On Wed, Jun 21, 2017 at 5:49 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
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.
Ok, having gotten over (at least some) of the BLE GATT communication issues with QtConnectivity, I can actually confirm that the problems are worse than that.
The command structure seems fairly similar, but the packetization of the commands is completely different over USB HID and BLE GATT.
The USB HID protocol is a simple "split it up into 64-byte packets by having a 62-byte payload and two byte per packet size information".
The BLE GATT protocol, in turn, depends on GATT to do the packetization, so it acts more like a streaming protocol, but then it HDLC-encapsulates it (it looks like it uses a crc32 checksum, but I haven't verified that part yet, but it would seem to fit).
I'm not quite sure *why* it does that (I thought BLE already gave you a reliable stream), but whatever. It is what it is.
I _think_ it may be so that the GATT receiver code can tell when it hits the last packet, without having to understand the stream data itself (that would explain why the USB HID protocol also uses a redundant packet size byte).
So the take-away is that sadly the libdivecomputer code does very much need to know which protocol it's running over, and do different things over different protocols.
At the same time, because BLE itself is so complicated, libdivecomputer can't actually do the IO itself on its own.
Linus
On 2017-06-22 02:49, Linus Torvalds wrote:
On Tue, Jun 20, 2017 at 11:59 PM, Jef Driesen jef@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
On Mon, Jun 26, 2017 at 10:06 PM, Jef Driesen jef@libdivecomputer.org wrote:
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.
Yeah, I guess having empty stubs that you can just leave as NULL isn't that painful.
It's effectively what the current "custom_io_t" does for the serial side.
Pretty much any modern equipment that will do something that looks serial over a modern line won't be caring about any of the low-level traditional serial details (ie break, dtr/rts, baud speed, parity etc).
But there is one big difference: the packet sizes are different for BLE than from USB HID.
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.
There definitely are other issues, having now gotten BLE to work with three different dive computers.
In fact, the last one (Shearwater Perdix) uses BLE in a rather odd way, and actually avoids some of the packet size limits that way.
Instead of treating the BLE GATT protocol as a way of sending a packet, the Perdix actually uses it differently, and sends the streams of data as "descriptor value changes" and let's the GATT protocol itself actually re-assemble multiple packets into bigger data. So even though each packet has a payload in the 20-byte range, I see single updates as multiple packets that then show up in user space as one big change.
So the 20-byte packet limit doesn't end up being a hard limit, it is related to a particular choice of BLE GATT interface use.
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.
Some of the nastiest differences end up being things that the front end has to know about.
The one I hit with the Shearwater Perdix was that the way you connect to it is different than the EON Steel or the Scubapro G2, because the Perdix ends up using a "Random Device Address" rather than a normal public address. And while nobody sane should care, they technically are different address spaces entirely, and the Linux Bluez layer *does* care. So even though you know the device address, you also have to know whether to connect to it using the random device address flag or not.
I have *no* idea why Shearwater did that. But I guess it means that they don't have to register a company ID for the address.
But it does mean that for BLE GATT, I need to know "does this device use a random device address or not".
I can make various hacks up (for example, I can just look at the address and say "I don't recognize the top 24 bits as a company that makes dive gear, so maybe it's a random address"), but what I think I will do is to just trigger on the name (ie "Perdix") during bluetooth scanning, and save away the quirk that "this device needs to be connected to using the random device address flag".
And since I'm pretty sure that libdivecomputer does *not* want to do the whole "scan for bluetooth devices" and also does *not* want to know about things like crazy protocol details that have absolutely no impact on libdivecomputer anyway, I need the custom IO model to be able to just add its own information to its own info block.
That's what the "custom_io_t" is meant for, and libdivecomputer doesn't even need to know about the details.
The older "customserial" protocol made the big mistake of not passing its own IO data descriptor around, just passing that "fill in this 'void **userdata' thing", and it works as long as you really *just* look like serial (which rfcomm does).
But immediately when you need to add your own transport quirks in there, you need something more.
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:
That's pretty much what I do for the G2 right now.
Note in particular how I use the "custom_io_t" not only as a way to pass in the IO function from outside (which, as mentioned, is pretty much required for BLE - I seriously doubt you want to handle all the different OS details yourself, when something like Qt has it reasonably well abstracted)..
I also actually use that "custom_io_t" inside of libdivecomputer itself, creating a new one dynamically and making it use the USB HID code that libdivecomputer already has:
if (io && io->packet_open) status = io->packet_open(io, context, name); else status = dc_usbhid_custom_io(context, 0x2e6c, 0x3201);
in other words, either that code gets passed an IO descrpiptor from outside, or it just generates its own custom IO descriptor using the usbhid code and that USB vendor ID.
It should be possible to extend that to other models too (for example, the non-HID USB transport that the Atomic Cobalt does), but right now the dive computers I care about are all either BLE or USB HID, so those are the two things I have worried about.
Anyway, right now that "custom_io_t" only has the packet size as a way to discriminate with, but that clearly is not sufficient. But neither is it good enough to say what type it is - it's also not sufficient to say "USB vs USB HID vs BLE". Because it turns out that within BLE there are variations, and obviously within USB there are certainly possibilities for variations way past just the USBHID vs "random vendor-specific USB" case.
Linus
On Mon, Jun 26, 2017 at 11:37 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
Anyway, right now that "custom_io_t" only has the packet size as a way to discriminate with, but that clearly is not sufficient. But neither is it good enough to say what type it is - it's also not sufficient to say "USB vs USB HID vs BLE". Because it turns out that within BLE there are variations, and obviously within USB there are certainly possibilities for variations way past just the USBHID vs "random vendor-specific USB" case.
Btw, that may have come off the wrong way - I do *not* think that the particular protocol should even be enumerated or described particularly well at all using some generic descriptors.
No, in many ways I'm arguing for the reverse: not to describe things very much at all, but instead just let each "custom_io_t" do its own thing, and just have a "void *" that the particular transport fills in with *its* particular details. And those details will be very different for bluetooth than they will be for USB.
And I already ended up chaining these custom_io_t's together: the way I did the Shearwater Perdix was to use the "rfcomm" serial interface to expose the serial side, but then when using BLE GATT, literally chaining that serial side though two levels of custom_io_t: the first level does the buffering to make it look like a serial line, and the second level is the regular BLE GATT packetization for the actual IO.
It's pretty hacky, and it's not some kind of truly generic "streams" thing that is designed to chain, but more of just a special case a "turn one interface into another" chain, but it works. And it largely avoids exposing the details of one level to another, and it's not passing any odd level descriptors around, because the custom_io_t itself *is* the descriptor and contains the knowledge of what it's doing and who it is acting to.
Linus
On Mon, Jun 26, 2017 at 11:37 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
The older "customserial" protocol made the big mistake of not passing its own IO data descriptor around, just passing that "fill in this 'void **userdata' thing", and it works as long as you really *just* look like serial (which rfcomm does).
But immediately when you need to add your own transport quirks in there, you need something more.
So I sent Dirk the pull request for my SSRF_CUSTOM_IO v2 code, which updates the custom serial parts to pass along the IO pointer, and also adds a opaque (as far as libdivecomputer is concerned) "dc_user_device_t" field to the IO model.
That allows me to pass down not just the IO functions, but also the "what device am I trying to download from" information, because the IO model gets filled in early, and gets passed around to all the openm code and IO accessor functions.
That, in turn, allows me to have different bluetooth logic for open and IO based on the device I'm downloading from. Which I need, since the rules are different for a Shearwater device than for a Suunto one.
It's not pretty, but then bluetooth LE really isn't pretty. It's very much a "designed by committee" thing, and (similarly to USB, presumably for similar reasons), bluetooth LE was explicitly designed to *not* just have a standardized serial protocol.
Morons. USB did the exact same thing, and I think the reasons are the same: both design committees wanted to explicitly avoid "transparent serial emulation" simply because serial can be used for anything, and they wanted to *describe* each protocol.
Never mind the fact that reality trumps the poor brains of committee people, and people want to do serial communications over new protocols, so they all then just make it happen anyway, but do it in some crazy vendor-specific manner.
So instead of having a nice clean architected serial line emulation of USB or over BLE, we have ugly shit-for-brains vendor-specific ones that are different and often subtly broken.
Yay for committee standards - those committees really showed people how much better things are when you try to force people to all do their own described protocols rather than use a standard serial one.
Linus
-
CZS -
Jef Driesen -
Linus Torvalds -
William Perry