[PATCH] Revisions requested

[John Van Ostrand]

On Sun, Nov 2, 2014 at 1:13 PM, Jef Driesen <jef at libdivecomputer.org> wrote:

> John,
>
> Nice work. This is already a huge step in the right direction, but we're
> not there yet.
>
> There is a problem in the way you store the cochran_data_t structure in
> the device handle. Most of this info only used by the dump/foreach function
> (or one of its helpers), so it doesn't really belong inside the handle.



The structure members under "// Config items" are used to allow for
different Cochran models and different feature configurations for a given
model. The fields are set during cochran_set_device_config() so that I know
how to handle reading the logbook and the sample data. So while I'm
detecting the model I thought I might as well set the variables needed in
foreach so I have one section of code to look at when adding a new model.

Some of the variables I need to keep between calls to foreach and this
seemed to be a good place.

I know I'm keeping id*, config* and misc* around after they are needed. I
could drop them sooner but they come in handy for the dump function.


> This results in a number of problems. For example you allocate several
> pieces of data (id, config, etc) in those functions and store them in the
> handle. But if someone calls one of those functions a second time, you
> allocate them again and overwrite the original pointers. So you'll end up
> with a memory leak. You can free the old data first, but I think you're
> better off using local data structures because there is no reason why any
> of this data needs to persist between the high-level calls.
>
>
I see, calling _open and _foreach a second time could leak memory.

Fixed.


> Overall, I find the logic rather hard to follow because it's spread over
> all your cochran_read_* functions. These functions do not only download
> some data, but also sets some fields, which are then used in one of the
> other functions. I think it would be easier if the processing was a bit
> more separated from the downloading. I have the feeling that there's
> something wrong, but I can't really tell what or where. Maybe it's just
> that I don't understand the format very well. Do you have some data files
> available?
>

There are two reasons I split the read up into several functions. The
cochran_read_id() function reads the blocks needed to detect the DC model
so it knows how to perform the other reads. I do this in the
cochran_common_device_open() because I wanted to give immediate feedback to
the application if the DC wasn't supported.

The other reads could be in one function but it would be a large function.
There is some handle data settings in the read functions.

cochran_read_id(): determine model type and features
cochran_read_config(): determine the head pointer to the sample buffer, I
see now that I don't use this.
cochran_read_misc(): read only to mimic Analyst
cochran_read_logbook(): Get number of logged dives(from config1), calculate
logbook_size
cochran_read_parms(): Doesn't read anything, calculate parameters needed
for reading samples.
cochran_read_samples(): Reads sample data.

There really isn't much happening inside the read calls that isn't needed
there. The read_config() has a few lines that can actually be removed. The
read_logbook has another few lines that calculate the size, and
read_samples has two lines that calculate size. Since all the read_*()
functions are called by read_all() it's trivial to move them there.

Done. Calculations are moved to the read_all function.


> Note that several of those data pieces have a fixed size, so why even
> allocate them? Just use a byte array with the right size. Some like:
>
> struct {
>     ...
>     unsigned char id0[67];
>     unsigned char id1[67];
>     unsigned char config1[512];
>     unsigned char config2[512];
>     ...
> };
>
> Now you no longer have to worry about freeing them. Instead of config1,
> config2, etc you could even use an array here. That will make your code
> even more generic. Instead of repeating the same code, you can use a loop.
>
>
I used mallocs to save memory. I can make them arrays. Done.


> BTW, I'm not sure what's in these config pages, but based on the name, I
> assume it contains settings. So this is something applications may want to
> access directly. I suspect there the protocol also support changing those
> settings. Have a look at the hw_ostc_device_eeprom_{read,write}. (This is
> certainly not a must have feature right now, but it's something you may
> want to keep in mind already.)
>
>
Yes, that is device configuration as well as current calculated tissue
loads. I didn't look at the OSTC code so I didn't see any evidence of
setting configs on the DCs. I intend to write code to do just that for the
Cochran and I wasn't sure where to do that since it looked like
libdivecomputer doesn't expose an API for it. There are some settings, like
date/time that aren't configurable on the DC. And I would like to be able
to set configuration from Linux or even a tablet.


> You have the same problem with the progress field. Again, I think it makes
> more sense to make this a local variable, and pass it as a parameter to the
> internal read function. Note that you're also reporting progress for each
> individual step, rather than the entire download.
>
>
I don't know the size of the entire download when I start needing a
progress bar. You'll note I don't have one for cochran_read_id(),
read_config() or read_misc(). These are small enough I figured one wasn't
needed. I don't know the size of the sample data until I read the logbook.
So it seemed the best approach was to have two.

The other backends I looked at read the from the DC in blocks of data and
were able to manage the progress within the function. The cochran does very
big reads at very high data rates (800Kb/s). So I can't do the progress at
the cochran_common_device_read() function because it issues one command to
read a huge block of data. So I did it in the cochran_packet() function.
Since that is exposed I couldn't add a parameter without breaking the API.
I could add a separate packet function that accepts a progress but this
seems to be a better approach.

Those high baudrates are indeed a bit odd. Do they even work? You don't
> check the return value of the serial_configure() function. So if the
> underlying driver (or OS) doesn't support it, unexpected results may
> happen. I implemented the support for non-standard baudrates long time ago,
> and unfortunately it's not guaranteed to work everywhere. Initially it was
> needed for the iconhd backend, in the end it turned out that this baudrate
> was a bug in the mares application. The iconhd uses some microchip with
> CDC-ACM, where the baudrate doesn't matter at all. So every value works
> equally well.
>

They are little twitchy but I get good reads most of the time. I've tried
different rates and it does require a non-standard rate. At least I'm not
aware of a standard rate that high.

On 28-10-14 01:18, John Van Ostrand wrote:
>
>> Removed dependency on packed structure.
>>
>
> Nice. I noticed that you introduced CMD_* and EMC_* constants to keep the
> code readable. An alternative I have used in some backends, is to have a
> layout structure containing those offsets. And then you define one such
> structure for each model, and store a pointer to the correct table in the
> device/parser handle. This allows you to write code like this:
>
> value = array_uint16_le (log + layout->xxx);
>
> You can see this for example in the hw_ostc_parser.c file. The nice thing
> is that you can re-use the same code for all models. Adding a new model is
> just a matter of adding a new structure.
>
> I'm not saying you can or must use this technique in your backend, but
> it's maybe something to consider.
>

That's a good idea.


>
>  Removed extraneous close/opens.
>>
>
> I'm glad those are not necessary after all.
>
>  Formated lines to 78 chars.
>>
>
> This wasn't needed. I have no strict limits on the line length. Of course
> you should try to keep the line length reasonable, but if you have a longer
> one once in a while, I won't complain. Artificially limiting the lines to
> 78 chars makes the code only less readable. So please don't do that.
>
> For example some of those long lines are because you have assignments in
> if conditions:
>
>         if ((rc = cochran_common_serial_open(device, context))
>                         != DC_STATUS_SUCCESS)
>                 return rc;
>
> replace that with:
>
>         rc = cochran_common_serial_open(device, context);
>         if (rc != DC_STATUS_SUCCESS)
>                 return rc;
>
> Much more readable and shorter too!
>

Fixed.


>
>  Merged EMC and Commander backends to common.
>>
>
> Great. Just a few issues remaining here.
>
> You named your backend DC_FAMILY_COCHRAN and the corresponding functions
> cochran_common_{device,parser}_*. Please name them after one specific
> model. For example DC_FAMILY_COCHRAN_COMMANDER and
> cochran_commander_{device,parser}_*, etc. That's because in the future,
> there might be more than one cochran backend. The backend name is purely
> internal (except for the family name), so it doesn't really matter that the
> commander backend also supports the emc.
>
> For the public header files, make one cochran_commander.h file containing
> the open and create functions (e.g. your current cochran.h file), and then
> a new cochran.h file that just includes this file. See the existing files
> for examples.
>

Done.


>
>  Change nanosleep() calls to serial_sleep().
>>
>
> We're getting closer, but the windows build is still broken:
>
>   CC       cochran_common_parser.lo
> In file included from ../../source/src/cochran_common_parser.c:33:
> ../../source/src/cochran_common.h:73: error: expected
> specifier-qualifier-list before 'time_t'
> ../../source/src/cochran_common_parser.c: In function
> 'cochran_common_parser_get_datetime':
> ../../source/src/cochran_common_parser.c:113: error: 'cochran_data_t' has
> no member named 'date_format'
> ../../source/src/cochran_common_parser.c: In function
> 'cochran_common_parser_get_field':
> ../../source/src/cochran_common_parser.c:147: warning: control reaches
> end of non-void function
> ../../source/src/cochran_common_parser.c: In function
> 'cochran_common_parser_samples_foreach':
> ../../source/src/cochran_common_parser.c:166: warning: control reaches
> end of non-void function
> ../../source/src/cochran_common_parser.c: In function
> 'cochran_common_handle_event':
> ../../source/src/cochran_common_parser.c:175: warning: unused variable
> 'data'
> make[1]: *** [cochran_common_parser.lo] Error 1
>
> I think you're missing an #include <time.h> somewhere for the use of
> time_t in the cochran_data_t structure. But you shouldn't need that because
> libdivecomputer has its own time functions and structures. Use those
> instead.
>

I think I've fixed that.


>
>  Change cochran_common_dump to use dc_buffer properly.
>>
>
> I still need to look at the details of your new dump implementation. One
> thing I did notice is that you forgot to remove the cochran_data_dump from
> the public header :-)
>

Done.


>
> That's it for now.
>
> Jef
>



-- 
John Van Ostrand
At large on sabbatical
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141102/88439bb3/attachment.html>