On 14-10-14 19:27, Dirk Hohndel wrote:

On Tue, Oct 14, 2014 at 06:41:29PM +0200, Dirk Hohndel wrote:

...

as discussed earlier today, here are patches that implement aligned multipage reads and mixed blocksize reads ONLY for dive computers that support 256 byte page reads. So far that appears to be only the A300CS but my guess is that we'll see more such dive computers in the future.

This way we should get performant downloads from the A300CS without risking to break older dive computers.

Man, you're fast! You have a patch ready before I'm even home :-)

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?

BTW: if you'd prefer we could add another field (something like "mixed_pagesize_support") to oceanic_atom2_device_t and then could enable this for those dive computers with 128 byte pages where we can test and verify that this works. That would make it less of a special case for the A300CS. I wasn't sure which version you'd prefer, so I implemented the A300CS specific one (or actually, the 256 byte page specific one), but I'm happy to go the other direction if you think that's better.

Actually, the more I think about it, maybe that's the better way in case Oceanic / Aeris come out with a 256 byte page read model that doesn't support mixed paged sizes...

I'm not sure if this really helps. A model that doesn't support mixing page sizes, will likely no longer support the smaller page size and always require the larger page size. (That appears to be what's happening with the F11.) But your "mixed_pagesize_support" patch does exactly the opposite: it falls back to using single pages only. But why would you enable bigpage in the first place? Just keep it at one, and you're done. Or am I missing something else?

With that in mind, I think your first set of patches is the better one.

[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.]

Jef

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