Hi,
I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
[1] that changes the "reverse" variable for an SDP query:
- device_browse_sdp(device, NULL, NULL, NULL, TRUE);
+ if (device_is_bredr(device))
+ device_browse_sdp(device, NULL, NULL, NULL, FALSE);
+ else
+ device_browse_primary(device, NULL, NULL, FALSE);
You can see the original had reverse as TRUE, but the patch may have
inadvertently changed it to FALSE. The only reason I notice this is
that for some reason I get a debug output that BlueZ is removing
"0000110d-0000-1000-8000-00805f9b34fb" (ADVANCED_AUDIO_UUID) which
then knocks out my HFP and A2DP profiles.
bluetoothd[766]: src/device.c:device_remove_drivers() UUID
0000110d-0000-1000-8000-00805f9b34fb was removed from device
00:17:E3:3B:4F:DD
bluetoothd[766]: audio/unix.c:unix_device_removed()
unix_device_removed(0x2a0d0358)
bluetoothd[766]: audio/gateway.c:path_unregister() Unregistered
interface org.bluez.HandsfreeGateway on path
/org/bluez/766/hci0/dev_00_17_E3_3B_4F_DD
bluetoothd[766]: audio/source.c:path_unregister() Unregistered
interface org.bluez.AudioSource on path
/org/bluez/766/hci0/dev_00_17_E3_3B_4F_DD
There may be another subtle bug that is the real culprit here (e.g.,
why does removing 0x110d knock out the other profiles), but I thought
I'd check on this first.
Mike
[1] http://git.kernel.org/?p=bluetooth/bluez.git;a=commitdiff;h=d2920be715974795b51f9cc3279947104da3647b
Hi Anderson,
--------------------------------------------------
From: "Anderson Lizardo" <[email protected]>
Sent: Wednesday, March 21, 2012 7:48 PM
To: "Jaganath" <[email protected]>
Cc: <[email protected]>
Subject: Re: Query on attribute api service object path
> Hi,
>
> On Wed, Mar 21, 2012 at 9:11 AM, Jaganath <[email protected]> wrote:
>> Hello,
>>
>> I have a doubt in attribute-api.txt.
>> In Device service hierarchy object path is
>> "[prefix]/{hci0}/{device0}/{service0, service1, ...}.
>> which is device path + service"x".
>> When I checked the code "x" is "prim->start" which is service start
>> handle.
>> But how user will get this handle to create object path?
>> Only uuids are passed to user in UUIDs signal afaik.
>
> Take a look on how to use the API on the test/test-attrib python
> script. You should not try to parse the path itself. it is returned by
> Methods and you simply use then with a D-Bus interface (e.g.
> Bluez.Attribute).
>
Thanks for your reply. Characteristic based Service object path is there in
device properties.
Jaganath
Hi,
On Wed, Mar 21, 2012 at 9:11 AM, Jaganath <[email protected]> wrote:
> Hello,
>
> I have a doubt in attribute-api.txt.
> In Device service hierarchy object path is
> "[prefix]/{hci0}/{device0}/{service0, service1, ...}.
> which is device path + service"x".
> When I checked the code "x" is "prim->start" which is service start handle.
> But how user will get this handle to create object path?
> Only uuids are passed to user in UUIDs signal afaik.
Take a look on how to use the API on the test/test-attrib python
script. You should not try to parse the path itself. it is returned by
Methods and you simply use then with a D-Bus interface (e.g.
Bluez.Attribute).
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
Hello,
I have a doubt in attribute-api.txt.
In Device service hierarchy object path is
"[prefix]/{hci0}/{device0}/{service0, service1, ...}.
which is device path + service"x".
When I checked the code "x" is "prim->start" which is service start handle.
But how user will get this handle to create object path?
Only uuids are passed to user in UUIDs signal afaik.
Thanks
Jaganath
Hi Vinicius,
On Tue, Mar 20, 2012, Vinicius Costa Gomes wrote:
> > As for the partial service removal changes to the device driver API, it
> > looks pretty messy to me. Is there any reason why we couldn't just
> > restrict calling remove() for when removing the entire device object
> > (and never call it when doing subsequent service discoveries)?
>
> I agree that the logic is quite messy. Just some comments:
>
> That assumes that the service list that a device implements is very much
> static, which only breaks for development (in which the services of an
> device are always changing) and you will need to remove that device and
> create it again.
We would still be able to handle added UUIDs just fine. The only issue
is with removed services in which case you'd just wouldn't be able to
connect to the remote device through the corresponding D-Bus interface
(something which can already now happen and which I think is ok).
Johan
On Tue, Mar 20, 2012 at 2:21 PM, Mike <[email protected]> wrote:
> On Tue, Mar 20, 2012 at 1:39 PM, Mike <[email protected]> wrote:
>> On Tue, Mar 20, 2012 at 1:27 PM, Mike <[email protected]> wrote:
>>> On Tue, Mar 20, 2012 at 12:35 PM, Vinicius Costa Gomes
>>> <[email protected]> wrote:
>>>> Hi,
>>>>
>>>> On 20:04 Mon 19 Mar, Johan Hedberg wrote:
>>>>> Hi Mike,
>>>>>
>>>>> On Mon, Mar 19, 2012, Mike wrote:
>>>>> > I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
>>>>> > [1] that changes the "reverse" variable for an SDP query:
>>>>> >
>>>>> > ?- ? ? ? device_browse_sdp(device, NULL, NULL, NULL, TRUE);
>>>>> > + ? ? ? if (device_is_bredr(device))
>>>>> > + ? ? ? ? ? ? ? device_browse_sdp(device, NULL, NULL, NULL, FALSE);
>>>>> > + ? ? ? else
>>>>> > + ? ? ? ? ? ? ? device_browse_primary(device, NULL, NULL, FALSE);
>>>>> >
>>>>> > You can see the original had reverse as TRUE, but the patch may have
>>>>> > inadvertently changed it to FALSE..kernel.org/majordomo-info.html
>>>>>
>>>>> That looks like a definitive bug. Good that you caught it. Vinicius
>>>>> (author of the commit) care to comment?
>>>>
>>>> It was changed to false by mistake, a patch to fix it should be arriving
>>>> on the mailing list in a few moments.
>>>>
>>>> The comment on src/device.c around line 1683 gives some hints on what
>>>> my mistake may have caused: it could be that the device while connected
>>>> "hides" some items from its service record. So with reverse set to
>>>> false, some records that are present are being removed by mistake.
>>>
>>> I'm not sure that's the case here. ?I added some code to print out the
>>> contents of "device->uuids" at the beginning of update_services
>>> (src/device.c), and here is what I got:
>>>
>>> UUID 0000110d-0000-1000-8000-00805f9b34fb
>>> UUID 0000111f-0000-1000-8000-00805f9b34fb
>>> UUID 0000110d-0000-1000-8000-00805f9b34fb
>>> UUID 0000111f-0000-1000-8000-00805f9b34fb
>>>
>>> A little curious why the UUIDs are duplicated, but besides that, we
>>> can see that my phone already attempted to connect _before_ it was
>>> paired, because it thought it was paired already (pairing deleted on
>>> the BlueZ side only). ?The UUIDs were added then before SDP could be
>>> done.
>>>
>>> This has me suspecting these lines of code in update_services:
>>>
>>> ? ? ? ? ? ? ? ?l = g_slist_find_custom(device->uuids, profile_uuid,
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(GCompareFunc) strcmp);
>>> ? ? ? ? ? ? ? ?if (!l)
>>> ? ? ? ? ? ? ? ? ? ? ? ?req->profiles_added =
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?g_slist_append(req->profiles_added,
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?profile_uuid);
>>> ? ? ? ? ? ? ? ?else {
>>> ? ? ? ? ? ? ? ? ? ? ? ?req->profiles_removed =
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?g_slist_remove(req->profiles_removed,
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?l->data);
>>> ? ? ? ? ? ? ? ? ? ? ? ?g_free(profile_uuid);
>>> ? ? ? ? ? ? ? ?}
>>>
>>> It appears that if a UUID already existed when you did the SDP browse,
>>> it is removed? ?Why would that be?
>>
>> Ah, I read that wrong. ?The UUID is removed from the list of profiles
>> to be removed! ?So that's sane, but I guess the problem is that
>> somehow we have the UUIDs in there twice, which makes this code fail.
>
> Argh. ?I guess update_services was being called twice, so the UUIDS
> aren't duplicated. ?I'm still tracing what the real issue is then.
Alright, now I think I understand what's going on. The avdtp.c code
adds ADVANCED_AUDIO_UUID to the UUID list if the device does not
exist. The problem is that ADVANCED_AUDIO_UUID, according to the
assigned numbers document, is only allowed as a profile class. The
code in update_services is checking for service classes, and finds
0x110a (audio source) rather than 0x110d (advanced audio). So, the
real problem in my case is that ADVANCED_AUDIO_UUID is being used as a
service class when it should not. Below is the SDP record from my
phone for the Audio Source:
Service Name: Audio Source
Service Description: Audio Source
Service Provider: /a/mobile/system/cl.gif
Service RecHandle: 0x1000d
Service Class ID List:
"Audio Source" (0x110a)
Protocol Descriptor List:
"L2CAP" (0x0100)
PSM: 25
"AVDTP" (0x0019)
uint16: 0x100
Language Base Attr List:
code_ISO639: 0x656e
encoding: 0x6a
base_offset: 0x100
code_ISO639: 0x6672
encoding: 0x6a
base_offset: 0xc800
code_ISO639: 0x6573
encoding: 0x6a
base_offset: 0xc803
code_ISO639: 0x7074
encoding: 0x6a
base_offset: 0xc806
Profile Descriptor List:
"Advanced Audio" (0x110d)
Version: 0x0100
On Tue, Mar 20, 2012 at 1:39 PM, Mike <[email protected]> wrote:
> On Tue, Mar 20, 2012 at 1:27 PM, Mike <[email protected]> wrote:
>> On Tue, Mar 20, 2012 at 12:35 PM, Vinicius Costa Gomes
>> <[email protected]> wrote:
>>> Hi,
>>>
>>> On 20:04 Mon 19 Mar, Johan Hedberg wrote:
>>>> Hi Mike,
>>>>
>>>> On Mon, Mar 19, 2012, Mike wrote:
>>>> > I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
>>>> > [1] that changes the "reverse" variable for an SDP query:
>>>> >
>>>> > ?- ? ? ? device_browse_sdp(device, NULL, NULL, NULL, TRUE);
>>>> > + ? ? ? if (device_is_bredr(device))
>>>> > + ? ? ? ? ? ? ? device_browse_sdp(device, NULL, NULL, NULL, FALSE);
>>>> > + ? ? ? else
>>>> > + ? ? ? ? ? ? ? device_browse_primary(device, NULL, NULL, FALSE);
>>>> >
>>>> > You can see the original had reverse as TRUE, but the patch may have
>>>> > inadvertently changed it to FALSE..kernel.org/majordomo-info.html
>>>>
>>>> That looks like a definitive bug. Good that you caught it. Vinicius
>>>> (author of the commit) care to comment?
>>>
>>> It was changed to false by mistake, a patch to fix it should be arriving
>>> on the mailing list in a few moments.
>>>
>>> The comment on src/device.c around line 1683 gives some hints on what
>>> my mistake may have caused: it could be that the device while connected
>>> "hides" some items from its service record. So with reverse set to
>>> false, some records that are present are being removed by mistake.
>>
>> I'm not sure that's the case here. ?I added some code to print out the
>> contents of "device->uuids" at the beginning of update_services
>> (src/device.c), and here is what I got:
>>
>> UUID 0000110d-0000-1000-8000-00805f9b34fb
>> UUID 0000111f-0000-1000-8000-00805f9b34fb
>> UUID 0000110d-0000-1000-8000-00805f9b34fb
>> UUID 0000111f-0000-1000-8000-00805f9b34fb
>>
>> A little curious why the UUIDs are duplicated, but besides that, we
>> can see that my phone already attempted to connect _before_ it was
>> paired, because it thought it was paired already (pairing deleted on
>> the BlueZ side only). ?The UUIDs were added then before SDP could be
>> done.
>>
>> This has me suspecting these lines of code in update_services:
>>
>> ? ? ? ? ? ? ? ?l = g_slist_find_custom(device->uuids, profile_uuid,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(GCompareFunc) strcmp);
>> ? ? ? ? ? ? ? ?if (!l)
>> ? ? ? ? ? ? ? ? ? ? ? ?req->profiles_added =
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?g_slist_append(req->profiles_added,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?profile_uuid);
>> ? ? ? ? ? ? ? ?else {
>> ? ? ? ? ? ? ? ? ? ? ? ?req->profiles_removed =
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?g_slist_remove(req->profiles_removed,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?l->data);
>> ? ? ? ? ? ? ? ? ? ? ? ?g_free(profile_uuid);
>> ? ? ? ? ? ? ? ?}
>>
>> It appears that if a UUID already existed when you did the SDP browse,
>> it is removed? ?Why would that be?
>
> Ah, I read that wrong. ?The UUID is removed from the list of profiles
> to be removed! ?So that's sane, but I guess the problem is that
> somehow we have the UUIDs in there twice, which makes this code fail.
Argh. I guess update_services was being called twice, so the UUIDS
aren't duplicated. I'm still tracing what the real issue is then.
On Tue, Mar 20, 2012 at 1:27 PM, Mike <[email protected]> wrote:
> On Tue, Mar 20, 2012 at 12:35 PM, Vinicius Costa Gomes
> <[email protected]> wrote:
>> Hi,
>>
>> On 20:04 Mon 19 Mar, Johan Hedberg wrote:
>>> Hi Mike,
>>>
>>> On Mon, Mar 19, 2012, Mike wrote:
>>> > I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
>>> > [1] that changes the "reverse" variable for an SDP query:
>>> >
>>> > ?- ? ? ? device_browse_sdp(device, NULL, NULL, NULL, TRUE);
>>> > + ? ? ? if (device_is_bredr(device))
>>> > + ? ? ? ? ? ? ? device_browse_sdp(device, NULL, NULL, NULL, FALSE);
>>> > + ? ? ? else
>>> > + ? ? ? ? ? ? ? device_browse_primary(device, NULL, NULL, FALSE);
>>> >
>>> > You can see the original had reverse as TRUE, but the patch may have
>>> > inadvertently changed it to FALSE..kernel.org/majordomo-info.html
>>>
>>> That looks like a definitive bug. Good that you caught it. Vinicius
>>> (author of the commit) care to comment?
>>
>> It was changed to false by mistake, a patch to fix it should be arriving
>> on the mailing list in a few moments.
>>
>> The comment on src/device.c around line 1683 gives some hints on what
>> my mistake may have caused: it could be that the device while connected
>> "hides" some items from its service record. So with reverse set to
>> false, some records that are present are being removed by mistake.
>
> I'm not sure that's the case here. ?I added some code to print out the
> contents of "device->uuids" at the beginning of update_services
> (src/device.c), and here is what I got:
>
> UUID 0000110d-0000-1000-8000-00805f9b34fb
> UUID 0000111f-0000-1000-8000-00805f9b34fb
> UUID 0000110d-0000-1000-8000-00805f9b34fb
> UUID 0000111f-0000-1000-8000-00805f9b34fb
>
> A little curious why the UUIDs are duplicated, but besides that, we
> can see that my phone already attempted to connect _before_ it was
> paired, because it thought it was paired already (pairing deleted on
> the BlueZ side only). ?The UUIDs were added then before SDP could be
> done.
>
> This has me suspecting these lines of code in update_services:
>
> ? ? ? ? ? ? ? ?l = g_slist_find_custom(device->uuids, profile_uuid,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(GCompareFunc) strcmp);
> ? ? ? ? ? ? ? ?if (!l)
> ? ? ? ? ? ? ? ? ? ? ? ?req->profiles_added =
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?g_slist_append(req->profiles_added,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?profile_uuid);
> ? ? ? ? ? ? ? ?else {
> ? ? ? ? ? ? ? ? ? ? ? ?req->profiles_removed =
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?g_slist_remove(req->profiles_removed,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?l->data);
> ? ? ? ? ? ? ? ? ? ? ? ?g_free(profile_uuid);
> ? ? ? ? ? ? ? ?}
>
> It appears that if a UUID already existed when you did the SDP browse,
> it is removed? ?Why would that be?
Ah, I read that wrong. The UUID is removed from the list of profiles
to be removed! So that's sane, but I guess the problem is that
somehow we have the UUIDs in there twice, which makes this code fail.
On Tue, Mar 20, 2012 at 12:35 PM, Vinicius Costa Gomes
<[email protected]> wrote:
> Hi,
>
> On 20:04 Mon 19 Mar, Johan Hedberg wrote:
>> Hi Mike,
>>
>> On Mon, Mar 19, 2012, Mike wrote:
>> > I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
>> > [1] that changes the "reverse" variable for an SDP query:
>> >
>> > ?- ? ? ? device_browse_sdp(device, NULL, NULL, NULL, TRUE);
>> > + ? ? ? if (device_is_bredr(device))
>> > + ? ? ? ? ? ? ? device_browse_sdp(device, NULL, NULL, NULL, FALSE);
>> > + ? ? ? else
>> > + ? ? ? ? ? ? ? device_browse_primary(device, NULL, NULL, FALSE);
>> >
>> > You can see the original had reverse as TRUE, but the patch may have
>> > inadvertently changed it to FALSE..kernel.org/majordomo-info.html
>>
>> That looks like a definitive bug. Good that you caught it. Vinicius
>> (author of the commit) care to comment?
>
> It was changed to false by mistake, a patch to fix it should be arriving
> on the mailing list in a few moments.
>
> The comment on src/device.c around line 1683 gives some hints on what
> my mistake may have caused: it could be that the device while connected
> "hides" some items from its service record. So with reverse set to
> false, some records that are present are being removed by mistake.
I'm not sure that's the case here. I added some code to print out the
contents of "device->uuids" at the beginning of update_services
(src/device.c), and here is what I got:
UUID 0000110d-0000-1000-8000-00805f9b34fb
UUID 0000111f-0000-1000-8000-00805f9b34fb
UUID 0000110d-0000-1000-8000-00805f9b34fb
UUID 0000111f-0000-1000-8000-00805f9b34fb
A little curious why the UUIDs are duplicated, but besides that, we
can see that my phone already attempted to connect _before_ it was
paired, because it thought it was paired already (pairing deleted on
the BlueZ side only). The UUIDs were added then before SDP could be
done.
This has me suspecting these lines of code in update_services:
l = g_slist_find_custom(device->uuids, profile_uuid,
(GCompareFunc) strcmp);
if (!l)
req->profiles_added =
g_slist_append(req->profiles_added,
profile_uuid);
else {
req->profiles_removed =
g_slist_remove(req->profiles_removed,
l->data);
g_free(profile_uuid);
}
It appears that if a UUID already existed when you did the SDP browse,
it is removed? Why would that be?
Hi,
On 20:04 Mon 19 Mar, Johan Hedberg wrote:
> Hi Mike,
>
> On Mon, Mar 19, 2012, Mike wrote:
> > I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
> > [1] that changes the "reverse" variable for an SDP query:
> >
> > - device_browse_sdp(device, NULL, NULL, NULL, TRUE);
> > + if (device_is_bredr(device))
> > + device_browse_sdp(device, NULL, NULL, NULL, FALSE);
> > + else
> > + device_browse_primary(device, NULL, NULL, FALSE);
> >
> > You can see the original had reverse as TRUE, but the patch may have
> > inadvertently changed it to FALSE..kernel.org/majordomo-info.html
>
> That looks like a definitive bug. Good that you caught it. Vinicius
> (author of the commit) care to comment?
It was changed to false by mistake, a patch to fix it should be arriving
on the mailing list in a few moments.
The comment on src/device.c around line 1683 gives some hints on what
my mistake may have caused: it could be that the device while connected
"hides" some items from its service record. So with reverse set to
false, some records that are present are being removed by mistake.
>
> As for the partial service removal changes to the device driver API, it
> looks pretty messy to me. Is there any reason why we couldn't just
> restrict calling remove() for when removing the entire device object
> (and never call it when doing subsequent service discoveries)?
I agree that the logic is quite messy. Just some comments:
That assumes that the service list that a device implements is very much
static, which only breaks for development (in which the services of an
device are always changing) and you will need to remove that device and
create it again.
>
> Johan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
--
Vinicius
Hi Mike,
On Mon, Mar 19, 2012, Mike wrote:
> I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
> [1] that changes the "reverse" variable for an SDP query:
>
> - device_browse_sdp(device, NULL, NULL, NULL, TRUE);
> + if (device_is_bredr(device))
> + device_browse_sdp(device, NULL, NULL, NULL, FALSE);
> + else
> + device_browse_primary(device, NULL, NULL, FALSE);
>
> You can see the original had reverse as TRUE, but the patch may have
> inadvertently changed it to FALSE..kernel.org/majordomo-info.html
That looks like a definitive bug. Good that you caught it. Vinicius
(author of the commit) care to comment?
As for the partial service removal changes to the device driver API, it
looks pretty messy to me. Is there any reason why we couldn't just
restrict calling remove() for when removing the entire device object
(and never call it when doing subsequent service discoveries)?
Johan
On Mon, Mar 19, 2012 at 5:19 PM, Mike <[email protected]> wrote:
>
> There may be another subtle bug that is the real culprit here (e.g.,
> why does removing 0x110d knock out the other profiles), but I thought
> I'd check on this first.
>
Ah, I can see that Mikel Astiz's patches (from inspection) should
solve this part of the problem:
[PATCH v0 2/3] Add device driver support for partial unloading
http://www.spinics.net/lists/linux-bluetooth/msg22427.html
[PATCH v0 3/3] audio: add partial unloading to audio driver
http://www.spinics.net/lists/linux-bluetooth/msg22428.html