Re: [PATCHv3 3/3] Add sixaxis plugin: USB pairing and LEDs settings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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: pgpLPAl9t4Qp3.pgp
Description: PGP signature


[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux