[PATCH BlueZ 0/5] plugin/sixaxis: use the sysfs leds class

[PATCH BlueZ 0/5] plugin/sixaxis: use the sysfs leds class

[Antonio Ospite]

Hi,

on kernels >= 3.14 the hid-sony driver exposes leds class entries in
sysfs for setting the Sixaxis LEDs, the patches in this set make BlueZ
use this interface as the default one and fall-back to hidraw in case
using sysfs fails (e.g. on older hid-sony versions).

Patch 1 is a cleanup, it's optional, feel free to reject it; the libudev
version bump is needed for patch 5 tho.

Patches 2,3 and 4 are refactorings in preparation of patch 5.

Patch 5 adds the actual support for the sysfs mechanism.

The changes have been tested with libudev-204 along with linux-3.13,
linux 3.14 and linux-3.15-rc4, however some more testing with different
libudev and kernel versions won't hurt :)

Thanks,
   Antonio

Antonio Ospite (5):
  plugins/sixaxis: simplify get_js_numbers()
  plugins/sixaxis: factor out a set_leds_hidraw() function
  plugins/sixaxis: factor out a calc_leds_bitmap() function
  plugins/sixaxis: add a get_leds_data() function
  plugins/sixaxis: add a set_leds_sysfs() function

 configure.ac      |   4 +-
 plugins/sixaxis.c | 256 ++++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 193 insertions(+), 67 deletions(-)

-- 
Antonio Ospite
http://ao2.it

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?

[PATCH BlueZ 1/5] plugins/sixaxis: simplify get_js_numbers()

[Antonio Ospite]

Use udev_enumerate_add_match_parent() to simplify get_js_number() a lot,
with this mechanism the old JOIN-like operation to find joystick nodes
is no longer necessary.

Also require libudev >= 172, this is where
udev_enumerate_add_match_parent() has been first introduced.

NOTE: using udev_enumerate_add_match_parent() results in a memory leak
when enumerating child devices, this has been fixed in udev 207; the
commit which fixes the issue is this one:
http://cgit.freedesktop.org/systemd/systemd/commit/?id=51cc07576e119dea6e65478eeba9472979fd0936
---
 configure.ac      |  4 +--
 plugins/sixaxis.c | 92 +++++++++++++++++++++++++++----------------------------
 2 files changed, 47 insertions(+), 49 deletions(-)

diff --git a/configure.ac b/configure.ac
index 54a387f..4208ad8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -125,8 +125,8 @@ AM_CONDITIONAL(MONITOR, test "${enable_monitor}" != "no")
 AC_ARG_ENABLE(udev, AC_HELP_STRING([--disable-udev],
 		[disable udev device support]), [enable_udev=${enableval}])
 if (test "${enable_tools}" != "no" && test "${enable_udev}" != "no"); then
-	PKG_CHECK_MODULES(UDEV, libudev >= 143, dummy=yes,
-				AC_MSG_ERROR(libudev >= 143 is required))
+	PKG_CHECK_MODULES(UDEV, libudev >= 172, dummy=yes,
+				AC_MSG_ERROR(libudev >= 172 is required))
 	AC_SUBST(UDEV_CFLAGS)
 	AC_SUBST(UDEV_LIBS)
 	AC_CHECK_LIB(udev, udev_hwdb_new,
diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 8045448..9db14ec 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -235,62 +235,60 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
 
 static int get_js_number(struct udev_device *udevice)
 {
-	struct udev_list_entry *devices, *dev_list_entry;
+	struct udev_list_entry *dev_list_entry;
 	struct udev_enumerate *enumerate;
 	struct udev_device *hid_parent;
-	const char *hidraw_node;
-	const char *hid_phys;
+	struct udev_device *transport_parent;
+	struct udev_device *js_dev;
+	const char *js_devname;
 	int number = 0;
 
-	hid_parent = udev_device_get_parent_with_subsystem_devtype(udevice,
-								"hid", NULL);
-
-	hid_phys = udev_device_get_property_value(hid_parent, "HID_PHYS");
-	hidraw_node = udev_device_get_devnode(udevice);
-	if (!hid_phys || !hidraw_node)
-		return 0;
+	/*
+	 * Go up by two levels from the hidraw device in order to support
+	 * older kernels, where the hierachy of HID devices is different than
+	 * newer kernels:
+	 *
+	 *   - on kernels < 3.14 the input device is child of the transport
+	 *     device (usbhid, hidp):
+	 *
+	 *       USB/BT
+	 *       |- hid
+	 *       |  `- hidraw *
+	 *       `- input
+	 *          `- js
+	 *
+	 *   - on kernels >= 3.14 the input device is child of the hid device
+	 *     itself, as it should be:
+	 *
+	 *       USB/BT
+	 *       `- hid
+	 *          |- hidraw *
+	 *          `- input
+	 *             `- js
+	 */
+	hid_parent = udev_device_get_parent(udevice);
+	transport_parent = udev_device_get_parent(hid_parent);
 
 	enumerate = udev_enumerate_new(udev_device_get_udev(udevice));
+	udev_enumerate_add_match_parent(enumerate, transport_parent);
 	udev_enumerate_add_match_sysname(enumerate, "js*");
 	udev_enumerate_scan_devices(enumerate);
-	devices = udev_enumerate_get_list_entry(enumerate);
-
-	udev_list_entry_foreach(dev_list_entry, devices) {
-		struct udev_device *input_parent;
-		struct udev_device *js_dev;
-		const char *input_phys;
-		const char *devname;
-
-		devname = udev_list_entry_get_name(dev_list_entry);
-		js_dev = udev_device_new_from_syspath(
-						udev_device_get_udev(udevice),
-						devname);
-
-		input_parent = udev_device_get_parent_with_subsystem_devtype(
-							js_dev, "input", NULL);
-		if (!input_parent)
-			goto next;
-
-		/* check if this is the joystick relative to the hidraw device
-		 * above */
-		input_phys = udev_device_get_sysattr_value(input_parent,
-									"phys");
-		if (!input_phys)
-			goto next;
-
-		if (!strcmp(input_phys, hid_phys)) {
-			number = atoi(udev_device_get_sysnum(js_dev));
-
-			/* joystick numbers start from 0, leds from 1 */
-			number++;
-
-			udev_device_unref(js_dev);
-			break;
-		}
-next:
-		udev_device_unref(js_dev);
-	}
 
+	/* get the first js* device, there should be only one anyway */
+	dev_list_entry = udev_enumerate_get_list_entry(enumerate);
+
+	if (dev_list_entry == NULL)
+		return number;
+
+	js_devname = udev_list_entry_get_name(dev_list_entry);
+	js_dev = udev_device_new_from_syspath(
+					udev_device_get_udev(udevice),
+					js_devname);
+
+	/* joystick numbers start from 0, leds from 1 */
+	number = atoi(udev_device_get_sysnum(js_dev)) + 1;
+
+	udev_device_unref(js_dev);
 	udev_enumerate_unref(enumerate);
 
 	return number;
-- 
2.0.0.rc0

Re: [PATCH BlueZ 1/5] plugins/sixaxis: simplify get_js_numbers()

[Szymon Janc]

Hi Antonio,

On Tuesday 06 May 2014 12:06:12 Antonio Ospite wrote:

Just a nitpick :) we usually start commit summary with capital letter eg.
"plugins/sixaxis: Simplify get_js_numbers function"

Same goes to rest of commits in this series.

> Use udev_enumerate_add_match_parent() to simplify get_js_number() a lot,
> with this mechanism the old JOIN-like operation to find joystick nodes
> is no longer necessary.
> 
> Also require libudev >= 172, this is where
> udev_enumerate_add_match_parent() has been first introduced.

This should be fine, any distro shipping BlueZ 5 is most likely already 
providing newer version of udev anyway.

> 
> NOTE: using udev_enumerate_add_match_parent() results in a memory leak
> when enumerating child devices, this has been fixed in udev 207; the
> commit which fixes the issue is this one:
> http://cgit.freedesktop.org/systemd/systemd/commit/?id=51cc07576e119dea6e654
> 78eeba9472979fd0936 ---
>  configure.ac      |  4 +--
>  plugins/sixaxis.c | 92
> +++++++++++++++++++++++++++---------------------------- 2 files changed, 47
> insertions(+), 49 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 54a387f..4208ad8 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -125,8 +125,8 @@ AM_CONDITIONAL(MONITOR, test "${enable_monitor}" !=
> "no") AC_ARG_ENABLE(udev, AC_HELP_STRING([--disable-udev],
>  		[disable udev device support]), [enable_udev=${enableval}])
>  if (test "${enable_tools}" != "no" && test "${enable_udev}" != "no"); then
> -	PKG_CHECK_MODULES(UDEV, libudev >= 143, dummy=yes,
> -				AC_MSG_ERROR(libudev >= 143 is required))
> +	PKG_CHECK_MODULES(UDEV, libudev >= 172, dummy=yes,
> +				AC_MSG_ERROR(libudev >= 172 is required))
>  	AC_SUBST(UDEV_CFLAGS)
>  	AC_SUBST(UDEV_LIBS)
>  	AC_CHECK_LIB(udev, udev_hwdb_new,
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index 8045448..9db14ec 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -235,62 +235,60 @@ static bool setup_device(int fd, int index, struct
> btd_adapter *adapter)
> 
>  static int get_js_number(struct udev_device *udevice)
>  {
> -	struct udev_list_entry *devices, *dev_list_entry;
> +	struct udev_list_entry *dev_list_entry;
>  	struct udev_enumerate *enumerate;
>  	struct udev_device *hid_parent;
> -	const char *hidraw_node;
> -	const char *hid_phys;
> +	struct udev_device *transport_parent;
> +	struct udev_device *js_dev;
> +	const char *js_devname;
>  	int number = 0;
> 
> -	hid_parent = udev_device_get_parent_with_subsystem_devtype(udevice,
> -								"hid", NULL);
> -
> -	hid_phys = udev_device_get_property_value(hid_parent, "HID_PHYS");
> -	hidraw_node = udev_device_get_devnode(udevice);
> -	if (!hid_phys || !hidraw_node)
> -		return 0;
> +	/*
> +	 * Go up by two levels from the hidraw device in order to support
> +	 * older kernels, where the hierachy of HID devices is different than
> +	 * newer kernels:
> +	 *
> +	 *   - on kernels < 3.14 the input device is child of the transport
> +	 *     device (usbhid, hidp):
> +	 *
> +	 *       USB/BT
> +	 *       |- hid
> +	 *       |  `- hidraw *
> +	 *       `- input
> +	 *          `- js
> +	 *
> +	 *   - on kernels >= 3.14 the input device is child of the hid device
> +	 *     itself, as it should be:
> +	 *
> +	 *       USB/BT
> +	 *       `- hid
> +	 *          |- hidraw *
> +	 *          `- input
> +	 *             `- js
> +	 */
> +	hid_parent = udev_device_get_parent(udevice);
> +	transport_parent = udev_device_get_parent(hid_parent);
> 
>  	enumerate = udev_enumerate_new(udev_device_get_udev(udevice));
> +	udev_enumerate_add_match_parent(enumerate, transport_parent);
>  	udev_enumerate_add_match_sysname(enumerate, "js*");
>  	udev_enumerate_scan_devices(enumerate);
> -	devices = udev_enumerate_get_list_entry(enumerate);
> -
> -	udev_list_entry_foreach(dev_list_entry, devices) {
> -		struct udev_device *input_parent;
> -		struct udev_device *js_dev;
> -		const char *input_phys;
> -		const char *devname;
> -
> -		devname = udev_list_entry_get_name(dev_list_entry);
> -		js_dev = udev_device_new_from_syspath(
> -						udev_device_get_udev(udevice),
> -						devname);
> -
> -		input_parent = udev_device_get_parent_with_subsystem_devtype(
> -							js_dev, "input", NULL);
> -		if (!input_parent)
> -			goto next;
> -
> -		/* check if this is the joystick relative to the hidraw device
> -		 * above */
> -		input_phys = udev_device_get_sysattr_value(input_parent,
> -									"phys");
> -		if (!input_phys)
> -			goto next;
> -
> -		if (!strcmp(input_phys, hid_phys)) {
> -			number = atoi(udev_device_get_sysnum(js_dev));
> -
> -			/* joystick numbers start from 0, leds from 1 */
> -			number++;
> -
> -			udev_device_unref(js_dev);
> -			break;
> -		}
> -next:
> -		udev_device_unref(js_dev);
> -	}
> 
> +	/* get the first js* device, there should be only one anyway */
> +	dev_list_entry = udev_enumerate_get_list_entry(enumerate);
> +
> +	if (dev_list_entry == NULL)

if (!dev_list_entry) ...

> +		return number;
> +
> +	js_devname = udev_list_entry_get_name(dev_list_entry);
> +	js_dev = udev_device_new_from_syspath(
> +					udev_device_get_udev(udevice),
> +					js_devname);

Indentation issue here.

> +
> +	/* joystick numbers start from 0, leds from 1 */
> +	number = atoi(udev_device_get_sysnum(js_dev)) + 1;
> +
> +	udev_device_unref(js_dev);
>  	udev_enumerate_unref(enumerate);
> 
>  	return number;

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCH BlueZ 2/5] plugins/sixaxis: factor out a set_leds_hidraw() function

[Antonio Ospite]

This is in preparation for a set_leds_sysfs() function.

Also use TRUE as the return value of set_leds_hidraw() in the success
case just to be more semantically nice; the return value is not used in
setup_leds() anyway because the latter has to always return FALSE.
---
 plugins/sixaxis.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 9db14ec..61fd0b5 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -125,8 +125,7 @@ static int set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
 	return ret;
 }
 
-static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
-							gpointer user_data)
+static gboolean set_leds_hidraw(int fd, int number)
 {
 	/*
 	 * the total time the led is active (0xff means forever)
@@ -147,14 +146,7 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 		0xff, 0x27, 0x10, 0x00, 0x32, /* LED_1 */
 		0x00, 0x00, 0x00, 0x00, 0x00,
 	};
-	int number = GPOINTER_TO_INT(user_data);
 	int ret;
-	int fd;
-
-	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
-		return FALSE;
-
-	DBG("number %d", number);
 
 	/* TODO we could support up to 10 (1 + 2 + 3 + 4) */
 	if (number > 7)
@@ -167,11 +159,9 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 
 	leds_report[10] |= 0x01 << number;
 
-	fd = g_io_channel_unix_get_fd(channel);
-
 	ret = write(fd, leds_report, sizeof(leds_report));
 	if (ret == sizeof(leds_report))
-		return FALSE;
+		return TRUE;
 
 	if (ret < 0)
 		error("sixaxis: failed to set LEDS (%s)", strerror(errno));
@@ -181,6 +171,23 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 	return FALSE;
 }
 
+static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
+							gpointer user_data)
+{
+	int number = GPOINTER_TO_INT(user_data);
+	int fd;
+
+	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
+		return FALSE;
+
+	DBG("number %d", number);
+
+	fd = g_io_channel_unix_get_fd(channel);
+	set_leds_hidraw(fd, number);
+
+	return FALSE;
+}
+
 static bool setup_device(int fd, int index, struct btd_adapter *adapter)
 {
 	char device_addr[18], master_addr[18], adapter_addr[18];
-- 
2.0.0.rc0

Re: [PATCH BlueZ 2/5] plugins/sixaxis: factor out a set_leds_hidraw() function

[Szymon Janc]

Hi Antonio,

On Tuesday 06 May 2014 12:06:13 Antonio Ospite wrote:
> This is in preparation for a set_leds_sysfs() function.
> 
> Also use TRUE as the return value of set_leds_hidraw() in the success
> case just to be more semantically nice; the return value is not used in
> setup_leds() anyway because the latter has to always return FALSE.

I'd just make it return void then.

> ---
>  plugins/sixaxis.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index 9db14ec..61fd0b5 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -125,8 +125,7 @@ static int set_master_bdaddr(int fd, const bdaddr_t
> *bdaddr) return ret;
>  }
> 
> -static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
> -							gpointer user_data)
> +static gboolean set_leds_hidraw(int fd, int number)
>  {
>  	/*
>  	 * the total time the led is active (0xff means forever)
> @@ -147,14 +146,7 @@ static gboolean setup_leds(GIOChannel *channel,
> GIOCondition cond, 0xff, 0x27, 0x10, 0x00, 0x32, /* LED_1 */
>  		0x00, 0x00, 0x00, 0x00, 0x00,
>  	};
> -	int number = GPOINTER_TO_INT(user_data);
>  	int ret;
> -	int fd;
> -
> -	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> -		return FALSE;
> -
> -	DBG("number %d", number);
> 
>  	/* TODO we could support up to 10 (1 + 2 + 3 + 4) */
>  	if (number > 7)
> @@ -167,11 +159,9 @@ static gboolean setup_leds(GIOChannel *channel,
> GIOCondition cond,
> 
>  	leds_report[10] |= 0x01 << number;
> 
> -	fd = g_io_channel_unix_get_fd(channel);
> -
>  	ret = write(fd, leds_report, sizeof(leds_report));
>  	if (ret == sizeof(leds_report))
> -		return FALSE;
> +		return TRUE;
> 
>  	if (ret < 0)
>  		error("sixaxis: failed to set LEDS (%s)", strerror(errno));
> @@ -181,6 +171,23 @@ static gboolean setup_leds(GIOChannel *channel,
> GIOCondition cond, return FALSE;
>  }
> 
> +static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
> +							gpointer user_data)
> +{
> +	int number = GPOINTER_TO_INT(user_data);
> +	int fd;
> +
> +	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> +		return FALSE;
> +
> +	DBG("number %d", number);
> +
> +	fd = g_io_channel_unix_get_fd(channel);
> +	set_leds_hidraw(fd, number);
> +
> +	return FALSE;
> +}
> +
>  static bool setup_device(int fd, int index, struct btd_adapter *adapter)
>  {
>  	char device_addr[18], master_addr[18], adapter_addr[18];

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCH BlueZ 3/5] plugins/sixaxis: factor out a calc_leds_bitmap() function

[Antonio Ospite]

This is also in preparation of set_leds_sysfs().
---
 plugins/sixaxis.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 61fd0b5..b629c06 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -125,7 +125,25 @@ static int set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
 	return ret;
 }
 
-static gboolean set_leds_hidraw(int fd, int number)
+static gboolean calc_leds_bitmap(int number, uint8_t *bitmap)
+{
+	*bitmap = 0;
+
+	/* TODO we could support up to 10 (1 + 2 + 3 + 4) */
+	if (number > 7)
+		return FALSE;
+
+	if (number > 4) {
+		*bitmap |= 0x10;
+		number -= 4;
+	}
+
+	*bitmap |= 0x01 << number;
+
+	return TRUE;
+}
+
+static gboolean set_leds_hidraw(int fd, uint8_t leds_bitmap)
 {
 	/*
 	 * the total time the led is active (0xff means forever)
@@ -148,16 +166,7 @@ static gboolean set_leds_hidraw(int fd, int number)
 	};
 	int ret;
 
-	/* TODO we could support up to 10 (1 + 2 + 3 + 4) */
-	if (number > 7)
-		return FALSE;
-
-	if (number > 4) {
-		leds_report[10] |= 0x10;
-		number -= 4;
-	}
-
-	leds_report[10] |= 0x01 << number;
+	leds_report[10] = leds_bitmap;
 
 	ret = write(fd, leds_report, sizeof(leds_report));
 	if (ret == sizeof(leds_report))
@@ -175,6 +184,7 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
 	int number = GPOINTER_TO_INT(user_data);
+	uint8_t bitmap = 0;
 	int fd;
 
 	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
@@ -183,7 +193,9 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 	DBG("number %d", number);
 
 	fd = g_io_channel_unix_get_fd(channel);
-	set_leds_hidraw(fd, number);
+
+	if (calc_leds_bitmap(number, &bitmap))
+		set_leds_hidraw(fd, bitmap);
 
 	return FALSE;
 }
-- 
2.0.0.rc0

Re: [PATCH BlueZ 3/5] plugins/sixaxis: factor out a calc_leds_bitmap() function

[Szymon Janc]

Hi Antonio,

On Tuesday 06 May 2014 12:06:14 Antonio Ospite wrote:
> This is also in preparation of set_leds_sysfs().
> ---
>  plugins/sixaxis.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index 61fd0b5..b629c06 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -125,7 +125,25 @@ static int set_master_bdaddr(int fd, const bdaddr_t
> *bdaddr) return ret;
>  }
> 
> -static gboolean set_leds_hidraw(int fd, int number)
> +static gboolean calc_leds_bitmap(int number, uint8_t *bitmap)
> +{

Make this return bool. Don't use glib types if this is not required due to use 
of glib API.

> +	*bitmap = 0;

Move this after number check. It is usually good to make function leave no 
side effects (if possible) if it failed.

You could also make this helper simply return bitmap value and then treat 0 as 
error.

> +
> +	/* TODO we could support up to 10 (1 + 2 + 3 + 4) */
> +	if (number > 7)
> +		return FALSE;
> +
> +	if (number > 4) {
> +		*bitmap |= 0x10;
> +		number -= 4;
> +	}
> +
> +	*bitmap |= 0x01 << number;
> +
> +	return TRUE;
> +}
> +
> +static gboolean set_leds_hidraw(int fd, uint8_t leds_bitmap)
>  {
>  	/*
>  	 * the total time the led is active (0xff means forever)
> @@ -148,16 +166,7 @@ static gboolean set_leds_hidraw(int fd, int number)
>  	};
>  	int ret;
> 
> -	/* TODO we could support up to 10 (1 + 2 + 3 + 4) */
> -	if (number > 7)
> -		return FALSE;
> -
> -	if (number > 4) {
> -		leds_report[10] |= 0x10;
> -		number -= 4;
> -	}
> -
> -	leds_report[10] |= 0x01 << number;
> +	leds_report[10] = leds_bitmap;
> 
>  	ret = write(fd, leds_report, sizeof(leds_report));
>  	if (ret == sizeof(leds_report))
> @@ -175,6 +184,7 @@ static gboolean setup_leds(GIOChannel *channel,
> GIOCondition cond, gpointer user_data)
>  {
>  	int number = GPOINTER_TO_INT(user_data);
> +	uint8_t bitmap = 0;
>  	int fd;
> 
>  	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> @@ -183,7 +193,9 @@ static gboolean setup_leds(GIOChannel *channel,
> GIOCondition cond, DBG("number %d", number);
> 
>  	fd = g_io_channel_unix_get_fd(channel);
> -	set_leds_hidraw(fd, number);
> +
> +	if (calc_leds_bitmap(number, &bitmap))
> +		set_leds_hidraw(fd, bitmap);
> 
>  	return FALSE;
>  }

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCH BlueZ 4/5] plugins/sixaxis: add a get_leds_data() function

[Antonio Ospite]

Get all the data necessary to set the LEDs (bitmap and/or sysfs paths)
in a single function, returning a leds_data structure to be passed as
argument to the setup_leds() callback.

For now only the bitmap field is populated, which is the only thing that
set_leds_hidraw() needs.
---
 plugins/sixaxis.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 7 deletions(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index b629c06..2b35d1c 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -61,6 +61,11 @@ static const struct {
 	},
 };
 
+struct leds_data {
+	char *syspaths[4];
+	uint8_t bitmap;
+};
+
 static struct udev *ctx = NULL;
 static struct udev_monitor *monitor = NULL;
 static guint watch_id = 0;
@@ -183,19 +188,25 @@ static gboolean set_leds_hidraw(int fd, uint8_t leds_bitmap)
 static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
-	int number = GPOINTER_TO_INT(user_data);
-	uint8_t bitmap = 0;
 	int fd;
+	int i;
+	int ret;
+	struct leds_data *data = (struct leds_data *)user_data;
 
-	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
+	if (data == NULL)
 		return FALSE;
 
-	DBG("number %d", number);
+	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
+		goto out;
 
 	fd = g_io_channel_unix_get_fd(channel);
 
-	if (calc_leds_bitmap(number, &bitmap))
-		set_leds_hidraw(fd, bitmap);
+	set_leds_hidraw(fd, data->bitmap);
+
+out:
+	for (i = 0; i < 4; i++)
+		free(data->syspaths[i]);
+	free(data);
 
 	return FALSE;
 }
@@ -313,6 +324,35 @@ static int get_js_number(struct udev_device *udevice)
 	return number;
 }
 
+static struct leds_data *get_leds_data(struct udev_device *udevice)
+{
+	struct leds_data *data;
+	int number;
+	int i;
+	int ret;
+
+	data = malloc(sizeof(*data));
+	if (data == NULL)
+		return NULL;
+
+	memset(data, 0, sizeof(*data));
+
+	number = get_js_number(udevice);
+	DBG("number %d", number);
+
+	ret = calc_leds_bitmap(number, &data->bitmap);
+	if (ret == FALSE)
+		goto err;
+
+	return data;
+
+err:
+	for (i = 0; i < 4; i++)
+		free(data->syspaths[i]);
+	free(data);
+	return NULL;
+}
+
 static int get_supported_device(struct udev_device *udevice, uint16_t *bus)
 {
 	struct udev_device *hid_parent;
@@ -374,7 +414,7 @@ static void device_added(struct udev_device *udevice)
 		/* wait for events before setting leds */
 		g_io_add_watch(io, G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
 				setup_leds,
-				GINT_TO_POINTER(get_js_number(udevice)));
+				get_leds_data(udevice));
 
 		break;
 	default:
-- 
2.0.0.rc0

Re: [PATCH BlueZ 4/5] plugins/sixaxis: add a get_leds_data() function

[Szymon Janc]

Hi Antonio,

On Tuesday 06 May 2014 12:06:15 Antonio Ospite wrote:
> Get all the data necessary to set the LEDs (bitmap and/or sysfs paths)
> in a single function, returning a leds_data structure to be passed as
> argument to the setup_leds() callback.
> 
> For now only the bitmap field is populated, which is the only thing that
> set_leds_hidraw() needs.
> ---
>  plugins/sixaxis.c | 54
> +++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 47
> insertions(+), 7 deletions(-)
> 
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index b629c06..2b35d1c 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -61,6 +61,11 @@ static const struct {
>  	},
>  };
> 
> +struct leds_data {
> +	char *syspaths[4];
> +	uint8_t bitmap;
> +};
> +
>  static struct udev *ctx = NULL;
>  static struct udev_monitor *monitor = NULL;
>  static guint watch_id = 0;
> @@ -183,19 +188,25 @@ static gboolean set_leds_hidraw(int fd, uint8_t
> leds_bitmap) static gboolean setup_leds(GIOChannel *channel, GIOCondition
> cond, gpointer user_data)
>  {
> -	int number = GPOINTER_TO_INT(user_data);
> -	uint8_t bitmap = 0;
>  	int fd;
> +	int i;
> +	int ret;
> +	struct leds_data *data = (struct leds_data *)user_data;
> 
> -	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> +	if (data == NULL)
>  		return FALSE;
> 
> -	DBG("number %d", number);
> +	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> +		goto out;
> 
>  	fd = g_io_channel_unix_get_fd(channel);
> 
> -	if (calc_leds_bitmap(number, &bitmap))
> -		set_leds_hidraw(fd, bitmap);
> +	set_leds_hidraw(fd, data->bitmap);
> +
> +out:
> +	for (i = 0; i < 4; i++)
> +		free(data->syspaths[i]);
> +	free(data);
> 
>  	return FALSE;
>  }
> @@ -313,6 +324,35 @@ static int get_js_number(struct udev_device *udevice)
>  	return number;
>  }
> 
> +static struct leds_data *get_leds_data(struct udev_device *udevice)
> +{
> +	struct leds_data *data;
> +	int number;
> +	int i;
> +	int ret;
> +
> +	data = malloc(sizeof(*data));
> +	if (data == NULL)

if (!data) ..

> +		return NULL;
> +
> +	memset(data, 0, sizeof(*data));

You can use malloc0() from src/shared instead.

> +
> +	number = get_js_number(udevice);
> +	DBG("number %d", number);
> +
> +	ret = calc_leds_bitmap(number, &data->bitmap);
> +	if (ret == FALSE)
> +		goto err;

ret seems to be redundant

if (!calc_leds_bitmap())
  goto err

> +
> +	return data;
> +
> +err:
> +	for (i = 0; i < 4; i++)
> +		free(data->syspaths[i]);
> +	free(data);

Please add helper (eg. leds_data_destroy()) for clearing this and use where 
needed.

Add empty line here.

> +	return NULL;
> +}
> +
>  static int get_supported_device(struct udev_device *udevice, uint16_t *bus)
> {
>  	struct udev_device *hid_parent;
> @@ -374,7 +414,7 @@ static void device_added(struct udev_device *udevice)
>  		/* wait for events before setting leds */
>  		g_io_add_watch(io, G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
>  				setup_leds,
> -				GINT_TO_POINTER(get_js_number(udevice)));
> +				get_leds_data(udevice));
> 
>  		break;
>  	default:

-- 
Szymon K. Janc
szymon.janc@gmail.com

Re: [PATCH BlueZ 4/5] plugins/sixaxis: add a get_leds_data() function

[Johan Hedberg]

Hi Antonio,

On Tue, May 06, 2014, Antonio Ospite wrote:
>  static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
>  							gpointer user_data)
>  {
> -	int number = GPOINTER_TO_INT(user_data);
> -	uint8_t bitmap = 0;
>  	int fd;
> +	int i;
> +	int ret;
> +	struct leds_data *data = (struct leds_data *)user_data;

No need to have an explicit cast for a void pointer.

>  
> -	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> +	if (data == NULL)
>  		return FALSE;
>  
> -	DBG("number %d", number);
> +	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> +		goto out;
>  
>  	fd = g_io_channel_unix_get_fd(channel);
>  
> -	if (calc_leds_bitmap(number, &bitmap))
> -		set_leds_hidraw(fd, bitmap);
> +	set_leds_hidraw(fd, data->bitmap);
> +
> +out:
> +	for (i = 0; i < 4; i++)
> +		free(data->syspaths[i]);
> +	free(data);
>  
>  	return FALSE;
>  }

I don't see you using the "int ret" anywhere in this function. Even if
you do end up using it later for returning something I suppose it should
be a boolean type and not int. However even then the variable should be
introduced in the patch that actually makes use of it. If you build test
each patch individually with --enable-maintainer-mode the compiler
should warn about unused variables (amongst many other things) for you.

> @@ -313,6 +324,35 @@ static int get_js_number(struct udev_device *udevice)
>  	return number;
>  }
>  
> +static struct leds_data *get_leds_data(struct udev_device *udevice)
> +{
> +	struct leds_data *data;
> +	int number;
> +	int i;
> +	int ret;
> +
> +	data = malloc(sizeof(*data));
> +	if (data == NULL)
> +		return NULL;
> +
> +	memset(data, 0, sizeof(*data));
> +
> +	number = get_js_number(udevice);
> +	DBG("number %d", number);
> +
> +	ret = calc_leds_bitmap(number, &data->bitmap);
> +	if (ret == FALSE)
> +		goto err;

Same here: if you're storing a boolean value into ret it should be
declared as a boolean type instead of int. Also, you don't need the
"== FALSE" comparison since boolean types are supposed to be valid
boolean expressions by themselves, i.e. "if (!ret)" should do.

Johan

Re: [PATCH BlueZ 4/5] plugins/sixaxis: add a get_leds_data() function

[Antonio Ospite]

On Fri, 9 May 2014 15:22:32 +0300
Johan Hedberg <johan.hedberg@gmail.com> wrote:

> Hi Antonio,
> 
[...] 
> >  
> > -	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> > +	if (data == NULL)
> >  		return FALSE;
> >  
> > -	DBG("number %d", number);
> > +	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> > +		goto out;
> >  
> >  	fd = g_io_channel_unix_get_fd(channel);
> >  
> > -	if (calc_leds_bitmap(number, &bitmap))
> > -		set_leds_hidraw(fd, bitmap);
> > +	set_leds_hidraw(fd, data->bitmap);
> > +
> > +out:
> > +	for (i = 0; i < 4; i++)
> > +		free(data->syspaths[i]);
> > +	free(data);
> >  
> >  	return FALSE;
> >  }
> 
> I don't see you using the "int ret" anywhere in this function. Even if
> you do end up using it later for returning something I suppose it should
> be a boolean type and not int. However even then the variable should be
> introduced in the patch that actually makes use of it. If you build test
> each patch individually with --enable-maintainer-mode the compiler
> should warn about unused variables (amongst many other things) for you.
>

Thanks Johan, I forgot about --enable-maintainer-mode.

I'll use it for the v2.

Ciao ciao,
   Antonio

-- 
Antonio Ospite
http://ao2.it

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?

[PATCH BlueZ 5/5] plugins/sixaxis: add a set_leds_sysfs() function

[Antonio Ospite]

On recent kernels the hid-sony driver exposes leds class entries in
sysfs for setting the Sixaxis LEDs, use this interface and fall back to
hidraw in case using sysfs fails (e.g. on older hid-sony versions).

Setting the LEDs via sysfs is the preferred way on newer kernels, the
rationale behind that is:

  1. the Sixaxis uses the same HID output report for setting both LEDs
     and rumble effects;
  2. hid-sony remembers the state of LEDs in order to preserve them when
     setting rumble effects;
  3. when the LEDs are set via hidraw hid-sony has no way to know the
     new LEDs state and thus can change the LEDs in an inconsistent way
     when setting rumble effects later.
---
 plugins/sixaxis.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 73 insertions(+), 4 deletions(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 2b35d1c..446d499 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -185,10 +185,40 @@ static gboolean set_leds_hidraw(int fd, uint8_t leds_bitmap)
 	return FALSE;
 }
 
+static gboolean set_leds_sysfs(struct leds_data *data)
+{
+	int i;
+
+	for (i = 0; i < 4; i++) {
+		char path[PATH_MAX] = { 0 };
+		char buf[2] = { 0 };
+		int fd;
+		int ret;
+
+		if (data->syspaths[i] == NULL)
+			return FALSE;
+
+		snprintf(path, PATH_MAX, "%s/brightness", data->syspaths[i]);
+		fd = open(path, O_WRONLY);
+		if (fd < 0) {
+			error("Cannot open %s (%s)", path, strerror(errno));
+			return FALSE;
+		}
+
+		/* i+1 because leds start from 1, LED0 is never used */
+		buf[0] = '0' + !!(data->bitmap & (1 << (i+1)));
+		ret = write(fd, buf, sizeof(buf));
+		close(fd);
+		if (ret != sizeof(buf))
+			return FALSE;
+	}
+
+	return TRUE;
+}
+
 static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
-	int fd;
 	int i;
 	int ret;
 	struct leds_data *data = (struct leds_data *)user_data;
@@ -199,9 +229,11 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
 		goto out;
 
-	fd = g_io_channel_unix_get_fd(channel);
-
-	set_leds_hidraw(fd, data->bitmap);
+	ret = set_leds_sysfs(data);
+	if (ret == FALSE) {
+		int fd = g_io_channel_unix_get_fd(channel);
+		set_leds_hidraw(fd, data->bitmap);
+	}
 
 out:
 	for (i = 0; i < 4; i++)
@@ -324,6 +356,39 @@ static int get_js_number(struct udev_device *udevice)
 	return number;
 }
 
+static int get_leds_devices(struct udev_device *udevice, char *paths[4])
+{
+	struct udev_list_entry *devices, *dev_list_entry;
+	struct udev_enumerate *enumerate;
+	struct udev_device *hid_parent;
+	int index = 0;
+	int ret;
+
+	hid_parent = udev_device_get_parent_with_subsystem_devtype(udevice,
+								"hid", NULL);
+
+	enumerate = udev_enumerate_new(udev_device_get_udev(udevice));
+	udev_enumerate_add_match_parent(enumerate, hid_parent);
+	udev_enumerate_add_match_subsystem(enumerate, "leds");
+	udev_enumerate_scan_devices(enumerate);
+	devices = udev_enumerate_get_list_entry(enumerate);
+
+	if (devices == NULL) {
+		ret = FALSE;
+		goto out;
+	}
+
+	udev_list_entry_foreach(dev_list_entry, devices) {
+		const char *syspath = udev_list_entry_get_name(dev_list_entry);
+		paths[index++] = strdup(syspath);
+	}
+
+	ret = TRUE;
+out:
+	udev_enumerate_unref(enumerate);
+	return ret;
+}
+
 static struct leds_data *get_leds_data(struct udev_device *udevice)
 {
 	struct leds_data *data;
@@ -344,6 +409,10 @@ static struct leds_data *get_leds_data(struct udev_device *udevice)
 	if (ret == FALSE)
 		goto err;
 
+	/* it's OK if this fails, set_leds_hidraw() will be used if
+	 * any of the values in data->syspaths is not defined */
+	get_leds_devices(udevice, data->syspaths);
+
 	return data;
 
 err:
-- 
2.0.0.rc0

Re: [PATCH BlueZ 5/5] plugins/sixaxis: add a set_leds_sysfs() function

[Szymon Janc]

Hi Antonio,

On Tuesday 06 May 2014 12:06:16 Antonio Ospite wrote:
> On recent kernels the hid-sony driver exposes leds class entries in
> sysfs for setting the Sixaxis LEDs, use this interface and fall back to
> hidraw in case using sysfs fails (e.g. on older hid-sony versions).
> 
> Setting the LEDs via sysfs is the preferred way on newer kernels, the
> rationale behind that is:
> 
>   1. the Sixaxis uses the same HID output report for setting both LEDs
>      and rumble effects;
>   2. hid-sony remembers the state of LEDs in order to preserve them when
>      setting rumble effects;
>   3. when the LEDs are set via hidraw hid-sony has no way to know the
>      new LEDs state and thus can change the LEDs in an inconsistent way
>      when setting rumble effects later.
> ---
>  plugins/sixaxis.c | 77
> ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73
> insertions(+), 4 deletions(-)
> 
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index 2b35d1c..446d499 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -185,10 +185,40 @@ static gboolean set_leds_hidraw(int fd, uint8_t
> leds_bitmap) return FALSE;
>  }
> 
> +static gboolean set_leds_sysfs(struct leds_data *data)

Make it return bool.

> +{
> +	int i;
> +
> +	for (i = 0; i < 4; i++) {
> +		char path[PATH_MAX] = { 0 };
> +		char buf[2] = { 0 };
> +		int fd;
> +		int ret;
> +
> +		if (data->syspaths[i] == NULL)

if (!data->..)

> +			return FALSE;
> +
> +		snprintf(path, PATH_MAX, "%s/brightness", data->syspaths[i]);
> +		fd = open(path, O_WRONLY);
> +		if (fd < 0) {
> +			error("Cannot open %s (%s)", path, strerror(errno));

Prefix error message with "sixaxis:"

> +			return FALSE;
> +		}
> +
> +		/* i+1 because leds start from 1, LED0 is never used */
> +		buf[0] = '0' + !!(data->bitmap & (1 << (i+1)));

Spaces around + in i+1.

> +		ret = write(fd, buf, sizeof(buf));
> +		close(fd);
> +		if (ret != sizeof(buf))
> +			return FALSE;
> +	}
> +
> +	return TRUE;
> +}
> +
>  static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
>  							gpointer user_data)
>  {
> -	int fd;
>  	int i;
>  	int ret;
>  	struct leds_data *data = (struct leds_data *)user_data;
> @@ -199,9 +229,11 @@ static gboolean setup_leds(GIOChannel *channel,
> GIOCondition cond, if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
>  		goto out;
> 
> -	fd = g_io_channel_unix_get_fd(channel);
> -
> -	set_leds_hidraw(fd, data->bitmap);
> +	ret = set_leds_sysfs(data);
> +	if (ret == FALSE) {

if (!set_leds_sysfs())

> +		int fd = g_io_channel_unix_get_fd(channel);
> +		set_leds_hidraw(fd, data->bitmap);
> +	}
> 
>  out:
>  	for (i = 0; i < 4; i++)
> @@ -324,6 +356,39 @@ static int get_js_number(struct udev_device *udevice)
>  	return number;
>  }
> 
> +static int get_leds_devices(struct udev_device *udevice, char *paths[4])
> +{
> +	struct udev_list_entry *devices, *dev_list_entry;
> +	struct udev_enumerate *enumerate;
> +	struct udev_device *hid_parent;
> +	int index = 0;
> +	int ret;
> +
> +	hid_parent = udev_device_get_parent_with_subsystem_devtype(udevice,
> +								"hid", NULL);
> +
> +	enumerate = udev_enumerate_new(udev_device_get_udev(udevice));
> +	udev_enumerate_add_match_parent(enumerate, hid_parent);
> +	udev_enumerate_add_match_subsystem(enumerate, "leds");
> +	udev_enumerate_scan_devices(enumerate);
> +	devices = udev_enumerate_get_list_entry(enumerate);
> +
> +	if (devices == NULL) {

if (!devices)..

Also this check should follow devices assignment without extra empty line.

> +		ret = FALSE;
> +		goto out;
> +	}
> +
> +	udev_list_entry_foreach(dev_list_entry, devices) {
> +		const char *syspath = udev_list_entry_get_name(dev_list_entry);
> +		paths[index++] = strdup(syspath);
> +	}

Is this guaranteed that this loop will iterate only 4 times? If yes please add 
comment, if not break loop to avoid writing on random memory.

> +
> +	ret = TRUE;
> +out:
> +	udev_enumerate_unref(enumerate);
> +	return ret;
> +}
> +
>  static struct leds_data *get_leds_data(struct udev_device *udevice)
>  {
>  	struct leds_data *data;
> @@ -344,6 +409,10 @@ static struct leds_data *get_leds_data(struct
> udev_device *udevice) if (ret == FALSE)
>  		goto err;
> 
> +	/* it's OK if this fails, set_leds_hidraw() will be used if
> +	 * any of the values in data->syspaths is not defined */

Multi line comment should be like that: (there is still some inconsistency 
about that in BlueZ code, but lets keep new code right)

/*
 * foo
 * bar
 */

> +	get_leds_devices(udevice, data->syspaths);

Since return code is not important in this function why not just make it 
return void? 

> +
>  	return data;
> 
>  err:

-- 
Szymon K. Janc
szymon.janc@gmail.com

[PATCHv2 BlueZ 0/5] plugin/sixaxis: Set leds using the sysfs leds class

[Antonio Ospite]

Hi,

here's the updated patch series, the commit messages have been fixed too!

I am leaving out patch 1/5 for now:

  plugins/sixaxis: Simplify get_js_numbers()

I decided to do so because there is still some timing issue: the new
procedure to retrieve the js number is faster, and on the _very_first_
run it fails and returns 0, I suspect this has to do with the fact that
the joydev module gets loaded after the hidraw uevent and the
enumeration in the plugin completes before the js node shows up in
sysfs, I'll do some more experiments on that.

So please apply only patches from 2 to 5, thanks, I kept the numbering
as in v1 for clarity.

Changes since v1 are annotated in each patch.

The changes have been tested with libudev-204 along with linux-3.13,
linux 3.14 and linux-3.15-rc4. 

To recap:
  - on kernels < 3.14 the hidraw mechanism is used
  - on kernels >= 3,14 the sysfs mechanism is used to avoid
    inconsistencies with rumble effects.

Thanks,
   Antonio

Antonio Ospite (4):
  plugins/sixaxis: Factor out a set_leds_hidraw() function
  plugins/sixaxis: Factor out a calc_leds_bitmap() function
  plugins/sixaxis: Add a get_leds_data() function
  plugins/sixaxis: Add a set_leds_sysfs() function

 configure.ac      |   4 +-
 plugins/sixaxis.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 158 insertions(+), 24 deletions(-)

-- 
Antonio Ospite
http://ao2.it

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?

[PATCHv2 BlueZ 2/5] plugins/sixaxis: Factor out a set_leds_hidraw() function

[Antonio Ospite]

This is in preparation for a set_leds_sysfs() function.

Make set_leds_hidraw() return void, as its return value is never used
by the caller: the setup_leds() callback has to always return FALSE.
---

Changes since v1:
  - Use capital letter after colon in the short commit message.
  - Make set_leds_hidraw() return void, its return value is never used by the
    caller.

 plugins/sixaxis.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 8045448..3a2cadf 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -125,8 +125,7 @@ static int set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
 	return ret;
 }
 
-static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
-							gpointer user_data)
+static void set_leds_hidraw(int fd, int number)
 {
 	/*
 	 * the total time the led is active (0xff means forever)
@@ -147,18 +146,11 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 		0xff, 0x27, 0x10, 0x00, 0x32, /* LED_1 */
 		0x00, 0x00, 0x00, 0x00, 0x00,
 	};
-	int number = GPOINTER_TO_INT(user_data);
 	int ret;
-	int fd;
-
-	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
-		return FALSE;
-
-	DBG("number %d", number);
 
 	/* TODO we could support up to 10 (1 + 2 + 3 + 4) */
 	if (number > 7)
-		return FALSE;
+		return;
 
 	if (number > 4) {
 		leds_report[10] |= 0x10;
@@ -167,16 +159,29 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 
 	leds_report[10] |= 0x01 << number;
 
-	fd = g_io_channel_unix_get_fd(channel);
-
 	ret = write(fd, leds_report, sizeof(leds_report));
 	if (ret == sizeof(leds_report))
-		return FALSE;
+		return;
 
 	if (ret < 0)
 		error("sixaxis: failed to set LEDS (%s)", strerror(errno));
 	else
 		error("sixaxis: failed to set LEDS (%d bytes written)", ret);
+}
+
+static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
+							gpointer user_data)
+{
+	int number = GPOINTER_TO_INT(user_data);
+	int fd;
+
+	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
+		return FALSE;
+
+	DBG("number %d", number);
+
+	fd = g_io_channel_unix_get_fd(channel);
+	set_leds_hidraw(fd, number);
 
 	return FALSE;
 }
-- 
2.0.0.rc2

[PATCHv2 BlueZ 3/5] plugins/sixaxis: Factor out a calc_leds_bitmap() function

[Antonio Ospite]

This is also in preparation of set_leds_sysfs().
---

Changes since v1:
  - Use capital letter after colon in the short commit message.
  - Make the calc_bitmap() helper simply return the bitmap value and then
    treat 0 as error.

 plugins/sixaxis.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 3a2cadf..1b7bb30 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -125,7 +125,25 @@ static int set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
 	return ret;
 }
 
-static void set_leds_hidraw(int fd, int number)
+static uint8_t calc_leds_bitmap(int number)
+{
+	uint8_t bitmap = 0;
+
+	/* TODO we could support up to 10 (1 + 2 + 3 + 4) */
+	if (number > 7)
+		return bitmap;
+
+	if (number > 4) {
+		bitmap |= 0x10;
+		number -= 4;
+	}
+
+	bitmap |= 0x01 << number;
+
+	return bitmap;
+}
+
+static void set_leds_hidraw(int fd, uint8_t leds_bitmap)
 {
 	/*
 	 * the total time the led is active (0xff means forever)
@@ -148,16 +166,7 @@ static void set_leds_hidraw(int fd, int number)
 	};
 	int ret;
 
-	/* TODO we could support up to 10 (1 + 2 + 3 + 4) */
-	if (number > 7)
-		return;
-
-	if (number > 4) {
-		leds_report[10] |= 0x10;
-		number -= 4;
-	}
-
-	leds_report[10] |= 0x01 << number;
+	leds_report[10] = leds_bitmap;
 
 	ret = write(fd, leds_report, sizeof(leds_report));
 	if (ret == sizeof(leds_report))
@@ -173,6 +182,7 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
 	int number = GPOINTER_TO_INT(user_data);
+	uint8_t bitmap;
 	int fd;
 
 	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
@@ -181,7 +191,10 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 	DBG("number %d", number);
 
 	fd = g_io_channel_unix_get_fd(channel);
-	set_leds_hidraw(fd, number);
+
+	bitmap = calc_leds_bitmap(number);
+	if (bitmap != 0)
+		set_leds_hidraw(fd, bitmap);
 
 	return FALSE;
 }
-- 
2.0.0.rc2

[PATCHv2 BlueZ 4/5] plugins/sixaxis: Add a get_leds_data() function

[Antonio Ospite]

Get all the data necessary to set the LEDs (bitmap and/or sysfs path
prefix) in a single function, returning a leds_data structure to be
passed as argument to the setup_leds() callback.

For now only the bitmap field is populated, which is the only thing that
set_leds_hidraw() needs.
---

Changes since v1:
  - Use capital letter after colon in the short commit message.
  - Add a leds_data_destroy() helper.
  - Use implicit NULL checks for pointers.
  - Use malloc0() from src/shared/utils.h.
  - Add empty line before the return statement in get_leds_data().
  - Remove casting on a void pointer.
  - Instead of 4 strings for the sysfs paths use a sigle string containing the
    common prefix of the sysfs paths of LEDs devices (i.e. the path omitting
    the LED number)

 plugins/sixaxis.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 8 deletions(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 1b7bb30..56110a4 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -44,6 +44,7 @@
 #include "src/device.h"
 #include "src/plugin.h"
 #include "src/log.h"
+#include "src/shared/util.h"
 
 static const struct {
 	const char *name;
@@ -61,6 +62,20 @@ static const struct {
 	},
 };
 
+struct leds_data {
+	char *syspath_prefix;
+	uint8_t bitmap;
+};
+
+static void leds_data_destroy(struct leds_data **data)
+{
+	free((*data)->syspath_prefix);
+	(*data)->syspath_prefix = NULL;
+
+	free(*data);
+	*data = NULL;
+}
+
 static struct udev *ctx = NULL;
 static struct udev_monitor *monitor = NULL;
 static guint watch_id = 0;
@@ -181,20 +196,21 @@ static void set_leds_hidraw(int fd, uint8_t leds_bitmap)
 static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
-	int number = GPOINTER_TO_INT(user_data);
-	uint8_t bitmap;
 	int fd;
+	struct leds_data *data = user_data;
 
-	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
+	if (!data)
 		return FALSE;
 
-	DBG("number %d", number);
+	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
+		goto out;
 
 	fd = g_io_channel_unix_get_fd(channel);
 
-	bitmap = calc_leds_bitmap(number);
-	if (bitmap != 0)
-		set_leds_hidraw(fd, bitmap);
+	set_leds_hidraw(fd, data->bitmap);
+
+out:
+	leds_data_destroy(&data);
 
 	return FALSE;
 }
@@ -314,6 +330,30 @@ next:
 	return number;
 }
 
+static struct leds_data *get_leds_data(struct udev_device *udevice)
+{
+	struct leds_data *data;
+	int number;
+
+	data = malloc0(sizeof(*data));
+	if (!data)
+		return NULL;
+
+	number = get_js_number(udevice);
+	DBG("number %d", number);
+
+	data->bitmap = calc_leds_bitmap(number);
+	if (data->bitmap == 0)
+		goto err;
+
+	return data;
+
+err:
+	leds_data_destroy(&data);
+
+	return NULL;
+}
+
 static int get_supported_device(struct udev_device *udevice, uint16_t *bus)
 {
 	struct udev_device *hid_parent;
@@ -375,7 +415,7 @@ static void device_added(struct udev_device *udevice)
 		/* wait for events before setting leds */
 		g_io_add_watch(io, G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
 				setup_leds,
-				GINT_TO_POINTER(get_js_number(udevice)));
+				get_leds_data(udevice));
 
 		break;
 	default:
-- 
2.0.0.rc2

Re: [PATCHv2 BlueZ 4/5] plugins/sixaxis: Add a get_leds_data() function

[Szymon Janc]

Hi Antonio,

On Wednesday 14 May 2014 23:40:04 Antonio Ospite wrote:
> Get all the data necessary to set the LEDs (bitmap and/or sysfs path
> prefix) in a single function, returning a leds_data structure to be
> passed as argument to the setup_leds() callback.
> 
> For now only the bitmap field is populated, which is the only thing that
> set_leds_hidraw() needs.
> ---
> 
> Changes since v1:
>   - Use capital letter after colon in the short commit message.
>   - Add a leds_data_destroy() helper.
>   - Use implicit NULL checks for pointers.
>   - Use malloc0() from src/shared/utils.h.
>   - Add empty line before the return statement in get_leds_data().
>   - Remove casting on a void pointer.
>   - Instead of 4 strings for the sysfs paths use a sigle string containing
> the common prefix of the sysfs paths of LEDs devices (i.e. the path
> omitting the LED number)
> 
>  plugins/sixaxis.c | 56
> +++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 48
> insertions(+), 8 deletions(-)
> 
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index 1b7bb30..56110a4 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -44,6 +44,7 @@
>  #include "src/device.h"
>  #include "src/plugin.h"
>  #include "src/log.h"
> +#include "src/shared/util.h"
> 
>  static const struct {
>  	const char *name;
> @@ -61,6 +62,20 @@ static const struct {
>  	},
>  };
> 
> +struct leds_data {
> +	char *syspath_prefix;

Just add this in patch where it is first used. So here struct leds_data will 
have only bitmap.

> +	uint8_t bitmap;
> +};
> +
> +static void leds_data_destroy(struct leds_data **data)
> +{
> +	free((*data)->syspath_prefix);
> +	(*data)->syspath_prefix = NULL;

No need to NULL this since data is freed line below.

> +
> +	free(*data);
> +	*data = NULL;

Make this function get just pointer, there is no need to NULL data from it.
Initially leds_data_destroy() can just free data, you can add more when 
introducing additional members.

> +}
> +
>  static struct udev *ctx = NULL;
>  static struct udev_monitor *monitor = NULL;
>  static guint watch_id = 0;
> @@ -181,20 +196,21 @@ static void set_leds_hidraw(int fd, uint8_t
> leds_bitmap) static gboolean setup_leds(GIOChannel *channel, GIOCondition
> cond, gpointer user_data)
>  {
> -	int number = GPOINTER_TO_INT(user_data);
> -	uint8_t bitmap;
>  	int fd;
> +	struct leds_data *data = user_data;
> 
> -	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> +	if (!data)
>  		return FALSE;
> 
> -	DBG("number %d", number);
> +	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> +		goto out;
> 
>  	fd = g_io_channel_unix_get_fd(channel);
> 
> -	bitmap = calc_leds_bitmap(number);
> -	if (bitmap != 0)
> -		set_leds_hidraw(fd, bitmap);
> +	set_leds_hidraw(fd, data->bitmap);
> +
> +out:
> +	leds_data_destroy(&data);
> 
>  	return FALSE;
>  }
> @@ -314,6 +330,30 @@ next:
>  	return number;
>  }
> 
> +static struct leds_data *get_leds_data(struct udev_device *udevice)
> +{
> +	struct leds_data *data;
> +	int number;
> +
> +	data = malloc0(sizeof(*data));
> +	if (!data)
> +		return NULL;

You can allocate data after calling calc_leds_bitmap(), function will be a bit 
simpler.

> +
> +	number = get_js_number(udevice);
> +	DBG("number %d", number);
> +
> +	data->bitmap = calc_leds_bitmap(number);
> +	if (data->bitmap == 0)
> +		goto err;
> +
> +	return data;
> +
> +err:
> +	leds_data_destroy(&data);
> +
> +	return NULL;
> +}
> +
>  static int get_supported_device(struct udev_device *udevice, uint16_t *bus)
> {
>  	struct udev_device *hid_parent;
> @@ -375,7 +415,7 @@ static void device_added(struct udev_device *udevice)
>  		/* wait for events before setting leds */
>  		g_io_add_watch(io, G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
>  				setup_leds,
> -				GINT_TO_POINTER(get_js_number(udevice)));
> +				get_leds_data(udevice));

This could fit same line as setup_leds.

> 
>  		break;
>  	default:

-- 
Szymon K. Janc
szymon.janc@gmail.com

Re: [PATCHv2 BlueZ 4/5] plugins/sixaxis: Add a get_leds_data() function

[Antonio Ospite]

On Thu, 15 May 2014 22:25:02 +0200
Szymon Janc <szymon.janc@gmail.com> wrote:

> Hi Antonio,
> 
> On Wednesday 14 May 2014 23:40:04 Antonio Ospite wrote:
> > Get all the data necessary to set the LEDs (bitmap and/or sysfs path
> > prefix) in a single function, returning a leds_data structure to be
> > passed as argument to the setup_leds() callback.
> > 
> > For now only the bitmap field is populated, which is the only thing that
> > set_leds_hidraw() needs.
> > ---
> > 
> > Changes since v1:
> >   - Use capital letter after colon in the short commit message.
> >   - Add a leds_data_destroy() helper.
> >   - Use implicit NULL checks for pointers.
> >   - Use malloc0() from src/shared/utils.h.
> >   - Add empty line before the return statement in get_leds_data().
> >   - Remove casting on a void pointer.
> >   - Instead of 4 strings for the sysfs paths use a sigle string containing
> > the common prefix of the sysfs paths of LEDs devices (i.e. the path
> > omitting the LED number)
> > 
> >  plugins/sixaxis.c | 56
> > +++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 48
> > insertions(+), 8 deletions(-)
> > 
> > diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> > index 1b7bb30..56110a4 100644
> > --- a/plugins/sixaxis.c
> > +++ b/plugins/sixaxis.c
> > @@ -44,6 +44,7 @@
> >  #include "src/device.h"
> >  #include "src/plugin.h"
> >  #include "src/log.h"
> > +#include "src/shared/util.h"
> > 
> >  static const struct {
> >  	const char *name;
> > @@ -61,6 +62,20 @@ static const struct {
> >  	},
> >  };
> > 
> > +struct leds_data {
> > +	char *syspath_prefix;
> 
> Just add this in patch where it is first used. So here struct leds_data will 
> have only bitmap.
>

Being this a preparatory patch, I though I'd get away with that :)
OK, I'll fix it in v3.

> > +	uint8_t bitmap;
> > +};
> > +
> > +static void leds_data_destroy(struct leds_data **data)
> > +{
> > +	free((*data)->syspath_prefix);
> > +	(*data)->syspath_prefix = NULL;
> 
> No need to NULL this since data is freed line below.
>

Right.

> > +
> > +	free(*data);
> > +	*data = NULL;
> 
> Make this function get just pointer, there is no need to NULL data from it.
> Initially leds_data_destroy() can just free data, you can add more when 
> introducing additional members.
>

The idea behind NULL the pointer is to make the function idempotent, so
that leds_data_destroy() may be called multiple times and the multiple
free() will be OK. I see this is overkill here, I'll remove that from
v3.

> > +}
> > +
> >  static struct udev *ctx = NULL;
> >  static struct udev_monitor *monitor = NULL;
> >  static guint watch_id = 0;
> > @@ -181,20 +196,21 @@ static void set_leds_hidraw(int fd, uint8_t
> > leds_bitmap) static gboolean setup_leds(GIOChannel *channel, GIOCondition
> > cond, gpointer user_data)
> >  {
> > -	int number = GPOINTER_TO_INT(user_data);
> > -	uint8_t bitmap;
> >  	int fd;
> > +	struct leds_data *data = user_data;
> > 
> > -	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> > +	if (!data)
> >  		return FALSE;
> > 
> > -	DBG("number %d", number);
> > +	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> > +		goto out;
> > 
> >  	fd = g_io_channel_unix_get_fd(channel);
> > 
> > -	bitmap = calc_leds_bitmap(number);
> > -	if (bitmap != 0)
> > -		set_leds_hidraw(fd, bitmap);
> > +	set_leds_hidraw(fd, data->bitmap);
> > +
> > +out:
> > +	leds_data_destroy(&data);
> > 
> >  	return FALSE;
> >  }
> > @@ -314,6 +330,30 @@ next:
> >  	return number;
> >  }
> > 
> > +static struct leds_data *get_leds_data(struct udev_device *udevice)
> > +{
> > +	struct leds_data *data;
> > +	int number;
> > +
> > +	data = malloc0(sizeof(*data));
> > +	if (!data)
> > +		return NULL;
> 
> You can allocate data after calling calc_leds_bitmap(), function will be a bit 
> simpler.
>

calc_leds_bitmap() uses data-> bitmap, but I can move the allocation
after get_js_number(udevice) of course; about the error path I can
avoid the goto here since leds_data_destroy() is called only in one
case.

> > +
> > +	number = get_js_number(udevice);
> > +	DBG("number %d", number);
> > +
> > +	data->bitmap = calc_leds_bitmap(number);
> > +	if (data->bitmap == 0)
> > +		goto err;
> > +
> > +	return data;
> > +
> > +err:
> > +	leds_data_destroy(&data);
> > +
> > +	return NULL;
> > +}
> > +
> >  static int get_supported_device(struct udev_device *udevice, uint16_t *bus)
> > {
> >  	struct udev_device *hid_parent;
> > @@ -375,7 +415,7 @@ static void device_added(struct udev_device *udevice)
> >  		/* wait for events before setting leds */
> >  		g_io_add_watch(io, G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
> >  				setup_leds,
> > -				GINT_TO_POINTER(get_js_number(udevice)));
> > +				get_leds_data(udevice));
> 
> This could fit same line as setup_leds.
>

OK.

> > 
> >  		break;
> >  	default:
> 
> -- 
> Szymon K. Janc
> szymon.janc@gmail.com


-- 
Antonio Ospite
http://ao2.it

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?

[PATCHv2 BlueZ 5/5] plugins/sixaxis: Add a set_leds_sysfs() function

[Antonio Ospite]

On recent kernels the hid-sony driver exposes leds class entries in
sysfs for setting the Sixaxis LEDs, use this interface and fall back to
hidraw in case using sysfs fails (e.g. on older hid-sony versions).

Setting the LEDs via sysfs is the preferred way on newer kernels, the
rationale behind that is:

  1. the Sixaxis uses the same HID output report for setting both LEDs
     and rumble effects;
  2. hid-sony remembers the state of LEDs in order to preserve them when
     setting rumble effects;
  3. when the LEDs are set via hidraw hid-sony has no way to know the
     new LEDs state and thus can change the LEDs in an inconsistent way
     when setting rumble effects later.

Also require libudev >= 172, this is where
udev_enumerate_add_match_parent() has been first introduced.

NOTE: using udev_enumerate_add_match_parent() results in a memory leak
when enumerating child devices, this has been fixed in udev 207; the
commit which fixes the issue is this one:
http://cgit.freedesktop.org/systemd/systemd/commit/?id=51cc07576e119dea6e65478eeba9472979fd0936
---

Changes since v1:
  - Use capital letter after colon in the short commit message.
  - Bump the libudev version here.
  - Make set_leds_sysfs() return a bool.
  - Use implicit NULL checks for pointers.
  - Rename get_leds_devices() to get_leds_syspath_prefix() and make it return
    a char * representing the common prefix for all LEDs sysfs paths.
  - Start the loop from 1 in set_leds_sysfs(), now that we use the syspath
    prefix the loop index can match the LEDs numbers.
  - Check the return value of set_leds_sysfs() directly in the condition, do
    not use a 'ret' variable. Being used to the linux kernel style I don't
    particularly like calling functions in conditions but Szymon said it's OK
    in BlueZ for function retuning boolean values.
  - Fix the style of a multi-line comment.

 configure.ac      |  4 +--
 plugins/sixaxis.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/configure.ac b/configure.ac
index 54a387f..4208ad8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -125,8 +125,8 @@ AM_CONDITIONAL(MONITOR, test "${enable_monitor}" != "no")
 AC_ARG_ENABLE(udev, AC_HELP_STRING([--disable-udev],
 		[disable udev device support]), [enable_udev=${enableval}])
 if (test "${enable_tools}" != "no" && test "${enable_udev}" != "no"); then
-	PKG_CHECK_MODULES(UDEV, libudev >= 143, dummy=yes,
-				AC_MSG_ERROR(libudev >= 143 is required))
+	PKG_CHECK_MODULES(UDEV, libudev >= 172, dummy=yes,
+				AC_MSG_ERROR(libudev >= 172 is required))
 	AC_SUBST(UDEV_CFLAGS)
 	AC_SUBST(UDEV_LIBS)
 	AC_CHECK_LIB(udev, udev_hwdb_new,
diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 56110a4..c6a3078 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -193,10 +193,40 @@ static void set_leds_hidraw(int fd, uint8_t leds_bitmap)
 		error("sixaxis: failed to set LEDS (%d bytes written)", ret);
 }
 
+static bool set_leds_sysfs(struct leds_data *data)
+{
+	int i;
+
+	/* start from 1, LED0 is never used */
+	for (i = 1; i <= 4; i++) {
+		char path[PATH_MAX] = { 0 };
+		char buf[2] = { 0 };
+		int fd;
+		int ret;
+
+		if (!data->syspath_prefix)
+			return FALSE;
+
+		snprintf(path, PATH_MAX, "%s%d/brightness", data->syspath_prefix, i);
+		fd = open(path, O_WRONLY);
+		if (fd < 0) {
+			error("sixaxis: cannot open %s (%s)", path, strerror(errno));
+			return FALSE;
+		}
+
+		buf[0] = '0' + !!(data->bitmap & (1 << i));
+		ret = write(fd, buf, sizeof(buf));
+		close(fd);
+		if (ret != sizeof(buf))
+			return FALSE;
+	}
+
+	return TRUE;
+}
+
 static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
-	int fd;
 	struct leds_data *data = user_data;
 
 	if (!data)
@@ -205,9 +235,10 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
 		goto out;
 
-	fd = g_io_channel_unix_get_fd(channel);
-
-	set_leds_hidraw(fd, data->bitmap);
+	if(!set_leds_sysfs(data)) {
+		int fd = g_io_channel_unix_get_fd(channel);
+		set_leds_hidraw(fd, data->bitmap);
+	}
 
 out:
 	leds_data_destroy(&data);
@@ -330,6 +361,45 @@ next:
 	return number;
 }
 
+static char *get_leds_syspath_prefix(struct udev_device *udevice)
+{
+	struct udev_list_entry *dev_list_entry;
+	struct udev_enumerate *enumerate;
+	struct udev_device *hid_parent;
+	const char *syspath;
+	char *syspath_prefix;
+
+	hid_parent = udev_device_get_parent_with_subsystem_devtype(udevice,
+								"hid", NULL);
+
+	enumerate = udev_enumerate_new(udev_device_get_udev(udevice));
+	udev_enumerate_add_match_parent(enumerate, hid_parent);
+	udev_enumerate_add_match_subsystem(enumerate, "leds");
+	udev_enumerate_scan_devices(enumerate);
+
+	dev_list_entry = udev_enumerate_get_list_entry(enumerate);
+	if (!dev_list_entry) {
+		syspath_prefix = NULL;
+		goto out;
+	}
+
+	syspath = udev_list_entry_get_name(dev_list_entry);
+
+	/* 
+	 * All the sysfs paths of the LEDs have the same structure, just the
+	 * number changes, so strip it and store only the common prefix.
+	 *
+	 * Subtracting 1 here means assuming that the LED number is a single
+	 * digit, this is safe as the kernel driver only exposes 4 LEDs.
+	 */
+	syspath_prefix = strndup(syspath, strlen(syspath) - 1);
+
+out:
+	udev_enumerate_unref(enumerate);
+
+	return syspath_prefix;
+}
+
 static struct leds_data *get_leds_data(struct udev_device *udevice)
 {
 	struct leds_data *data;
@@ -346,6 +416,12 @@ static struct leds_data *get_leds_data(struct udev_device *udevice)
 	if (data->bitmap == 0)
 		goto err;
 
+	/* 
+	 * It's OK if this fails, set_leds_hidraw() will be used in
+	 * case data->syspath_prefix is NULL.
+	 */
+	data->syspath_prefix = get_leds_syspath_prefix(udevice);
+
 	return data;
 
 err:
-- 
2.0.0.rc2

Re: [PATCHv2 BlueZ 5/5] plugins/sixaxis: Add a set_leds_sysfs() function

[Szymon Janc]

Hi Antonio,

On Wednesday 14 May 2014 23:40:05 Antonio Ospite wrote:
> On recent kernels the hid-sony driver exposes leds class entries in
> sysfs for setting the Sixaxis LEDs, use this interface and fall back to
> hidraw in case using sysfs fails (e.g. on older hid-sony versions).
> 
> Setting the LEDs via sysfs is the preferred way on newer kernels, the
> rationale behind that is:
> 
>   1. the Sixaxis uses the same HID output report for setting both LEDs
>      and rumble effects;
>   2. hid-sony remembers the state of LEDs in order to preserve them when
>      setting rumble effects;
>   3. when the LEDs are set via hidraw hid-sony has no way to know the
>      new LEDs state and thus can change the LEDs in an inconsistent way
>      when setting rumble effects later.
> 
> Also require libudev >= 172, this is where
> udev_enumerate_add_match_parent() has been first introduced.
> 
> NOTE: using udev_enumerate_add_match_parent() results in a memory leak
> when enumerating child devices, this has been fixed in udev 207; the
> commit which fixes the issue is this one:
> http://cgit.freedesktop.org/systemd/systemd/commit/?id=51cc07576e119dea6e654
> 78eeba9472979fd0936 ---
> 
> Changes since v1:
>   - Use capital letter after colon in the short commit message.
>   - Bump the libudev version here.
>   - Make set_leds_sysfs() return a bool.
>   - Use implicit NULL checks for pointers.
>   - Rename get_leds_devices() to get_leds_syspath_prefix() and make it
> return a char * representing the common prefix for all LEDs sysfs paths. -
> Start the loop from 1 in set_leds_sysfs(), now that we use the syspath
> prefix the loop index can match the LEDs numbers.
>   - Check the return value of set_leds_sysfs() directly in the condition, do
> not use a 'ret' variable. Being used to the linux kernel style I don't
> particularly like calling functions in conditions but Szymon said it's OK
> in BlueZ for function retuning boolean values.
>   - Fix the style of a multi-line comment.
> 
>  configure.ac      |  4 +--
>  plugins/sixaxis.c | 84
> ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 82
> insertions(+), 6 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 54a387f..4208ad8 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -125,8 +125,8 @@ AM_CONDITIONAL(MONITOR, test "${enable_monitor}" !=
> "no") AC_ARG_ENABLE(udev, AC_HELP_STRING([--disable-udev],
>  		[disable udev device support]), [enable_udev=${enableval}])
>  if (test "${enable_tools}" != "no" && test "${enable_udev}" != "no"); then
> -	PKG_CHECK_MODULES(UDEV, libudev >= 143, dummy=yes,
> -				AC_MSG_ERROR(libudev >= 143 is required))
> +	PKG_CHECK_MODULES(UDEV, libudev >= 172, dummy=yes,
> +				AC_MSG_ERROR(libudev >= 172 is required))
>  	AC_SUBST(UDEV_CFLAGS)
>  	AC_SUBST(UDEV_LIBS)
>  	AC_CHECK_LIB(udev, udev_hwdb_new,
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index 56110a4..c6a3078 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -193,10 +193,40 @@ static void set_leds_hidraw(int fd, uint8_t
> leds_bitmap) error("sixaxis: failed to set LEDS (%d bytes written)", ret);
>  }
> 
> +static bool set_leds_sysfs(struct leds_data *data)
> +{
> +	int i;
> +
> +	/* start from 1, LED0 is never used */
> +	for (i = 1; i <= 4; i++) {
> +		char path[PATH_MAX] = { 0 };
> +		char buf[2] = { 0 };
> +		int fd;
> +		int ret;
> +
> +		if (!data->syspath_prefix)
> +			return FALSE;

Use true/false if using bool type. You can also check this once before loop.

> +
> +		snprintf(path, PATH_MAX, "%s%d/brightness", data->syspath_prefix, i);

This is not fitting 80 chars limit.

> +		fd = open(path, O_WRONLY);
> +		if (fd < 0) {
> +			error("sixaxis: cannot open %s (%s)", path, strerror(errno));
> +			return FALSE;
> +		}
> +
> +		buf[0] = '0' + !!(data->bitmap & (1 << i));
> +		ret = write(fd, buf, sizeof(buf));
> +		close(fd);
> +		if (ret != sizeof(buf))
> +			return FALSE;
> +	}
> +
> +	return TRUE;
> +}
> +
>  static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
>  							gpointer user_data)
>  {
> -	int fd;
>  	struct leds_data *data = user_data;
> 
>  	if (!data)
> @@ -205,9 +235,10 @@ static gboolean setup_leds(GIOChannel *channel,
> GIOCondition cond, if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
>  		goto out;
> 
> -	fd = g_io_channel_unix_get_fd(channel);
> -
> -	set_leds_hidraw(fd, data->bitmap);
> +	if(!set_leds_sysfs(data)) {
> +		int fd = g_io_channel_unix_get_fd(channel);
> +		set_leds_hidraw(fd, data->bitmap);
> +	}
> 
>  out:
>  	leds_data_destroy(&data);
> @@ -330,6 +361,45 @@ next:
>  	return number;
>  }
> 
> +static char *get_leds_syspath_prefix(struct udev_device *udevice)
> +{
> +	struct udev_list_entry *dev_list_entry;
> +	struct udev_enumerate *enumerate;
> +	struct udev_device *hid_parent;
> +	const char *syspath;
> +	char *syspath_prefix;
> +
> +	hid_parent = udev_device_get_parent_with_subsystem_devtype(udevice,
> +								"hid", NULL);
> +
> +	enumerate = udev_enumerate_new(udev_device_get_udev(udevice));
> +	udev_enumerate_add_match_parent(enumerate, hid_parent);
> +	udev_enumerate_add_match_subsystem(enumerate, "leds");
> +	udev_enumerate_scan_devices(enumerate);
> +
> +	dev_list_entry = udev_enumerate_get_list_entry(enumerate);
> +	if (!dev_list_entry) {
> +		syspath_prefix = NULL;
> +		goto out;
> +	}
> +
> +	syspath = udev_list_entry_get_name(dev_list_entry);
> +
> +	/*
> +	 * All the sysfs paths of the LEDs have the same structure, just the
> +	 * number changes, so strip it and store only the common prefix.
> +	 *
> +	 * Subtracting 1 here means assuming that the LED number is a single
> +	 * digit, this is safe as the kernel driver only exposes 4 LEDs.
> +	 */
> +	syspath_prefix = strndup(syspath, strlen(syspath) - 1);
> +
> +out:
> +	udev_enumerate_unref(enumerate);
> +
> +	return syspath_prefix;
> +}
> +
>  static struct leds_data *get_leds_data(struct udev_device *udevice)
>  {
>  	struct leds_data *data;
> @@ -346,6 +416,12 @@ static struct leds_data *get_leds_data(struct
> udev_device *udevice) if (data->bitmap == 0)
>  		goto err;
> 
> +	/*
> +	 * It's OK if this fails, set_leds_hidraw() will be used in
> +	 * case data->syspath_prefix is NULL.
> +	 */
> +	data->syspath_prefix = get_leds_syspath_prefix(udevice);
> +
>  	return data;
> 
>  err:

-- 
Szymon K. Janc
szymon.janc@gmail.com

Re: [PATCHv2 BlueZ 0/5] plugin/sixaxis: Set leds using the sysfs leds class

[Frank Praznik]

The current version of get_js_numbers() won't work correctly on 
Bluetooth gamepads if more than one is connected.  Currently it looks 
for the correct js* device via just the PHYS attribute which works on 
USB, but PHYS is the same value for all Bluetooth devices on a single 
adapter (MAC of the host Bluetooth adapter).  As is, all Bluetooth DS3 
controllers will end up with the LEDs set to the controller number of 
the first js* device on the Bluetooth adapter.  A simple fix would be to 
compare against PHYS and UNIQ when searching for the correct js device 
since UNIQ contains the individual device MAC address.

On 5/14/2014 17:40, Antonio Ospite wrote:
> Hi,
>
> here's the updated patch series, the commit messages have been fixed too!
>
> I am leaving out patch 1/5 for now:
>
>    plugins/sixaxis: Simplify get_js_numbers()
>
> I decided to do so because there is still some timing issue: the new
> procedure to retrieve the js number is faster, and on the _very_first_
> run it fails and returns 0, I suspect this has to do with the fact that
> the joydev module gets loaded after the hidraw uevent and the
> enumeration in the plugin completes before the js node shows up in
> sysfs, I'll do some more experiments on that.
>
> So please apply only patches from 2 to 5, thanks, I kept the numbering
> as in v1 for clarity.
>
> Changes since v1 are annotated in each patch.
>
> The changes have been tested with libudev-204 along with linux-3.13,
> linux 3.14 and linux-3.15-rc4.
>
> To recap:
>    - on kernels < 3.14 the hidraw mechanism is used
>    - on kernels >= 3,14 the sysfs mechanism is used to avoid
>      inconsistencies with rumble effects.
>
> Thanks,
>     Antonio
>
> Antonio Ospite (4):
>    plugins/sixaxis: Factor out a set_leds_hidraw() function
>    plugins/sixaxis: Factor out a calc_leds_bitmap() function
>    plugins/sixaxis: Add a get_leds_data() function
>    plugins/sixaxis: Add a set_leds_sysfs() function
>
>   configure.ac      |   4 +-
>   plugins/sixaxis.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++-------
>   2 files changed, 158 insertions(+), 24 deletions(-)
>

Re: [PATCHv2 BlueZ 0/5] plugin/sixaxis: Set leds using the sysfs leds class

[Antonio Ospite]

On Thu, 15 May 2014 11:28:34 -0400
Frank Praznik <frank.praznik@gmail.com> wrote:

> The current version of get_js_numbers() won't work correctly on 
> Bluetooth gamepads if more than one is connected.  Currently it looks 
> for the correct js* device via just the PHYS attribute which works on 
> USB, but PHYS is the same value for all Bluetooth devices on a single 
> adapter (MAC of the host Bluetooth adapter).  As is, all Bluetooth DS3 
> controllers will end up with the LEDs set to the controller number of 
> the first js* device on the Bluetooth adapter.  A simple fix would be to 
> compare against PHYS and UNIQ when searching for the correct js device 
> since UNIQ contains the individual device MAC address.
>

Thanks Frank, I'll make sure I test with multiple controllers when I
get back on simplifying get_js_numbers(), if that takes too long I
will send a fix to the current version following your suggestion.

The patch 1 I sent in the first round would result in picking the
correct number in the general case but, as I said, timing becomes more
critical the first time the joydev module is inserted.

Maybe we could tell libudev to monitor js devices and then navigate to
the hidraw node, instead of the other way around like it is done now.

Ciao,
   Antonio

> On 5/14/2014 17:40, Antonio Ospite wrote:
> > Hi,
> >
> > here's the updated patch series, the commit messages have been fixed too!
> >
> > I am leaving out patch 1/5 for now:
> >
> >    plugins/sixaxis: Simplify get_js_numbers()
> >
> > I decided to do so because there is still some timing issue: the new
> > procedure to retrieve the js number is faster, and on the _very_first_
> > run it fails and returns 0, I suspect this has to do with the fact that
> > the joydev module gets loaded after the hidraw uevent and the
> > enumeration in the plugin completes before the js node shows up in
> > sysfs, I'll do some more experiments on that.
> >
> > So please apply only patches from 2 to 5, thanks, I kept the numbering
> > as in v1 for clarity.
> >
> > Changes since v1 are annotated in each patch.
> >
> > The changes have been tested with libudev-204 along with linux-3.13,
> > linux 3.14 and linux-3.15-rc4.
> >
> > To recap:
> >    - on kernels < 3.14 the hidraw mechanism is used
> >    - on kernels >= 3,14 the sysfs mechanism is used to avoid
> >      inconsistencies with rumble effects.
> >
> > Thanks,
> >     Antonio
> >
> > Antonio Ospite (4):
> >    plugins/sixaxis: Factor out a set_leds_hidraw() function
> >    plugins/sixaxis: Factor out a calc_leds_bitmap() function
> >    plugins/sixaxis: Add a get_leds_data() function
> >    plugins/sixaxis: Add a set_leds_sysfs() function
> >
> >   configure.ac      |   4 +-
> >   plugins/sixaxis.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++-------
> >   2 files changed, 158 insertions(+), 24 deletions(-)
> >
> 
> 


-- 
Antonio Ospite
http://ao2.it

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?

Re: [PATCHv2 BlueZ 0/5] plugin/sixaxis: Set leds using the sysfs leds class

[Szymon Janc]

Hi Antonio,

On Wednesday 14 of May 2014 23:40:01 Antonio Ospite wrote:
> Hi,
> 
> here's the updated patch series, the commit messages have been fixed too!
> 
> I am leaving out patch 1/5 for now:
> 
>   plugins/sixaxis: Simplify get_js_numbers()
> 
> I decided to do so because there is still some timing issue: the new
> procedure to retrieve the js number is faster, and on the _very_first_
> run it fails and returns 0, I suspect this has to do with the fact that
> the joydev module gets loaded after the hidraw uevent and the
> enumeration in the plugin completes before the js node shows up in
> sysfs, I'll do some more experiments on that.
> 
> So please apply only patches from 2 to 5, thanks, I kept the numbering
> as in v1 for clarity.
> 
> Changes since v1 are annotated in each patch.
> 
> The changes have been tested with libudev-204 along with linux-3.13,
> linux 3.14 and linux-3.15-rc4. 
> 
> To recap:
>   - on kernels < 3.14 the hidraw mechanism is used
>   - on kernels >= 3,14 the sysfs mechanism is used to avoid
>     inconsistencies with rumble effects.
> 
> Thanks,
>    Antonio
> 
> Antonio Ospite (4):
>   plugins/sixaxis: Factor out a set_leds_hidraw() function
>   plugins/sixaxis: Factor out a calc_leds_bitmap() function
>   plugins/sixaxis: Add a get_leds_data() function
>   plugins/sixaxis: Add a set_leds_sysfs() function
> 
>  configure.ac      |   4 +-
>  plugins/sixaxis.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 158 insertions(+), 24 deletions(-)

I've applied patches 2/5 and 3/5 so no need to resend those. Thanks.

-- 
Best regards, 
Szymon Janc

[PATCHv3 BlueZ 0/5] plugin/sixaxis: Set leds using the sysfs leds class

[Antonio Ospite]

Hi,

here's the updated patch series.

Fixes to get_js_numbers() are coming later.

Thanks,
   Antonio


Antonio Ospite (2):
  plugins/sixaxis: Add a get_leds_data() function
  plugins/sixaxis: Add a set_leds_sysfs() function

 configure.ac      |   4 +-
 plugins/sixaxis.c | 132 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 123 insertions(+), 13 deletions(-)

-- 
Antonio Ospite
http://ao2.it

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?

[PATCHv3 BlueZ 4/5] plugins/sixaxis: Add a get_leds_data() function

[Antonio Ospite]

Get all the data necessary to set the LEDs in a single function,
returning a leds_data structure to be passed as argument to the
setup_leds() callback.

For now only a 'bitmap' field is used, which is the only thing that
set_leds_hidraw() needs.
---

Changes since v2:
  - Don't introduce leds_data.syspath_prefix just yet as it is not used so
    far.
  - Make leds_data_destroy() get just pointer, not a pointer to pointer, also
    there is no need to NULL data from it as we have control on where it is
    called: it's not invoked multiple times and hence there is no risk of
    a double free().
  - Move the data allocation in get_leds_data() after get_js_number(udevice)
    and simplify the error path: avoid the goto since leds_data_destroy() is
    called only in one case. 
  - Fix indentation of the arguments of g_io_add_watch(), the last two fit on
    the same line.

Changes since v1:
  - Use capital letter after colon in the short commit message.
  - Add a leds_data_destroy() helper.
  - Use implicit NULL checks for pointers.
  - Use malloc0() from src/shared/utils.h.
  - Add empty line before the return statement in get_leds_data().
  - Remove casting on a void pointer.
  - Instead of 4 strings for the sysfs paths use a sigle string containing the
    common prefix of the sysfs paths of LEDs devices (i.e. the path omitting
    the LED number)

 plugins/sixaxis.c | 49 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 9 deletions(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 1b7bb30..0279e8e 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -44,6 +44,7 @@
 #include "src/device.h"
 #include "src/plugin.h"
 #include "src/log.h"
+#include "src/shared/util.h"
 
 static const struct {
 	const char *name;
@@ -61,6 +62,15 @@ static const struct {
 	},
 };
 
+struct leds_data {
+	uint8_t bitmap;
+};
+
+static void leds_data_destroy(struct leds_data *data)
+{
+	free(data);
+}
+
 static struct udev *ctx = NULL;
 static struct udev_monitor *monitor = NULL;
 static guint watch_id = 0;
@@ -181,20 +191,21 @@ static void set_leds_hidraw(int fd, uint8_t leds_bitmap)
 static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
-	int number = GPOINTER_TO_INT(user_data);
-	uint8_t bitmap;
 	int fd;
+	struct leds_data *data = user_data;
 
-	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
+	if (!data)
 		return FALSE;
 
-	DBG("number %d", number);
+	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
+		goto out;
 
 	fd = g_io_channel_unix_get_fd(channel);
 
-	bitmap = calc_leds_bitmap(number);
-	if (bitmap != 0)
-		set_leds_hidraw(fd, bitmap);
+	set_leds_hidraw(fd, data->bitmap);
+
+out:
+	leds_data_destroy(data);
 
 	return FALSE;
 }
@@ -314,6 +325,27 @@ next:
 	return number;
 }
 
+static struct leds_data *get_leds_data(struct udev_device *udevice)
+{
+	struct leds_data *data;
+	int number;
+
+	number = get_js_number(udevice);
+	DBG("number %d", number);
+
+	data = malloc0(sizeof(*data));
+	if (!data)
+		return NULL;
+
+	data->bitmap = calc_leds_bitmap(number);
+	if (data->bitmap == 0) {
+		leds_data_destroy(data);
+		return NULL;
+	}
+
+	return data;
+}
+
 static int get_supported_device(struct udev_device *udevice, uint16_t *bus)
 {
 	struct udev_device *hid_parent;
@@ -374,8 +406,7 @@ static void device_added(struct udev_device *udevice)
 	case BUS_BLUETOOTH:
 		/* wait for events before setting leds */
 		g_io_add_watch(io, G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
-				setup_leds,
-				GINT_TO_POINTER(get_js_number(udevice)));
+				setup_leds, get_leds_data(udevice));
 
 		break;
 	default:
-- 
2.0.0.rc4

[PATCHv3 BlueZ 5/5] plugins/sixaxis: Add a set_leds_sysfs() function

[Antonio Ospite]

On recent kernels the hid-sony driver exposes leds class entries in
sysfs for setting the Sixaxis LEDs, use this interface and fall back to
hidraw in case using sysfs fails (e.g. on older hid-sony versions).

Setting the LEDs via sysfs is the preferred way on newer kernels, the
rationale behind that is:

  1. the Sixaxis uses the same HID output report for setting both LEDs
     and rumble effects;
  2. hid-sony remembers the state of LEDs in order to preserve them when
     setting rumble effects;
  3. when the LEDs are set via hidraw hid-sony has no way to know the
     new LEDs state and thus can change the LEDs in an inconsistent way
     when setting rumble effects later.

Also require libudev >= 172, this is where
udev_enumerate_add_match_parent() has been first introduced.

NOTE: using udev_enumerate_add_match_parent() results in a memory leak
when enumerating child devices, this has been fixed in udev 207; the
commit which fixes the issue is this one:
http://cgit.freedesktop.org/systemd/systemd/commit/?id=51cc07576e119dea6e65478eeba9472979fd0936
---

Changes since v2:
  - Remove some trailing spaces.
  - Re-introduce leds_data.syspath_prefix here where it's actually used for the
    first time.
  - In set_leds_sysfs() move the check for data->syspath_prefix outside the
    loop as it's loop-invariant.
  - Return true/false instead of TRUE/FALSE in set_leds_sysfs() as its return
    type is bool, not gboolean.
  - Wrap the snprintf() invocation in set_leds_sysfs() to fit the 80 chars
    limit.

Changes since v1:
  - Use capital letter after colon in the short commit message.
  - Make set_leds_sysfs() return a bool.
  - Use implicit NULL checks for pointers.
  - Rename get_leds_devices() to get_leds_syspath_prefix() and make it return
    a char * representing the common prefix for all LEDs sysfs paths.
  - Start the loop from 1 in set_leds_sysfs(), now that we use the syspath
    prefix the loop index can match the LEDs numbers.
  - Check the return value of set_leds_sysfs() directly in the condition, do
    not use a 'ret' variable. Being used to the linux kernel style I don't
    particularly like calling functions in conditions but Szymon said it's OK
    in BlueZ for function retuning boolean values.
  - Fix the style of a multi-line comment.

 configure.ac      |  4 +--
 plugins/sixaxis.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 85 insertions(+), 6 deletions(-)

diff --git a/configure.ac b/configure.ac
index d858ff6..b527530 100644
--- a/configure.ac
+++ b/configure.ac
@@ -125,8 +125,8 @@ AM_CONDITIONAL(MONITOR, test "${enable_monitor}" != "no")
 AC_ARG_ENABLE(udev, AC_HELP_STRING([--disable-udev],
 		[disable udev device support]), [enable_udev=${enableval}])
 if (test "${enable_tools}" != "no" && test "${enable_udev}" != "no"); then
-	PKG_CHECK_MODULES(UDEV, libudev >= 143, dummy=yes,
-				AC_MSG_ERROR(libudev >= 143 is required))
+	PKG_CHECK_MODULES(UDEV, libudev >= 172, dummy=yes,
+				AC_MSG_ERROR(libudev >= 172 is required))
 	AC_SUBST(UDEV_CFLAGS)
 	AC_SUBST(UDEV_LIBS)
 	AC_CHECK_LIB(udev, udev_hwdb_new,
diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 0279e8e..de878d1 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -63,11 +63,13 @@ static const struct {
 };
 
 struct leds_data {
+	char *syspath_prefix;
 	uint8_t bitmap;
 };
 
 static void leds_data_destroy(struct leds_data *data)
 {
+	free(data->syspath_prefix);
 	free(data);
 }
 
@@ -188,10 +190,41 @@ static void set_leds_hidraw(int fd, uint8_t leds_bitmap)
 		error("sixaxis: failed to set LEDS (%d bytes written)", ret);
 }
 
+static bool set_leds_sysfs(struct leds_data *data)
+{
+	int i;
+
+	if (!data->syspath_prefix)
+		return false;
+
+	/* start from 1, LED0 is never used */
+	for (i = 1; i <= 4; i++) {
+		char path[PATH_MAX] = { 0 };
+		char buf[2] = { 0 };
+		int fd;
+		int ret;
+
+		snprintf(path, PATH_MAX, "%s%d/brightness",
+			 data->syspath_prefix, i);
+		fd = open(path, O_WRONLY);
+		if (fd < 0) {
+			error("sixaxis: cannot open %s (%s)", path, strerror(errno));
+			return false;
+		}
+
+		buf[0] = '0' + !!(data->bitmap & (1 << i));
+		ret = write(fd, buf, sizeof(buf));
+		close(fd);
+		if (ret != sizeof(buf))
+			return false;
+	}
+
+	return true;
+}
+
 static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
-	int fd;
 	struct leds_data *data = user_data;
 
 	if (!data)
@@ -200,9 +233,10 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
 	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
 		goto out;
 
-	fd = g_io_channel_unix_get_fd(channel);
-
-	set_leds_hidraw(fd, data->bitmap);
+	if(!set_leds_sysfs(data)) {
+		int fd = g_io_channel_unix_get_fd(channel);
+		set_leds_hidraw(fd, data->bitmap);
+	}
 
 out:
 	leds_data_destroy(data);
@@ -325,6 +359,45 @@ next:
 	return number;
 }
 
+static char *get_leds_syspath_prefix(struct udev_device *udevice)
+{
+	struct udev_list_entry *dev_list_entry;
+	struct udev_enumerate *enumerate;
+	struct udev_device *hid_parent;
+	const char *syspath;
+	char *syspath_prefix;
+
+	hid_parent = udev_device_get_parent_with_subsystem_devtype(udevice,
+								"hid", NULL);
+
+	enumerate = udev_enumerate_new(udev_device_get_udev(udevice));
+	udev_enumerate_add_match_parent(enumerate, hid_parent);
+	udev_enumerate_add_match_subsystem(enumerate, "leds");
+	udev_enumerate_scan_devices(enumerate);
+
+	dev_list_entry = udev_enumerate_get_list_entry(enumerate);
+	if (!dev_list_entry) {
+		syspath_prefix = NULL;
+		goto out;
+	}
+
+	syspath = udev_list_entry_get_name(dev_list_entry);
+
+	/*
+	 * All the sysfs paths of the LEDs have the same structure, just the
+	 * number changes, so strip it and store only the common prefix.
+	 *
+	 * Subtracting 1 here means assuming that the LED number is a single
+	 * digit, this is safe as the kernel driver only exposes 4 LEDs.
+	 */
+	syspath_prefix = strndup(syspath, strlen(syspath) - 1);
+
+out:
+	udev_enumerate_unref(enumerate);
+
+	return syspath_prefix;
+}
+
 static struct leds_data *get_leds_data(struct udev_device *udevice)
 {
 	struct leds_data *data;
@@ -343,6 +416,12 @@ static struct leds_data *get_leds_data(struct udev_device *udevice)
 		return NULL;
 	}
 
+	/*
+	 * It's OK if this fails, set_leds_hidraw() will be used in
+	 * case data->syspath_prefix is NULL.
+	 */
+	data->syspath_prefix = get_leds_syspath_prefix(udevice);
+
 	return data;
 }
 
-- 
2.0.0.rc4

Re: [PATCHv3 BlueZ 0/5] plugin/sixaxis: Set leds using the sysfs leds class

[Szymon Janc]

Hi Antonio,

On Tuesday 27 May 2014 13:25:13 Antonio Ospite wrote:
> Hi,
> 
> here's the updated patch series.
> 
> Fixes to get_js_numbers() are coming later.
> 
> Thanks,
>    Antonio
> 
> 
> Antonio Ospite (2):
>   plugins/sixaxis: Add a get_leds_data() function
>   plugins/sixaxis: Add a set_leds_sysfs() function
> 
>  configure.ac      |   4 +-
>  plugins/sixaxis.c | 132
> +++++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 123
> insertions(+), 13 deletions(-)

Both patches applied. Thanks.

-- 
Szymon K. Janc
szymon.janc@gmail.com