Oceanic / Aeris A300CS multipage patches

[Jef Driesen]

On 2014-10-14 22:20, Dirk Hohndel wrote:
> On Tue, Oct 14, 2014 at 10:11:03PM +0200, Jef Driesen wrote:
>> Looks good to me. Just one comment. Are you aware that you are only 
>> creating
>> aligned reads for the profile data, and not the logbook data?
> 
> That's interesting. When I traced this it got me aligned reads 
> everywhere.
> I wonder if this was a coincidence... more to test...

I think that's indeed a coincidence. I can even explain you why. The 
A300CS logbook ringbuffer starts at address 0x900, which is nicely 
aligned. Each next dive will add a 16 byte logbook. So, for your memory 
dump containing only 9 dives, they are located at these pages:

0090
0091
0092
0093
0094
0095
0096
0097
0098

The logic in the oceanic_common_device_foreach() function will start at 
the most recent entry (0098) and move backwards to the oldest entry 
(0090). Because you have fewer than 16 dives, the multipage code will 
request 9 pages, starting from page 0090. So this is an aligned read. 
Although with fewer than 16 pages, so the B8 command isn't used.

Now, fast forward to a situation where you have 17 dives (or any larger 
number that's not a multiple of 16). Your most recent dive will now be 
located at page 00A0. The multipage code will request 16 pages, starting 
at page 0091, followed by another request for the remaining page at 
0090. As you you can see that first request is not aligned. There is 
simply no code that causes it to be aligned.

You might be confused by the fact that there is code that does create 
aligned reads. But that's for a completely different purpose. With the 
normal 8 byte logbook entries (e.g. not the A300CS which has 16 byte 
logbook entries), the oldest/newest entries may not be aligned to the 16 
byte pagesize. So that's the kind of alignment that's done there. But 
that's a completely different type of alignment then the one we are 
talking about here.

> The mixed pages were introduced when I removed the caching code.
> But we could completely remove that and simply do the hand full of 
> extra
> redundant reads and not switch page sizes for any of the models.
> 
> But I guess I shouldn't even offer that since you seem fine with the 
> first
> set of patches:
> 
>> With that in mind, I think your first set of patches is the better 
>> one.
> 
> Forget what I said above :-)

I'm happy to apply your A300CS patches, in order to have a working 
solution for the A300CS immediately.

But that's not the complete picture. I think that in the long term (and 
"long" could be rather short in this context), we'll need to move to a 
different solution. One that never mixes different page sizes. I know 
that's not a requirement for the A300CS, but it looks like it is for the 
F11. In theory, we could do one thing for the A300CS, and something 
completely different for the F11. But if possible, I do prefer a generic 
solution that works for both models. That's not only easier to maintain, 
but will also simplify the code.

The reason I was holding back your patches, was not because I didn't 
want or like them. I was just looking a bit ahead already, for a 
solution that can support both the A300CS and F11. But as we discussed 
in person on Tuesday, we don't necessary need a perfect solution 
straight away. So in that sense, I'm happy to accept your current 
patches now, and then change whatever is necessary for the F11 later...

>> [The more I think about it, the more I actually like the idea of the
>> caching. Internally we can always use a fixed page size, regardless of 
>> the
>> requested size. Thus completely transparent to the calling code. But 
>> okay,
>> let's proceed with your latest patches now, and we'll look at what 
>> needs to
>> change for the F11 once we have more info about that. [Me thinking out 
>> load
>> now] I wonder if we would even need a full cache? Since 
>> libdivecomputer
>> always reads successive pages, we basically only need the latest page 
>> again.
>> Another thing that we may have to watch out for, are those few models 
>> with
>> an inaccessible last page. But I think I've only seen that for devices 
>> which
>> support the B1 read, so that should be fine.]
> 
> I can bring the cache back as well, no problem.
> 
> Literally, tell me what you want, I'll implement it :-)

I still don't have confirmation for the F11, but I'm like 99% sure that 
we should not mix different page size there, and instead use the B4 
command exclusively. The caching does fulfill that requirement, so right 
now the caching has my preference. The F11 is not even the only reason 
for that.

With the caching, the whole alignment problem for the logbooks that I 
explained above disappears. We don't even have to bother integrating the 
multipage algorithm. I'm not really concerned about the extra function 
calls, only the actual I/O operations. I wouldn't be surprised that one 
B4 or B8 command takes roughly the same time as a B1 command. So even if 
we would only use one page out of the 8 or 16 pages and discard the 
rest, the performance wouldn't be worse. Or course we do want the cache 
to speedup the whole thing :-)

Jef