[Rack] Building some new electronics to open the gate?

[Jonathan Lassoff]

On Sun, Nov 13, 2011 at 3:50 PM, Michael Shields <shields at msrl.com> wrote:

> On Sat, Nov 12, 2011 at 9:37 PM, Jonathan Lassoff <jof at thejof.com> wrote:
> >
> > This is my hack job: https://gist.github.com/1361655
> > I'm still a little new to writing C from scratch (I'm usually modifying,
> > porting, or fixing stuff), so I'd appreciate any feedback on this. It
> > compiles and works on my machine, but that's the extent of my testing --
> I
> > still haven't tested it out at 2169.
>
> This code looks pretty good overall.
>
> It has a bug: if the packet received is larger than 255 bytes, the
> buffer won't be NUL-terminated, so strncmp will read past the end of
> it.  You can either check for this case, explicitly set the last byte
> of command_buffer to NUL, or use memcmp.
>

This was why I tried using strncmp and the sizeof of the largest command
first so that it would stop comparing once it's read as much as it could
possibly need. This probably needs a comment.

is_buzzer_ringing() can return -1, but its only caller doesn't check for
> this.
>

Gotcha.

You'll want to run it through gcc -Wall, which has a few suggestions:
>
>    parport_daemon.c: In function ‘is_buzzer_ringing’:
>    parport_daemon.c:76:3: warning: suggest parentheses around
> comparison in operand of ‘&’ [-Wparentheses]
>
> This is a style issue but I agree.  Using & instead of && and =
> instead of == in a condition is such a common bug that there needs to
> be some way to indicate that you know what you're doing.
>

Makes sense.


>    parport_daemon.c: In function ‘main’:
>    parport_daemon.c:222:7: warning: implicit declaration of function
> ‘strncmp’ [-Wimplicit-function-declaration]
>
> Fix this with #include <string.h>.
>

Gah. Why is there both string.h and strings.h?

   parport_daemon.c:226:66: warning: passing argument 4 of
> ‘send_response’ from incompatible pointer type [enabled by default]
>    parport_daemon.c:58:5: note: expected ‘char *’ but argument is of
> type ‘char (*)[31]’
>    parport_daemon.c:231:66: warning: passing argument 4 of
> ‘send_response’ from incompatible pointer type [enabled by default]
>    parport_daemon.c:58:5: note: expected ‘char *’ but argument is of
> type ‘char (*)[25]’
>    parport_daemon.c:239:66: warning: passing argument 4 of
> ‘send_response’ from incompatible pointer type [enabled by default]
>    parport_daemon.c:58:5: note: expected ‘char *’ but argument is of
> type ‘char (*)[6]’
>    parport_daemon.c:244:66: warning: passing argument 4 of
> ‘send_response’ from incompatible pointer type [enabled by default]
>    parport_daemon.c:58:5: note: expected ‘char *’ but argument is of
> type ‘char (*)[9]’
>
> You mean to use r_acknowleged instead of &r_acknowledged, etc.  Or
> &r_acknowleged[0] if you prefer.
>

Good point. That's a much simpler way to pointing at the beginning of an
array.


> By the way, you might well make these const.
>
>    parport_daemon.c:168:9: warning: unused variable ‘childpid’
> [-Wunused-variable]
>
> Remove.
>
>    parport_daemon.c: In function ‘send_response’:
>    parport_daemon.c:65:1: warning: control reaches end of non-void
> function [-Wreturn-type]
>
> Return 0 at the end.
>
> The macros on lines 37-42 aren't used and can be removed.
>
> ringer_state and buzzer_state can be bools.  You might also want to
> give them names like recently_buzzed.  If they could have more than
> two states, an enum would be a good choice.
>

All very good ideas.

-Wall, good idea. Thanks for the kind feedback.

Cheers,
jof
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.noisebridge.net/pipermail/rack/attachments/20111113/052e0ff9/attachment.html>