OSTC3 firmware upgrades

[Jef Driesen]

On 14-11-14 18:01, Anton Lundin wrote:
> This is is a RFC series of patches to add support for firmware upgrades
> on the OSTC3. The code have bin tested both against a "pcb" by HW and
> against my OSTC3 so it works and does the right things. JeanDo's
> ostc-companion[1] have bin the reference implementation I've looked at
> when I wrote this code.
>
> There are still a couple of pain-points that I would like some feedback
> on:

I still need to have a closer look at all the implementation details. But let's 
already start with the higher level questions.

> How to start the device in service mode:
> To get access to the necessary commands for service mode, we can't INIT
> the device with INIT(0xBB), we need to INIT it with a special service
> mode sequence(0xAA, 0xAB, 0xCD, 0xEF). Now the INIT is sent in
> hw_ostc3_device_open, so how do you suggest that we refactor the code to
> allow another sequence after open?
>
> The current quite crude hack is to just EXIT(0xFF) the device and
> re-init it after a 6s sleep. This won't work against the OSTC Sport.

I'm not really sure what would be the best solution here, so I'm just providing 
some ideas here:

One solution would be to remove the INIT from open, and maintain the current 
state (none, download or service) in the device handle. Initially the state 
would be "none". Then, in each of the public functions, we check the state and 
if it's "none", we automatically enter download or service mode. If we're in the 
wrong state, we fail an error.

Another alternative is to have a second variant of the open function that takes 
a parameter to indicate which mode you want. The tricky part here, is that the 
high-level dc_device_open() function won't know about this function, so it will 
always call the download variant. Thus to flash the firmware, an application 
would be forced to use the new variant. (Note that after v0.5 I intend to remove 
the vendor_product_device_open() functions from the public api and require 
applications to use the generic dc_device_open() function instead. But the 
service mode variant can stay as a vendor specific function, just like the 
hw_ostc3_device_fwupdate function itself.)

Yet another option is to remove the dc_device_t parameter from the 
hw_ostc3_device_fwupdate function, and let it open the device itself. Then we 
call the service mode variant of the open function internally.

On the other hand, I noticed some comment that the key is different for the ostc 
sport. So that means we somehow have to detect the right model. This is done 
with the version command, but I'm not sure if you can use this command in 
service mode. So we may need to identify the exact model in download mode first 
and then switch to service mode anyway. You say this won't work for the sport, 
so what's the problem there? Is that maybe because it exits bluetooth mode, and 
thus the serial port disappears? (I haven't used a sport myself, so I simply 
don't know.)

Right now, I have a preference towards the second option, with a separate open 
function for the service mode. It looks the least complex to me. But I'm not 
ruling out other options yet.

> The timing between command and data in write_block:
> The current hw_ostc3_device_write_block is a bit of copy-paste from
> hw_ostc3_transfer, to be able to send the data buffer to the device,
> without a additional copy.

I wouldn't worry about an extra data copy. That's inexpensive compared to the 
actual I/O. But based on the comments, I guess the real problem is something 
else? Could it be that it seems there is no echo for the WRITE_BLOCK command?

> Library triggered closed state:
> The device reboots when it gets the FLASH_FIRM-command so how can we
> "not fail" when the application then tries to talk to the device
> afterwards, or even call hw_ostc3_device_close?

I assume that the problem is that after the FLASH_FIRM command, we don't need to 
send the EXIT command in hw_ostc3_device_close? Or did you mean something else?

> There are also a couple of other FIXME's in the code, which I plan on
> cleaning up, but I thought that I should send out a first RFC patch
> series to get some additional feedback going.
>
>
> Any other ideas about how the code looks?

I noticed a large number of sleep and flush calls. Are those really necessary? I 
have the feeling some of them were just copy and pasted.

Jef