Re: [concordance-devel] libconcord new full patch (Re: Next try for the big IR learning patch..)
On Tuesday 24 June 2008, Phil Dibowitz wrote: Andreas Schulz wrote: As I just recognized, better make that delete_*, to be consistent with delete_blob.. Delete blob always deletes the same thing - in your case you have lots of different things.. Sure, by 'delete_*' I meant of course to rename each of them, not all to one, giving: ... void delete_key_names(char **garbage); void delete_ir_signal(uint32_t *garbage); void delete_post_string(char *garbage); - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php ___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] libconcord new full patch (Re: Next try for the big IR learning patch..)
On Tue, June 24, 2008 1:04 am, Andreas Schulz wrote:
So, maybe we can agree on the following interface:
That should work. Just a few mostly small comments:
In the delete_* functions, I'd use a more descriptive name for the
parameter than garbage, e.g.:
void delete_key_names(char **key_names);
In the description for get_key_names:
* Memory allocated for the strings must be freed by the caller
* via destroy_key_names() when not needed any longer, i.e.
* after all codes have been posted via post_new_code.
I would avoid specifically mentioning deleting the memory after
post_new_code, because some clients will probably do this:
char ** key_names = get_key_names(...);
duplicate key_names into some internal structure (e.g. Python strings in a
list)
delete_key_names(key_names);
process the internal structure
The internal structure could be created by duplicating all the strings and
indexing them in some different way. Note: The key names are obviously
required to call post_new_code later, but there's no requirement it be the
same actual string (memory location holding it, that is), but simply that
the content of the string must be the same.
So, perhaps simply say this instead:
* Memory allocated for the strings must be freed by the caller
* via destroy_key_names() when no longer needed.
Same for other allocating functions.
In get_key_names, rather than returning an array with a NULL entry to
indicate the end, how about this:
int get_key_names(uint8_t *data, uint32_t size,
char *** key_names, uint32_t *key_names_length);
and called/used like this:
char **key_names;
uint32_t key_names_length;
int err = get_key_names(d, s, key_names, key_names_length);
if (err != SUCCESS) {
// handle error here
return;
}
for (int i = 0; i key_names_length; ++i) {
printf(Key name %d is '%s'\n, i, key_names[i]);
}
This:
a) Can allow more meaningful error status
b) Is explicit about the array length up front
One could make a similar change to encode_for_posting, but since that's
always returning just one string, I'm not worried about that so much; your
call.
I would rename post_new_code's code parameter to encoded, to hint that
it comes from encode_for_posting.
Thanks for implementing this guys, and being open to API changes.
-
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
___
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] libconcord new full patch ( Re: Next try for the big IR learning patch ..)
(I re-added some cuts by Phil to answer both posts in one) On Sunday 22 June 2008, Phil Dibowitz wrote: On Sunday 22 June 2008, Stephen Warren wrote: A pulse is usually a low-high-low transition, whereas these docs talk about pulses and spaces. There should be differentiation between pulses (mark followed by space), marks (high time), and spaces (low time) I actually wasn't really happy about the naming either initially, but missed to change it to something more appropriate, and then got used to it... I have changed it now in libIRremotes to ir_signal and ir_signal_length and will adapt libconcord accordingly, also making the documentation clearer, as you suggested below. In the documentation for get_key_name, it may be worth mentioning that valid index values start at 1 anything wrong with index starting at 1 for '1'st entry? and increment by 1 until NULL is returned. Well, probably some things just become too obvious when you have been working on them for several days... I really don't like the static buffer used by get_key_name. It's nasty that a client can only use this API on a single data blob at a time. So the question is - do we want to support more than one file AT THE SAME TIME? Given the current API you can in fact go through more than one file at a time. So being pragmatic, it's not so bad. Thus I've gone back and forth a few times, and can't quite decide. I currently don't see any use for being able to handle multiple files in parallel - maybe sequential, in case someone comes up with a concordance daemon lurking 24/7 in the background - but parallel? Multiple parallel connections to Logitech, or collecting a bunch of downloaded files and procesing them in parallel? It's all still driven by the software on the Logitech servers, anyway... that the API internally maintains non-obvious implicit state. I'd prefer something more like the API below, where the XMLBuffer is dynamically allocated per iterator, but the type/content is still opaque to the client: void *keyiter_create(uint8_t *data, uint32_t size); char *keyiter_next(void* keyiter); void keyiter_destroy(void *keyiter); void keyiter_key_destroy(char *); No reversing or random access allowed either; should be much simpler. So I guess the idea with this feature is being able to go oh, damn, I hit the wrong button on the other remote and don't want to have to go back and redo the whole sequence. I like that. To which Stephen replied: At least in congruity, I will make the client application use get_key_name to retrieve a list of all known key names into a Python list, and then iterate back/forth within that. Well, concordance could do that as well, so what about: char **get_keynames(uint8_t *data, uint32_t size); void destroy_keynames(char **names); and be done with the XML data in one call? Please document that the memory pointed at by get_key_name's return value needs to be free'd by the client. libconcord needs an API for the client to call to do this. encode_for_posting needs to define who owns the memory returned, and how to destroy it. I started adding deallocators in libIRremotes, and will do so for libconcord. I'm not sure about the internals of python - will deallocators be of any use there, and when should they be called? post_new_code and encode_for_posting check all the output/pointer parameters against being NULL, but learn_from_remote doesn't. Guess I thought anybody so stupid to call learn_from_remote(NULL,NULL,NULL) should deserve a SIGSEGV... well, adding a check won't hurt, and may be useful when interfacing to some other language.. I'm not convinced that the Python bindings changes for verbose are the correct way to go; they don't handle exceptions very well. I didn't read through those changes, but I would say that seems reasonable to be able to enable debug in a production version so you can tell your users to do this when they report some problem you can't reproduce. At least it was helpful to me in some stage of the develpment to see what was going on and coming out - I also don't see any big difference in the exception handling - except maybe for the case that 'print' in the wrapper itself throws an exception (is that likely?). It'd be best if you removed the verbose part from the Python bindings as you like it..There's probably also still to much 'verbose' stuff in my proposal for congruity (e.g. it prints geometry data when you zoom the IR signal plot), and probably nobody is actually interested in the way the signal is encoded for posting to Logitech either...some information needed while coding is surely useless once you've got the code right, so what should a verbose mode show? An enthusiast may enjoy to see the exact mark/space durations and even feed them into some other decoding tool, while a plain user will just think 'what the ?' And-probably
Re: [concordance-devel] libconcord new full patch (Re: Next try for the big IR learning patch..)
Andreas Schulz wrote: In the documentation for get_key_name, it may be worth mentioning that valid index values start at 1 anything wrong with index starting at 1 for '1'st entry? All arrays have their 1st entry at 0. That's how CS works. :) I would say, start at 0. I currently don't see any use for being able to handle multiple files in parallel - maybe sequential, in case someone comes up with a concordance daemon lurking 24/7 in the background - but parallel? Multiple parallel connections to Logitech, or collecting a bunch of downloaded files and procesing them in parallel? It's all still driven by the software on the Logitech servers, anyway... I'm inclined to agree... Well, concordance could do that as well, so what about: char **get_keynames(uint8_t *data, uint32_t size); void destroy_keynames(char **names); and be done with the XML data in one call? I don't think we need the call to destory() in this case. In this case you're handed back an array of strings, which C knows how to deal with, and any C-to-high-level language should be able to handle as well. For example, in SWIG-to-perl I'd just allocate a scalar array, copy the data, and then nuke the char**. There are other alternatives that SWIG provides as well. IMO, we only need the destroy() if they're opaque or non-C data. In this case it's not a token is data that the user is expected to do something with - data the user actually requested. -- Phil Dibowitz [EMAIL PROTECTED] Open Source software and tech docsInsanity Palace of Metallica http://www.phildev.net/ http://www.ipom.com/ Never write it in C if you can do it in 'awk'; Never do it in 'awk' if 'sed' can handle it; Never use 'sed' when 'tr' can do the job; Never invoke 'tr' when 'cat' is sufficient; Avoid using 'cat' whenever possible -- Taylor's Laws of Programming signature.asc Description: OpenPGP digital signature - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] libconcord new full patch (Re: Next try for the big IR learning patch..)
On Mon, June 23, 2008 3:11 pm, Andreas Schulz wrote: On Sunday 22 June 2008, Stephen Warren wrote: In the documentation for get_key_name, it may be worth mentioning that valid index values start at 1 anything wrong with index starting at 1 for '1'st entry? Probably not, but it's sufficiently unusual for C (and many other languages) that it's just worth mentioning. Well, concordance could do that as well, so what about: char **get_keynames(uint8_t *data, uint32_t size); void destroy_keynames(char **names); and be done with the XML data in one call? That'd work too. I'll have to check whether it's easy to deal with char** in the Python bindings; it should be... I started adding deallocators in libIRremotes, and will do so for libconcord. I'm not sure about the internals of python - will deallocators be of any use there, and when should they be called? Absolutely useful. libconcord.py should simply expose the function call as-is; never automatically call them (except in a situation where the bindings actually copy the returned data into a Python type, rather than returning a ctypes type referencing the memory, but we don't ever do that, currently at least). The client app (e.g. congruity) should call them at the same time a C application would; whenever it's done using the returned data. - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php ___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] libconcord new full patch (Re: Next try for the big IR learning patch..)
On Mon, June 23, 2008 3:28 pm, Phil Dibowitz wrote: Andreas Schulz wrote: Well, concordance could do that as well, so what about: char **get_keynames(uint8_t *data, uint32_t size); void destroy_keynames(char **names); and be done with the XML data in one call? I don't think we need the call to destory() in this case. I don't agree. Why should the client (or bindings) know how the memory was allocated, and hence how to free it. malloc() is common, but if the client/bindings simply assume this, and call free() on the data, things will break very horribly if libconcord was built using a debug malloc library that redefines malloc/free to be something else, and perhaps actually allocates a different (larger) sized chunk of memory in order to add debug information headers/trailers, meaning that the actual pointer passed to real free should be, say, 8 bytes less than that returned by the debug malloc. - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php ___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] libconcord new full patch (Re: Next try for the big IR learning patch..)
Andreas Schulz wrote:
Next, here comes the latest version of my IR learning patch for
libconcord.
A few comments on the API:
+ * pulse_count : total number of pulse and space durations in pulses
+ * pulses should start with a pulse and end with a space duration,
+ * hence pulse_count will always be an even number.
A pulse is usually a low-high-low transition, whereas these docs
talk about pulses and spaces. There should be differentiation between
pulses (mark followed by space), marks (high time), and spaces (low time)
So, I'd rewrite the docs as:
+/*
+ * Data structure information:
+ *
+ * carrier_clock: In Hz, usually ~36000..4
+ * pulses: IR mark/space durations (alternating) in microseconds
+ * pulse_count: Total number of pulses, i.e. total number of marks plus
+ * total number of spaces. Pulses consist of a mark plus a space,
+ * hence pulse_count will always be an even number.
In the documentation for get_key_name, it may be worth mentioning that
valid index values start at 1 (rather than the typical 0) and increment
by 1 until NULL is returned. With the current docs, it could be legal
for index values 1, 2 to be defined, 3, 4 not, then 5, 6, 7 to be
defined. Actually, see below for further comments on the semantics of
this API.
Please document that the memory pointed at by get_key_name's return
value needs to be free'd by the client. libconcord needs an API for the
client to call to do this; not all clients are C, and hence they can't
all call C's free(), and delete_blob is only defined for file blobs
(which are free()d using delete[], which should only be used for new[]'d
pointers, and not for malloc/strdup/...-returned pointers).
I really don't like the static buffer used by get_key_name. It's nasty
that a client can only use this API on a single data blob at a time, and
that the API internally maintains non-obvious implicit state. I'd prefer
something more like the API below, where the XMLBuffer is dynamically
allocated per iterator, but the type/content is still opaque to the client:
void *keyiter_create(uint8_t *data, uint32_t size);
char *keyiter_next(void* keyiter);
void keyiter_destroy(void *keyiter);
void keyiter_key_destroy(char *);
where the client uses this like:
void *i = keyiter_create(d, s);
for (;;) {
char *s = keyiter_next(i);
if (!s) {
break;
printf(Key: '%s'\n, s);
keyiter_key_destroy(s);
}
keyiter_destroy(i);
No reversing or random access allowed either; should be much simpler.
One could combine keyiter_create/keyiter_next in a similar fashion to
how strtok detects first v.s. subsequent invocations, but I think the
above API is a little clearer.
encode_for_posting needs to define who owns the memory returned, and how
to destroy it.
post_new_code and encode_for_posting check all the output/pointer
parameters against being NULL, but learn_from_remote doesn't.
The version number of the DLL should be increased to indicate that the
API changed incompatibly.
I'm not convinced that the Python bindings changes for verbose are the
correct way to go; they don't handle exceptions very well. It'd be best
if you removed the verbose part from the Python bindings, and I'll
work on a more automated and robust solution, and one that doesn't
require the host application to configure it, since I don't really want
to include that kind of debug logic in the production version of congruity.
Thanks!
-
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
___
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] libconcord new full patch (Re: Next try for the big IR learning patch..)
Stephen Warren wrote: The version number of the DLL should be increased to indicate that the API changed incompatibly. Oh - just as a side note, please don't do this in patches. There's no need to increase it everytime a CHECKIN causes a change. At _release_ time, I'll compare the APIs and ABIs and adjust the so-version numbers as appropriate based on the GNU library numbering scheme. -- Phil Dibowitz [EMAIL PROTECTED] Open Source software and tech docsInsanity Palace of Metallica http://www.phildev.net/ http://www.ipom.com/ Never write it in C if you can do it in 'awk'; Never do it in 'awk' if 'sed' can handle it; Never use 'sed' when 'tr' can do the job; Never invoke 'tr' when 'cat' is sufficient; Avoid using 'cat' whenever possible -- Taylor's Laws of Programming signature.asc Description: OpenPGP digital signature - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
