This implements fairly basic support for the A300CS
- adds a cached reading function that supports transparent caching of 16 byte reads, implemented as 256 byte reads (I cannot get the A300CS to respond to the B1 command, but with B8 I can read 256 bytes; so I implemented a neat little caching layer that makes 16 byte reads available to the upper layers while in fact reading the larger pages that contain the 16 byte block and caching them for later use). - adds support for an oceanic_atom2 open function that gets passed the model information, since the A300CS needs us to toggle DTR and RTS twice before it's willing to talk to us. The old oceanic_atom2 open function remains unchanged in order not to break apps that might be using it directly (tsk, tsk, tsk, your're not supposed to...) - adds the necessary changes to the parser to deal with the different locations for various information; it supports the basics: date, time, duration, max depth, gasmixes plus depth, temperature and sensor pressure from the samples
Missing / issues:
- there is a TON more information available. Some I know where it is, but can't easily pass it back because of libdivecomputer architecture issues (e.g. the firmware version). Others are clearly there but I haven't figured out where (e.g. the deco state).
- the 256 byte read uses a two byte crc and I haven't been able to figure out the algorithm. I'm sure it's obvious and I'm just being stupid, but for now the check is commented out
This has been tested with Linus' new computer which has all of six dives on it - and those six dives are rather homogeneous. This needs a lot more testing, obviously. But so far so good - it works well from Subsurface :-)
/D
PS: Jef, I looked at the multiblock support. It adds more complexity for no benefit - so I decided not to add this to my patches. The reduced number of calls to the read function really makes no difference whatsoever, but the changes make the code more complex and (IMHO) more fragile. The caching read on the other hand is easy, straight forward, and reasonably obviously correct (famous last words).
Dirk,
We don't have to use the existing multipage support. I just wanted to check whether we can re-use the existing infrastructure before adding something completely new. If it turns out your caching is a better solution, then fine no problem.
I went through your patches and have a couple of comments:
* You used global variables for the cache and its bitmap. That's simply not acceptable. Please move this into the device handle. (I suggest you use the name "bitmap" instead of "tracker". In the ostc backend we already used "bitmap" for the exact same purpose.) And then you can pre-allocate everything in the open function.
* Remove the read_big_pages from the layout. The layout is intended for the logic in the shared oceanic_common_device_xxx functions. But didn't change anything there. You actually introduced a bug in the vtpro and veo250 backends, because their layouts were not updated with the new field. So instead add some "bigpage" field directly to the device handle.
* There are some other devices like the OC1 that also support the newer B4 command. But they appear to use 128 byte packets (8 pages) instead of 256 byte packets (16 pages). So I would prefer that BIGPAGESIZE isn't hardcoded. That will make it easier to adapt it to other devices if needed. For example by setting the "bigpage" field above to 0 to disable the bigpage feature, and to 8 (OC1) or 16 (A300CS). I don't think such a change really affects your caching logic.
* Libdivecomputer uses unsigned integers everywhere, especially if bit manipulations are involved.
* For the checksum function. Just a crazy idea, but you never know. Maybe it's not a 256 byte packet with a 2 byte checksum. But two times a 128 byte packet with a 1 byte checksum? That means all your second halves shift one byte, and you would probably notice some problems during parsing, but you never know. The reason why I think this might be a possibility is that the OC1 seemed to use such 128 byte packets with a 1 byte checksum. Most likely I'm just wrong here, but it's worth checking.
* Setting DTR/RTS twice looks really odd. Are you sure this is really needed? Does the Aeris application does this too? (Note that due to how the win32 api works, DTR/RTS are automatically set when configuring the line settings, but there are also separate calls for DTR/RTS.) For other devices I noticed that setting DTR/RTS may require a small delay after opening the serial port. So maybe you are just not doing it at the right time. Try to move it after that 100ms sleep.
* I don't see the different baudrate anywhere in the patches. Did you forgot to include that part?
* In the aeris_a300cs_version string, mask the firmware version with "\0\0". Otherwise it will only work for one firmware version.
Can you send me a full memory dump so I can try your code? Preferable with a portmon capture too.
Jef
On 2014-09-26 06:12, Dirk Hohndel wrote:
This implements fairly basic support for the A300CS
- adds a cached reading function that supports transparent caching of
16 byte reads, implemented as 256 byte reads (I cannot get the A300CS to respond to the B1 command, but with B8 I can read 256 bytes; so I implemented a neat little caching layer that makes 16 byte reads available to the upper layers while in fact reading the larger pages that contain the 16 byte block and caching them for later use).
- adds support for an oceanic_atom2 open function that gets passed the model information, since the A300CS needs us to toggle DTR and RTS
twice before it's willing to talk to us. The old oceanic_atom2 open function remains unchanged in order not to break apps that might be using it directly (tsk, tsk, tsk, your're not supposed to...)
- adds the necessary changes to the parser to deal with the different locations for various information; it supports the basics: date,
time, duration, max depth, gasmixes plus depth, temperature and sensor pressure from the samples
Missing / issues:
- there is a TON more information available. Some I know where it is,
but can't easily pass it back because of libdivecomputer architecture issues (e.g. the firmware version). Others are clearly there but I haven't figured out where (e.g. the deco state).
- the 256 byte read uses a two byte crc and I haven't been able to
figure out the algorithm. I'm sure it's obvious and I'm just being stupid, but for now the check is commented out
This has been tested with Linus' new computer which has all of six dives on it - and those six dives are rather homogeneous. This needs a lot more testing, obviously. But so far so good - it works well from Subsurface :-)
/D
PS: Jef, I looked at the multiblock support. It adds more complexity for no benefit - so I decided not to add this to my patches. The reduced number of calls to the read function really makes no difference whatsoever, but the changes make the code more complex and (IMHO) more fragile. The caching read on the other hand is easy, straight forward, and reasonably obviously correct (famous last words).
devel mailing list devel@libdivecomputer.org http://libdivecomputer.org/cgi-bin/mailman/listinfo/devel
On Fri, Sep 26, 2014 at 09:52:05AM +0200, Jef Driesen wrote:
- You used global variables for the cache and its bitmap.
I did? Where? There are two static variables in the caching read function.
That's simply not acceptable. Please move this into the device handle.
Why would I move these variables further up? No one else should ever touch them but the function that allocated and manages their content. I don't understand.
(I suggest you use the name "bitmap" instead of "tracker". In the ostc backend we already used "bitmap" for the exact same purpose.) And then you can pre-allocate everything in the open function.
I'll be happy to change the name but as I said above, I don't understand why they wouldn't be static to the only function that uses them.
- Remove the read_big_pages from the layout. The layout is intended for the
logic in the shared oceanic_common_device_xxx functions. But didn't change anything there. You actually introduced a bug in the vtpro and veo250 backends, because their layouts were not updated with the new field. So instead add some "bigpage" field directly to the device handle.
- There are some other devices like the OC1 that also support the newer B4
command. But they appear to use 128 byte packets (8 pages) instead of 256 byte packets (16 pages). So I would prefer that BIGPAGESIZE isn't hardcoded. That will make it easier to adapt it to other devices if needed. For example by setting the "bigpage" field above to 0 to disable the bigpage feature, and to 8 (OC1) or 16 (A300CS). I don't think such a change really affects your caching logic.
OK, no problem. It changes the math in the caching logic but that's completely straight forward.
- Libdivecomputer uses unsigned integers everywhere, especially if bit
manipulations are involved.
OK
- For the checksum function. Just a crazy idea, but you never know. Maybe
it's not a 256 byte packet with a 2 byte checksum. But two times a 128 byte packet with a 1 byte checksum? That means all your second halves shift one byte, and you would probably notice some problems during parsing, but you never know. The reason why I think this might be a possibility is that the OC1 seemed to use such 128 byte packets with a 1 byte checksum. Most likely I'm just wrong here, but it's worth checking.
The checksum for 256 times 'FF' is '00 FF' so I think you are wrong.
- Setting DTR/RTS twice looks really odd. Are you sure this is really
needed? Does the Aeris application does this too? (Note that due to how the win32 api works, DTR/RTS are automatically set when configuring the line settings, but there are also separate calls for DTR/RTS.) For other devices I noticed that setting DTR/RTS may require a small delay after opening the serial port. So maybe you are just not doing it at the right time. Try to move it after that 100ms sleep.
I can play with it some more. The Windows app does exactly that (I would never have come up with trying to do it twice) but it's entirely possible that it's instead a delay that I need. I also noticed that I didn't insert these four calls in the correct spot - the check of the return code of the serial_configuration call should come first.
And staring at this code some more (this is basically your code just moved into a different function) I notice that we are inconsistent in when we use rc and when we directly check the result of a function call. I'll clean that up as well.
- I don't see the different baudrate anywhere in the patches. Did you
forgot to include that part?
No, after Linus' comments I went back and did more experiments and things work fine with 38400 - what was needed was the RTS/DTR toggling.
- In the aeris_a300cs_version string, mask the firmware version with
"\0\0". Otherwise it will only work for one firmware version.
Thanks - I was wondering about that. Hand't noticed that little piece of magic
Can you send me a full memory dump so I can try your code? Preferable with a portmon capture too.
Sure. Do you have a USBACM simulator? Or are you just telling libdivecomputer to use a normal serial port for testing it? Attached.
/D
On 2014-09-26 15:59, Dirk Hohndel wrote:
On Fri, Sep 26, 2014 at 09:52:05AM +0200, Jef Driesen wrote:
- You used global variables for the cache and its bitmap.
I did? Where? There are two static variables in the caching read function.
That's simply not acceptable. Please move this into the device handle.
Why would I move these variables further up? No one else should ever touch them but the function that allocated and manages their content. I don't understand.
Just a quick answer on this item. Each device handle should have it's own cache. Right now, with the static variable (which is basically a global variable accessible from only one function), all device handles are sharing the same cache. It's about thread-safety too.
I'm aware that in practice it's very unlikely that an application will be talking to two devices simultaneously, but that is supported by libdivecomputer.
Jef
On September 26, 2014 7:19:24 AM Jef Driesen jef@libdivecomputer.org wrote:
On 2014-09-26 15:59, Dirk Hohndel wrote:
On Fri, Sep 26, 2014 at 09:52:05AM +0200, Jef Driesen wrote:
- You used global variables for the cache and its bitmap.
I did? Where? There are two static variables in the caching read function.
That's simply not acceptable. Please move this into the device handle.
Why would I move these variables further up? No one else should ever touch them but the function that allocated and manages their content. I don't understand.
Just a quick answer on this item. Each device handle should have it's own cache. Right now, with the static variable (which is basically a global variable accessible from only one function), all device handles are sharing the same cache. It's about thread-safety too.
I'm aware that in practice it's very unlikely that an application will be talking to two devices simultaneously, but that is supported by libdivecomputer.
Duh. That makes perfect sense. Will fix.
/D
On 2014-09-26 15:59, Dirk Hohndel wrote:
The checksum for 256 times 'FF' is '00 FF' so I think you are wrong.
I think that gives away the checksum function. 256 times 0xFF is 0xFF00. Store that as little endian and you have a perfect match with your checksum bytes! So it's summing up 256 bytes in a 16bit integer. Now just check on some other, more random packets to confirm this.
- Setting DTR/RTS twice looks really odd. Are you sure this is really
needed? Does the Aeris application does this too? (Note that due to how the win32 api works, DTR/RTS are automatically set when configuring the line settings, but there are also separate calls for DTR/RTS.) For other devices I noticed that setting DTR/RTS may require a small delay after opening the serial port. So maybe you are just not doing it at the right time. Try to move it after that 100ms sleep.
I can play with it some more. The Windows app does exactly that (I would never have come up with trying to do it twice) but it's entirely possible that it's instead a delay that I need. I also noticed that I didn't insert these four calls in the correct spot - the check of the return code of the serial_configuration call should come first.
I need to look at the portmon files before I can tell anything useful here.
And staring at this code some more (this is basically your code just moved into a different function) I notice that we are inconsistent in when we use rc and when we directly check the result of a function call. I'll clean that up as well.
I noticed that too, just forgot to mention it.
- I don't see the different baudrate anywhere in the patches. Did you
forgot to include that part?
No, after Linus' comments I went back and did more experiments and things work fine with 38400 - what was needed was the RTS/DTR toggling.
Ah great. That means that we can maybe set RTS/DTR unconditionally. And then we no longer need to pass the model number. I need to check whether setting RTS/DTR affects current devices or not.
Can you send me a full memory dump so I can try your code? Preferable with a portmon capture too.
Sure. Do you have a USBACM simulator? Or are you just telling libdivecomputer to use a normal serial port for testing it? Attached.
The kernel g_serial module can emulate an USB-ACM devices. I've used it in the past, to attach the USB-CDC device to a VM in order to use the manufacturer software. But for the libdivecomputer simulator a normal serial port (or even a pty) will do.
Of course I still need to update the simulator with the B4 command. It's already partially there, from when I was experimenting with the OC1 command B4 command.
Jef
On Fri, Sep 26, 2014 at 04:58:57PM +0200, Jef Driesen wrote:
On 2014-09-26 15:59, Dirk Hohndel wrote:
The checksum for 256 times 'FF' is '00 FF' so I think you are wrong.
I think that gives away the checksum function. 256 times 0xFF is 0xFF00. Store that as little endian and you have a perfect match with your checksum bytes! So it's summing up 256 bytes in a 16bit integer. Now just check on some other, more random packets to confirm this.
Brilliant! You are indeed correct. Now given that you wanted me to generalize this so it can also deal with the 128 byte pages - do you have a trace for that where I can see the crc returned there? Your sample code seemed to indicate that in the 128 byte case just one extra byte was returned, so I'm guessing it's a simply add8...
- Setting DTR/RTS twice looks really odd. Are you sure this is really
needed? Does the Aeris application does this too? (Note that due to how the win32 api works, DTR/RTS are automatically set when configuring the line settings, but there are also separate calls for DTR/RTS.) For other devices I noticed that setting DTR/RTS may require a small delay after opening the serial port. So maybe you are just not doing it at the right time. Try to move it after that 100ms sleep.
That did the trick - it now works fine with just one set of DTR/RTS calls.
And staring at this code some more (this is basically your code just moved into a different function) I notice that we are inconsistent in when we use rc and when we directly check the result of a function call. I'll clean that up as well.
I noticed that too, just forgot to mention it.
Hehe - it was your code :-)
- I don't see the different baudrate anywhere in the patches. Did you
forgot to include that part?
No, after Linus' comments I went back and did more experiments and things work fine with 38400 - what was needed was the RTS/DTR toggling.
Ah great. That means that we can maybe set RTS/DTR unconditionally. And then we no longer need to pass the model number. I need to check whether setting RTS/DTR affects current devices or not.
That would be nice. Less complication. How many devices do you have access to in order to test this?
Can you send me a full memory dump so I can try your code? Preferable with a portmon capture too.
Sure. Do you have a USBACM simulator? Or are you just telling libdivecomputer to use a normal serial port for testing it? Attached.
The kernel g_serial module can emulate an USB-ACM devices. I've used it in the past, to attach the USB-CDC device to a VM in order to use the manufacturer software. But for the libdivecomputer simulator a normal serial port (or even a pty) will do.
Funny, I was just in the process of creating a USB-ACM simulator using g_serial and some userspace glue. If you already have that working, I'd appreciate if you could share it.
Of course I still need to update the simulator with the B4 command. It's
It's B8 for the 256 byte page.
/D
On Fri, Sep 26, 2014 at 09:52:05AM +0200, Jef Driesen wrote:
- You used global variables for the cache and its bitmap. That's simply not
acceptable. Please move this into the device handle. (I suggest you use the name "bitmap" instead of "tracker". In the ostc backend we already used "bitmap" for the exact same purpose.) And then you can pre-allocate everything in the open function.
- Remove the read_big_pages from the layout. The layout is intended for the
logic in the shared oceanic_common_device_xxx functions. But didn't change anything there. You actually introduced a bug in the vtpro and veo250 backends, because their layouts were not updated with the new field. So instead add some "bigpage" field directly to the device handle.
Just to make sure I understand you correctly. You want me to add this to dc_device_t directly? That seems a fairly generic structure to me, I'm surprised you want such device specific data in there.
Or is there some more libdivecomputer magic that I'm missing where you can add extra data to that? I just want to avoid more round trips with thow away code.
/D
On Fri, Sep 26, 2014 at 08:37:19AM -0700, Dirk Hohndel wrote:
On Fri, Sep 26, 2014 at 09:52:05AM +0200, Jef Driesen wrote:
- You used global variables for the cache and its bitmap. That's simply not
acceptable. Please move this into the device handle. (I suggest you use the name "bitmap" instead of "tracker". In the ostc backend we already used "bitmap" for the exact same purpose.) And then you can pre-allocate everything in the open function.
- Remove the read_big_pages from the layout. The layout is intended for the
logic in the shared oceanic_common_device_xxx functions. But didn't change anything there. You actually introduced a bug in the vtpro and veo250 backends, because their layouts were not updated with the new field. So instead add some "bigpage" field directly to the device handle.
Just to make sure I understand you correctly. You want me to add this to dc_device_t directly? That seems a fairly generic structure to me, I'm surprised you want such device specific data in there.
Or is there some more libdivecomputer magic that I'm missing where you can add extra data to that? I just want to avoid more round trips with thow away code.
Responding to myself here... should have done more reading before asking :-)
I'm pretty sure you want this in oceanic_atom2_device_t :-)
/D
In the oceanic_atom2_device_t struct.
On September 26, 2014 5:37:19 PM CEST, Dirk Hohndel dirk@hohndel.org wrote:
On Fri, Sep 26, 2014 at 09:52:05AM +0200, Jef Driesen wrote:
- You used global variables for the cache and its bitmap. That's
simply not
acceptable. Please move this into the device handle. (I suggest you
use the
name "bitmap" instead of "tracker". In the ostc backend we already
used
"bitmap" for the exact same purpose.) And then you can pre-allocate everything in the open function.
- Remove the read_big_pages from the layout. The layout is intended
for the
logic in the shared oceanic_common_device_xxx functions. But didn't
change
anything there. You actually introduced a bug in the vtpro and veo250 backends, because their layouts were not updated with the new field.
So
instead add some "bigpage" field directly to the device handle.
Just to make sure I understand you correctly. You want me to add this to dc_device_t directly? That seems a fairly generic structure to me, I'm surprised you want such device specific data in there.
Or is there some more libdivecomputer magic that I'm missing where you can add extra data to that? I just want to avoid more round trips with thow away code.
/D _______________________________________________ devel mailing list devel@libdivecomputer.org http://libdivecomputer.org/cgi-bin/mailman/listinfo/devel
Jef,
I think I have taken all of your feedback into account.
On Fri, Sep 26, 2014 at 09:52:05AM +0200, Jef Driesen wrote:
We don't have to use the existing multipage support. I just wanted to check whether we can re-use the existing infrastructure before adding something completely new. If it turns out your caching is a better solution, then fine no problem.
I figured out why the multipage support didn't work for me. It doesn't create aligned reads - but the A300CS only works with aligned reads. Not sure about the other models that support this feature, but the patch series enclosed includes one commit that fixes the oceanic common layer to make those multipage calls page aligned and suddenly this becomes more useful.
Granted, it STILL doesn't save any reads from the device (the caching made sure that every page was only read once), but it at least now is more efficient.
I kept the caching in order to avoid multiple reads when the common layer calls for unaligned reads or smaller reads than the multipage size.
If you really worry about the caching code then yes, we could not include it (on my current download that would create about 30 additional reads of pages that we already read before).
- You used global variables for the cache and its bitmap. That's simply not
acceptable. Please move this into the device handle. (I suggest you use the name "bitmap" instead of "tracker". In the ostc backend we already used "bitmap" for the exact same purpose.) And then you can pre-allocate everything in the open function.
Fixed.
- Remove the read_big_pages from the layout. The layout is intended for the
logic in the shared oceanic_common_device_xxx functions. But didn't change anything there. You actually introduced a bug in the vtpro and veo250 backends, because their layouts were not updated with the new field. So instead add some "bigpage" field directly to the device handle.
Fixed.
- There are some other devices like the OC1 that also support the newer B4
command. But they appear to use 128 byte packets (8 pages) instead of 256 byte packets (16 pages). So I would prefer that BIGPAGESIZE isn't hardcoded. That will make it easier to adapt it to other devices if needed. For example by setting the "bigpage" field above to 0 to disable the bigpage feature, and to 8 (OC1) or 16 (A300CS). I don't think such a change really affects your caching logic.
Fixed (but I did not enable this for any other devices as I can't test it... the code is all there)
- Libdivecomputer uses unsigned integers everywhere, especially if bit
manipulations are involved.
Fixed.
- For the checksum function. Just a crazy idea, but you never know. Maybe
it's not a 256 byte packet with a 2 byte checksum. But two times a 128 byte packet with a 1 byte checksum? That means all your second halves shift one byte, and you would probably notice some problems during parsing, but you never know. The reason why I think this might be a possibility is that the OC1 seemed to use such 128 byte packets with a 1 byte checksum. Most likely I'm just wrong here, but it's worth checking.
It was indeed the 16bit add function. I had tried that, but I had messed up the assembly of the crc data in the buffer (:facepalm:) and that's why I thought this wasn't the right answer...
- Setting DTR/RTS twice looks really odd. Are you sure this is really
needed? Does the Aeris application does this too? (Note that due to how the win32 api works, DTR/RTS are automatically set when configuring the line settings, but there are also separate calls for DTR/RTS.) For other devices I noticed that setting DTR/RTS may require a small delay after opening the serial port. So maybe you are just not doing it at the right time. Try to move it after that 100ms sleep.
This is now just two calls (instead of doing those two calls twice) and done unconditional for all Atom2 devices (which of course needs to be tested). This simplified the code quite a bit.
- I don't see the different baudrate anywhere in the patches. Did you
forgot to include that part?
See previous comment - this isn't needed.
- In the aeris_a300cs_version string, mask the firmware version with
"\0\0". Otherwise it will only work for one firmware version.
Fixed.
Let me know if we are getting closer or if there are more parts you want me to rework (e.g. the cache code).
/D
On 2014-09-26 20:19, Dirk Hohndel wrote:
Let me know if we are getting closer or if there are more parts you want me to rework (e.g. the cache code).
I still need to look at your code, but I received some additional test data that will be of interest for your work. Someone tried to download an A300CS with an unpatched libdivecomputer build. Strange enough, reading the data packets worked out of the box, and the download failed only because of the wrong (default) memory layout. Downloading a memory dump succeeded after just a minor patch for the larger memory size.
So it seems that the old B1 command is still working, and without any DTR/RTS changes. You told me none of that worked for you, so I wonder what is going on here.
I may have an explanation for the DTR/RTS part, but not for the B1 part. The tester is using Windows. If you look at the serial_win32.c code, you'll notice that the serial_configure function always sets DTR/RTS with:
dcb.fDtrControl = DTR_CONTROL_ENABLE; dcb.fRtsControl = RTS_CONTROL_ENABLE;
So that means when you configure the serial settings on Windows, you are forced to also set (or clear) the DTR/RTS lines. This is different on Linux, where the api to control DTR/RTS is completely independent from the other settings. This independent api also exists on Windows, but DTR/RTS are already set and the extra calls are thus not necessary. That probably explains why it works out of the box on Windows, but not on Linux.
I have attached the data files, so you can have a look too.
Jef
On Thu, Oct 02, 2014 at 05:08:42PM +0200, Jef Driesen wrote:
On 2014-09-26 20:19, Dirk Hohndel wrote:
Let me know if we are getting closer or if there are more parts you want me to rework (e.g. the cache code).
I still need to look at your code, but I received some additional test data that will be of interest for your work. Someone tried to download an A300CS with an unpatched libdivecomputer build. Strange enough, reading the data packets worked out of the box, and the download failed only because of the wrong (default) memory layout. Downloading a memory dump succeeded after just a minor patch for the larger memory size.
So it seems that the old B1 command is still working, and without any DTR/RTS changes. You told me none of that worked for you, so I wonder what is going on here.
That's strange. I kept trying B1 and couldn't get it to work on mine. I will try again with my latest build to see if this was some weird cross dependency on other changes that I made... If B1 does work I can completely get rid of the cache, so that would be nice.
I may have an explanation for the DTR/RTS part, but not for the B1 part. The tester is using Windows. If you look at the serial_win32.c code, you'll notice that the serial_configure function always sets DTR/RTS with:
dcb.fDtrControl = DTR_CONTROL_ENABLE; dcb.fRtsControl = RTS_CONTROL_ENABLE;
So that means when you configure the serial settings on Windows, you are forced to also set (or clear) the DTR/RTS lines. This is different on Linux, where the api to control DTR/RTS is completely independent from the other settings. This independent api also exists on Windows, but DTR/RTS are already set and the extra calls are thus not necessary. That probably explains why it works out of the box on Windows, but not on Linux.
That would make sense - I did all that testing on Linux. What do you recommend we do about this? Make the additional DTR/RTS Linux specific, or simply leave them in place as they shouldn't hurt on Windows, either?
I have attached the data files, so you can have a look too.
Excellent. I'll play with them.
I was getting close to sending you V3 of my patches (with correct interval setting, support for NDL/Deco and salinity), but I think I'll first go back and test B1 again.
/D
On Thu, Oct 02, 2014 at 08:18:58AM -0700, Dirk Hohndel wrote:
So it seems that the old B1 command is still working, and without any DTR/RTS changes. You told me none of that worked for you, so I wonder what is going on here.
That's strange. I kept trying B1 and couldn't get it to work on mine. I will try again with my latest build to see if this was some weird cross dependency on other changes that I made... If B1 does work I can completely get rid of the cache, so that would be nice.
Oh FUN. I think I now get it. Using B1 appears to be much more timing sensitive than B8 on the A300CS that I have access to.
Specifically, in my setup my first attempt to use B1 almost always fails a couple of times (but the algorithm then simply retries and eventually succeeds). I almost never see failures with B8.
The way I tested things initially I saw the failures and didn't retry but assumed (looking at the trace of what the Aeris software does, which ALWAYS uses B8) that I should try B8... and that worked. And then when I tried B1 again at a later point, it once again failed and I figured they had removed this. Turns out that I'm wrong.
This is excellent news. I can rip out a big chunk of the new code that I wrote :-)
/D
OK, here we go. MUCH simpler now. This needs to be run through your regression tests with other atom2 style dive computers. But it seems to work well both with the computer I have access to and with the memory dump you sent. Sadly, that one has even more boring dives than mine does - but the poor person does appear to have one of the totally broken first generation tank transmitters. It cuts out all the time and occationally sends completely bogus values. I'm proud to say that the dive plot with Subsurface looks orders of magnitude better than what the Aeris software shows. :-)
/D
And here is one more, this one detects how many cylinders the A300CS thinks we used during a dive. This way no unused cylinders are reported back to the downloading application.
/D
On 02-10-14 19:49, Dirk Hohndel wrote:
And here is one more, this one detects how many cylinders the A300CS thinks we used during a dive. This way no unused cylinders are reported back to the downloading application.
For patches #1 and #2, I need to check how this will interact with the other two oceanic backends. For example the multipage code certainly doesn't require aligned read. So this causes more I/O operations for those backends. But since the A300CS also support the older B1 command, we don't really need those two patches immediately to get the A300CS to work. (That gives me some time to also find a solution for the Aeris F11 that plays nicely together with your A300CS work.)
So I would like to already go ahead and apply the other patches:
Patch #3 and #4 look good to me. As explained before, the DTR/RTS shouldn't cause any problems for other devices, because if it did I would already had received plenty of bug reports from Windows users. One minor issues is that I'm pretty sure the rb_profile_end field of the layout is actually 0x3FE00 instead of 0x40000. That's why I asked you for a full memory dump.
For patch #5, where did you get the density value 1003 from? According to your patch, only the water type is stored in the data, but not the density. Since this is optional value, maybe we better leave this as zero?
Patch #6 is the only one where I don't understand the logic. Are they really using this very weird encoding, or are we missing something? Am I correct that if only 1,2,3 or 4 tanks are enabled, these are always the first ones?
Jef
On Tue, Oct 07, 2014 at 09:09:35PM +0200, Jef Driesen wrote:
On 02-10-14 19:49, Dirk Hohndel wrote:
And here is one more, this one detects how many cylinders the A300CS thinks we used during a dive. This way no unused cylinders are reported back to the downloading application.
For patches #1 and #2, I need to check how this will interact with the other two oceanic backends. For example the multipage code certainly doesn't require aligned read. So this causes more I/O operations for those backends. But since the A300CS also support the older B1 command, we don't really need those two patches immediately to get the A300CS to work. (That gives me some time to also find a solution for the Aeris F11 that plays nicely together with your A300CS work.)
Yep.
So I would like to already go ahead and apply the other patches:
Patch #3 and #4 look good to me. As explained before, the DTR/RTS shouldn't cause any problems for other devices, because if it did I would already had received plenty of bug reports from Windows users. One minor issues is that I'm pretty sure the rb_profile_end field of the layout is actually 0x3FE00 instead of 0x40000. That's why I asked you for a full memory dump.
Sorry, busy flying to Bonaire and diving.
For patch #5, where did you get the density value 1003 from? According to your patch, only the water type is stored in the data, but not the density. Since this is optional value, maybe we better leave this as zero?
OK
Patch #6 is the only one where I don't understand the logic. Are they really using this very weird encoding, or are we missing something? Am I correct that if only 1,2,3 or 4 tanks are enabled, these are always the first ones?
I spent an evening playing with the simulator and their app. Trying every single value this is the logic that I found. I think it would be a nice feature not to get the unused tanks, but my confidence that this is 100% correct is not 100% :-/. The problem is... the only way to get this more widely tested is to add the code and wait if it works as expected in all circumstances.
/D
On 2014-10-08 12:27, Dirk Hohndel wrote:
On Tue, Oct 07, 2014 at 09:09:35PM +0200, Jef Driesen wrote:
On 02-10-14 19:49, Dirk Hohndel wrote: So I would like to already go ahead and apply the other patches:
Patch #3 and #4 look good to me. As explained before, the DTR/RTS shouldn't cause any problems for other devices, because if it did I would already had received plenty of bug reports from Windows users. One minor issues is that I'm pretty sure the rb_profile_end field of the layout is actually 0x3FE00 instead of 0x40000. That's why I asked you for a full memory dump.
Sorry, busy flying to Bonaire and diving.
For patch #5, where did you get the density value 1003 from? According to your patch, only the water type is stored in the data, but not the density. Since this is optional value, maybe we better leave this as zero?
OK
I have attached a slightly modified versions of your four patches. If you're happy with the changes I made, I'll push them already to the master branch.
Patch #6 is the only one where I don't understand the logic. Are they really using this very weird encoding, or are we missing something? Am I correct that if only 1,2,3 or 4 tanks are enabled, these are always the first ones?
I spent an evening playing with the simulator and their app. Trying every single value this is the logic that I found. I think it would be a nice feature not to get the unused tanks, but my confidence that this is 100% correct is not 100% :-/. The problem is... the only way to get this more widely tested is to add the code and wait if it works as expected in all circumstances.
Does that mean you found the trick to make the Aeris application talk to the simulator? Can you share what you did, because I had so success so far.
I think I found the logic. Look at the three relevant bits:
001 -> 1 011 -> 1 101 -> 1 111 -> 1 010 -> 2 110 -> 2 100 -> 3 000 -> 4
There is clearly a pattern here. All 4 combinations that map to one tank have the lowest bit set. With two tanks, the second bit is set. And so on. Thus this code should do the trick:
if (data[0x39] & 0x04) { *((unsigned int *) value) = 1; } else if (data[0x39] & 0x08) { *((unsigned int *) value) = 2; } else if (data[0x39] & 0x10) { *((unsigned int *) value) = 3; } else { *((unsigned int *) value) = 4; }
I included this change already in the attached patches.
Originally I was puzzled by the fact that there are multiple ways to encode some values. And that makes no sense at all. But then I realized you probably made up data with the simulator to try all possible bit patterns. I guess that in real data, we'll only see these 4 bit patterns:
001 -> 1 010 -> 2 100 -> 3 000 -> 4
Jef
On Fri, Oct 10, 2014 at 02:20:20PM +0200, Jef Driesen wrote:
I spent an evening playing with the simulator and their app. Trying every single value this is the logic that I found. I think it would be a nice feature not to get the unused tanks, but my confidence that this is 100% correct is not 100% :-/. The problem is... the only way to get this more widely tested is to add the code and wait if it works as expected in all circumstances.
Does that mean you found the trick to make the Aeris application talk to the simulator? Can you share what you did, because I had so success so far.
Sure. I used the patches that I sent you. I did exactly what you suggested I should do. It took about five attempts to get the drivers to install in Windows (what a crap OS that pile of rubbish is). After that all I did was to start the simulator on /dev/ttyGS0 and to have the Windows app talk to the fake device.
I wrote a few little scripts that allowed me to easily edit the data file that I used and then it was all just a matter of dealing with the unbearably stupid Aeris app. This thing is so bad, it wants me to go all Linus and curse and stuff.
I think I found the logic. Look at the three relevant bits:
001 -> 1 011 -> 1 101 -> 1 111 -> 1 010 -> 2 110 -> 2 100 -> 3 000 -> 4
There is clearly a pattern here. All 4 combinations that map to one tank have the lowest bit set. With two tanks, the second bit is set. And so on. Thus this code should do the trick:
if (data[0x39] & 0x04) { *((unsigned int *) value) = 1; } else if (data[0x39] & 0x08) { *((unsigned int *) value) = 2; } else if (data[0x39] & 0x10) { *((unsigned int *) value) = 3; } else { *((unsigned int *) value) = 4; }
Yes, that is equivalent to my case statement.
I included this change already in the attached patches.
Originally I was puzzled by the fact that there are multiple ways to encode some values. And that makes no sense at all. But then I realized you probably made up data with the simulator to try all possible bit patterns. I
Correct
guess that in real data, we'll only see these 4 bit patterns:
001 -> 1
nope: 111 is what they use for 1 (1C, actually)
010 -> 2 100 -> 3 000 -> 4
I'll need to look what I can find in this week's dives as I simulated having two and three tanks.
BTW: your patches are completely unusable. I recommend against pushing them as they are and claiming A300CS support. At least not with my SOB line.
I have 27 dives on my A300CS. The download has been running for 15 minutes now. We're on dive 6. No, you cannot simply say "oh, the A300CS supports B1, let's force using that". This turns a perfectly usable dive computer which downloads half a dozen dives in under 10 seconds into a completely unusable piece of garbage that takes forever to download a day's worth of diving.
/D
On Sat, Oct 11, 2014 at 10:25:46PM -0400, Dirk Hohndel wrote:
BTW: your patches are completely unusable. I recommend against pushing them as they are and claiming A300CS support. At least not with my SOB line.
I have 27 dives on my A300CS. The download has been running for 15 minutes now. We're on dive 6. No, you cannot simply say "oh, the A300CS supports B1, let's force using that". This turns a perfectly usable dive computer which downloads half a dozen dives in under 10 seconds into a completely unusable piece of garbage that takes forever to download a day's worth of diving.
Oh CRUD. So after about 20 minutes the download finished with only eight dives downloaded. WTH? This $1000 dive computer (at 2sec sample rate) can store only about 9-10h worth of dive data, or in my case 8 dives.
That is so beyond ridiculous. But it also means that I can no longer download the dives where I pretended to have 2 or 3 cylinders. I did that earlier this week and hadn't bothered to download them, yet, as I wanted to make sure that I actually have the time to work on the data. Diving is over for this trip...
Aeris, you suck.
I couldn't believe this was true, so I switched back to my branch and re-downloaded. Took maybe half a minute and gave me the same eight dives...
/D
On 12-10-14 04:37, Dirk Hohndel wrote:
On Sat, Oct 11, 2014 at 10:25:46PM -0400, Dirk Hohndel wrote:
BTW: your patches are completely unusable. I recommend against pushing them as they are and claiming A300CS support. At least not with my SOB line.
I have 27 dives on my A300CS. The download has been running for 15 minutes now. We're on dive 6. No, you cannot simply say "oh, the A300CS supports B1, let's force using that". This turns a perfectly usable dive computer which downloads half a dozen dives in under 10 seconds into a completely unusable piece of garbage that takes forever to download a day's worth of diving.
Oh CRUD. So after about 20 minutes the download finished with only eight dives downloaded. WTH? This $1000 dive computer (at 2sec sample rate) can store only about 9-10h worth of dive data, or in my case 8 dives.
That is so beyond ridiculous. But it also means that I can no longer download the dives where I pretended to have 2 or 3 cylinders. I did that earlier this week and hadn't bothered to download them, yet, as I wanted to make sure that I actually have the time to work on the data. Diving is over for this trip...
Aeris, you suck.
I couldn't believe this was true, so I switched back to my branch and re-downloaded. Took maybe half a minute and gave me the same eight dives...
That's sad indeed. But unfortunately there's not much we can do about that. The only solution here is to download more frequently or increase the sample rate :-(
Jef
On 12-10-14 04:25, Dirk Hohndel wrote:
On Fri, Oct 10, 2014 at 02:20:20PM +0200, Jef Driesen wrote:
I spent an evening playing with the simulator and their app. Trying every single value this is the logic that I found. I think it would be a nice feature not to get the unused tanks, but my confidence that this is 100% correct is not 100% :-/. The problem is... the only way to get this more widely tested is to add the code and wait if it works as expected in all circumstances.
Does that mean you found the trick to make the Aeris application talk to the simulator? Can you share what you did, because I had so success so far.
Sure. I used the patches that I sent you. I did exactly what you suggested I should do. It took about five attempts to get the drivers to install in Windows (what a crap OS that pile of rubbish is). After that all I did was to start the simulator on /dev/ttyGS0 and to have the Windows app talk to the fake device.
The Windows driver still fails to load for me. Not sure why.
I wrote a few little scripts that allowed me to easily edit the data file that I used and then it was all just a matter of dealing with the unbearably stupid Aeris app. This thing is so bad, it wants me to go all Linus and curse and stuff.
:-)
I think I found the logic. Look at the three relevant bits:
001 -> 1 011 -> 1 101 -> 1 111 -> 1 010 -> 2 110 -> 2 100 -> 3 000 -> 4
There is clearly a pattern here. All 4 combinations that map to one tank have the lowest bit set. With two tanks, the second bit is set. And so on. Thus this code should do the trick:
if (data[0x39] & 0x04) { *((unsigned int *) value) = 1; } else if (data[0x39] & 0x08) { *((unsigned int *) value) = 2; } else if (data[0x39] & 0x10) { *((unsigned int *) value) = 3; } else { *((unsigned int *) value) = 4; }
Yes, that is equivalent to my case statement.
I included this change already in the attached patches.
Originally I was puzzled by the fact that there are multiple ways to encode some values. And that makes no sense at all. But then I realized you probably made up data with the simulator to try all possible bit patterns. I
Correct
guess that in real data, we'll only see these 4 bit patterns:
001 -> 1
nope: 111 is what they use for 1 (1C, actually)
010 -> 2 100 -> 3 000 -> 4
I'll need to look what I can find in this week's dives as I simulated having two and three tanks.
Maybe it's something like this then:
111 -> 1 110 -> 2 100 -> 3 000 -> 4
Thus for every 1 bit one tank gets removed. If tanks can only be disabled in order (e.g. disabling the 3th tank is only allowed if the 4th tank is disabled too) that even makes sense.
Is it possible to disable all 4 tanks? Because that doesn't seem to be possible with just 3 bits. Could there be a 4th bit?
BTW: your patches are completely unusable. I recommend against pushing them as they are and claiming A300CS support. At least not with my SOB line.
I have 27 dives on my A300CS. The download has been running for 15 minutes now. We're on dive 6. No, you cannot simply say "oh, the A300CS supports B1, let's force using that". This turns a perfectly usable dive computer which downloads half a dozen dives in under 10 seconds into a completely unusable piece of garbage that takes forever to download a day's worth of diving.
There's probably a performance difference of roughly a factor 16 between the B1 and B8 commands.
From your previous email, I understood that you were okay with already applying those 4 patches now and postpone the implementation of the more efficient B8 command later. But if you prefer to wait until we have a complete solution that's fine too.
The current status is as follows:
1. Your patch #1 changes the multipage code to always create aligned reads. But the vtpro and veo250 backends, for which the multipage algorithm was created, doesn't need aligned reads. So this results in more I/O operations. Probably not a great deal on its own, but if possible I would like to avoid this.
One question for you. Did you tried unaligned reads on your A300CS. I know the Aeris/Oceanic software always does aligned reads, but that might be due to how it reads the data. It always starts at the first page, and then advances to the next. Of course this always results in aligned reads by definition. But maybe unaligned reads work too?
2. Your patch #2 switches between B1, B4 and B8 commands dynamically, depending on the alignment and length. But for the Aeris F11, it seems that it's not possible to use the B1 command for reading pages past 64K. If that's indeed correct (I'm still waiting for confirmation here), then we'll have to stick to one variant of the read command only.
So applying those two patches now, while there is good indication that they won't work for other devices, sounds like a bad idea. So that's why I proposed to already apply the other ones, and address the performance issue later, once we know exactly what we need there. But right now, I'm still waiting for confirmation on the F11 issue.
Jef
On Sun, Oct 12, 2014 at 12:08:10PM +0200, Jef Driesen wrote:
After that all I did was to start the simulator on /dev/ttyGS0 and to have the Windows app talk to the fake device.
The Windows driver still fails to load for me. Not sure why.
There are multiple ways to install it. I think for me installing it from within the Aeris app was what finally worked.
guess that in real data, we'll only see these 4 bit patterns:
001 -> 1
nope: 111 is what they use for 1 (1C, actually)
010 -> 2 100 -> 3 000 -> 4
I'll need to look what I can find in this week's dives as I simulated having two and three tanks.
Maybe it's something like this then:
111 -> 1 110 -> 2 100 -> 3 000 -> 4
Thus for every 1 bit one tank gets removed. If tanks can only be disabled in order (e.g. disabling the 3th tank is only allowed if the 4th tank is disabled too) that even makes sense.
Is it possible to disable all 4 tanks? Because that doesn't seem to be possible with just 3 bits. Could there be a 4th bit?
Careful. You don't "disable tanks" manually on the Aeris. It simply tells you how many were used. So this value changes when you change gas.
BTW: your patches are completely unusable. I recommend against pushing them as they are and claiming A300CS support. At least not with my SOB line.
I have 27 dives on my A300CS. The download has been running for 15 minutes now. We're on dive 6. No, you cannot simply say "oh, the A300CS supports B1, let's force using that". This turns a perfectly usable dive computer which downloads half a dozen dives in under 10 seconds into a completely unusable piece of garbage that takes forever to download a day's worth of diving.
There's probably a performance difference of roughly a factor 16 between the B1 and B8 commands.
From your previous email, I understood that you were okay with already applying those 4 patches now and postpone the implementation of the more efficient B8 command later. But if you prefer to wait until we have a complete solution that's fine too.
While I find this frustating, sure, go ahead, apply them. Carrying them outside of libdivecomputer makes no sense.
The current status is as follows:
- Your patch #1 changes the multipage code to always create aligned reads.
But the vtpro and veo250 backends, for which the multipage algorithm was created, doesn't need aligned reads. So this results in more I/O operations. Probably not a great deal on its own, but if possible I would like to avoid this.
So let's just do the alignment if bigpage is 16.
One question for you. Did you tried unaligned reads on your A300CS. I know the Aeris/Oceanic software always does aligned reads, but that might be due to how it reads the data. It always starts at the first page, and then advances to the next. Of course this always results in aligned reads by definition. But maybe unaligned reads work too?
I tried and got NAK back. That's when I noticed that the multipage code doesn't align the reads in the first place :-)
- Your patch #2 switches between B1, B4 and B8 commands dynamically,
depending on the alignment and length. But for the Aeris F11, it seems that it's not possible to use the B1 command for reading pages past 64K. If that's indeed correct (I'm still waiting for confirmation here), then we'll have to stick to one variant of the read command only.
Here I disagree. We have to stick with one variant FOR THE F11. There is no reason to do suboptimal implementation for the other DCs just because one of them doesn't support something. Again, we could trigger the different behavior based on either the actual DC type or on whether bigpage is 8 or 16.
So applying those two patches now, while there is good indication that they won't work for other devices, sounds like a bad idea. So that's why I proposed to already apply the other ones, and address the performance issue later, once we know exactly what we need there. But right now, I'm still waiting for confirmation on the F11 issue.
So let's apply the four and consider my proposal above for making the other two not break older Oceanic devices.
/D
On 2014-10-02 17:18, Dirk Hohndel wrote:
On Thu, Oct 02, 2014 at 05:08:42PM +0200, Jef Driesen wrote:
I may have an explanation for the DTR/RTS part, but not for the B1 part. The tester is using Windows. If you look at the serial_win32.c code, you'll notice that the serial_configure function always sets DTR/RTS with:
dcb.fDtrControl = DTR_CONTROL_ENABLE; dcb.fRtsControl = RTS_CONTROL_ENABLE;
So that means when you configure the serial settings on Windows, you are forced to also set (or clear) the DTR/RTS lines. This is different on Linux, where the api to control DTR/RTS is completely independent from the other settings. This independent api also exists on Windows, but DTR/RTS are already set and the extra calls are thus not necessary. That probably explains why it works out of the box on Windows, but not on Linux.
That would make sense - I did all that testing on Linux. What do you recommend we do about this? Make the additional DTR/RTS Linux specific, or simply leave them in place as they shouldn't hurt on Windows, either?
I see two possible solutions:
The first solution, which is what you did, is to set DTR/RTS unconditionally in the atom2 driver. On Windows this isn't strictly necessary, but it shouldn't really hurt either. If possible, operating system specific code should be kept out of the device backends, because it doesn't belong there.
The second solution is to set DTR/RTS in the Linux serial_configure() function. Right now the state of DTR/RTS is basically undefined after this call returs on Linux, while on Windows they are always set as a side effect of how the Windows api works. By setting DTR/RTS in the Linux code too, the resulting state would be the same. So this would lead to more consistent behavior across different operating systems. On the other hand, devices that need DTR/RTS to be in a specific state should always set them explicitly, because the serial_configure() has never guaranteed anything about the state of DTR/RTS.
These two options are not mutually exclusive. So I think setting DTR/RTS in the atom2 driver is fine. But in addition it might be a good idea to change the Linux serial code to match its Windows counterpart. Just to avoid similar surprises in the future.
Jef
-
Dirk Hohndel -
Jef Driesen