2014-05-06 10:06:11

by Antonio Ospite

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

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?

2014-05-27 11:25:15

by Antonio Ospite

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

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

2014-05-27 11:25:13

by Antonio Ospite

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

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?

2014-05-27 11:25:14

by Antonio Ospite

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

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

2014-05-16 12:37:46

by Szymon Janc

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

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

2014-05-16 08:13:14

by Antonio Ospite

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

On Thu, 15 May 2014 22:25:02 +0200
Szymon Janc <[email protected]> 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
> [email protected]


--
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?

2014-05-15 20:25:04

by Szymon Janc

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

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
[email protected]

2014-05-15 20:25:02

by Szymon Janc

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

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
[email protected]

2014-05-15 16:33:37

by Antonio Ospite

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

On Thu, 15 May 2014 11:28:34 -0400
Frank Praznik <[email protected]> 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?

2014-05-15 15:28:34

by Frank Praznik

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

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(-)
>

2014-05-14 21:40:01

by Antonio Ospite

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

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?

2014-05-14 21:40:05

by Antonio Ospite

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

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

2014-05-14 21:40:04

by Antonio Ospite

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

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

2014-05-14 21:40:02

by Antonio Ospite

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

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

2014-05-14 21:40:03

by Antonio Ospite

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

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

2014-05-09 13:31:42

by Antonio Ospite

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

On Fri, 9 May 2014 15:22:32 +0300
Johan Hedberg <[email protected]> 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?

2014-05-09 12:22:32

by Johan Hedberg

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

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

2014-05-07 20:24:41

by Szymon Janc

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

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
[email protected]

2014-05-07 20:04:42

by Szymon Janc

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

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
[email protected]

2014-05-07 19:59:02

by Szymon Janc

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

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
[email protected]

2014-05-07 19:57:49

by Szymon Janc

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

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
[email protected]

2014-05-07 19:58:09

by Szymon Janc

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

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
[email protected]

2014-05-06 10:06:14

by Antonio Ospite

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

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

2014-05-06 10:06:12

by Antonio Ospite

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

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

2014-05-06 10:06:15

by Antonio Ospite

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

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

2014-05-06 10:06:13

by Antonio Ospite

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

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

2014-05-06 10:06:16

by Antonio Ospite

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

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

2014-06-08 12:58:41

by Szymon Janc

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

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
[email protected]