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()
> 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
>
------------------------------------------------------------------------------
Sell apps to millions through the Intel(R) Atom(Tm) Developer Program
Be part of this innovative community and reach millions of netbook users
worldwide. Take advantage of special opportunities to increase revenue and
speed time-to-market. Join now, and jumpstart your future.
http://p.sf.net/sfu/intel-atom-d2d
_______________________________________________
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel
