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

[Anton Lundin]

On 17 December, 2014 - Jef Driesen wrote:

> 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?
> 

I'd say REBOOTING is better than CLOSED, because CLOSED might be
confusing because the file descriptor is still open, and the only viable
action is to close the device.

> 
> 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.
> 

Yes, all those commands can be issued while in service mode to, but none
of the service mode commands can be used in download mode.

> 
> 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?
> 

Yepp. Thats the plan when everything baked for this round of review.

I'll add a reviewed-by tag the patches to.


//Anton


-- 
Anton Lundin	+46702-161604