On 08/29/2010 06:42 AM, Phil Dibowitz wrote:
> 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. :)
:-) I guess I was thinking of keeping context, but that's what mailing
list archives are fo.
>> 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.
congruity creates a user-interface with a text label and progress bar
for each step. When performing the steps, the progress bars get updated
in the callbacks from libconcord. Currently, libconcord hard-codes the
set of progress bars to create, but I'm hoping that if libconcord
defines the set of required steps, then congruity can simply query that
set from libconcord instead of hard-coding the user-interface setup
code. That querying needs to happen before executing the steps, so the
UI is fully populated before beginning slow programming operations.
>> 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 think the step ID itself would just be an integer 0..n. Other
functions could map from step ID to step type, step name, etc. Those
functions would basically be (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.
>
> 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?
Yes, "set time" on the 700 appears to be a complete no-op; the command
is accepted, but doesn't seem to do anything (get time always returns
the current time unmodified) Note that the remote itself never displays
the time as far as I can tell, so I guess it makes sense the clock
commands do nothing.
> 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?
Well, the list above was just a rough idea, and probably isn't complete
either.
> 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.
I thought on the 880, the reboot was needed to pick up all the new
configuration? If so, I think that should probably be part of an
update-configuration step list. Equally, isn't a set-time needed after a
reboot, since it get's lost? If so, that operation should be included in
the update-configuration step list.
>> 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.
Sure. I just mentioned this explicitly to emphasize that I see it
returning a new struct instead of the current "find the binary data
pointer" API.
> 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, ...)
> }
The application would call a single pof_execute function (or one
function per operation type, not per step). The implementation of
whatever function the application calls might look like the code you
wrote above.
> ? 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;
> }
I'm not completely sure if your objection was to (a) where to iterate
over the step list (i.e. what I replied to above), or (b) using the same
top-level function for each operation type.
Either one pof_execute for anything *or* one separate
UpdateConfiguration/UpdateFirmware/... function for each operation type
is fine by me.
For a completely generic application, having a single top-level function
that does anything might be easier, but congruity can handle either just
fine.
> 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.
I guess I don't see anything too fundamentally wrong with having a
high-level API for typical operations (which I think does include the
other stuff you mentioned) and a separate low-level API for more
"internal" operations.
That said, keeping things as similar as possible makes sense.
Certain operations I see being always simple enough they can be
available as a direct function call; set_time, reset_remote being great
examples.
Pretty much any other operation seems like it could be a multi-step
process; even "back up firmware to a binary file" that currently reads a
single chunk of flash and returns it might be a multi-step read process
if there were multiple firmware regions that needed to be read. I could
easily imagine separate text/rodata/data stored non-contiguously in
different regions. Or firmware backups needing to back up both a
bootloader and application.
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, ...)
(In the case of separate functions per operation type, the
TopLevelOperation class would mainly act as a vehicle for the metadata
query APIs which allow applications to know what steps get executed.)
The alternative to all those gen_* functions would be for the
application to iterate over the steps like in the code above, but
perhaps calling more specific functions:
for (int step = 0; step < pof_num_steps(pof): i++) {
step_type = pof_step_type(pof, step);
switch (step_type) {
case ERASE_REGION:
region_address = pof_get_parameter(pof, step, ADDRESS);
region_size = pof_get_parameter(pof, step, SIZE);
lc_erase_region(remote, region_address, region_size);
break;
case WRITE_REGION:
write_address = pof_get_parameter(pof, step, ADDRESS);
write_size = pof_get_parameter(pof, step, SIZE);
data_ptr = pof_get_parameter(pof, step, DATA_PTR);
lc_write_region(remote, write_address, write_size, data_ptr);
break;
case RESET_REMOTE:
lc_reset_remote(remote);
break;
//...
}
}
But even with that, I think libconcord should probably define the set of
steps required for even "simple" things like config backup, in case the
steps become more complex.
> 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.
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.
------------------------------------------------------------------------------
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
