[PATCH 5/6] Cochran: Dump of Commander TM devices is now supported

[Jef Driesen]

On 2017-07-15 22:39, John Van Ostrand wrote:
>  static dc_status_t
> -cochran_commander_read_config (cochran_commander_device_t *device,
> dc_event_progress_t *progress, unsigned char data[], unsigned int
> size)
> +cochran_commander_read_config (cochran_commander_device_t *device,
> dc_event_progress_t *progress, unsigned char data[])

Removing the size parameter is a bad idea. Any function that takes some 
buffer as argument should also have a corresponding size parameter. Even 
if it's only used to verify whether the caller passed the right amount:

if (size % 512 != 0)
    return some_error;

If you simply trust the caller to always pass a buffer of the right 
size, you'll run into trouble someday!

>  	dc_device_t *abstract = (dc_device_t *) device;
>  	dc_status_t rc = DC_STATUS_SUCCESS;
> 
> +	unsigned int pages = 2;
> +
> +	if (device->layout->model == COCHRAN_MODEL_COMMANDER_TM)
> +		pages = 1;
> +
>  	// Read two 512 byte blocks into one 1024 byte buffer
> -	for (unsigned int i = 0; i < 2; i++) {
> -		const unsigned int len = size / 2;
> +	for (unsigned int i = 0; i < pages; i++) {
> 
>  		unsigned char command[2] = {0x96, i};
> -		rc = cochran_commander_packet(device, progress, command,
> sizeof(command), data + i * len, len, 0);
> +		unsigned int command_size = 2;
> +		if (device->layout->model == COCHRAN_MODEL_COMMANDER_TM)
> +			command_size = 1;
> +
> +		rc = cochran_commander_packet(device, progress, command,
> command_size, data + i * 512, 512, 0);
>  		if (rc != DC_STATUS_SUCCESS)
>  			return rc;

How about using the size argument to calculate the number of pages? That 
way the function becomes even more generic. Only the caller needs to 
know how many pages there are. The function just needs to handle the 
difference between the 1 or 2 byte command variant (and even that can be 
based on the number of pages).

> +		if (device->layout->baudrate == 9600) {
> +			// Older commander, use low-speed read command
> +			command[0] = 0x05;
> +			command[1] = (address      ) & 0xff;
> +			command[2] = (address >>  8) & 0xff;
> +			command[3] = (address >> 16) & 0xff;
> +			command[4] = (size         ) & 0xff;
> +			command[5] = (size >> 8    ) & 0xff;
> +			command_size = 6;
> +		} else {
> +			// Newer commander with high-speed read command
> +			command[0] = 0x15;
> +			command[1] = (address      ) & 0xff;
> +			command[2] = (address >>  8) & 0xff;
> +			command[3] = (address >> 16) & 0xff;
> +			command[4] = (size         ) & 0xff;
> +			command[5] = (size >>  8   ) & 0xff;
> +			command[6] = (size >> 16   ) & 0xff;
> +			command[7] = 0x04;
> +			command_size = 8;
> +		}

So this means that the byte sequences:

05 9D FF 00 43 00
05 BD 7F 00 43 00

that are send in the cochran_commander_read_id() function are actually 
read operations at the addresses 0xFF9D and 0x7FBD (and a size of 0x0043 
bytes). Interesting. I didn't realize that before.

> -	// Read the sample data, from 0 to sample end will include logbook
> -	rc = cochran_commander_read (device, &progress,
> device->layout->rb_logbook_begin, dc_buffer_get_data(buffer),
> device->layout->rb_profile_end);
> +	// Read the sample data, logbook and sample data are contiguous
> +	rc = cochran_commander_read (device, &progress,
> device->layout->rb_logbook_begin, dc_buffer_get_data(buffer), size);
>  	if (rc != DC_STATUS_SUCCESS) {
>  		ERROR (abstract->context, "Failed to read the sample data.");
>  		return rc;

This can't work. The size variable is calculated as:

layout->rb_profile_end - layout->rb_logbook_begin

For the Commander TM that is 0x010000 bytes (0x020000 - 0x010000). But 
the size field in the command is only 16 bits wide. So you'll try to 
read 0 bytes. Oops. I think the only solution here is to split the large 
read operation into multiple smaller blocks.

Jef