On 09/19/2010 09:23 AM, Phil Dibowitz wrote: > Attached is a patch (which is just a diff from HEAD to the > zwave_work_branch) which moves us to a new and simpler API based on the > discussions between Stephen and I on this list. It's not exactly as we > discussed, I made various changes to make it a smooth transition and to more > closely match the way the library is used.
I've really only reviewed the new libconcord.h: 1) The new API design isn't sufficient for congruity to continue to operate in its current form while using the new high-level APIs. There is no way to query the set of steps that will be performed for a given operations file. I suppose I could simply give up on the current UI, and just have the UI built dynamically in response to the callbacks (or just remove all the progress bars, and spew out text status like concord or the Logitech SW). However, I'd rather that applications have the flexibility to operate in whichever mode they want. 2) The new API doesn't support multiple config or firmware blocks: 2a) The stage_id parameter to the callback is a stage type; there is no unique ID per stage. Those two things aren't the same if more than one stage of a given type must exist. This applies when more than one block of flash must be erased/written for a given operation. IIRC, the Logitech SW does this: Erase block 1 Program block 1 Verify block 1 (?) Erase block 2 Program block 2 Verify block 2 (?) ... Whereas the lc_callback design is limited to: Erase block 1 \_ LC_CB_STAGE_ERASE_FLASH Erase block 2 / Program block 1 \_ LC_CB_STAGE_WRITE_CONFIG Program block 2 / Verify block 1 (?) \_ LC_CB_STAGE_VERIFY_CONFIG Verify block 2 (?) / ... or repeating a given stage type/ID at multiple places in the sequence of operations, which would be pretty confusing. 2b) The read_config_from_remote/read_firmware_from_remote assume a single binary blob output. 3) I don't know if this was intentional or not: The write_config_to_remote and write_firmware_to_remote functions used to take in/size parameters that specified the data to write. Hence, those functions were useful for both executing a website operations file, *and* for writing config/firmware previously backed up using read_config_from_remote or read_firmware_from_remote. That said, the old forms of these APIs also weren't suitable for operations on multiple regions. 4) There is now even more global state hidden implicitly inside libconcord; not only does init_concord stash away all the remote information as before, but now read_and_parse_file stashes away the parsed version of an operations file rather than returning a handle to a (possibly opaque) data structure which in turn could be passed back to other functions. It really bugs me that the API is so oriented towards a single context stored in global data, rather than passing remote and file handles about. I don't see any way for "static class OperationFile *of" to be freed at a specific time (say an app wants to both update firmware and update configuration in one pass), and nor does deinit_concord free it. 5) I did take a quick look at the new implementation of update_configuration. I can't understand why half of the remote-specific knowledge is implemented in the CRemoteBase class, yet half of the knowledge is split across a slew of functions outside the CRemote* classes: update_configuration calls either _update_configuration_zwave or _update_configuration_hid depending on remote type. update_configuration may or may not reset the remote based on type. Both _update_configuration_hid and _update_configuration_zwave call _write_config_to_remote (well, actually _update_configuration_hid calls write_config_to_remote which then calls _write_config_to_remote) which then branches again to call either rmt->UpdateConfig or rmt->WriteFlash. Isn't the whole point of the remote interface to provide a single point to abstract these differences? If that isn't the whole point of the remote interface, it certainly seems like it *should* be. Or perhaps one interface and set of implementations for the HID protocol formatting, and a higher level interface and set of implementations for the sequencing. I think libconcord.cpp could/should be simply a C->C++ calling convention converter, with almost all functions doing nothing but calling a virtual function on the appropriate C++ object, with the object's class being chosen when init_libconcord is called based on USB ID. > In addition, it adds support for configuration updates on the 895 series > remote as well. > > The patch is big - the API rework entailed moving around a lot of code, > which in turn meant other code was moved to make the order of various files > logical. > > I'd very much like a comments before I merge it into head. Based on testing > with my 880, there are no regressions, but that doesn't mean there won't be > regressions I couldn't find. > > At the _very_ least, I'd encourage everyone to test that branch with your > remotes. Unfortunately, I won't be able to test until at least next week; I'm away from how this week on business. I'll try and make some time when I get back. > I'll be moving this week and I don't plan to touch concordance for at least > a week or so, but if I don't hear anything after that, I'll move forward > with merging this - though I'd rather have comments first. > > Thanks! ------------------------------------------------------------------------------ Start uncovering the many advantages of virtual appliances and start using them to simplify application deployment and accelerate your shift to cloud computing. http://p.sf.net/sfu/novell-sfdev2dev _______________________________________________ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
