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..40000
+ * 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
