Andreas Schulz wrote: > OK, here we go.. Sorry this too me so long to get to.
> This is against the CVS as of today (26.04.2009).
> In contrast to my first approach, there is now a module
> prontocodes that is statically linked to concordance and
> provides just the basic stuff to convert pronto hex codes.
I'm not super fond of this approach. Is there a reason not to have it be a
library? You prevent congruity from having this functionality.
I think it might be nice to be able to have a file of pronto hex codes (my
limited understanding is that this is common), so we may want to have a "-L
pronto.txt -P" type invocation that will read from a file and learn all codes.
Given such a use-case I can see congruity wanting to support this. And I
don't know if there's a generic pronto open source library out there, but it
might be nice to provide one.
> It will only accept raw modulated hex format, i.e. Pronto
> codes starting with 0000 ....
> The complete hex code must be input as a single line of
> hexadecimal words, separated by blanks (which seems to be
> the usual format of these codes), without any number base
> markers (like 0x.... or h....), just like:
> 0000 1234 5678 9ABC DEF0 ....
It would be nice to allow 0x versions.
> See attachments for the patch to concordance (concordance-pronto.patch)
> and the additional static library prontocodes.h/.c.
I've included my code review below. Nothing too major, but tons of style
things, and a few cleanups, and one or two things I didn't follow. A few
more general things though:
- Please add documentation. You don't include anything in the README or in
the man page
- Is this tested on both windows and linux?
- In future version, please make a patch that includes the new files ("diff
-Nru" will include new files)
protocodes.h
> #ifndef PRONTOCODES_H
> #define PRONTOCODES_H
>
> #ifndef uint32_t
> #define uint32_t unsigned int
> #endif
Why is this necessary?
>
> /*
> * return codes:
> */
> #define PCO_RETURN_OK 0
> #define PCO_ERROR_BAD_CODE 1 /* could not find all required values */
> #define PCO_ERROR_OUTOFMEM 2 /* failed to malloc returned arrays */
> #define PCO_ERROR_NOT_IMPL 11 /* not yet implemented */
Why the skip?
> /*
> * Translate a return value into an actual error message. Pass
> * in the int you received, get back a string.
> */
> const char *pco_strerror(int err);
Can your prefix be pcodes_ ?
Or, if you make a shared library, lpc_
>
> /*
> * Native Pronto code data structure. For raw format, once/repeat_signal
> * contain word pairs for mark/space durations (in carrier clock cycles).
> *
> * Note: the internal Pronto format header counts mark/space pairs; for
> * convenience, we count total marks + spaces instead , i.e. twice
> * the Pronto counts.
> *
> * For decoded formats, format_id determines the encoding and once/
> * repeat_signal contain the corresponding address/command codes
> * (or whatever data is needed for the specific encoding):
> * For reference, the original names from Evgueni's document have
> * been added in brackets <>:
> *
> * format_id <wFmtId>:
> * determines format (raw or several decoded)
> * carrier_clock_divider <wFrqDiv>:
> * IR carrier clock in internal units
> * once_signal_length <nOnceSeq>:
> * length of once_signal (total of marks+spaces ->
> even number)
> * repeat_signal_length <nRepeatSeq>:
> * length of repeat_signal (total of marks+spaces ->
> even number)
> * *once_signal *<OncePulses>:
> * sequence to send once for command execution
> * *repeat_signal *<RepeatPulses>:
> * sequence to send repeatedly for command repetition
> */
Please ensure your lines are <=80 chars.
> /*
> * Read from string:
> * Allocates memory for once/repeat_signal in result, caller is responsible
> * for deallocation.
> */
Line-length.
prontocodes.c
> #define PRONTO_FMTID_RAW_MOD 0x0000 /* – raw oscillated code (with
> carrier) */
> #define PRONTO_FMTID_RAW_UNMOD 0x0100 /* – raw unmodulated code (no
carrier) */
line-length
> /*
> * - predefined formats:
> */
> #define PRONTO_FMTID_UDB_INDEX 0x8000 /* – index to UDB */
> /* - variable length: */
beginning of line, please.
> #define PRONTO_FMTID_VAR_PREDEF 0x7000 /* – predefined code of variable
> length */
> /* - fixed length: */
Same.
I would remove the "- " from the first "predefined formats" line, so that
these to will stick out to the right with the "- ", since that seems to be
your goal. But please don't indent the comments.
> /*
> * Translate a return value into an actual error message. Pass in the int
> * you received, get back a string.
> */
>
> const char *pco_strerror(int err)
Remove blank line, please.
> {
> switch (err) {
> case PCO_RETURN_OK:
> return "Success";
Extra indent, please.
> break;
> case PCO_ERROR_BAD_CODE:
> return "Bad Pronto Hex format";
Same.
> break;
> case PCO_ERROR_OUTOFMEM:
> return "Memory allocation failed";
Same.
> break;
> case PCO_ERROR_NOT_IMPL:
> return "Function not yet implemented";
Same.
> /*
> * read a hex word from source string:
> */
>
> int _get_hex_word(char **a_string, char *string_end, uint32_t *result)
No blank line, please.
> {
> int chars_read;
> int err = 0;
> if (*a_string <= string_end) {
> err = sscanf(*a_string, "%4x%n", result, &chars_read);
> if (err == 1) {
You probably want if err != -1. My understanding of %n is that you cannot
rely on it to not increment the "number of matched items." So really you
want to make sure you didn't err.
> /*
> * read from string:
> */
> int pco_sscanf_pronto_code(char *hex_string,
> struct pco_pronto_code *pronto_code)
> {
> char *hs_cursor = hex_string;
> char *hs_end = hex_string + strlen(hex_string) - 1;
> uint32_t next_word;
> uint32_t index = 0;
This is only used as a loop counter, please define it in those loops.
>
> /* hex_string contains blank-separated 4-digit hex numbers */
> if (_get_hex_word(&hs_cursor, hs_end, &next_word) != 1 ) {
No trailing space please. I'll just point this out here and let you fix all
of them.
> /*
> * following are lengths of once_signal and repeat_signal, counting
> * mark/space duration pairs. We multiply them by 2 to store the total
> * number of durations (marks + spaces) for each code:
> */
Line-lengths.
> /* allocate memory for sequences: */
> if (pronto_code->once_signal_length > 0) {
> pronto_code->once_signal =
> calloc(pronto_code->once_signal_length,
> sizeof(uint32_t));
Line-length.
> }
> if (pronto_code->repeat_signal_length > 0) {
> pronto_code->repeat_signal =
> calloc(pronto_code->repeat_signal_length,
> sizeof(uint32_t));
Line length.
> }
> for (index = 0; index < pronto_code->once_signal_length; index ++) {
index++.
Does "index ++" even compile?
> if (_get_hex_word(&hs_cursor, hs_end, &next_word) != 1 ) {
> return PCO_ERROR_BAD_CODE;
Extra indent.
> }
> pronto_code->once_signal[index] = next_word;
> }
>
> for (index = 0; index < pronto_code->repeat_signal_length; index ++) {
> if (_get_hex_word(&hs_cursor, hs_end, &next_word) != 1 ) {
> return PCO_ERROR_BAD_CODE;
Extra indent.
> }
> pronto_code->repeat_signal[index] = next_word;
> }
> return PCO_RETURN_OK;
> }
>
> /*
> * Conversion between different kinds of Pronto codes and IR signal streams:
> */
Line-length.
> int _convert_raw_mod(struct pco_pronto_code source, uint32_t carrier_cycle,
> uint32_t repetitions, uint32_t **ir_signal, uint32_t *ir_signal_length)
> {
> uint32_t source_index = 0;
This is only used in the for loop below, so please define it there.
> uint32_t ir_signal_index = 0;
> uint32_t repeat_count = 0;
Please define repeat_count in the loop it's used in.
Please define ir_signal_index just before the first loop it's in. Normally I
wouldn't care so much about that one, but it'll make it more obvious there
are two counters in that loop which is otherwise a bit confusing.
>
> /* basic validity check : */
> if (((source.once_signal_length > 0) && (source.once_signal == NULL)) ||
> ((source.repeat_signal_length > 0) && (source.repeat_signal ==
> NULL)) ||
Line-length.
> /*
> * alternating pairs of uint32_t wLEDflash, uint32_t wLEDoff in units
> * of carrier cycles: just convert to microseconds and copy to
> ir_signal:
> */
Line-length.
>
> /*
> * add OnceSeq:
> */
> for ( source_index = 0; source_index < source.once_signal_length;
> source_index++ ) {
No head/tail space. Line-length.
> /*
> * add RepeatSeq as many times as requested:
> */
> for ( repeat_count = repetitions; repeat_count > 0; repeat_count-- ) {
> for ( source_index = 0; source_index <
> source.repeat_signal_length;
> source_index++ ) {
No head/tail space. Line-length.
> (*ir_signal)[ir_signal_index++] =
> carrier_cycle *
> source.repeat_signal[source_index];
> }
> }
> return PCO_RETURN_OK;
Line-length.
> int _convert_nec_0(struct pco_pronto_code source, uint32_t carrier_cycle,
> uint32_t repetitions, uint32_t **ir_signal, uint32_t *ir_signal_length)
> {
> /* uint32_t mark_cycles = 0;
> uint32_t space_cycles = 0;
> */
> return PCO_ERROR_BAD_CODE;
> } /* convert_nec_0 */
Please use a more verbose name, or at least add a good comment. What's nec?
Also, this function does nothing... why?
> /*
> * decode Pronto format to ir signal stream and carrier clock.
> * repetitions determines the repetitions of repeat_signal
> * part of the pronto code - 0 means that repeat_signal is
> * not included at all.
> */
> int pco_pronto_to_ir_signal(
> struct pco_pronto_code source, uint32_t repetitions,
the first parameter can fit on the first line.
> /*
> * convert carrier freq from Pronto cycles to Hz and µs:
> */
> if (source.carrier_clock_divider != 0 ) {
> *carrier_freq = PRONTO_CLOCK/source.carrier_clock_divider;
> carrier_cycle = 1000000/(*carrier_freq); /* in
Please use spaces before/after the math operators.
Please put the comment on the line above, but itself. That's (a) the
preferred style in concordance and (b) will keep it from line wrapping.
> microseconds */
> }
>
> switch (source.format_id) {
> case PRONTO_FMTID_RAW_MOD: /* learned (raw) cmds */
> err = _convert_raw_mod(source, carrier_cycle, repetitions,
> ir_signal,
> ir_signal_length);
> break;
In general, I prefer this indentation-style of switch-statements, but it's
not what our codebase uses, so please conform.
Also, you have significant line-length issues.
>
> case PRONTO_FMTID_RAW_UNMOD:
> /*
> * basically the same as raw_mod, just no modulation with
> carrier.
> * Replacing dummy carrier from Pronto format ; on/off times are
> * still multiples of (dummy) carrier cycles in pronto code:
> */
> *carrier_freq = 0;
> err = _convert_raw_mod(source, carrier_cycle, repetitions,
> ir_signal,
> ir_signal_length);
I don't quite follow. If the carrier_freq is dummy in this case, then
doesn't carrier_cycle not make any sense the way you calculated it above?
Also, line-length.
> break;
> /*
> * Predefined formats have different structure, but for the reason of
> * compatibility with format 0000, fields wFrqDiv, nOnceSeq, nRepeatSeq
> * leave as dummy, so that code “looks” the same.
> * aOnceSeq and aRepeatSeq are replaced with sCode, that consist real
> * code info. Also, nOnceSeq and nRepeatSeq must meet the condition
> * (nOnceSeq + nRepeatSeq) * 2 = sizeOf(sCode):
> * Carrier clock is usually determined by the encoding, so it has to
> * be returned by the encoding functions.
> */
Line-length.
> --- Makefile.am 14 Apr 2008 01:55:46 -0000 1.7
> +++ Makefile.am 26 Apr 2009 22:14:23 -0000
> @@ -1,5 +1,5 @@
> bin_PROGRAMS = concordance
> -concordance_SOURCES = concordance.c
> +concordance_SOURCES = concordance.c prontocodes.h prontocodes.c
No, just the C file which will include the header file.
> --- concordance.c 6 Apr 2009 20:34:48 -0000 1.38
> +++ concordance.c 26 Apr 2009 22:14:23 -0000
...
> char get_cmd(char *prompt, char *allowed, char def) {
> char result = 0;
> char got_key;
> @@ -274,7 +302,7 @@ int learn_ir_commands(uint8_t *data, uin
> }
> printf("\nKey name : <%s> : \n", key_names[index]);
> user_cmd = get_cmd(
> - "[L]earn, [N]ext, [P]revious, [Q]uit",
> + "[L]earn, Pronto [H]ex code, [N]ext, [P]revious,
> [Q]uit",
Line length
> + case 'H':
> + case 'h': /* requested to get Pronto (from stdin): */
> + err = get_pronto_code(&carrier_clock,
> &ir_signal,
> + &ir_signal_length);
> + if (err != 0) {
> + printf("??? Failed: %s - try again!\n",
> + pco_strerror(err));
How about a nicer error message. Like:
Decoding Pronto Hex code failed: %s
?
--
Phil Dibowitz p...@ipom.com
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
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july
_______________________________________________ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
