Cochran changes

[Jef Driesen]

On 2015-01-19 02:05, John Van Ostrand wrote:
> I'm working though some of the changes you recommended and I'm looking 
> for
> advice.
> 
> 1. Fingerprint optimized download. The current logic only works when 
> the
> DC's logbook buffer hasn't wrapped and will likely fail with a segfault
> when not the case. I have to treat the logbook like a ring buffer 
> buffer
> and maybe I have to treat the profile data like ring buffer too. The
> logbook is small enough that optimizing its download makes less sense 
> but
> optimizing the profile data would. It would mean more complicated logic 
> and
> potentially downloading two segments if the profile data has wrapped. 
> I'd
> create a buffer the size of DC memory and write the profile data in the
> proper areas to make the logic easier.

A ringbuffer should be treated as a ringbuffer. Anything else is just 
plain wrong. You might be able to get away with it for now (e.g. because 
many hundreds of dives are required to fill a very large logbook 
ringbuffer), but rest assured: someday there will be someone that hits 
those limits!

> 2. Foreach function size. You seem to want the read logic in the 
> foreach
> function. If I put it all in-line, including the progress updates, 
> logic to
> locate the fingerprint, the logic to determine which profile data to 
> read
> it's going to be a very large function. It seems that keeping it 
> seperate
> would make it much more readable. With those functions outside 
> foreach() is
> 70 lines. You proposed version which is lacking many lines of code is 
> 140
> lines and

Don't get me wrong. I certainly don't mind helper functions to keep the 
length of functions under control. I do care a lot about readable source 
code, and I consider that more important than small functions. So I 
don't mind a longer function, as long as the logic is easy to follow.

Originally you had several helper functions that download a piece of 
data, parse it and store the result somewhere in the device handle. 
That's not only the wrong way to pass the result back to the caller, but 
it also makes the code hard to understand. You need to actually read the 
code of the helper function in order to see what exactly is calculated 
and where it gets stored. That's what I complained about.

I'll illustrate with an example. Currently there is code that looks 
similar to this:

static dc_status_t
cochran_read_config (abstract, progress)
{
    if (emc)
       data->config_count = 2;
    else
       data->config_count = 4;

    for (i = 0; i < data->config_count; i++) {
       /* Download the i-th config page here. */
       unsigned char command[2] = {0x96, i};
       cochran_packet(abstract, progress, command, 2, data->config[i], 
512, 0);
    }
}

static dc_status_t
cochran_commander_device_foreach (abstract, callback, userdata)
{
    dc_event_progress_t progress;
    progress.current = 0;
    progress.maximum = ...; /* How do I know how much will be downloaded? 
*/

    cochran_read_config(abstract, &progress);

    /* How do I know where the data is? */
}

Compare that with a simple helper function that only downloads the data, 
and leaves the processing to the caller. The calling function may indeed 
become a bit longer (note that the processing could be done in yet 
another helper function), but the flow will be easier to follow:

static dc_status_t
cochran_read_config (abstract, data, size, n)
{
    /* Download the n-th config page here. */
    unsigned char command[2] = {0x96, n};
    cochran_packet(abstract, NULL, command, 2, data, size, 0);
}

static dc_status_t
cochran_commander_device_foreach (abstract, callback, userdata)
{
    unsigned char config[512];

    dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER;
    progress.maximum = sizeof(config); /* Size is known here. */

    /* We only need the 0-th page. No need to know how many pages there 
are. */
    cochran_read_config(abstract, config, sizeof(config), 0);

    progress.current += sizeof(config);
    device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);

    /* We can use the local config array here. */
}

When reading the foreach function now, it's very clear what's going on, 
even without any knowledge of the internals of the helper function.

> 3. I'm concerned about the performance cost of using ring-buffer logic 
> for
> profile data, although it solves existing bugs. The Cochran's profile 
> data
> is not stored in blocks, it's a byte stream in which two or three bytes
> describes a second's worth of most data. Some data, like NDL, deco stop
> time and dive events are spread across many bytes and require reading
> ahead. As a result the code would be calling ring buffer_increment a 
> lot,
> samples are every second, so a 1 hour dive would require over 11,000 
> calls
> to ring buffer_increment. I could add some logic to workaround that, it
> might not be too bad.

Why would you need ringbuffer logic for parsing? At that point, the 
ringbuffer has already been "flattened". All the ringbuffer logic should 
be handled in the device foreach function. The dive data that gets 
passed to the callback function should be a normal linear buffer. Am I 
missing something?

Jef