Re: [concordance-devel] API feedback wanted: Supporting zwave remotes
Phew. The new abstractions are there for get identity, connectivity tests, and config updates. Additionally, the new call-back system is in place. It works on zwave and non-zwave remotes - and in the same way. I still have to convert all the firmware and IR code over (which is currently #ifdef'd out). Movers come Monday, so I may out of reach for a while, but if anyone wants to look at how things will be organized in the future, see the zwave_work_branch branch. -- Phil Dibowitz p...@ipom.com Open Source software and tech docsInsanity 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: OpenPGP 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
Re: [concordance-devel] API feedback wanted: Supporting zwave remotes
On 09/18/2010 04:18 PM, Phil Dibowitz wrote: Phew. The new abstractions are there for get identity, connectivity tests, and config updates. Additionally, the new call-back system is in place. It works on zwave and non-zwave remotes - and in the same way. I still have to convert all the firmware and IR code over (which is currently #ifdef'd out). OK, all the code is now ported and works. There's much cleanup yet to do, but the hard work is now complete. I don't know if I'll get a patch out for review tomorrow or not, but if not it'll probably be a while before I can get back to it. Anyway, if you have an 895, the zwave_work_branch should work for you. -- Phil Dibowitz p...@ipom.com Open Source software and tech docsInsanity 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: OpenPGP 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
Re: [concordance-devel] API feedback wanted: Supporting zwave remotes
On 08/31/2010 06:08 AM, Stephen Warren wrote: So, perhaps: // Returns TLOConnectivity, TLOUpdateConfiguration, TLOUpdateFirmware TopLevelOperation *gen_op_from_website_file(remote, filename); // Always returns TLOBackupFirwmare TopLevelOperation *gen_backup_firmware_op(remote, filename)\ // ... TopLevelOperation *gen_restore_firmware_op(remote, filename) TopLevelOperation *gen_backup_configuration_op(remote, filename) TopLevelOperation *gen_restore_configuration_op(remote, filename) // or one function per operation type: Status tlo_execute(tlo, callback, ...) I guess I just don't see the advantage here over just making our existing API more top-level-oriented, ala: update_configuration(pof) backup_configuration(filename) update_firmware(pof) backup_firmware(filename) set_time() get_time() reset() I totally buy your argument that update_configuration should reset and set time. In practice, users can press CTRL-C while running any application, or click the cancel button in congruity (which will abort between libconcord API calls right now), or just unplug their remote. In this case, the worst that should happen is the remote booting into safe mode and needing another firmware or configuration update attempt. Sure, but then you KNOW your breaking out of a program in the middle, as opposed to a nice cancel button that implies a nicer cancel. No? -- Phil Dibowitz p...@ipom.com Open Source software and tech docsInsanity 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: OpenPGP digital signature -- This SF.net Dev2Dev email is sponsored by: Show off your parallel programming skills. Enter the Intel(R) Threading Challenge 2010. http://p.sf.net/sfu/intel-thread-sfd___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] API feedback wanted: Supporting zwave remotes
On 08/31/2010 02:26 PM, Phil Dibowitz wrote: On 08/31/2010 06:08 AM, Stephen Warren wrote: So, perhaps: // Returns TLOConnectivity, TLOUpdateConfiguration, TLOUpdateFirmware TopLevelOperation *gen_op_from_website_file(remote, filename); // Always returns TLOBackupFirwmare TopLevelOperation *gen_backup_firmware_op(remote, filename)\ // ... TopLevelOperation *gen_restore_firmware_op(remote, filename) TopLevelOperation *gen_backup_configuration_op(remote, filename) TopLevelOperation *gen_restore_configuration_op(remote, filename) // or one function per operation type: Status tlo_execute(tlo, callback, ...) I guess I just don't see the advantage here over just making our existing API more top-level-oriented, ala: update_configuration(pof) backup_configuration(filename) update_firmware(pof) backup_firmware(filename) set_time() get_time() reset() The primary advantage is having a unified way for an application to query which steps are involved in an operation ahead of time. Without a single TopLevelOperation object that can be queried for all the steps, in order to support libconcord defining operation steps instead of hard-coding them in an application, there would need to be some API per operation type for querying this information. Instead of: tlo_get_step_count(tlo) tlo_get_step_type(tlo, step_id) tlo_get_... You'd have something like: update_config_get_step_count(?) update_config_get_step_type(?, step_id) update_config_get_... backup_firmware_get_step_count(?) backup_firmware_get_step_type(?, step_id) backup_firmware_get_... ... .. an explosion of functions. The unified TopLevelObject also gives a good place to store operation-specific state; e.g. a TLO created for an 880 config update might include the set-time step, but one of a 700 config wouldn't, and having some specific object other than global variables in libconcord.cpp would be a good idea. I totally buy your argument that update_configuration should reset and set time. In practice, users can press CTRL-C while running any application, or click the cancel button in congruity (which will abort between libconcord API calls right now), or just unplug their remote. In this case, the worst that should happen is the remote booting into safe mode and needing another firmware or configuration update attempt. Sure, but then you KNOW your breaking out of a program in the middle, as opposed to a nice cancel button that implies a nicer cancel. No? I'm not sure if the distinction is that obvious to end-users? congruity has had a cancel option since the beginning IIRC (personally I find apps that can't cancel very annoying), and I don't recall any reports of users screwing themselves with it; -- This SF.net Dev2Dev email is sponsored by: Show off your parallel programming skills. Enter the Intel(R) Threading Challenge 2010. http://p.sf.net/sfu/intel-thread-sfd ___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] API feedback wanted: Supporting zwave remotes
Sorry for top-posting, but I'm making a general response rather than to individual points. So, I don't think it'd be that hard for congruity to support any of the API designs below, including different paths for different remote architectures all determined at run-time. However, I'd prefer a single unified API. My reasoning is this: congruity/concordance's purpose is to provide a pretty UI over libconcord, and not to implement knowledge of how to program the different remotes; such abstraction (whether it be a unified API, or even just a database that maps from arch to a list of required operations by the UI application) belongs in libconcord, since that's where all other knowledge re: remote programming is. From my perspective, I think the perfect API would be a single top-level API to do each of: 1) Identify/parse any update/... file 1a) A function to load/parse/... the file 1b) Function(s) to query any information about the parsed file (e.g. what type of operation is being performed, so this information can be presented to the user) 1c) Function(s) to query the number and type of steps required to implement the operation. 2) A single function to perform the entire operation (or perhaps a single function per type of operation) This would completely internalize all knowledge of file-formats, operations, which remotes exist, set of steps required to implement the operations, etc. The callback from step 2 would need to be enhanced to include a step ID as well as percentage or byte-count, in order to match with the data returned by function(s) in 1c above. I propose this also because I see that some of the operations have XML files that can (and do) list multiple regions to be updated. Thus, the set of operations to be executed is not only driven by remote architecture, but also by update/... file content. Currently, the API is structured to only handle a single contiguous region that must be erased and written. With the above API, any changes to support N regions would be entirely internal to libconcord, and an application would simply see extra entries in the step list; something at least congruity could easily adapt to. Perhaps something like what's below (names need more though; this is just a rough outline): struct ParsedOperationFile enum OperationType Connectivity UpdateConfiguration UpdateFirmware LearnIR enum StepType InitialWebPing PrepareForUpdate EraseRegion WriteRegion VerifyRegion FinalizeUpdate Reset ReconnectToRemote SetTime // e.g. yes for 880 no for 700 FinalWebPing ... enum StepStatus Starting Executing Complete_Success Failure Status Callback(ParsedOperationFile *pof, void *cbcontext, uint32 step, StepStatus step_status, uint complete_count, uint target_count) ParsedOperationFile *load_file(char *filename) void destroy_file(ParsedOperationFile *pof) OperationType pof_type(ParsedOperationFile *pof) uint pof_step_count(ParsedOperationFile *pof) StepType pof_step_type(ParsedOperationFile *pof, uint step) // e.g. region ID for erase/write/verify, which can be added // to (or interpolated into printf-style) step labels in the UI ??? pof_step_parameters(ParsedOperationFile *pof, uint step, // ???: enum parameter_type, uint parameter_id) // or update_config_execute, update_firmware_execute, ...? Status pof_execute(ParsedOperationFile *pof, Callback *cb, void *context) ? pof_get_failure_information(ParsedOperationFile *pof, ...?) congruity would use pof_step_* to create the UI widgets when entering e.g. the update configuration page, then whenever a callback was executed, map from step number to UI widget, and update the percentage completion bar. Perhaps extra APIs to determine if a step's completion level is Kb, percent, ... Perhaps pof_step_parameters would return both data that forms part of a UI label for the step, and other metadata like this? Or, perhaps have specific functions for specific step types. The callback could return Continue/Abort to allow implementation of a cancel button in a UI. How does that sound? On 08/26/2010 12:50 PM, Phil Dibowitz wrote: OK all, [ Stephen, as the primary user of the libconcord API, I'm particularly looking for input here from you ] I now have fully functioning 89x support for config updates, connectivity tests, and web communication. (No work yet on firmware or learn-ir). The current libconcord API was designed very much around the HID remotes, and so a config update in 0.22 looks like: prep_config() invalidate_flash() erase_config() write_config_to_remote() verify_remote_config() finish_config() reset_remote() [... reconnect] set_time() The ZWave remotes don't expose the low levels that the HID remotes do. There's no flash addresses to worry about, or even manual invalidation and erasing. It looks like this: write_config_to_remote()
[concordance-devel] API feedback wanted: Supporting zwave remotes
OK all, [ Stephen, as the primary user of the libconcord API, I'm particularly looking for input here from you ] I now have fully functioning 89x support for config updates, connectivity tests, and web communication. (No work yet on firmware or learn-ir). The current libconcord API was designed very much around the HID remotes, and so a config update in 0.22 looks like: prep_config() invalidate_flash() erase_config() write_config_to_remote() verify_remote_config() finish_config() reset_remote() [... reconnect] set_time() The ZWave remotes don't expose the low levels that the HID remotes do. There's no flash addresses to worry about, or even manual invalidation and erasing. It looks like this: write_config_to_remote() set_time() If we're on a zwave remote, write_config_to_remote() calls UpdateConfig() (instead of WriteFlash()) which only exists on the zwave branch of the class hierarchy. Later in this email I'll address the possibility of splitting up UpdateConfig(), but suffice to say that at best you'd split it into prep/update/finish, and nothing like what we have for HID. So the question is - what should the API for libconcord look like? I see a few possibilities: OPTION 1 We wrap up what prep_config(), invalidate_flash(), erase_config(), verify_remote_config(), verify_remote_config(), finish_config(), do into just three calls: // send prep 'commands' and invalidate flash? prep_config() // erase flash and write new data update_config() // a rename of write_config_to_remote() // verify and commands to re-enable flash finish_config() // Possibly finish_config() would reset the remote // or possibly we'd have a reset_remote() that isn't // required for nonzwave remotes (this data is exported) This would then align with the zwave remotes. OPTION 1-B The same as option 1, but leave those low-level calls still available. OPTION 2 Just leave the separate call path for updating HID remotes vs. zwave remotes as they are (outlined above). OPTION 3 Something new I haven't thought of yet. Here are some things to think about: * the 89x remotes are a weird hybrid of the HID and zwave remotes. It is unclear what API changes may or may not be needed for the 1000/xbox/etc. remotes. * Have a simple API is nice, but giving some control to the end-user is also useful, and I feel like we'd be ripping all that out with option 1 * A few calls are common among config and firmware updates: invalidate_flash, and reset_remote. - invalidate_flash is a single command sent to tell the remote not to use it's flash. This looks, in hindsight, much like the other stuff in prep_config() and prep_firmware(), and I'm very sure that's actually where it belongs, regardless of whether we collapse the other calls or not. - reset_remote *could* be part of post, but I argue it should be it's own call even if we do do that. So potentially we do #1, but with a reset_remote you only have to call for some remotes (it works on all though). * The current HID API provides a flexible way of giving lots of useful feedback to the user, and simplifying it will remove this. Obviously we could rework the callback structure to allow the library to provide more status information to make up for that, if we wanted, but still... Now, a few words about what the underlying CRemoteZ_HID::UpdateConfig() function is doing under the hood. In reality it does a lot of work under the hood: * Send the UDP-HID command to start a TCP-HID connection * Do the SYN-ACK handshake for TCP-HID * Tell the remote want to start the config update (this actually happens in the SYN-ACK of the SYN, SYN-ACK, ACK handshake). * Send the header for the update * Send the data for the update * Ask for a checksum of the data * Tell it we're done with the update * Do an involved ack,ack,fin-ack,ack teardown I've _thought_ of splitting the first few steps into prep_config(), but there's no decent place to draw the line. The START_UPDATE command is part of the TCPHID handshake... breaking it after that is doable, but seems like we're doing it just to have a prep. I also see no real benefit. Now, because the status-update callback procedure for the update does it based on bytes sent, only the send the data part is updating the status. Which would be fine - we have before/after steps in HID that aren't part of the transfer as well - except that asking the remote to calculate the checksum takes like 15 seconds, and so that appears as a hang to the user. Separating that out as finish_config() is possible, but again, it's awkward because it's all part of the expected order once you've sent START_UPDATE. I've had times where option 1 seemed clearly the right choice to me, and other times where it seems totally wrong, and I go back forth a bit. I think option 2 is probably not right, but I wonder if there's something more -- Phil Dibowitz p...@ipom.com
