[PATCH 09/11] Add a function upgrade the firmware in the OSTC3

[Jef Driesen]

On 16-12-14 21:39, Anton Lundin wrote:
> On 16 December, 2014 - Jef Driesen wrote:
>
>> On 2014-11-21 21:28, Anton Lundin wrote:
>>> +static dc_status_t
>>> +hw_ostc3_device_upgrade_firmware (dc_device_t *abstract, unsigned int
>>> checksum)
>>> +{
>>> +	dc_status_t rc = DC_STATUS_SUCCESS;
>>> +	hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
>>> +	dc_context_t *context = (abstract ? abstract->context : NULL);
>>> +	unsigned char buffer[5];
>>> +	uint32_le_array(checksum, buffer);
>>> +
>>> +	// Compute a one byte checksum, so the device can validate the firmware
>>> image.
>>> +	buffer[4] = 0x55;
>>> +	buffer[4] ^= buffer[0];
>>> +	buffer[4]  = (buffer[4]<<1 | buffer[4]>>7);
>>> +	buffer[4] ^= buffer[1];
>>> +	buffer[4]  = (buffer[4]<<1 | buffer[4]>>7);
>>> +	buffer[4] ^= buffer[2];
>>> +	buffer[4]  = (buffer[4]<<1 | buffer[4]>>7);
>>> +	buffer[4] ^= buffer[3];
>>> +	buffer[4]  = (buffer[4]<<1 | buffer[4]>>7);
>>
>> Can you change this into a loop? It's already more than cryptic enough. Any
>> idea whether this is some known checksum?
>>
>
> I've looked around and there are no hints in ostc-companion or the ostc3
> firmware. I've asked around a bit if someone have seen it somewhere
> else, but as far as i know, its something home grown.
>
> I've compacted it with a loop as you requested.

I couldn't find anything either. It's a bit of a weird checksum. First of all 
it's a checksum of another checksum. Those circular shifts are a bit unusual 
too. But if that's what we need, then that's what we implement :-)

>>> +	// Now the device resets, and if everything is well, it reprograms.
>>> +	serial_sleep (device->port, 500);
>>> +
>>> +	// FIXME: How should we force the application to close the device here?
>>
>> Why do we need to wait here? If the device is rebooting, then the firmware
>> update is successful, right? Since we won't get any confirmation from the
>> ostc, what's the point of waiting here? It only blocks the caller from
>> calling close. Or am I missing something else?
>
> That sleep was probably just re-implemented from ostc-companion. Works
> just fine without it. Removed it.

Cool. No need to slow things down for no good reason.

>> What happens at this point? Does the usb-serial device node (e.g.
>> /dev/ttyUSBx) disappear? Setting the state to REBOOTING (as you changed the
>> FIXME in one of the other patches) prevents to do anything, except for
>> calling close. That's probably the right thing to do, but I just wonder
>> what's going on under the hood.
>>
>
> For the ostc3, the host computer sees the ftdi-chip all the time.
>
> The FIXME here was just left because I've managed to squash the commit
> that removed it into the next one in the series, and thats why its
> removed there.
>
>> This might be different for the ostc3 (usb-serial) and sport (bluetooth),
>> because I suspect the sport will terminate the bluetooth during the reboot,
>> while for the ostc3 the usb-serial chip is still there.
>
> Yepp, it terminates the connection.

Makes sense indeed.

> So, what do you suggest, that we should change the REBOOTING state to
> CLOSED and close the device in hw_ostc3_device_upgrade_firmware?
>
> I think that would be a really confusing flow.

No, that's not what I meant. The way you implemented it with the special 
REBOOTING state (or CLOSED or whatever you want to call it) is fine. Closing the 
actual serial connection should be done only by the close function. Otherwise we 
would indeed end up with a very confusing flow. The serial connection remains 
open for as long as the device handle is open. After a firmware update you may 
not be able to re-use it any longer, but that's another problem. We can check 
the state for that.

[For background information: For the new api design, I want to move open/close 
of the serial connection to the application side. The rationale behind that is 
to support alternative implementations (e.g. custom user-space usb-serial 
drivers for Android), and device enumeration. In that case the libdivecomputer 
device backends should certainly never close the underlying connection, because 
that'll become the responsibility of the application.]

Jef