Andreas, Phew - I finally had some time to sit down and read through this. It's pretty solid. I like the new libconcord API for IR learning, and the API for libirremotes seems pretty straight forward. I have a fair number of comments below though, although most of them are fairly minor.
Before we get there, I have a request for future submissions of patches
that have multiple parts (i.e. patchsets). I'd like to follow the
general LKML standard here for this, and it's as follows:
* Send a preceeding email with a subject like:
[PATCH 0/3] Learn API change and libirremote integration
* This email should document the various changes of the patchset as a
whole from a high level. But even more importantly, it should list all
the patches to follow and what they do. For example:
1/3 - Libconcord change move from old learnir API to new one
previously discussed on list. Also includes concordance changes to use
new API.
2/3 - libirremotes. The entirety of the library. This provides a
mechanism for dealing with and converting pronto remote codes in files
and strings.
3/3 - Adaptation of concordance to use libirremotes. This builds
upon patch 1/3, so please apply in order.
* Each patch should then be sent with a subject like:
[PATCH 1/3] Libconcord Learn IR API Update
[PATCH 2/3] Complete libIRremote addition
[PATCH 2/3] Use libIRremote in concordance
* In addition, if you do something like make a change to the
libconcord API that's stand-alone (such as this one is), then either
included in that patch or in another patch in the patch set, you should
provide a patch to use that API. Libirremotes integration should be done
in an entirely separate patch (or, arguably, a separate patchSET). You
provided this with a patch to remove libirremote support, which was
nice, but I'd prefer to be able to merge in pieces, not merge, merge,
unmerge (just API change), remove un-merge (now have libirremote).
You'll find this is preferred in most open source projects.
I'll try to get that into the docs. OK, on with the code review!
LIBCONCORD LEARNIR API CHANGE
* At the bottom of _get_next_key() you said you need to set the index
and the name. You actually set to the index in the parent function
(get_key_name()), and set the name here. I'm not quite sure which
approach is correct yet, but I do know they shouldn't claim one thing
and do another.
* It doesn't seem to me like previous_index and previous_name are
properly named. At the end of the call they are the CURRENT index and
the CURRENT name. On the next call they are the previous index/name,
obviously, but then we update them to the current one. The naming
convention is remarkably confusing.
* On this change:
- if (search - data >= data_size) {
+ if (search >= data + data_size) {
One says, "If our current position IN THE DATA is larger than the data
size..." and yours says "If our current position is greater than the
position at the end of the data." Clearly, they equate to the same
thing, but why change it? Especially given I think the former is clearer...
LIBIRREMOTE
* Note here I'm not digging into the code at all, just the header file -
i.e. the public API
* Why do your error codes in the opposite (numerical) direction? Are you
trying to avoid conflict, or ...?
* I really, really, really really dislike variables with capitols -
unless you're in java (i still don't like them there, but it's the
standard for the language). If you end up hosting your own copy of
libirremotes, clearly it's up to you... but if we end up hosting as part
of the concordance project, I'd like style the same across all projects.
And yes, I know the insides of libconcord don't follow this, but I
haven't cleaned it all up yet - the public API was written to this
standard. Anyway, consider num_pulses, pulses, repeat_seq, etc.
* Nice API documentation - makes things easy. Thanks.
CONCORDANCE
* Given how simple it would be I'm contemplating maybe making
libirremotes support a compile-time option.... I'm not sure though. I
*would* like to (as I mentioned earlier), merge just the libconcord API
change, let it bake in CVS for a bit, have people test it, and *then*
merge the libirremote integration. This gives staged rollout, easy
rollback, more specific testing, etc.
* Your update to the man page.... Eh, I don't want that list of authors
in the man page growing everytime someone adds a feature. You're welcome
to take credit for libirremote wherever it's mentioned as you are the
primary and original author of it, but others have contributed to
concordance that don't have their name in the man page. You will of
course get full credit for the entire patchset in the CVS commit
message. That is there only because AUTHOR is a standard man page
section and thus I included the original (and most substantive) authors
of the project.
* Hmm - should be be showing pulses and details in non-verbose mode? I
would think that _dump_new_code() should be wrapped in if
((*options).verbose).
* It seems like a fair amount of thought went into this UI... though I
can't comment on it until I actually TRY it, which I can't do until I
get my stuff, which is another month or so away (I'm in a temporary
apartment, and my stuff is still in a container somewhere in the
states). I suspect I'll want to tweak it somewhere - probably just for
consistency and brevity... but it's hard to say without seeing it in
action. (hmm - feel like copy-n-pasting a session of use?)
Thanks again for your work here, this is shaping up very nicely!
--
Phil Dibowitz [EMAIL PROTECTED]
Open Source software and tech docs Insanity 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
