Re: [concordance-devel] API feedback wanted: Supporting zwave remotes

[Phil D. <ph...@ip...>]

On 08/29/2010 06:18 AM, Stephen Warren wrote:
> Sorry for top-posting, but I'm making a general response rather than to
> individual points.

You can avoid top-posting in such a situation by just not including the
original email at all. :)

> However, I'd prefer a single unified API.

I agree.

> 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.

I like a and b. As for c, I think it depends on the rest of the API. If we
really abstract all config updates into one (or atleast one consistent set
of) functions, c isn't needed.

> 2) A single function to perform the entire operation (or perhaps a
> single function per type of operation)

And given this, I see no need for 1-c above.

> 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.

Agreed. Would the step ID be a string, or an ID? Would you want a function
to return a string version (the way the lc_error stuff currently works), or
would that be left up to the client?

> 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.

Yes, the 1000 and later all have multiple regions. In fact, the current code
has a not-nice hack where the region is always hard-coded as 4 because
that's how the 89x remotes work. I'll have to fix that once I start working
on the 1000.

> 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

I don't understand what you mean by yes or no. Do you just mean we'd never
return that on the 700? Can the 700 not have it's time set? FWIW, the 89x
never gets rebooted, so I don't _need_ to set the time after the
configuration update, but it seems like a good idea.

Which actually leads me to a more interesting question about your API
design... is setting the time *really* a setp in UpdateConfiguration? You
can update the configuration without touching the time and you can update
the time without ever updating the configuration. It's certainly true that I
want to support "just fix the time on my remote" for example.

>   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)

And target_type which would be an enum of Percent, Kb, much like we have
"is_bytes" or whatever it is now, but more extensible.

> ParsedOperationFile *load_file(char *filename)

We already have this. It would just need a bit of tweaking to combined
read_file() and determine_file_type() and give it a nice struct. I had
started working on something like it when I started doing the zipfile stuff
for the 89x, but decided to not do too much API surgery until we'd talked
about it a bit.

Rather than various functions such as pof_type() I had envisioned returning
a well-defined struct the user could read. I had no planned on exposing
things like "how many regions" and stuff because that should get abstracted
away. I'm find with the accessor functions though.

> // 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)

I'm not sure I follow you here. You're expecting something like:

for (int step = 0; step < pof_num_steps(pof): i++) {
  type = pof_step_type(pof, step);
  // somehow figure out paramenters
  pof_execute(pof, step, ...)
}

? I dislike this. I'd rather keep it a bit more similar to what we have,
with say:

pof_type = pof_type(pof)
switch (pof_type) {
  case ConfigUpdate:
    UpdateConfiguration(pof, cb, cb_data);
    ResetRemote(...); //does nothing if not supported
    SetTime(...);
    break;
  case FirmwareUpdate:
    UpdateFirmware(pof, cb, cb_data);
    ResetRemote(...);
    SetTime(...);
    break;
}

It's worth noting here that Congruity only provides the functionality of
*part* of the libconcord API. The ability to backup your config, set time,
backup your firmware, etc. is valuable to many of our users. Such users are
generally happy to use the CLI and that's awesome, and I want to continue
supporting such uses cases at the library level. Also, I don't want the API
for cases were no operations file exists to be significantly different from
those were we do.

Anyway, then the callback system could be updated in much the way you describe.

Which is all basically "option 1" from my email, but you flushed out the
"and we need to fix the callback system", plus I like your suggestion of
extending our API around the operations files.

> The callback could return Continue/Abort to allow implementation of a
> cancel button in a UI.

In *theory* this would be really cool, but in reality this will almost
always leave the remote in a very bad state, and so I think we should avoid
this.

Thoughts?
-- 
Phil Dibowitz                             ph...@ip...
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