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