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