On Tue, Sep 21, 2010 at 10:40:53PM -0700, Stephen Warren wrote: > 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:
Thanks for taking the time to look at it. Comments below. > 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). Ah, right, I missed this. For the record you can still provide status bars - one per stage, the way concordance does. But its no problem to add this. > The new API doesn't support multiple config or firmware blocks: Ish. I don't know how that'll be implmeneted under the hood, but update_configuration()'s API doesn't care. More below. > > 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 (?) > ... I think this fine. Everytime your stage_id changes, you're in a new stage. 1,2,3,1,2,3 is perfectly clear to me. I can add some new stage-counter var, but I don't really see the need. > The read_config_from_remote/read_firmware_from_remote assume a single > binary blob output. Yeah, these will need to be updated once we support something with more than one region of code. I don't know how that'll look. > 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. This was not intentional. I have to think about this. What about an additional function call to populate the OperationFile data? Like: populate_external_region_data(data, size, region=NULL) region would be required only for zwave remotes. ? > 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. Indeed. I didn't want to have a context variable for the file data and the remote data be static to the library. I want to be consistent. For now I'd like to stick to this (I'm OK with a context variable in theory, but since updating multiple remotes simultaneously would require a multitude of other changes _anyway_ [such as moving to libusb1], I see no problem with the current approach). If at some point we want to change that we can change both at the same time. > 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. Good call, this needs to be added. > 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: Because... within the HID remotes the differences are small enough the overall steps were the same, but the implementation was different. It was easy to encapsulate. Between Zwave and HID the architecture is so different, it's a bit more difficult. That said, I can certainly roll update_configuration into CRemote::UpdateConfiguration, but I rather liked the idea of these layers where each low-level step was exported from the CRemote* classes and then libconcord.cpp provided the "TLO" wrapper functions. That was the split I tried to maintain. In order to do this, it meant some logic in libconcord.cpp. Your approach would move the TLO wrappers into the CRemote classes. The split of low level vs. high level at different layers of code seemed clean to me. It's worth noting, this is nothing new - there are several instances of this "knowledge" in the existing libconcord.cpp code, such as "if (ri.architecture == 14)" and similar. -- Phil Dibowitz p...@ipom.com Open Source software and tech docs Insanity Palace of Metallica http://www.phildev.net/ http://www.ipom.com/ "Be who you are and say what you feel, because those who mind don't matter and those who matter don't mind." - Dr. Seuss
signature.asc
Description: Digital signature
------------------------------------------------------------------------------ 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
