[PATCH] Revisions requested

[Jef Driesen]

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

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?

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.

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

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.

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.


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.

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

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

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

> 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 :-)

That's it for now.

Jef