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