first stab at Aeris A300CS support

[Dirk Hohndel]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: A300CS.zip
Type: application/zip
Size: 66316 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20140926/deacf930/attachment-0001.zip>