On Wed, 8 Jun 2011 14:52:51 -0300
Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxxxxxx> wrote:
> Hi Antonio,
>
> On 15:09 Wed 08 Jun, Antonio Ospite wrote:
> > Add a plugin which handles the connection of a Sixaxis device, when a
> > new hidraw device is connected the plugin:
> > - Filters udev events, and select the Sixaxis device
> > - Sets LEDs to match the joystick system number (for USB and BT)
> > - Sets the Master bluetooth address in the Sixaxis (USB pairing)
> > - Adds the device to the database of the current default
> > adapter (BT association)
>
> Just a few (mostly cosmetic) comments.
>
OK, it's the classic indentation/alignment issue, I used spaces for
_alignment_ not indentation (not very consistently, I have to admit),
but I am just going to comply to BLueZ style, we don't need any more
time wasted on these debates :)
And I am going to keep lines shorter than 78 chars as requested.
Basically this is what checkpatch.pl from linux complains about.
About the double newlines, I used them to separate logical sections of
the code:
0. Preamble (license header, includes, defines, etc.)
1. BT association
2. Usb cable pairing
3. Led setting (That's also why some defines were here, I considered
them "local" to this section)
4. Udev loop
5. Plugin framework stuff
They are more visible when the editor folds functions bodies, but I
have no problem about removing them if you like so.
Some further comments below, I left out all the parts where I agree and
I have nothing more to say.
> > +{
> > + DBusConnection *conn;
> > + sdp_record_t *rec;
> > + struct btd_device *device;
> > + bdaddr_t src, dst;
> > + char srcaddr[18];
> > + int ret = 0;
>
> The most common idiom is to call this kind of variable 'err'.
>
OK on that too, just for the records I tend to use 'err' when I
check != 0 [like in if (err) {...] I use 'ret' when I check for < 0.
[...]
> > + buf = calloc(len, sizeof(*buf));
>
> Usually in BlueZ code we try to use the allocations function provided by
> GLib, g_malloc0() in this case.
>
[...]
> > +
> > + address = calloc(BDADDR_STR_SIZE, sizeof(*address));
>
> Same here.
>
[...]
> > + error("%s:%s() Parsing the bt address failed", __FILE__, __func__);
> > + return FALSE;
> > + }
> > +
> > + report = malloc(8);
>
> g_malloc0()?
>
Would you mind if I keep these three stdlib ones in? I use this code
verbatim in another program which does not link against Glib.
> > +{
> > + struct udev_device *udevice;
> > +
> > + udevice = udev_monitor_receive_device(monitor);
> > + if (udevice == NULL)
> > + goto out;
> > + if (g_strcmp0(udev_device_get_action(udevice), "add") != 0) {
> > + udev_device_unref(udevice);
> > + goto out;
> > + }
> > +
> > + g_timeout_add_seconds(1, (GSourceFunc) device_event_idle, udevice);
>
> Just a question: why is this delay between finding the device and plug'ing it
> is needed? Perhaps a short comment explaining why it is needed would be nice.
>
/* Give UDEV some time to load kernel modules */
I guess this is the main reason, fact is that without that it does not
always work: we may get a null ID_USB_DRIVER.
> Cheers,
> --
> Vinicius
>
Thanks for the review,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Attachment:
pgpeeQOE3iO_Q.pgp
Description: PGP signature
