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.
- 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!
- 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.
- 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