[PATCH 03/11] Lift the OSTC3 INIT out of open

[Jef Driesen]

On 15-12-14 23:43, Anton Lundin wrote:
> On 15 December, 2014 - Jef Driesen wrote:
>> Why do we need both NONE and OPEN? I don't see any difference between the
>> two. The api prevents you from returning an handle in the NONE state. So I
>> see no reason to keep the OPEN state.
>>
>
> I just thought it would be sane to actually represent all the states we
> can have, if we where to change something in the future in how we do
> things.

Sure, but my point is that your NONE state is not really a valid state. It only 
exists for a very brief moment in the open function. If it succeeds the final 
state is OPEN, and if it fails we don't return a valid device handle.

With all the other patches in mind, we basically need 4 different states:

  * NONE: This is the initial state after opening the connection. At this point 
we the serial connection is opened, but the device hasn't been initialized yet. 
 From here you can go into DOWNLOAD or SERVICE mode.

  * DOWNLOAD: The normal download mode.

  * SERVICE: The state during firmware upgrades. You can only get stuck in this 
state if a firmware upgrade fails. If it succeeds, you automatically go to the 
next state.

  * REBOOTING: This is the final state after a successful firmware update. In 
this state no other communication is possible and the caller should close the 
device handle.

Maybe it's more intuitive to rename NONE to OPEN and REBOOTING to CLOSED?


BTW, do you know whether there is a limitation on which commands can be send in 
download or service mode? Can you use for example the IDENTITY, CLOCK, 
CUSTOMTEXT, READ, WRITE, RESET in service mode, or are they restricted to 
download mode only? This matters because these commands have corresponding 
public functions that can be called by the user.


The remaining comments I have are mostly purely cosmetic things. Nothing 
critical, just nice to have. For example the new functions are named with two 
different prefixes: hw_ostc3_firmware_xxx and hw_ostc3_device_xxx. It would be 
nice to make this more consistent. For example:

hw_ostc3_device_init              -> hw_ostc3_device_init_download
hw_ostc3_device_service_mode      -> hw_ostc3_device_init_service
hw_ostc3_device_erase_range       -> hw_ostc3_firmware_erase
hw_ostc3_device_read_block        -> hw_ostc3_firmware_block_read
hw_ostc3_device_write_block       -> hw_ostc3_firmware_block_write
hw_ostc3_device_upgrade_firmware  -> hw_ostc3_firmware_upgrade

And maybe also prefix the firmware commands with S_ to distinguish them from the 
normal ones. For example:

S_BLOCK_READ
S_BLOCK_WRITE
S_ERASE
S_READY
S_UPGRADE

If you don't mind, also move the hw_ostc3_firmware_t struct and ostc3_key near 
the top of the file.


Other than that, I think you have addressed all my comments and questions. So 
we're almost there. Can you submit a new patch series once you have put 
everything together?

Jef