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