On 2017-06-14 23:19, Linus Torvalds wrote:
On Wed, Jun 14, 2017 at 8:14 PM, Jef Driesen <[email protected]>
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
_______________________________________________
subsurface mailing list
[email protected]
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
