Hello!
This is a rework of the autopair.c plugin we had at some point in BlueZ 4.
The goal of this patch set is to make the pairing process easier for
devices that have a fixed and dumb pincode (like 0000 or 1234) or that
accept any pincode as long as the same pincode is entered on the device
(keyboards/combos). The goal is:
* Don't ask the user (i.e. the agent) a question if we know the right answer.
This makes the user happier. =)
The covered constraints and scenarios are:
1. If the device has a well known pincode we should pair to it without
asking the agent for a pin code (no RequestPincode call). Comon case are
mice and other devices with very limited input.
2. If the device accepts any pincode (as long as it is also entered in the
device), we should provide the agent a random pincode (calling
DisplayPincode) avoiding the unnecessary step of asking the agent for a
[random] pincode (with RequestPincode). Comon case are keyboards/combos.
3. Don't return a failed error because we failed to guess the pincode. Retry
instead until is not our fault (i.e., the device is out of range, the
agent provided a wrong code, etc)
4. If the device accepts only a fixed pincode (and we don't know it) we
should ask the agent for the pincode and be able to pair with the device.
Read this as: don't break any compatibility.
The implemented logic is:
For each new org.bluez.Device1.Pair call that ends in a pin request (not SSP)
we *iterate* the list of pincode callbacks from plugins trying every callback
until it returns 0 (no pincode). For each pincode in this iteration, we try
to use it for the pin request. If it fails with an auth_failed, then we try
again with the next one until we reach the end of the list.
When we reach the end of the list, we try again but this time asking the
agent, thus following the current normal course (i.e. failing if there isn't
an agent registered, etc).
I'm open to hear your comments and look forward to have this patch set in.
If you find a problem or a device that doesn't work with this patch, please
let me know.
Thanks!
Alex Deymo (6):
core: Convert the pincode callback to an interable list.
plugins: Extend the pin code callback with the call number
core: Add support for retrying a bonding
core: retry bonding attempt until the iterator reachs the end.
core: Add device_get_class to the public interface.
autopair: Add the autopair plugin.
Makefile.plugins | 3 ++
plugins/autopair.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++++
plugins/wiimote.c | 7 ++-
src/adapter.c | 125 +++++++++++++++++++++++++++++++++++++--------
src/adapter.h | 10 +++-
src/device.c | 82 ++++++++++++++++++++++++++++--
src/device.h | 5 ++
7 files changed, 350 insertions(+), 27 deletions(-)
create mode 100644 plugins/autopair.c
--
1.8.1.3
Hi all!
I'll address all the comments in this email. First of, keep in mind
the goal and that context is usually dumb devices:
* Don't ask the user (i.e. the agent) a question if we know the right answer.
1) Requiring paring vs non requiring pairing (mice):
This thing of asking the agent implies that we are talking about
paring, i.e. calling org.bluez.Device1.Pair . If you want to pair with
the device, you call Device1.Pair and we do our best to pair with it.
If the device doesn't support that, or you don't know the pincode, we
fail the Device1.Pair call and your free to call Device1.Connect and
have fun there.
2) The pin-code-database.xml
I'm not a fan of having such a database, and that one in particular
doesn't seem to fit very well in our context. Matching with the name
(see next point) and not requiring pairing for mice is something I
don't like. Pairing with a mouse (if supported) is really good because
then it will connect back to your computer next time you turn it on.
Most of the BT mice I have support pairing except two (microsoft 5000
and logitech).
There is a lot of questions in the database approach I don't like:
Do we ship the DB with BlueZ? Do we read the db each time we need a pin?
Do we really want an xml parser in C somehow linked in the plugin?
parsing xml is a lot of code for such a simple pincode db. (Note: we
had an out of bounds read bug in the parsing functions in textfile.c.
Those functions read very very very simple text file formatted like
"key value\n" in a unnecessarily complicated way in C. Do we really
want to parse xml just for this?) If we ship the db with the bluez
code, why don't embed it in the autopair.c file in a sort of logic? It
allow us to have more logic around it.
Anyway, what I want to get done here is the retrying mechanism to
allow the list of plugins. This will allow anyone to write a database
based plugin for this if they want one, and send it together with the
plugin.
3) Use class vs other ids.
When you have your shiny new whatever bluetooth device still smelling
like new, you turn it on, click on "scan for new device" (or your OS
equivalent) and desperately click in "add" (i.e Pair) when you see a
device in the list that matches the whatever class of your shiny new
whatever device... Even before we ask for the name of the device or
the services it exports. (well... maybe we have the name just before
providing the pin). So the only reliable information is the mac
address and the class. Using information that may or may not be
available at that time doesn't look good to me.
Someone said that the class field is variable. That's true... but I
think it's not the case with the dumb devices that are the target of
this patch.
4) Repeated pairing attempts
bluetoothd will handle several pairing attempts for the same
Device1.Pair call, getting the error back.
Szymon mentioned that we have to deal with the 'repeated attempts'
problem. In some way, we deal with it when this patch retries the pair
only after a auth_failed error (it returns an error to Pair if the
underlying error is something else). But yes, I agree we should threat
that error in a different way and add a sort of timeout for the next
attempt. Szymon, do you have any information on these timeout values?
How long should I wait until the device stops reporting a Repeated
attempts error?
5) Devices exiting the pairing mode after wrong pincode
Although the plan is to provide the right pincode for the right
device, we may fail.
Do you have a device that behaves like that? (exits the discovery mode
just after an auth failure)
Resuming, will you agree in the following idea?
The plugin's callback list iterator can span several Pair attempts,
and has the agent (if any) at the end (technically is not in the list,
to allow enable/disable it between Pair calls).
When you call Device1.Pair, a pair is attempted. When (if) the pincode
is requested, the iterator gets the next pincode in the plugin's list
(calling several functions until we get a pincode). If we got a
pincode from a plugin in this iteration we send that to the device. If
we don't have a pincode, then we try the agent (if any) or just return
a negative reply to the pin request if no agent.
If the pin works, we just return successfully the Pair call and reset
the iterator state.
If the pin doesn't work, it could be caused by an auth failed, or any
other error. If it is because an auth failed, we retry the pairing. If
it's something else (like the repeated attempts) we return to the user
with an error in Device1.Pair... BUT... we keep the iterator state for
futures retries: If the error was a repeated attempts and we provided
a pincode, the pincode was wrong and doesn't make sense to retry all
the pincodes from the beginning. If the error was something else (page
timeout, or disconnected right after the connect with a repeated
attempts) and we didn't get an auth_failed for the provided pincode,
we restore the iterator to the previous state, to retry that pincode
next time.
This handles the case of having a loooong list of plugins providing
random codes, since it will allow the agent to provide its pincode at
some point, even after a few Device1.Pair calls.
Comments? Complaints? =)?
Alex.
Hi Scott
On Fri, Mar 22, 2013 at 12:12 AM, Scott James Remnant
<[email protected]> wrote:
> On Wed, Mar 20, 2013 at 9:10 PM, Alex Deymo <[email protected]> wrote:
>
>> + /* Only try the pin code once per device. If it's not correct then it's
>> + * an unknown device. */
>> + if (count > 0)
>> + return 0;
>> +
>
> Semi-interesting fact:
>
> There are actually two possible PINs for a wiimote, depending which
> pairing mode is used. This plugin could be used to iterate between
> them.
No, you cannot. The Wii Remote closes the connection after a failed
attempt (at least some revisions do). And after a reconnect, you
cannot be sure that the old PIN is still wrong.
Hence, the only recommended way to pair Wii Remotes is via the "red
sync" button on the back.
I'm currently trying to find a way to distinguish both connection
methods before pairing, but I haven't succeeded, yet.
Regards
David
On Fri, 2013-03-22 at 09:23 +0100, Bastien Nocera wrote:
> > Again these should be handled similarly to above - informing the
> user
> > that pairing is not required, but that connection is possible
> without
> > pairing.
>
> They actually
Unfinished thought, and I don't remember what I wanted to say. In any
case, the list at the end of the email should clarify all this.
On Thu, 2013-03-21 at 16:07 -0700, Scott James Remnant wrote:
> On Thu, Mar 21, 2013 at 12:49 AM, Bastien Nocera <[email protected]> wrote:
>
> > On Wed, 2013-03-20 at 21:10 -0700, Alex Deymo wrote:
> >> The goal of this patch set is to make the pairing process easier for
> >> devices that have a fixed and dumb pincode (like 0000 or 1234) or that
> >> accept any pincode as long as the same pincode is entered on the device
> >> (keyboards/combos). The goal is:
> >> * Don't ask the user (i.e. the agent) a question if we know the right answer.
> >> This makes the user happier. =)
> >
> > Is there any chance you could use:
> > https://git.gnome.org/browse/gnome-bluetooth/tree/wizard/pin-code-database.xml
> > even it means converting it to a different format?
> >
>
> One weird issue with this database is that it includes entries for
> devices with which pairing is not possible:
It's a device database for gnome-bluetooth's wizard, not something
solely for pairing.
> <device oui="00:19:C1:" name="BD Remote Control" pin="NULL"/>
> <device oui="00:1E:3D:" name="BD Remote Control" pin="NULL"/>
> <device oui="00:06:F5:" name="BD Remote Control" pin="NULL"/>
>
> These could be handled by BlueZ refusing to pair with them
> automatically, but it could be argued that BlueZ should somehow reveal
> to the user that pairing is not possible so it is not offered to begin
> with.
Completely agreed.
> It also includes entries for devices (with no distinguishing from the
> above)
from the above what?
> for devices that should not ordinarily be paired:
>
> <device type="mouse" pin="NULL"/>
Yes. I didn't want those device classes hard-coded in the wizard code
itself.
> Again these should be handled similarly to above - informing the user
> that pairing is not required, but that connection is possible without
> pairing.
They actually
> It seems to confuse the types of devices that use fixed-length random PINs:
>
> <device type="audio" oui="00:1A:80:" name="CMT-DH5BT" pin="max:4"/>
>
> With keyboards, which should be just max:6. Note that BlueZ already
> has the DisplayPinCode agent method precisely to deal with showing a
> PIN - so the issue of help text is strictly an application one.
It's absolutely not a confusion. This device is a speaker box, with
input devices. The device supports a maximum of 4 digits for the PIN but
can enter any digits. So we truncate the maximum number of digits
requested from the device so that pairing is possible.
> <device type="keyboard" pin="KEYBOARD"/>
>
> > This would mean more supported devices out-of-the-box? I'd be happy
> > maintaining the list outside of the bluez tree if the bluez developers
> > don't want to take on the burden of maintaining it.
> >
>
> If the list isn't maintained in the BlueZ tree, then how will more
> devices be supported out-of-the-box?
It can be a separate project that BlueZ (optionally) depends on.
> This, to me, has always seemed like something that should just work
> without requiring any special behavior out of the agent. bluetoothctl
> should be just as capable as a GNOME Wizard.
I completely agree.
The database solves a number of problems, not all of them relevant to
the autopair plugin:
1) separates devices that can be paired, and those that will just be
connected to and marked as trusted (apple mouse vs. normal mouse)
2) gives out the PIN code for particular devices (see the list of GPS
devices in the database)
3) explains how to generate random PINs for particular devices (such as
the CMT-DH5BT above)
4) hints the UI as to how to present the pairing. For non-SSP keyboards,
pressing return is necessary, for the iCade, we want to use joystick
directions instead of digits.
So there's definitely a subset of that database which is useful.
I don't particularly care if you use the same format or not, but using
it would mean that the autopair plugin would work better out-of-the-box
(would you have guessed the non-numeric "NAVMAN" password for the navman
GPS one?).
Cheers
Historical note: the use of classes for this approach is based
unashamedly on the BlueZ Agent inside Android. I figured what worked
for them should work for us ;-)
On Wed, Mar 20, 2013 at 9:10 PM, Alex Deymo <[email protected]> wrote:
> The autopair plugin tries standard pincodes for different devices with dumb
> pincodes. It also generates a random 6 digit pincode for keyboards that
> support any pincode but fallbacks to the agent call in case the random
> generated pincode didn't work.
> ---
> Makefile.plugins | 3 ++
> plugins/autopair.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 148 insertions(+)
> create mode 100644 plugins/autopair.c
>
> diff --git a/Makefile.plugins b/Makefile.plugins
> index f497782..651a970 100644
> --- a/Makefile.plugins
> +++ b/Makefile.plugins
> @@ -5,6 +5,9 @@ builtin_sources += plugins/hostname.c
> builtin_modules += wiimote
> builtin_sources += plugins/wiimote.c
>
> +builtin_modules += autopair
> +builtin_sources += plugins/autopair.c
> +
> if MAINTAINER_MODE
> builtin_modules += gatt_example
> builtin_sources += plugins/gatt-example.c
> diff --git a/plugins/autopair.c b/plugins/autopair.c
> new file mode 100644
> index 0000000..1feb04d
> --- /dev/null
> +++ b/plugins/autopair.c
> @@ -0,0 +1,145 @@
> +/*
> + *
> + * BlueZ - Bluetooth protocol stack for Linux
> + *
> + * Copyright (C) 2012-2013 Google Inc.
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <stdbool.h>
> +#include <stdlib.h>
> +
> +#include <bluetooth/bluetooth.h>
> +#include <glib.h>
> +
> +#include "plugin.h"
> +#include "adapter.h"
> +#include "device.h"
> +#include "log.h"
> +#include "storage.h"
> +
> +/*
> + * Plugin to handle automatic pairing of devices with reduced user
> + * interaction, including implementing the recommendation of the HID spec
> + * for keyboard devices.
> + *
> + * The plugin works by intercepting the PIN request for devices; if the
> + * device is a keyboard a random six-digit numeric PIN is generated and
> + * returned, flagged for displaying using DisplayPinCode.
> + *
> + */
> +
> +static ssize_t autopair_pincb(struct btd_adapter *adapter,
> + struct btd_device *device,
> + char *pinbuf, gboolean *display,
> + uint32_t count)
> +{
> + char addr[18];
> + char pinstr[7];
> + uint32_t class;
> +
> + ba2str(device_get_address(device), addr);
> +
> + class = btd_device_get_class(device);
> +
> + DBG("device %s 0x%x", addr, class);
> +
> + /* This is a class-based pincode guesser. Ignore devices with an unknown
> + * class. */
> + if (class == 0)
> + return 0;
> +
> + switch ((class & 0x1f00) >> 8) {
> + case 0x04: /* Audio/Video */
> + switch ((class & 0xfc) >> 2) {
> + case 0x01: /* Wearable Headset Device */
> + case 0x02: /* Hands-free Device */
> + case 0x06: /* Headphones */
> + case 0x07: /* Portable Audio */
> + case 0x0a: /* HiFi Audio Device */
> + if (count == 0)
> + memcpy(pinbuf, "0000", 4);
> + else if (count == 1)
> + memcpy(pinbuf, "1234", 4);
> + else
> + return 0;
> + return 4;
> + break;
> + }
> + break;
> + case 0x05: /* Peripheral */
> + switch ((class & 0xc0) >> 6) {
> + case 0x01: /* Keyboard */
> + case 0x03: /* Combo keyboard/pointing device */
> + if (count > 0)
> + return 0;
> + srand(time(NULL));
> + snprintf(pinstr, sizeof pinstr, "%06d",
> + rand() % 1000000);
> + *display = TRUE;
> + memcpy(pinbuf, pinstr, 6);
> + return 6;
> + break;
> + case 0x02: /* Pointing device */
> + if (count > 0)
> + return 0;
> + memcpy(pinbuf, "0000", 4);
> + return 4;
> + break;
> + }
> + break;
> + }
> +
> + return 0;
> +}
> +
> +
> +static int autopair_probe(struct btd_adapter *adapter)
> +{
> + btd_adapter_register_pin_cb(adapter, autopair_pincb);
> +
> + return 0;
> +}
> +
> +static void autopair_remove(struct btd_adapter *adapter)
> +{
> + btd_adapter_unregister_pin_cb(adapter, autopair_pincb);
> +}
> +
> +static struct btd_adapter_driver autopair_driver = {
> + .name = "autopair",
> + .probe = autopair_probe,
> + .remove = autopair_remove,
> +};
> +
> +static int autopair_init(void)
> +{
> + return btd_register_adapter_driver(&autopair_driver);
> +}
> +
> +static void autopair_exit(void)
> +{
> + btd_unregister_adapter_driver(&autopair_driver);
> +}
> +
> +BLUETOOTH_PLUGIN_DEFINE(autopair, VERSION,
> + BLUETOOTH_PLUGIN_PRIORITY_DEFAULT, autopair_init, autopair_exit)
> --
> 1.8.1.3
>
On Wed, Mar 20, 2013 at 9:10 PM, Alex Deymo <[email protected]> wrote:
> The autopair plugin tries standard pincodes for different devices with dumb
> pincodes. It also generates a random 6 digit pincode for keyboards that
> support any pincode but fallbacks to the agent call in case the random
> generated pincode didn't work.
> ---
> Makefile.plugins | 3 ++
> plugins/autopair.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 148 insertions(+)
> create mode 100644 plugins/autopair.c
>
> diff --git a/Makefile.plugins b/Makefile.plugins
> index f497782..651a970 100644
> --- a/Makefile.plugins
> +++ b/Makefile.plugins
> @@ -5,6 +5,9 @@ builtin_sources += plugins/hostname.c
> builtin_modules += wiimote
> builtin_sources += plugins/wiimote.c
>
> +builtin_modules += autopair
> +builtin_sources += plugins/autopair.c
> +
> if MAINTAINER_MODE
> builtin_modules += gatt_example
> builtin_sources += plugins/gatt-example.c
> diff --git a/plugins/autopair.c b/plugins/autopair.c
> new file mode 100644
> index 0000000..1feb04d
> --- /dev/null
> +++ b/plugins/autopair.c
> @@ -0,0 +1,145 @@
> +/*
> + *
> + * BlueZ - Bluetooth protocol stack for Linux
> + *
> + * Copyright (C) 2012-2013 Google Inc.
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <stdbool.h>
> +#include <stdlib.h>
> +
> +#include <bluetooth/bluetooth.h>
> +#include <glib.h>
> +
> +#include "plugin.h"
> +#include "adapter.h"
> +#include "device.h"
> +#include "log.h"
> +#include "storage.h"
> +
> +/*
> + * Plugin to handle automatic pairing of devices with reduced user
> + * interaction, including implementing the recommendation of the HID spec
> + * for keyboard devices.
> + *
> + * The plugin works by intercepting the PIN request for devices; if the
> + * device is a keyboard a random six-digit numeric PIN is generated and
> + * returned, flagged for displaying using DisplayPinCode.
> + *
> + */
> +
> +static ssize_t autopair_pincb(struct btd_adapter *adapter,
> + struct btd_device *device,
> + char *pinbuf, gboolean *display,
> + uint32_t count)
> +{
> + char addr[18];
> + char pinstr[7];
> + uint32_t class;
> +
> + ba2str(device_get_address(device), addr);
> +
> + class = btd_device_get_class(device);
> +
> + DBG("device %s 0x%x", addr, class);
> +
> + /* This is a class-based pincode guesser. Ignore devices with an unknown
> + * class. */
> + if (class == 0)
> + return 0;
> +
> + switch ((class & 0x1f00) >> 8) {
> + case 0x04: /* Audio/Video */
> + switch ((class & 0xfc) >> 2) {
> + case 0x01: /* Wearable Headset Device */
> + case 0x02: /* Hands-free Device */
> + case 0x06: /* Headphones */
> + case 0x07: /* Portable Audio */
> + case 0x0a: /* HiFi Audio Device */
> + if (count == 0)
> + memcpy(pinbuf, "0000", 4);
> + else if (count == 1)
> + memcpy(pinbuf, "1234", 4);
> + else
> + return 0;
> + return 4;
> + break;
> + }
> + break;
> + case 0x05: /* Peripheral */
> + switch ((class & 0xc0) >> 6) {
> + case 0x01: /* Keyboard */
> + case 0x03: /* Combo keyboard/pointing device */
> + if (count > 0)
> + return 0;
> + srand(time(NULL));
> + snprintf(pinstr, sizeof pinstr, "%06d",
> + rand() % 1000000);
> + *display = TRUE;
> + memcpy(pinbuf, pinstr, 6);
> + return 6;
> + break;
> + case 0x02: /* Pointing device */
> + if (count > 0)
> + return 0;
> + memcpy(pinbuf, "0000", 4);
> + return 4;
> + break;
> + }
> + break;
> + }
> +
> + return 0;
> +}
> +
> +
> +static int autopair_probe(struct btd_adapter *adapter)
> +{
> + btd_adapter_register_pin_cb(adapter, autopair_pincb);
> +
> + return 0;
> +}
> +
> +static void autopair_remove(struct btd_adapter *adapter)
> +{
> + btd_adapter_unregister_pin_cb(adapter, autopair_pincb);
> +}
> +
> +static struct btd_adapter_driver autopair_driver = {
> + .name = "autopair",
> + .probe = autopair_probe,
> + .remove = autopair_remove,
> +};
> +
> +static int autopair_init(void)
> +{
> + return btd_register_adapter_driver(&autopair_driver);
> +}
> +
> +static void autopair_exit(void)
> +{
> + btd_unregister_adapter_driver(&autopair_driver);
> +}
> +
> +BLUETOOTH_PLUGIN_DEFINE(autopair, VERSION,
> + BLUETOOTH_PLUGIN_PRIORITY_DEFAULT, autopair_init, autopair_exit)
> --
> 1.8.1.3
>
--
Scott James Remnant | Chrome OS Systems | [email protected] | Google
On Wed, Mar 20, 2013 at 9:10 PM, Alex Deymo <[email protected]> wrote:
> + /* Only try the pin code once per device. If it's not correct then it's
> + * an unknown device. */
> + if (count > 0)
> + return 0;
> +
Semi-interesting fact:
There are actually two possible PINs for a wiimote, depending which
pairing mode is used. This plugin could be used to iterate between
them.
Scott
--
Scott James Remnant | Chrome OS Systems | [email protected] | Google
On Wed, Mar 20, 2013 at 9:10 PM, Alex Deymo <[email protected]> wrote:
> The implemented logic is:
> For each new org.bluez.Device1.Pair call that ends in a pin request (not SSP)
> we *iterate* the list of pincode callbacks from plugins trying every callback
> until it returns 0 (no pincode).
>
Does this mean that the user/agent will have to issue repeated Pair
attempts/method calls to successfully pair a device? That seems like a
poor user experience to me, and is different to the patches previously
submitted which would retry inside the Bluetooth daemon.
There's also the risk that the first failed pairing attempt will, if
you drop the connection to the device, cause the device to leave page
scan mode entirely requiring user interaction there as well
(re-pushing the Connect/Pair button, for example).
Scott
--
Scott James Remnant | Chrome OS Systems | [email protected] | Google
On Thu, Mar 21, 2013 at 12:49 AM, Bastien Nocera <[email protected]> wrote:
> On Wed, 2013-03-20 at 21:10 -0700, Alex Deymo wrote:
>> The goal of this patch set is to make the pairing process easier for
>> devices that have a fixed and dumb pincode (like 0000 or 1234) or that
>> accept any pincode as long as the same pincode is entered on the device
>> (keyboards/combos). The goal is:
>> * Don't ask the user (i.e. the agent) a question if we know the right answer.
>> This makes the user happier. =)
>
> Is there any chance you could use:
> https://git.gnome.org/browse/gnome-bluetooth/tree/wizard/pin-code-database.xml
> even it means converting it to a different format?
>
One weird issue with this database is that it includes entries for
devices with which pairing is not possible:
<device oui="00:19:C1:" name="BD Remote Control" pin="NULL"/>
<device oui="00:1E:3D:" name="BD Remote Control" pin="NULL"/>
<device oui="00:06:F5:" name="BD Remote Control" pin="NULL"/>
These could be handled by BlueZ refusing to pair with them
automatically, but it could be argued that BlueZ should somehow reveal
to the user that pairing is not possible so it is not offered to begin
with.
It also includes entries for devices (with no distinguishing from the
above) for devices that should not ordinarily be paired:
<device type="mouse" pin="NULL"/>
Again these should be handled similarly to above - informing the user
that pairing is not required, but that connection is possible without
pairing.
It seems to confuse the types of devices that use fixed-length random PINs:
<device type="audio" oui="00:1A:80:" name="CMT-DH5BT" pin="max:4"/>
With keyboards, which should be just max:6. Note that BlueZ already
has the DisplayPinCode agent method precisely to deal with showing a
PIN - so the issue of help text is strictly an application one.
<device type="keyboard" pin="KEYBOARD"/>
> This would mean more supported devices out-of-the-box? I'd be happy
> maintaining the list outside of the bluez tree if the bluez developers
> don't want to take on the burden of maintaining it.
>
If the list isn't maintained in the BlueZ tree, then how will more
devices be supported out-of-the-box?
This, to me, has always seemed like something that should just work
without requiring any special behavior out of the agent. bluetoothctl
should be just as capable as a GNOME Wizard.
Scott
--
Scott James Remnant | Chrome OS Systems | [email protected] | Google
Hi Again,
On Thu, Mar 21, 2013 at 12:30 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Szymon,
>
> On Thu, Mar 21, 2013 at 6:11 AM, Szymon Janc <[email protected]> wrote:
>> Hi Alex,
>>
>> On Thursday 21 of March 2013 06:10:02 Alex Deymo wrote:
>>> Hello!
>>> This is a rework of the autopair.c plugin we had at some point in BlueZ 4.
>>>
>>> The goal of this patch set is to make the pairing process easier for
>>> devices that have a fixed and dumb pincode (like 0000 or 1234) or that
>>> accept any pincode as long as the same pincode is entered on the device
>>> (keyboards/combos). The goal is:
>>> * Don't ask the user (i.e. the agent) a question if we know the right answer.
>>> This makes the user happier. =)
>>>
>>> The covered constraints and scenarios are:
>>> 1. If the device has a well known pincode we should pair to it without
>>> asking the agent for a pin code (no RequestPincode call). Comon case are
>>> mice and other devices with very limited input.
>>> 2. If the device accepts any pincode (as long as it is also entered in the
>>> device), we should provide the agent a random pincode (calling
>>> DisplayPincode) avoiding the unnecessary step of asking the agent for a
>>> [random] pincode (with RequestPincode). Comon case are keyboards/combos.
>>> 3. Don't return a failed error because we failed to guess the pincode. Retry
>>> instead until is not our fault (i.e., the device is out of range, the
>>> agent provided a wrong code, etc)
>>> 4. If the device accepts only a fixed pincode (and we don't know it) we
>>> should ask the agent for the pincode and be able to pair with the device.
>>> Read this as: don't break any compatibility.
>>>
>>> The implemented logic is:
>>> For each new org.bluez.Device1.Pair call that ends in a pin request (not SSP)
>>> we *iterate* the list of pincode callbacks from plugins trying every callback
>>> until it returns 0 (no pincode). For each pincode in this iteration, we try
>>> to use it for the pin request. If it fails with an auth_failed, then we try
>>> again with the next one until we reach the end of the list.
>>
>> I think 'repeated attempts' error should be covered e.g. retry with current
>> one after some timeout.
>
> We need to be careful here, there are devices that would exist pairing
> mode if pairing fails so the user would have to reset it again to
> reattempt, also we can only attempt to guess if pairing is initiated
> locally otherwise a remote can use this feature to pair without user
> consent.
>
>>> When we reach the end of the list, we try again but this time asking the
>>> agent, thus following the current normal course (i.e. failing if there isn't
>>> an agent registered, etc).
>>>
>>> I'm open to hear your comments and look forward to have this patch set in.
>>> If you find a problem or a device that doesn't work with this patch, please
>>> let me know.
>>
>> I have a vague plan to make neard plugin not limited to pairing only and do
>> other actions depending on device cod/uuids so maybe it would make sense to
>> not use hardcoded magic numbers in autopair plugin and provide some generic
>> code for testing major/minor cod number?
>
> Class is not something I would use to autopair, it is too dynamic/not
> deterministic, IMO we should do this by Device id but it is quite
> possible that none of the legacy device where SSP is not supported
> would support DID.
Beside that we would have to read the DID record before pairing to
make this work, so problem seems to be how to do an educated guess an
not a wild guess based on cod/uuids.
--
Luiz Augusto von Dentz
Hi Szymon,
On Thu, Mar 21, 2013 at 6:11 AM, Szymon Janc <[email protected]> wrote:
> Hi Alex,
>
> On Thursday 21 of March 2013 06:10:02 Alex Deymo wrote:
>> Hello!
>> This is a rework of the autopair.c plugin we had at some point in BlueZ 4.
>>
>> The goal of this patch set is to make the pairing process easier for
>> devices that have a fixed and dumb pincode (like 0000 or 1234) or that
>> accept any pincode as long as the same pincode is entered on the device
>> (keyboards/combos). The goal is:
>> * Don't ask the user (i.e. the agent) a question if we know the right answer.
>> This makes the user happier. =)
>>
>> The covered constraints and scenarios are:
>> 1. If the device has a well known pincode we should pair to it without
>> asking the agent for a pin code (no RequestPincode call). Comon case are
>> mice and other devices with very limited input.
>> 2. If the device accepts any pincode (as long as it is also entered in the
>> device), we should provide the agent a random pincode (calling
>> DisplayPincode) avoiding the unnecessary step of asking the agent for a
>> [random] pincode (with RequestPincode). Comon case are keyboards/combos.
>> 3. Don't return a failed error because we failed to guess the pincode. Retry
>> instead until is not our fault (i.e., the device is out of range, the
>> agent provided a wrong code, etc)
>> 4. If the device accepts only a fixed pincode (and we don't know it) we
>> should ask the agent for the pincode and be able to pair with the device.
>> Read this as: don't break any compatibility.
>>
>> The implemented logic is:
>> For each new org.bluez.Device1.Pair call that ends in a pin request (not SSP)
>> we *iterate* the list of pincode callbacks from plugins trying every callback
>> until it returns 0 (no pincode). For each pincode in this iteration, we try
>> to use it for the pin request. If it fails with an auth_failed, then we try
>> again with the next one until we reach the end of the list.
>
> I think 'repeated attempts' error should be covered e.g. retry with current
> one after some timeout.
We need to be careful here, there are devices that would exist pairing
mode if pairing fails so the user would have to reset it again to
reattempt, also we can only attempt to guess if pairing is initiated
locally otherwise a remote can use this feature to pair without user
consent.
>> When we reach the end of the list, we try again but this time asking the
>> agent, thus following the current normal course (i.e. failing if there isn't
>> an agent registered, etc).
>>
>> I'm open to hear your comments and look forward to have this patch set in.
>> If you find a problem or a device that doesn't work with this patch, please
>> let me know.
>
> I have a vague plan to make neard plugin not limited to pairing only and do
> other actions depending on device cod/uuids so maybe it would make sense to
> not use hardcoded magic numbers in autopair plugin and provide some generic
> code for testing major/minor cod number?
Class is not something I would use to autopair, it is too dynamic/not
deterministic, IMO we should do this by Device id but it is quite
possible that none of the legacy device where SSP is not supported
would support DID.
--
Luiz Augusto von Dentz
Hi Alex,
On Thursday 21 of March 2013 06:10:02 Alex Deymo wrote:
> Hello!
> This is a rework of the autopair.c plugin we had at some point in BlueZ 4.
>
> The goal of this patch set is to make the pairing process easier for
> devices that have a fixed and dumb pincode (like 0000 or 1234) or that
> accept any pincode as long as the same pincode is entered on the device
> (keyboards/combos). The goal is:
> * Don't ask the user (i.e. the agent) a question if we know the right answer.
> This makes the user happier. =)
>
> The covered constraints and scenarios are:
> 1. If the device has a well known pincode we should pair to it without
> asking the agent for a pin code (no RequestPincode call). Comon case are
> mice and other devices with very limited input.
> 2. If the device accepts any pincode (as long as it is also entered in the
> device), we should provide the agent a random pincode (calling
> DisplayPincode) avoiding the unnecessary step of asking the agent for a
> [random] pincode (with RequestPincode). Comon case are keyboards/combos.
> 3. Don't return a failed error because we failed to guess the pincode. Retry
> instead until is not our fault (i.e., the device is out of range, the
> agent provided a wrong code, etc)
> 4. If the device accepts only a fixed pincode (and we don't know it) we
> should ask the agent for the pincode and be able to pair with the device.
> Read this as: don't break any compatibility.
>
> The implemented logic is:
> For each new org.bluez.Device1.Pair call that ends in a pin request (not SSP)
> we *iterate* the list of pincode callbacks from plugins trying every callback
> until it returns 0 (no pincode). For each pincode in this iteration, we try
> to use it for the pin request. If it fails with an auth_failed, then we try
> again with the next one until we reach the end of the list.
I think 'repeated attempts' error should be covered e.g. retry with current
one after some timeout.
> When we reach the end of the list, we try again but this time asking the
> agent, thus following the current normal course (i.e. failing if there isn't
> an agent registered, etc).
>
> I'm open to hear your comments and look forward to have this patch set in.
> If you find a problem or a device that doesn't work with this patch, please
> let me know.
I have a vague plan to make neard plugin not limited to pairing only and do
other actions depending on device cod/uuids so maybe it would make sense to
not use hardcoded magic numbers in autopair plugin and provide some generic
code for testing major/minor cod number?
--
BR
Szymon Janc
Hey Alex,
On Wed, 2013-03-20 at 21:10 -0700, Alex Deymo wrote:
> Hello!
> This is a rework of the autopair.c plugin we had at some point in BlueZ 4.
Happy to see it come back!
> The goal of this patch set is to make the pairing process easier for
> devices that have a fixed and dumb pincode (like 0000 or 1234) or that
> accept any pincode as long as the same pincode is entered on the device
> (keyboards/combos). The goal is:
> * Don't ask the user (i.e. the agent) a question if we know the right answer.
> This makes the user happier. =)
Is there any chance you could use:
https://git.gnome.org/browse/gnome-bluetooth/tree/wizard/pin-code-database.xml
even it means converting it to a different format?
This would mean more supported devices out-of-the-box? I'd be happy
maintaining the list outside of the bluez tree if the bluez developers
don't want to take on the burden of maintaining it.
Cheers
The autopair plugin tries standard pincodes for different devices with dumb
pincodes. It also generates a random 6 digit pincode for keyboards that
support any pincode but fallbacks to the agent call in case the random
generated pincode didn't work.
---
Makefile.plugins | 3 ++
plugins/autopair.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 148 insertions(+)
create mode 100644 plugins/autopair.c
diff --git a/Makefile.plugins b/Makefile.plugins
index f497782..651a970 100644
--- a/Makefile.plugins
+++ b/Makefile.plugins
@@ -5,6 +5,9 @@ builtin_sources += plugins/hostname.c
builtin_modules += wiimote
builtin_sources += plugins/wiimote.c
+builtin_modules += autopair
+builtin_sources += plugins/autopair.c
+
if MAINTAINER_MODE
builtin_modules += gatt_example
builtin_sources += plugins/gatt-example.c
diff --git a/plugins/autopair.c b/plugins/autopair.c
new file mode 100644
index 0000000..1feb04d
--- /dev/null
+++ b/plugins/autopair.c
@@ -0,0 +1,145 @@
+/*
+ *
+ * BlueZ - Bluetooth protocol stack for Linux
+ *
+ * Copyright (C) 2012-2013 Google Inc.
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdbool.h>
+#include <stdlib.h>
+
+#include <bluetooth/bluetooth.h>
+#include <glib.h>
+
+#include "plugin.h"
+#include "adapter.h"
+#include "device.h"
+#include "log.h"
+#include "storage.h"
+
+/*
+ * Plugin to handle automatic pairing of devices with reduced user
+ * interaction, including implementing the recommendation of the HID spec
+ * for keyboard devices.
+ *
+ * The plugin works by intercepting the PIN request for devices; if the
+ * device is a keyboard a random six-digit numeric PIN is generated and
+ * returned, flagged for displaying using DisplayPinCode.
+ *
+ */
+
+static ssize_t autopair_pincb(struct btd_adapter *adapter,
+ struct btd_device *device,
+ char *pinbuf, gboolean *display,
+ uint32_t count)
+{
+ char addr[18];
+ char pinstr[7];
+ uint32_t class;
+
+ ba2str(device_get_address(device), addr);
+
+ class = btd_device_get_class(device);
+
+ DBG("device %s 0x%x", addr, class);
+
+ /* This is a class-based pincode guesser. Ignore devices with an unknown
+ * class. */
+ if (class == 0)
+ return 0;
+
+ switch ((class & 0x1f00) >> 8) {
+ case 0x04: /* Audio/Video */
+ switch ((class & 0xfc) >> 2) {
+ case 0x01: /* Wearable Headset Device */
+ case 0x02: /* Hands-free Device */
+ case 0x06: /* Headphones */
+ case 0x07: /* Portable Audio */
+ case 0x0a: /* HiFi Audio Device */
+ if (count == 0)
+ memcpy(pinbuf, "0000", 4);
+ else if (count == 1)
+ memcpy(pinbuf, "1234", 4);
+ else
+ return 0;
+ return 4;
+ break;
+ }
+ break;
+ case 0x05: /* Peripheral */
+ switch ((class & 0xc0) >> 6) {
+ case 0x01: /* Keyboard */
+ case 0x03: /* Combo keyboard/pointing device */
+ if (count > 0)
+ return 0;
+ srand(time(NULL));
+ snprintf(pinstr, sizeof pinstr, "%06d",
+ rand() % 1000000);
+ *display = TRUE;
+ memcpy(pinbuf, pinstr, 6);
+ return 6;
+ break;
+ case 0x02: /* Pointing device */
+ if (count > 0)
+ return 0;
+ memcpy(pinbuf, "0000", 4);
+ return 4;
+ break;
+ }
+ break;
+ }
+
+ return 0;
+}
+
+
+static int autopair_probe(struct btd_adapter *adapter)
+{
+ btd_adapter_register_pin_cb(adapter, autopair_pincb);
+
+ return 0;
+}
+
+static void autopair_remove(struct btd_adapter *adapter)
+{
+ btd_adapter_unregister_pin_cb(adapter, autopair_pincb);
+}
+
+static struct btd_adapter_driver autopair_driver = {
+ .name = "autopair",
+ .probe = autopair_probe,
+ .remove = autopair_remove,
+};
+
+static int autopair_init(void)
+{
+ return btd_register_adapter_driver(&autopair_driver);
+}
+
+static void autopair_exit(void)
+{
+ btd_unregister_adapter_driver(&autopair_driver);
+}
+
+BLUETOOTH_PLUGIN_DEFINE(autopair, VERSION,
+ BLUETOOTH_PLUGIN_PRIORITY_DEFAULT, autopair_init, autopair_exit)
--
1.8.1.3
Exports the device class to plugins.
---
src/device.c | 5 +++++
src/device.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/src/device.c b/src/device.c
index 224f858..35c8d3e 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2124,6 +2124,11 @@ void device_set_class(struct btd_device *device, uint32_t class)
DEVICE_INTERFACE, "Class");
}
+uint32_t btd_device_get_class(struct btd_device *device)
+{
+ return device->class;
+}
+
uint16_t btd_device_get_vendor(struct btd_device *device)
{
return device->vendor;
diff --git a/src/device.h b/src/device.h
index f81721a..ab243f9 100644
--- a/src/device.h
+++ b/src/device.h
@@ -37,6 +37,7 @@ void device_set_name(struct btd_device *device, const char *name);
void device_get_name(struct btd_device *device, char *name, size_t len);
bool device_name_known(struct btd_device *device);
void device_set_class(struct btd_device *device, uint32_t class);
+uint32_t btd_device_get_class(struct btd_device *device);
uint16_t btd_device_get_vendor(struct btd_device *device);
uint16_t btd_device_get_vendor_src(struct btd_device *device);
uint16_t btd_device_get_product(struct btd_device *device);
--
1.8.1.3
When a bonding fails with an authentication error, retry with the
next function (or next call to the same function) in the pincode callback
list.
---
src/adapter.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++---------
src/adapter.h | 3 +++
src/device.c | 48 ++++++++++++++++++++++++++++++++++-----
src/device.h | 2 ++
4 files changed, 109 insertions(+), 16 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 862f64c..7be7ee3 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -4932,6 +4932,41 @@ static void bonding_complete(struct btd_adapter *adapter,
check_oob_bonding_complete(adapter, bdaddr, status);
}
+static void bonding_attempt_complete(struct btd_adapter *adapter,
+ const bdaddr_t *bdaddr,
+ uint8_t addr_type, uint8_t status)
+{
+ int err = 0;
+ struct btd_device *device;
+ char addr[18];
+
+ ba2str(bdaddr, addr);
+ DBG("hci%u bdaddr %s type %u status 0x%x", adapter->dev_id, addr,
+ addr_type, status);
+
+ if (status == 0)
+ device = adapter_get_device(adapter, bdaddr, addr_type);
+ else
+ device = adapter_find_device(adapter, bdaddr);
+
+ if (status == MGMT_STATUS_AUTH_FAILED) {
+
+ /* On faliure, issue a bonding_retry if possible. */
+ if (device != NULL) {
+ err = device_bonding_attempt_retry(device);
+ if (err == 0)
+ return;
+ }
+ }
+
+ /* Ignore disconnects during retry. */
+ if (status == MGMT_STATUS_DISCONNECTED && device_is_retrying(device))
+ return;
+
+ /* In any other case, finish the bonding. */
+ bonding_complete(adapter, bdaddr, addr_type, status);
+}
+
struct pair_device_data {
struct btd_adapter *adapter;
bdaddr_t bdaddr;
@@ -4985,7 +5020,7 @@ static void pair_device_complete(uint8_t status, uint16_t length,
error("Pair device failed: %s (0x%02x)",
mgmt_errstr(status), status);
- bonding_complete(adapter, &data->bdaddr,
+ bonding_attempt_complete(adapter, &data->bdaddr,
data->addr_type, status);
return;
}
@@ -4995,17 +5030,13 @@ static void pair_device_complete(uint8_t status, uint16_t length,
return;
}
- bonding_complete(adapter, &rp->addr.bdaddr, rp->addr.type, status);
+ bonding_attempt_complete(adapter, &rp->addr.bdaddr, rp->addr.type,
+ status);
}
int adapter_create_bonding(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
uint8_t addr_type, uint8_t io_cap)
{
- struct mgmt_cp_pair_device cp;
- char addr[18];
- struct pair_device_data *data;
- unsigned int id;
-
if (adapter->pair_device_id > 0) {
error("Unable pair since another pairing is in progress");
return -EBUSY;
@@ -5013,6 +5044,17 @@ int adapter_create_bonding(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
suspend_discovery(adapter);
+ return adapter_bonding_attempt(adapter, bdaddr, addr_type, io_cap);
+}
+
+int adapter_bonding_attempt(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
+ uint8_t addr_type, uint8_t io_cap)
+{
+ struct mgmt_cp_pair_device cp;
+ char addr[18];
+ struct pair_device_data *data;
+ unsigned int id;
+
ba2str(bdaddr, addr);
DBG("hci%u bdaddr %s type %d io_cap 0x%02x",
adapter->dev_id, addr, addr_type, io_cap);
@@ -5065,7 +5107,7 @@ static void dev_disconnected(struct btd_adapter *adapter,
if (device)
adapter_remove_connection(adapter, device);
- bonding_complete(adapter, &addr->bdaddr, addr->type,
+ bonding_attempt_complete(adapter, &addr->bdaddr, addr->type,
MGMT_STATUS_DISCONNECTED);
}
@@ -5119,7 +5161,8 @@ static void auth_failed_callback(uint16_t index, uint16_t length,
return;
}
- bonding_complete(adapter, &ev->addr.bdaddr, ev->addr.type, ev->status);
+ bonding_attempt_complete(adapter, &ev->addr.bdaddr, ev->addr.type,
+ ev->status);
}
static void store_link_key(struct btd_adapter *adapter,
@@ -5720,13 +5763,20 @@ static void connect_failed_callback(uint16_t index, uint16_t length,
device = adapter_find_device(adapter, &ev->addr.bdaddr);
if (device) {
if (device_is_bonding(device, NULL))
- device_bonding_failed(device, ev->status);
+ device_cancel_authentication(device, FALSE);
if (device_is_temporary(device))
adapter_remove_device(adapter, device, TRUE);
}
/* In the case of security mode 3 devices */
- bonding_complete(adapter, &ev->addr.bdaddr, ev->addr.type, ev->status);
+ bonding_attempt_complete(adapter, &ev->addr.bdaddr, ev->addr.type,
+ ev->status);
+
+ if (device && device_is_bonding(device, NULL)
+ && !device_is_retrying(device)) {
+ device_cancel_authentication(device, TRUE);
+ device_bonding_failed(device, ev->status);
+ }
}
static void unpaired_callback(uint16_t index, uint16_t length,
diff --git a/src/adapter.h b/src/adapter.h
index ba57b15..1489995 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -178,6 +178,9 @@ int btd_adapter_passkey_reply(struct btd_adapter *adapter,
int adapter_create_bonding(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
uint8_t addr_type, uint8_t io_cap);
+int adapter_bonding_attempt(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
+ uint8_t addr_type, uint8_t io_cap);
+
int adapter_cancel_bonding(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
uint8_t addr_type);
diff --git a/src/device.c b/src/device.c
index 62e4d3a..224f858 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3740,6 +3740,46 @@ gboolean device_is_bonding(struct btd_device *device, const char *sender)
return g_str_equal(sender, dbus_message_get_sender(bonding->msg));
}
+static gboolean device_bonding_retry(gpointer data)
+{
+ struct btd_device *device = data;
+ struct btd_adapter *adapter = device_get_adapter(device);
+ struct bonding_req *bonding = device->bonding;
+ int err;
+
+ if (!bonding)
+ return FALSE;
+
+ DBG("retrying bonding");
+ bonding->retry_timer = 0;
+
+ err = adapter_bonding_attempt(adapter, &device->bdaddr,
+ device->bdaddr_type, bonding->capability);
+ if (err < 0)
+ device_bonding_complete(device, bonding->status);
+
+ return FALSE;
+}
+
+int device_bonding_attempt_retry(struct btd_device *device) {
+ struct bonding_req *bonding = device->bonding;
+
+ /* Ignore other failure events while retrying */
+ if (device_is_retrying(device))
+ return 0;
+
+ if (!bonding)
+ return -EINVAL;
+
+ if (pincb_iter_end(bonding->cb_iter))
+ return -EINVAL;
+
+ DBG("scheduling retry with io_cap %u", bonding->capability);
+ bonding->retry_timer = g_timeout_add(3000,
+ device_bonding_retry, device);
+ return 0;
+}
+
void device_bonding_failed(struct btd_device *device, uint8_t status)
{
struct bonding_req *bonding = device->bonding;
@@ -3750,17 +3790,15 @@ void device_bonding_failed(struct btd_device *device, uint8_t status)
if (!bonding)
return;
- if (device->authr)
- device_cancel_authentication(device, FALSE);
-
reply = new_authentication_return(bonding->msg, status);
g_dbus_send_message(dbus_conn, reply);
bonding_request_free(bonding);
}
-struct pincb_iter *device_bonding_iter(struct btd_device *device) {
- if (device->bonding == NULL)
+struct pincb_iter *device_bonding_iter(struct btd_device *device)
+{
+ if (!device->bonding)
return NULL;
return device->bonding->cb_iter;
diff --git a/src/device.h b/src/device.h
index 61a294b..f81721a 100644
--- a/src/device.h
+++ b/src/device.h
@@ -76,8 +76,10 @@ gboolean device_is_connected(struct btd_device *device);
gboolean device_is_retrying(struct btd_device *device);
void device_bonding_complete(struct btd_device *device, uint8_t status);
gboolean device_is_bonding(struct btd_device *device, const char *sender);
+void device_bonding_attempt_failed(struct btd_device *device, uint8_t status);
void device_bonding_failed(struct btd_device *device, uint8_t status);
struct pincb_iter *device_bonding_iter(struct btd_device *device);
+int device_bonding_attempt_retry(struct btd_device *device);
int device_request_pincode(struct btd_device *device, gboolean secure);
int device_request_passkey(struct btd_device *device);
int device_confirm_passkey(struct btd_device *device, uint32_t passkey,
--
1.8.1.3
In order to retry a bonding we need a timer that will perform the
retry, we need to stash the status and capability of the bonding
request so it can use them again, and in the case of a retrying
bonding attempt we need to not tear down the temporary D-Bus device
object on the adapter.
---
src/adapter.c | 2 +-
src/device.c | 20 ++++++++++++++++++--
src/device.h | 1 +
3 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index adf2e66..862f64c 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -4262,7 +4262,7 @@ static void adapter_remove_connection(struct btd_adapter *adapter,
if (device_is_authenticating(device))
device_cancel_authentication(device, TRUE);
- if (device_is_temporary(device)) {
+ if (device_is_temporary(device) && !device_is_retrying(device)) {
const char *path = device_get_path(device);
DBG("Removing temporary device %s", path);
diff --git a/src/device.c b/src/device.c
index abedb38..62e4d3a 100644
--- a/src/device.c
+++ b/src/device.c
@@ -86,6 +86,9 @@ struct bonding_req {
struct btd_device *device;
struct agent *agent;
struct pincb_iter *cb_iter;
+ uint8_t capability;
+ uint8_t status;
+ guint retry_timer;
};
typedef enum {
@@ -1397,7 +1400,8 @@ static void device_svc_resolved(struct btd_device *dev, int err)
static struct bonding_req *bonding_request_new(DBusMessage *msg,
struct btd_device *device,
- struct agent *agent)
+ struct agent *agent,
+ uint8_t io_cap)
{
struct bonding_req *bonding;
char addr[18];
@@ -1411,6 +1415,8 @@ static struct bonding_req *bonding_request_new(DBusMessage *msg,
bonding->cb_iter = pincb_iter_new(device->adapter);
+ bonding->capability = io_cap;
+
if (agent)
bonding->agent = agent_ref(agent);
@@ -1464,7 +1470,7 @@ static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg,
else
io_cap = IO_CAPABILITY_NOINPUTNOOUTPUT;
- bonding = bonding_request_new(msg, device, agent);
+ bonding = bonding_request_new(msg, device, agent, io_cap);
if (agent)
agent_unref(agent);
@@ -1545,6 +1551,9 @@ static void bonding_request_free(struct bonding_req *bonding)
bonding->agent = NULL;
}
+ if (bonding->retry_timer)
+ g_source_remove(bonding->retry_timer);
+
if (bonding->device)
bonding->device->bonding = NULL;
@@ -3591,6 +3600,13 @@ static void device_auth_req_free(struct btd_device *device)
device->authr = NULL;
}
+gboolean device_is_retrying(struct btd_device *device)
+{
+ struct bonding_req *bonding = device->bonding;
+
+ return bonding && bonding->retry_timer != 0;
+}
+
void device_bonding_complete(struct btd_device *device, uint8_t status)
{
struct bonding_req *bonding = device->bonding;
diff --git a/src/device.h b/src/device.h
index 725bd7a..61a294b 100644
--- a/src/device.h
+++ b/src/device.h
@@ -73,6 +73,7 @@ void device_set_bonded(struct btd_device *device, gboolean bonded);
void device_set_legacy(struct btd_device *device, bool legacy);
void device_set_rssi(struct btd_device *device, int8_t rssi);
gboolean device_is_connected(struct btd_device *device);
+gboolean device_is_retrying(struct btd_device *device);
void device_bonding_complete(struct btd_device *device, uint8_t status);
gboolean device_is_bonding(struct btd_device *device, const char *sender);
void device_bonding_failed(struct btd_device *device, uint8_t status);
--
1.8.1.3
The plugin's pin code callback doesn't know about the pairing process. It
just provides a pin code based on the information provided to this function.
Although limited state could be added trough other new callbacks, this fix
achieves this by providing more information to the callbacks itself. The
new argument "count" states the pin callback attempt of the particular
plugin for the current pairing of the device. This allows a plugin to try
different pincodes for the same device until it returns 0.
---
plugins/wiimote.c | 7 ++++++-
src/adapter.c | 2 +-
src/adapter.h | 3 ++-
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/plugins/wiimote.c b/plugins/wiimote.c
index 90d6d7b..4c514c6 100644
--- a/plugins/wiimote.c
+++ b/plugins/wiimote.c
@@ -70,12 +70,17 @@ static const char *wii_names[] = {
};
static ssize_t wii_pincb(struct btd_adapter *adapter, struct btd_device *device,
- char *pinbuf, gboolean *display)
+ char *pinbuf, gboolean *display, uint32_t count)
{
uint16_t vendor, product;
char addr[18], name[25];
unsigned int i;
+ /* Only try the pin code once per device. If it's not correct then it's
+ * an unknown device. */
+ if (count > 0)
+ return 0;
+
ba2str(device_get_address(device), addr);
vendor = btd_device_get_vendor(device);
diff --git a/src/adapter.c b/src/adapter.c
index b5e49b4..adf2e66 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -4804,7 +4804,7 @@ static ssize_t pincb_iter_next(struct pincb_iter* iter,
while (iter->it != NULL) {
cb = iter->it->data;
- ret = cb(adapter, device, pin_buf, display);
+ ret = cb(adapter, device, pin_buf, display, iter->count);
iter->count++;
if (ret > 0)
return ret;
diff --git a/src/adapter.h b/src/adapter.h
index d158334..ba57b15 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -132,7 +132,8 @@ int btd_cancel_authorization(guint id);
int btd_adapter_restore_powered(struct btd_adapter *adapter);
typedef ssize_t (*btd_adapter_pin_cb_t) (struct btd_adapter *adapter,
- struct btd_device *dev, char *out, gboolean *display);
+ struct btd_device *dev, char *out, gboolean *display,
+ uint32_t count);
void btd_adapter_register_pin_cb(struct btd_adapter *adapter,
btd_adapter_pin_cb_t cb);
void btd_adapter_unregister_pin_cb(struct btd_adapter *adapter,
--
1.8.1.3
---
src/adapter.c | 51 +++++++++++++++++++++++++++++++++++++++++++--------
src/adapter.h | 4 ++++
src/device.c | 13 +++++++++++++
src/device.h | 1 +
4 files changed, 61 insertions(+), 8 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index e553626..b5e49b4 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -121,6 +121,12 @@ struct service_auth {
struct agent *agent; /* NULL for queued auths */
};
+struct pincb_iter {
+ GSList* it; /* current callback function */
+ int count; /* numer of times it() was called */
+ /* When the iterator reachs the end, it is NULL and count is -1 */
+};
+
struct btd_adapter {
int ref_count;
@@ -4771,22 +4777,43 @@ static void user_passkey_notify_callback(uint16_t index, uint16_t length,
error("device_notify_passkey: %s", strerror(-err));
}
-static ssize_t adapter_get_pin(struct btd_adapter *adapter,
- struct btd_device *dev, char *pin_buf,
+struct pincb_iter *pincb_iter_new(struct btd_adapter *adapter) {
+ struct pincb_iter *iter = g_new0(struct pincb_iter, 1);
+
+ iter->it = adapter->pin_callbacks;
+
+ return iter;
+}
+
+void pincb_iter_free(struct pincb_iter *iter) {
+ g_free(iter);
+}
+
+gboolean pincb_iter_end(struct pincb_iter *iter) {
+ return iter->it == NULL && iter->count == -1;
+}
+
+static ssize_t pincb_iter_next(struct pincb_iter* iter,
+ struct btd_adapter *adapter,
+ struct btd_device *device,
+ char *pin_buf,
gboolean *display)
{
btd_adapter_pin_cb_t cb;
ssize_t ret;
- GSList *l;
- for (l = adapter->pin_callbacks; l != NULL; l = g_slist_next(l)) {
- cb = l->data;
- ret = cb(adapter, dev, pin_buf, display);
+ while (iter->it != NULL) {
+ cb = iter->it->data;
+ ret = cb(adapter, device, pin_buf, display);
+ iter->count++;
if (ret > 0)
return ret;
+ iter->count = 0;
+ iter->it = g_slist_next(iter->it);
}
+ iter->count = -1;
- return -1;
+ return 0;
}
static void pin_code_request_callback(uint16_t index, uint16_t length,
@@ -4800,6 +4827,7 @@ static void pin_code_request_callback(uint16_t index, uint16_t length,
ssize_t pinlen;
char addr[18];
int err;
+ struct pincb_iter* iter;
if (length < sizeof(*ev)) {
error("Too small PIN code request event");
@@ -4817,7 +4845,14 @@ static void pin_code_request_callback(uint16_t index, uint16_t length,
}
memset(pin, 0, sizeof(pin));
- pinlen = adapter_get_pin(adapter, device, pin, &display);
+
+ iter = device_bonding_iter(device);
+
+ if (iter == NULL)
+ pinlen = 0;
+ else
+ pinlen = pincb_iter_next(iter, adapter, device, pin, &display);
+
if (pinlen > 0 && (!ev->secure || pinlen == 16)) {
if (display && device_is_bonding(device, NULL)) {
err = device_notify_pincode(device, ev->secure, pin);
diff --git a/src/adapter.h b/src/adapter.h
index 8d23a64..d158334 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -138,6 +138,10 @@ void btd_adapter_register_pin_cb(struct btd_adapter *adapter,
void btd_adapter_unregister_pin_cb(struct btd_adapter *adapter,
btd_adapter_pin_cb_t cb);
+struct pincb_iter *pincb_iter_new(struct btd_adapter *adapter);
+void pincb_iter_free(struct pincb_iter *iter);
+gboolean pincb_iter_end(struct pincb_iter *iter);
+
/* If TRUE, enables fast connectabe, i.e. reduces page scan interval and changes
* type. If FALSE, disables fast connectable, i.e. sets page scan interval and
* type to default values. Valid for both connectable and discoverable modes. */
diff --git a/src/device.c b/src/device.c
index 3cd7f10..abedb38 100644
--- a/src/device.c
+++ b/src/device.c
@@ -85,6 +85,7 @@ struct bonding_req {
guint listener_id;
struct btd_device *device;
struct agent *agent;
+ struct pincb_iter *cb_iter;
};
typedef enum {
@@ -1408,6 +1409,8 @@ static struct bonding_req *bonding_request_new(DBusMessage *msg,
bonding->msg = dbus_message_ref(msg);
+ bonding->cb_iter = pincb_iter_new(device->adapter);
+
if (agent)
bonding->agent = agent_ref(agent);
@@ -1533,6 +1536,9 @@ static void bonding_request_free(struct bonding_req *bonding)
if (bonding->msg)
dbus_message_unref(bonding->msg);
+ if (bonding->cb_iter)
+ g_free(bonding->cb_iter);
+
if (bonding->agent) {
agent_cancel(bonding->agent);
agent_unref(bonding->agent);
@@ -3737,6 +3743,13 @@ void device_bonding_failed(struct btd_device *device, uint8_t status)
bonding_request_free(bonding);
}
+struct pincb_iter *device_bonding_iter(struct btd_device *device) {
+ if (device->bonding == NULL)
+ return NULL;
+
+ return device->bonding->cb_iter;
+}
+
static void pincode_cb(struct agent *agent, DBusError *err, const char *pin,
void *data)
{
diff --git a/src/device.h b/src/device.h
index d072015..725bd7a 100644
--- a/src/device.h
+++ b/src/device.h
@@ -76,6 +76,7 @@ gboolean device_is_connected(struct btd_device *device);
void device_bonding_complete(struct btd_device *device, uint8_t status);
gboolean device_is_bonding(struct btd_device *device, const char *sender);
void device_bonding_failed(struct btd_device *device, uint8_t status);
+struct pincb_iter *device_bonding_iter(struct btd_device *device);
int device_request_pincode(struct btd_device *device, gboolean secure);
int device_request_passkey(struct btd_device *device);
int device_confirm_passkey(struct btd_device *device, uint32_t passkey,
--
1.8.1.3
On Wed, Apr 10, 2013 at 10:11 PM, Marcel Holtmann <[email protected]> wrote:
> as mentioned earlier, I do not like the count naming. Here it makes it even worse.
Ok. Got it ;)
> And what I would be doing is creating a string array { "0000", "1234" } that just can be walked and not this hardcoded if statement.
As I mentioned before I like more the idea of the plugin having its
logic because it allows the plugin to do more than just a database.
When I said that I had in mind some ideas that you will find in my
next patch set. I'll change this dumb code to just "0000" at the
beginning and see what devices are out there requiring a dumb pincode
other than "0000".
Hi Scott,
>>>>> +uint32_t btd_device_get_class(struct btd_device *device)
>>>>> +{
>>>>> + return device->class;
>>>>> +}
>>>
>>>> why not export functions for getting the major and minor class. That seems to better then making a plugin decode that by themselves. Especially since we know they will get it work.
>>>
>>> Basically we had the device class exported like that in the previous
>>> plugin. I don't see a huge advantage on splitting the class in this
>>> file instead of in the plugin... and a plugin could also compare with
>>> both the major and minor class at the same time. Anyway, is an easy
>>> change and I don't have a strong preference.
>>
>> the reason why I prefer major and minor class is that then the core does the "math" ones and does it right. We had enough issues with it being done wrong. Including a kernel bug that we have to live with now.
>>
>> In addition you actually did not consider the 2 bits that define a format type of major/minor class. Only when they are 0, then the mapping is what you are checking against. These 3 bytes are the worst combination of smashing bits into each other. Check print_dev_class() of btmon to see what I mean.
>>
>
> We have do this parsing in applications as well, since BlueZ just
> exports the class as a uint32 via D-Bus properties; so if BlueZ is
> going to have a single place to parse this, we should also offer the
> parsed values to applications as D-Bus properties.
>
> Even the parsing of the Minor Device Class is pretty hairy, since it
> depends on the Major Class; so to make sure that's done properly, it
> also seems that BlueZ should do that and provide a complete parsed
> description of the device.
we are giving you the Icon property or with LE the Appearance property. That we in addition hand out the raw class of device value is just for the crazy people that pretend to know better.
The Icon property follows the Freedesktop Icon specification and Appearance property follows the Bluetooth GAP service specification.
Regards
Marcel
On Thu, Apr 11, 2013 at 10:23 AM, Marcel Holtmann <[email protected]> wro=
te:
>>>> +uint32_t btd_device_get_class(struct btd_device *device)
>>>> +{
>>>> + return device->class;
>>>> +}
>>
>>> why not export functions for getting the major and minor class. That se=
ems to better then making a plugin decode that by themselves. Especially si=
nce we know they will get it work.
>>
>> Basically we had the device class exported like that in the previous
>> plugin. I don't see a huge advantage on splitting the class in this
>> file instead of in the plugin... and a plugin could also compare with
>> both the major and minor class at the same time. Anyway, is an easy
>> change and I don't have a strong preference.
>
> the reason why I prefer major and minor class is that then the core does =
the "math" ones and does it right. We had enough issues with it being done =
wrong. Including a kernel bug that we have to live with now.
>
> In addition you actually did not consider the 2 bits that define a format=
type of major/minor class. Only when they are 0, then the mapping is what =
you are checking against. These 3 bytes are the worst combination of smashi=
ng bits into each other. Check print_dev_class() of btmon to see what I mea=
n.
>
We have do this parsing in applications as well, since BlueZ just
exports the class as a uint32 via D-Bus properties; so if BlueZ is
going to have a single place to parse this, we should also offer the
parsed values to applications as D-Bus properties.
Even the parsing of the Minor Device Class is pretty hairy, since it
depends on the Major Class; so to make sure that's done properly, it
also seems that BlueZ should do that and provide a complete parsed
description of the device.
I guess something exported in Properties like:
major-service-class: "audio"
major-device-class: "audio-video"
minor-device-class: "loudspeaker"
Or for the complex HID example:
major-service-class: none
major-device-class: "peripheral"
minor-device-class: "pointing-device"
device-functions: "joystick,gamepad,digitizer-tablet"
Scott
--
Scott James Remnant | Chrome OS Systems | [email protected] | Google
Hi Alex,
>>> +uint32_t btd_device_get_class(struct btd_device *device)
>>> +{
>>> + return device->class;
>>> +}
>
>> why not export functions for getting the major and minor class. That seems to better then making a plugin decode that by themselves. Especially since we know they will get it work.
>
> Basically we had the device class exported like that in the previous
> plugin. I don't see a huge advantage on splitting the class in this
> file instead of in the plugin... and a plugin could also compare with
> both the major and minor class at the same time. Anyway, is an easy
> change and I don't have a strong preference.
the reason why I prefer major and minor class is that then the core does the "math" ones and does it right. We had enough issues with it being done wrong. Including a kernel bug that we have to live with now.
In addition you actually did not consider the 2 bits that define a format type of major/minor class. Only when they are 0, then the mapping is what you are checking against. These 3 bytes are the worst combination of smashing bits into each other. Check print_dev_class() of btmon to see what I mean.
Regards
Marcel
>> +uint32_t btd_device_get_class(struct btd_device *device)
>> +{
>> + return device->class;
>> +}
> why not export functions for getting the major and minor class. That seems to better then making a plugin decode that by themselves. Especially since we know they will get it work.
Basically we had the device class exported like that in the previous
plugin. I don't see a huge advantage on splitting the class in this
file instead of in the plugin... and a plugin could also compare with
both the major and minor class at the same time. Anyway, is an easy
change and I don't have a strong preference.
Hi Alex,
>>> +struct pincb_iter {
>>> + GSList *it; /* current callback function */
>>> + int count; /* numer of times it() was called */
>>> + /* When the iterator reaches the end, it is NULL and count is -1 */
>>> +};
>>
>> explain to me why this iter struct is needed. I am a little bit lost on the nested GSList in it.
>>
>> I get the basic idea what this patch tries to achieve, but it looks a bit too complicated to me.
>
> The struct pincb_iter is an iterator object over the list of pincodes
> dinamically generated by the pincode callbacks stored in the adapter.
> This list of callbacks is a GSList * that lives in the adapter. To
> iterate once every element of the list, we could use a GSList * and
> ->next, but this will iterate once every callback. Before, the
> callbacks were generating only one code, but now I want a plugin to be
> able to generate more than one pincode (calling the callback several
> times).
>
> So, what I need is a pincode iterator (not a callback iterator). This
> struct pincb_iter has an iterator to the callbacks list (it) and an
> integer representing how many times was the function called. Then it
> implements the function pincb_iter_next() that will give you the next
> pincode generated by the list of plugin callbacks handling all the
> logic of calling several times the same function or moving to the next
> function if there aren't more codes for that one.
>
> So you create a fresh new iterator and call pincb_iter_next on it
> until you get a 0, which means that there no more codes to provide.
> You know you reached the end when you ask for the next and you get 0,
> but you can't predict if you will get a 0 before calling it (its more
> like a python iterator rather than a c++ iterator).
>
> The reason why I need to store a struct pincb_iter* in the struct
> bonding_req is because the iterator is implemented in adapter.c but it
> is stored in device.c. Moving its implementation to device.c creates
> more problems, and after all the iterator is over the adapter's list
> of plugin callbacks... But there's no nested GSList * on it. The
> GSList * is just a GSList iterator, not a new list.
I still get the feeling this is too heavy weight compared to what we are trying to do. Not that I have a better idea right on how to achieve what you need.
Johan, any ideas?
Regards
Marcel
On Wed, Apr 10, 2013 at 10:06 PM, Marcel Holtmann <[email protected]> wrote:
> so I do not think that "count" is a really good name here. Wouldn't it be better to call it "attempt" or something like that?
No strong preference. "attempt" looks good.
> And I prefer to make these things unsigned if we are not planning to have -1 handed to it.
Actually, I'm using a -1 internally in the struct pincb_iter to mark
that the iterator reached the end (see pincb_iter_end()), but it is
never passed to the plugin.
On Wed, Apr 10, 2013 at 10:05 PM, Marcel Holtmann <[email protected]> wrote:
>> +struct pincb_iter {
>> + GSList *it; /* current callback function */
>> + int count; /* numer of times it() was called */
>> + /* When the iterator reaches the end, it is NULL and count is -1 */
>> +};
>
> explain to me why this iter struct is needed. I am a little bit lost on the nested GSList in it.
>
> I get the basic idea what this patch tries to achieve, but it looks a bit too complicated to me.
The struct pincb_iter is an iterator object over the list of pincodes
dinamically generated by the pincode callbacks stored in the adapter.
This list of callbacks is a GSList * that lives in the adapter. To
iterate once every element of the list, we could use a GSList * and
->next, but this will iterate once every callback. Before, the
callbacks were generating only one code, but now I want a plugin to be
able to generate more than one pincode (calling the callback several
times).
So, what I need is a pincode iterator (not a callback iterator). This
struct pincb_iter has an iterator to the callbacks list (it) and an
integer representing how many times was the function called. Then it
implements the function pincb_iter_next() that will give you the next
pincode generated by the list of plugin callbacks handling all the
logic of calling several times the same function or moving to the next
function if there aren't more codes for that one.
So you create a fresh new iterator and call pincb_iter_next on it
until you get a 0, which means that there no more codes to provide.
You know you reached the end when you ask for the next and you get 0,
but you can't predict if you will get a 0 before calling it (its more
like a python iterator rather than a c++ iterator).
The reason why I need to store a struct pincb_iter* in the struct
bonding_req is because the iterator is implemented in adapter.c but it
is stored in device.c. Moving its implementation to device.c creates
more problems, and after all the iterator is over the adapter's list
of plugin callbacks... But there's no nested GSList * on it. The
GSList * is just a GSList iterator, not a new list.
Hope it helps.
Best regards,
Alex.
Hi Alex,
> Exports the device class to plugins.
> ---
> src/device.c | 5 +++++
> src/device.h | 1 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/src/device.c b/src/device.c
> index 032fc68..4ce7b8c 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -2127,6 +2127,11 @@ void device_set_class(struct btd_device *device, uint32_t class)
> DEVICE_INTERFACE, "Class");
> }
>
> +uint32_t btd_device_get_class(struct btd_device *device)
> +{
> + return device->class;
> +}
> +
why not export functions for getting the major and minor class. That seems to better then making a plugin decode that by themselves. Especially since we know they will get it work.
Regards
Marcel
Hi Alex,
> The autopair plugin tries standard pincodes for different devices with dumb
> pincodes. It also generates a random 6 digit pincode for keyboards that
> support any pincode but fallbacks to the agent call in case the random
> generated pincode didn't work.
> ---
> Makefile.plugins | 3 ++
> plugins/autopair.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 148 insertions(+)
> create mode 100644 plugins/autopair.c
>
> diff --git a/Makefile.plugins b/Makefile.plugins
> index f497782..651a970 100644
> --- a/Makefile.plugins
> +++ b/Makefile.plugins
> @@ -5,6 +5,9 @@ builtin_sources += plugins/hostname.c
> builtin_modules += wiimote
> builtin_sources += plugins/wiimote.c
>
> +builtin_modules += autopair
> +builtin_sources += plugins/autopair.c
> +
> if MAINTAINER_MODE
> builtin_modules += gatt_example
> builtin_sources += plugins/gatt-example.c
> diff --git a/plugins/autopair.c b/plugins/autopair.c
> new file mode 100644
> index 0000000..40b8f8b
> --- /dev/null
> +++ b/plugins/autopair.c
> @@ -0,0 +1,145 @@
> +/*
> + *
> + * BlueZ - Bluetooth protocol stack for Linux
> + *
> + * Copyright (C) 2012-2013 Google Inc.
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <stdbool.h>
> +#include <stdlib.h>
> +
> +#include <bluetooth/bluetooth.h>
> +#include <glib.h>
> +
> +#include "plugin.h"
> +#include "adapter.h"
> +#include "device.h"
> +#include "log.h"
> +#include "storage.h"
> +
> +/*
> + * Plugin to handle automatic pairing of devices with reduced user
> + * interaction, including implementing the recommendation of the HID spec
> + * for keyboard devices.
> + *
> + * The plugin works by intercepting the PIN request for devices; if the
> + * device is a keyboard a random six-digit numeric PIN is generated and
> + * returned, flagged for displaying using DisplayPinCode.
> + *
> + */
> +
> +static ssize_t autopair_pincb(struct btd_adapter *adapter,
> + struct btd_device *device,
> + char *pinbuf, gboolean *display,
> + int count)
> +{
> + char addr[18];
> + char pinstr[7];
> + uint32_t class;
> +
> + ba2str(device_get_address(device), addr);
> +
> + class = btd_device_get_class(device);
> +
> + DBG("device %s 0x%x", addr, class);
> +
> + /* This is a class-based pincode guesser. Ignore devices with an unknown
> + * class. */
> + if (class == 0)
> + return 0;
> +
> + switch ((class & 0x1f00) >> 8) {
> + case 0x04: /* Audio/Video */
> + switch ((class & 0xfc) >> 2) {
> + case 0x01: /* Wearable Headset Device */
> + case 0x02: /* Hands-free Device */
> + case 0x06: /* Headphones */
> + case 0x07: /* Portable Audio */
> + case 0x0a: /* HiFi Audio Device */
> + if (count == 0)
> + memcpy(pinbuf, "0000", 4);
> + else if (count == 1)
> + memcpy(pinbuf, "1234", 4);
> + else
> + return 0;
as mentioned earlier, I do not like the count naming. Here it makes it even worse.
And what I would be doing is creating a string array { "0000", "1234" } that just can be walked and not this hardcoded if statement.
Regards
Marcel
Hi Alex,
> The plugin's pin code callback doesn't know about the pairing process. It
> just provides a pin code based on the information provided to this function.
> Although limited state could be added through other new callbacks, this fix
> achieves this by providing more information to the callback itself. The
> new argument "count" states the pin callback attempt of the particular
> plugin for the current pairing of the device. This allows a plugin to try
> different pincodes for the same device in the same pairing process.
>
> To signal that the plugin doesn't provide any pin code for the provided device
> the current implementation returns 0 (an empty pin code). Analogously, with
> this fix, a plugin should return 0 when it doesn't have any other pin code to
> provide for the given device.
> ---
> plugins/wiimote.c | 7 ++++++-
> src/adapter.c | 2 +-
> src/adapter.h | 3 ++-
> 3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/plugins/wiimote.c b/plugins/wiimote.c
> index 90d6d7b..c8d024f 100644
> --- a/plugins/wiimote.c
> +++ b/plugins/wiimote.c
> @@ -70,12 +70,17 @@ static const char *wii_names[] = {
> };
>
> static ssize_t wii_pincb(struct btd_adapter *adapter, struct btd_device *device,
> - char *pinbuf, gboolean *display)
> + char *pinbuf, gboolean *display, int count)
> {
> uint16_t vendor, product;
> char addr[18], name[25];
> unsigned int i;
>
> + /* Only try the pin code once per device. If it's not correct then it's
> + * an unknown device. */
> + if (count > 0)
> + return 0;
> +
> ba2str(device_get_address(device), addr);
>
> vendor = btd_device_get_vendor(device);
> diff --git a/src/adapter.c b/src/adapter.c
> index 957f829..f2193cf 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -4802,7 +4802,7 @@ static ssize_t pincb_iter_next(struct pincb_iter *iter,
>
> while (iter->it != NULL) {
> cb = iter->it->data;
> - ret = cb(adapter, device, pin_buf, display);
> + ret = cb(adapter, device, pin_buf, display, iter->count);
> iter->count++;
> if (ret > 0)
> return ret;
> diff --git a/src/adapter.h b/src/adapter.h
> index d158334..692963f 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -132,7 +132,8 @@ int btd_cancel_authorization(guint id);
> int btd_adapter_restore_powered(struct btd_adapter *adapter);
>
> typedef ssize_t (*btd_adapter_pin_cb_t) (struct btd_adapter *adapter,
> - struct btd_device *dev, char *out, gboolean *display);
> + struct btd_device *dev, char *out, gboolean *display,
> + int count);
so I do not think that "count" is a really good name here. Wouldn't it be better to call it "attempt" or something like that?
And I prefer to make these things unsigned if we are not planning to have -1 handed to it.
Regards
Marcel
Hi Alex,
> The current pincode callback list on the adapter keeps track of all the
> pincode callbacks registered by a plugin for that adapter and calls each
> one until one provides a pincode for the current bonding. This mechanism
> forgets about what happened with previous bonding attempts and pushes the
> status track to the plugin side.
>
> This patch creates an iterator struct (struct pincb_iter) that keeps track
> of the last function called and the number of times called. This will
> allow to provide more information about the bonding status to the pincode
> callback.
> ---
> src/adapter.c | 51 +++++++++++++++++++++++++++++++++++++++++++--------
> src/adapter.h | 4 ++++
> src/device.c | 13 +++++++++++++
> src/device.h | 1 +
> 4 files changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 6255da6..957f829 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -121,6 +121,12 @@ struct service_auth {
> struct agent *agent; /* NULL for queued auths */
> };
>
> +struct pincb_iter {
> + GSList *it; /* current callback function */
> + int count; /* numer of times it() was called */
> + /* When the iterator reaches the end, it is NULL and count is -1 */
> +};
> +
explain to me why this iter struct is needed. I am a little bit lost on the nested GSList in it.
I get the basic idea what this patch tries to achieve, but it looks a bit too complicated to me.
Regards
Marcel