* Re: [PATCH 1/8] obexd: Handle empty path name in SetPath
From: Luiz Augusto von Dentz @ 2013-01-14 17:33 UTC (permalink / raw)
To: Christian Fetzer; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <50F3D948.9010605@oss.bmw-carit.de>
Hi Christian,
On Mon, Jan 14, 2013 at 12:09 PM, Christian Fetzer
<christian.fetzer@oss.bmw-carit.de> wrote:
> Hi Luiz,
>
>
> On 01/13/2013 04:55 PM, Luiz Augusto von Dentz wrote:
>>
>> But what empty means in this case, I would like to stick with cd behavior,
>> so empty should set back to root. Otherwise the patchset seems fine. -- Luiz
>> Augusto von Dentz
>
> But this is exactly what the patch is doing.
> g_obex_setpath called with an empty path won't set the parent flag,
> and thus it requests going back to the root folder.
>
> < ACL data: handle 21 flags 0x00 dlen 21
> L2CAP(d): cid 0x0041 len 17 [psm 3]
> RFCOMM(d): UIH: cr 1 dlci 32 pf 0 ilen 13 fcs 0xd8
> OBEX: SetPath cmd(f): len 13 flags 2 constants 0
> Connection ID (0xcb) = 98
> Name (0x01) = Unicode length 0
>> HCI Event: Number of Completed Packets (0x13) plen 5
> handle 21 packets 1
>> ACL data: handle 21 flags 0x02 dlen 11
> L2CAP(d): cid 0x0041 len 7 [psm 3]
> RFCOMM(d): UIH: cr 0 dlci 32 pf 0 ilen 3 fcs 0x2
> OBEX: SetPath rsp(f): status 200 len 3
>
> Or did I misunderstand you?
I was just confirming this would not cd back to parent, anyway this
set is now upstream, thanks!
Btw, Ive modified a little bit patch 06/08 to use hexadecimal values
to indicate better the size.
--
Luiz Augusto von Dentz
^ permalink raw reply
* [PATCH BlueZ v2 01/11] transport: Initialize the "Volume" property
From: João Paulo Rechi Vita @ 2013-01-14 16:10 UTC (permalink / raw)
To: linux-bluetooth
Cc: luiz.dentz, vinicius.gomes, claudio.takahasi, luiz.von.dentz,
João Paulo Rechi Vita
In-Reply-To: <CAAngNMa=ZS0d-yWhpEVUGD3FZaWb_E6awVnwJoO68tje2fdEtA@mail.gmail.com>
---
profiles/audio/transport.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index a4370a5..8defc6f 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -787,7 +787,6 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
struct a2dp_transport *a2dp;
a2dp = g_new0(struct a2dp_transport, 1);
- a2dp->volume = -1;
transport->resume = resume_a2dp;
transport->suspend = suspend_a2dp;
@@ -795,14 +794,17 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
transport->data = a2dp;
transport->destroy = destroy_a2dp;
- if (strcasecmp(uuid, A2DP_SOURCE_UUID) == 0)
+ if (strcasecmp(uuid, A2DP_SOURCE_UUID) == 0) {
+ a2dp->volume = -1;
transport->sink_watch = sink_add_state_cb(
sink_state_changed,
transport);
- else
+ } else {
+ a2dp->volume = 127;
transport->source_watch = source_add_state_cb(
source_state_changed,
transport);
+ }
} else
goto fail;
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%
From: Joao Paulo Rechi Vita @ 2013-01-14 15:43 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Mikel Astiz, Von Dentz, Luiz, linux-bluetooth@vger.kernel.org,
Vinicius Gomes, Claudio Takahasi
In-Reply-To: <CABBYNZLjtmgNG5RDSVWC5qaz5-ARH84giVShV+Lra1TAS3ehkA@mail.gmail.com>
Hello Mikel and Luiz,
On Mon, Jan 14, 2013 at 12:34 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Mikel,
>
> On Mon, Jan 14, 2013 at 4:55 PM, Mikel Astiz <Mikel.Astiz@bmw-carit.de> wrote:
>> Hi all,
>>
>>> Hi Joao,
>>>
>>> On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
>>> <jprvita@openbossa.org> wrote:
>>> > On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
>>> > <luiz.dentz@gmail.com> wrote:
>>> >> Hi Joao,
>>> >>
>>> >> On Fri, Jan 11, 2013 at 10:25 PM, João Paulo Rechi Vita
>>> >> <jprvita@openbossa.org> wrote:
>>> >>> ---
>>> >>> profiles/audio/transport.c | 2 +-
>>> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> >>>
>>> >>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>>> >>> index a4370a5..6ffa98a 100644
>>> >>> --- a/profiles/audio/transport.c
>>> >>> +++ b/profiles/audio/transport.c
>>> >>> @@ -787,7 +787,7 @@ struct media_transport
>>> *media_transport_create(struct media_endpoint *endpoint,
>>> >>> struct a2dp_transport *a2dp;
>>> >>>
>>> >>> a2dp = g_new0(struct a2dp_transport, 1);
>>> >>> - a2dp->volume = -1;
>>> >>> + a2dp->volume = 63;
>>> >>>
>>> >>> transport->resume = resume_a2dp;
>>> >>> transport->suspend = suspend_a2dp;
>>> >>> --
>>> >>> 1.7.11.7
>>> >>
>>> >> Does the spec say anything regarding this? Actually it seems this
>>> >> value must be set by PA if it does support volume notification, which
>>> >> means a new version of PA, then it should set the value when the card
>>> >> is initialized, otherwise if the endpoint doesn't set a value it
>>> >> should remain -1/not available. If volume is not set by the endpoint
>>> >> we should either return and error upon register notification or
>>> >> return maximum volume always and refuse to SetAbsoluteVolume, my
>>> >> guess is that the latter is better for IOP reasons since the remote
>>> >> device may register to volume while the endpoint is setting up the
>>> >> transport so the volume may be set latter.
>>> >>
>>> >>
>>> >
>>> > I agree the right value will come from PA. The problem is that the
>>> > org.bluez.MediaTransport.Volume property doesn't exist when volume is
>>> > < 0 or > 127 and PA will not be able to inform the correct volume
>>> > value. Should we simply remove volume_exists(), or do you have any
>>> > other suggestion?
>>>
>>> I guess we could use the role of transport, if it is a sink/controller transport it
>>> should be initialized to max volume otherwise it should still be initialized with
>>> -1 so we can continue to omit in case of source/target role.
>>
>> A side question: AFAIK this feature has been added in AVRCP 1.4. How would the endpoint (PulseAudio) know if it's supported or not by a certain device?
>
> For the controller/sink role there is no point of PulseAudio knowing
> about this, it basically need to set the volume and that about it, if
> the remote device is interested it register to be notified otherwise
> we don't do anything with the value, plain and simple.
>
I think PA will also want to register for notifications of changes on
that property, so it can reflects the current volume on the remote
through the master volume of the bluetooth card. The volume can be
changed on the remote side and it will inform bluez through the
SetAbsoluteVolume command.
>> My proposal would be that the property is not present if the feature is not supported (or also if AVRCP is not connected). Therefore, I'd propose exposing a special value in D-Bus (i.e. -1) as soon as the feature becomes available (AVRCP 1.4 connected) so the endpoint can set the initial value.
>
> That is a bad idea, the notification mechanism of AVRCP is not
> something I would like to see exposed over D-Bus, it is way too broken
> since you have to registration again every time the value changes.
>
I personally thinks it makes sense to not support the property if
AVRCP < 1.4 (this was on my initial plans also), or if the remote
doesn't implement AVRCP (A2DP only). We just need to find an elegant
way to make this information available for the transport. Otherwise
AVRCP will be connected every time A2DP is also connected, right? Or
am I missing some corner case here?
--
João Paulo Rechi Vita
Openbossa Labs - INdT
^ permalink raw reply
* Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%
From: Luiz Augusto von Dentz @ 2013-01-14 15:37 UTC (permalink / raw)
To: Joao Paulo Rechi Vita
Cc: Von Dentz, Luiz, linux-bluetooth@vger.kernel.org, Vinicius Gomes,
Claudio Takahasi
In-Reply-To: <CAAngNMa=ZS0d-yWhpEVUGD3FZaWb_E6awVnwJoO68tje2fdEtA@mail.gmail.com>
Hi Joao,
On Mon, Jan 14, 2013 at 5:32 PM, Joao Paulo Rechi Vita
<jprvita@openbossa.org> wrote:
> On Mon, Jan 14, 2013 at 11:21 AM, Von Dentz, Luiz
> <luiz.von.dentz@intel.com> wrote:
>> Hi Joao,
>>
>> On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
>> <jprvita@openbossa.org> wrote:
>>> On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com> wrote:
>>>> Hi Joao,
>>>>
>>>> On Fri, Jan 11, 2013 at 10:25 PM, João Paulo Rechi Vita
>>>> <jprvita@openbossa.org> wrote:
>>>>> ---
>>>>> profiles/audio/transport.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>>>>> index a4370a5..6ffa98a 100644
>>>>> --- a/profiles/audio/transport.c
>>>>> +++ b/profiles/audio/transport.c
>>>>> @@ -787,7 +787,7 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
>>>>> struct a2dp_transport *a2dp;
>>>>>
>>>>> a2dp = g_new0(struct a2dp_transport, 1);
>>>>> - a2dp->volume = -1;
>>>>> + a2dp->volume = 63;
>>>>>
>>>>> transport->resume = resume_a2dp;
>>>>> transport->suspend = suspend_a2dp;
>>>>> --
>>>>> 1.7.11.7
>>>>
>>>> Does the spec say anything regarding this? Actually it seems this
>>>> value must be set by PA if it does support volume notification, which
>>>> means a new version of PA, then it should set the value when the card
>>>> is initialized, otherwise if the endpoint doesn't set a value it
>>>> should remain -1/not available. If volume is not set by the endpoint
>>>> we should either return and error upon register notification or return
>>>> maximum volume always and refuse to SetAbsoluteVolume, my guess is
>>>> that the latter is better for IOP reasons since the remote device may
>>>> register to volume while the endpoint is setting up the transport so
>>>> the volume may be set latter.
>>>>
>>>>
>>>
>>> I agree the right value will come from PA. The problem is that the
>>> org.bluez.MediaTransport.Volume property doesn't exist when volume is
>>> < 0 or > 127 and PA will not be able to inform the correct volume
>>> value. Should we simply remove volume_exists(), or do you have any
>>> other suggestion?
>>
>> I guess we could use the role of transport, if it is a sink/controller
>> transport it should be initialized to max volume otherwise it should
>> still be initialized with -1 so we can continue to omit in case of
>> source/target role.
>
> I agree we omit it by now, but when we add support for AVC on the
> Source/TG role we can use it to control the volume on the remote,
> sending a SetAbsoluteVolume command. I'll send an updated version in a
> few moments.
For Source/TG it is different because as soon as we are able to
register the volume notification we can update the volume and then it
became available, in fact it should be already working like this.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%
From: Luiz Augusto von Dentz @ 2013-01-14 15:34 UTC (permalink / raw)
To: Mikel Astiz
Cc: Von Dentz, Luiz, Joao Paulo Rechi Vita,
linux-bluetooth@vger.kernel.org, Vinicius Gomes, Claudio Takahasi
In-Reply-To: <0E5B0602414B684FAFC3B07F5A858CC40FFB5CDF@Exchange2010.bmw-carit.intra>
Hi Mikel,
On Mon, Jan 14, 2013 at 4:55 PM, Mikel Astiz <Mikel.Astiz@bmw-carit.de> wrote:
> Hi all,
>
>> Hi Joao,
>>
>> On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
>> <jprvita@openbossa.org> wrote:
>> > On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
>> > <luiz.dentz@gmail.com> wrote:
>> >> Hi Joao,
>> >>
>> >> On Fri, Jan 11, 2013 at 10:25 PM, João Paulo Rechi Vita
>> >> <jprvita@openbossa.org> wrote:
>> >>> ---
>> >>> profiles/audio/transport.c | 2 +-
>> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>> >>> index a4370a5..6ffa98a 100644
>> >>> --- a/profiles/audio/transport.c
>> >>> +++ b/profiles/audio/transport.c
>> >>> @@ -787,7 +787,7 @@ struct media_transport
>> *media_transport_create(struct media_endpoint *endpoint,
>> >>> struct a2dp_transport *a2dp;
>> >>>
>> >>> a2dp = g_new0(struct a2dp_transport, 1);
>> >>> - a2dp->volume = -1;
>> >>> + a2dp->volume = 63;
>> >>>
>> >>> transport->resume = resume_a2dp;
>> >>> transport->suspend = suspend_a2dp;
>> >>> --
>> >>> 1.7.11.7
>> >>
>> >> Does the spec say anything regarding this? Actually it seems this
>> >> value must be set by PA if it does support volume notification, which
>> >> means a new version of PA, then it should set the value when the card
>> >> is initialized, otherwise if the endpoint doesn't set a value it
>> >> should remain -1/not available. If volume is not set by the endpoint
>> >> we should either return and error upon register notification or
>> >> return maximum volume always and refuse to SetAbsoluteVolume, my
>> >> guess is that the latter is better for IOP reasons since the remote
>> >> device may register to volume while the endpoint is setting up the
>> >> transport so the volume may be set latter.
>> >>
>> >>
>> >
>> > I agree the right value will come from PA. The problem is that the
>> > org.bluez.MediaTransport.Volume property doesn't exist when volume is
>> > < 0 or > 127 and PA will not be able to inform the correct volume
>> > value. Should we simply remove volume_exists(), or do you have any
>> > other suggestion?
>>
>> I guess we could use the role of transport, if it is a sink/controller transport it
>> should be initialized to max volume otherwise it should still be initialized with
>> -1 so we can continue to omit in case of source/target role.
>
> A side question: AFAIK this feature has been added in AVRCP 1.4. How would the endpoint (PulseAudio) know if it's supported or not by a certain device?
For the controller/sink role there is no point of PulseAudio knowing
about this, it basically need to set the volume and that about it, if
the remote device is interested it register to be notified otherwise
we don't do anything with the value, plain and simple.
> My proposal would be that the property is not present if the feature is not supported (or also if AVRCP is not connected). Therefore, I'd propose exposing a special value in D-Bus (i.e. -1) as soon as the feature becomes available (AVRCP 1.4 connected) so the endpoint can set the initial value.
That is a bad idea, the notification mechanism of AVRCP is not
something I would like to see exposed over D-Bus, it is way too broken
since you have to registration again every time the value changes.
Btw, everytime someone else jump in this conversation please read at
least the Absolute Volume Control part of the spec, quite often people
think the volume notification are from the source to the sink when in
fact it is the opposite.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%
From: Joao Paulo Rechi Vita @ 2013-01-14 15:32 UTC (permalink / raw)
To: Von Dentz, Luiz
Cc: Luiz Augusto von Dentz, linux-bluetooth@vger.kernel.org,
Vinicius Gomes, Claudio Takahasi
In-Reply-To: <CACumGOKTYkTM+bzHEBo2aL=L8Kycgpsza0U-ag=iDixbfnqjzw@mail.gmail.com>
On Mon, Jan 14, 2013 at 11:21 AM, Von Dentz, Luiz
<luiz.von.dentz@intel.com> wrote:
> Hi Joao,
>
> On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
> <jprvita@openbossa.org> wrote:
>> On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Joao,
>>>
>>> On Fri, Jan 11, 2013 at 10:25 PM, João Paulo Rechi Vita
>>> <jprvita@openbossa.org> wrote:
>>>> ---
>>>> profiles/audio/transport.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>>>> index a4370a5..6ffa98a 100644
>>>> --- a/profiles/audio/transport.c
>>>> +++ b/profiles/audio/transport.c
>>>> @@ -787,7 +787,7 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
>>>> struct a2dp_transport *a2dp;
>>>>
>>>> a2dp = g_new0(struct a2dp_transport, 1);
>>>> - a2dp->volume = -1;
>>>> + a2dp->volume = 63;
>>>>
>>>> transport->resume = resume_a2dp;
>>>> transport->suspend = suspend_a2dp;
>>>> --
>>>> 1.7.11.7
>>>
>>> Does the spec say anything regarding this? Actually it seems this
>>> value must be set by PA if it does support volume notification, which
>>> means a new version of PA, then it should set the value when the card
>>> is initialized, otherwise if the endpoint doesn't set a value it
>>> should remain -1/not available. If volume is not set by the endpoint
>>> we should either return and error upon register notification or return
>>> maximum volume always and refuse to SetAbsoluteVolume, my guess is
>>> that the latter is better for IOP reasons since the remote device may
>>> register to volume while the endpoint is setting up the transport so
>>> the volume may be set latter.
>>>
>>>
>>
>> I agree the right value will come from PA. The problem is that the
>> org.bluez.MediaTransport.Volume property doesn't exist when volume is
>> < 0 or > 127 and PA will not be able to inform the correct volume
>> value. Should we simply remove volume_exists(), or do you have any
>> other suggestion?
>
> I guess we could use the role of transport, if it is a sink/controller
> transport it should be initialized to max volume otherwise it should
> still be initialized with -1 so we can continue to omit in case of
> source/target role.
I agree we omit it by now, but when we add support for AVC on the
Source/TG role we can use it to control the volume on the remote,
sending a SetAbsoluteVolume command. I'll send an updated version in a
few moments.
--
João Paulo Rechi Vita
Openbossa Labs - INdT
^ permalink raw reply
* AW: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%
From: Mikel Astiz @ 2013-01-14 14:55 UTC (permalink / raw)
To: Von Dentz, Luiz, Joao Paulo Rechi Vita
Cc: Luiz Augusto von Dentz, linux-bluetooth@vger.kernel.org,
Vinicius Gomes, Claudio Takahasi
In-Reply-To: <CACumGOKTYkTM+bzHEBo2aL=L8Kycgpsza0U-ag=iDixbfnqjzw@mail.gmail.com>
Hi all,
> Hi Joao,
>
> On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
> <jprvita@openbossa.org> wrote:
> > On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> >> Hi Joao,
> >>
> >> On Fri, Jan 11, 2013 at 10:25 PM, João Paulo Rechi Vita
> >> <jprvita@openbossa.org> wrote:
> >>> ---
> >>> profiles/audio/transport.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> >>> index a4370a5..6ffa98a 100644
> >>> --- a/profiles/audio/transport.c
> >>> +++ b/profiles/audio/transport.c
> >>> @@ -787,7 +787,7 @@ struct media_transport
> *media_transport_create(struct media_endpoint *endpoint,
> >>> struct a2dp_transport *a2dp;
> >>>
> >>> a2dp = g_new0(struct a2dp_transport, 1);
> >>> - a2dp->volume = -1;
> >>> + a2dp->volume = 63;
> >>>
> >>> transport->resume = resume_a2dp;
> >>> transport->suspend = suspend_a2dp;
> >>> --
> >>> 1.7.11.7
> >>
> >> Does the spec say anything regarding this? Actually it seems this
> >> value must be set by PA if it does support volume notification, which
> >> means a new version of PA, then it should set the value when the card
> >> is initialized, otherwise if the endpoint doesn't set a value it
> >> should remain -1/not available. If volume is not set by the endpoint
> >> we should either return and error upon register notification or
> >> return maximum volume always and refuse to SetAbsoluteVolume, my
> >> guess is that the latter is better for IOP reasons since the remote
> >> device may register to volume while the endpoint is setting up the
> >> transport so the volume may be set latter.
> >>
> >>
> >
> > I agree the right value will come from PA. The problem is that the
> > org.bluez.MediaTransport.Volume property doesn't exist when volume is
> > < 0 or > 127 and PA will not be able to inform the correct volume
> > value. Should we simply remove volume_exists(), or do you have any
> > other suggestion?
>
> I guess we could use the role of transport, if it is a sink/controller transport it
> should be initialized to max volume otherwise it should still be initialized with
> -1 so we can continue to omit in case of source/target role.
A side question: AFAIK this feature has been added in AVRCP 1.4. How would the endpoint (PulseAudio) know if it's supported or not by a certain device?
My proposal would be that the property is not present if the feature is not supported (or also if AVRCP is not connected). Therefore, I'd propose exposing a special value in D-Bus (i.e. -1) as soon as the feature becomes available (AVRCP 1.4 connected) so the endpoint can set the initial value.
Cheers,
Mikel
^ permalink raw reply
* Re: [PATCH] doc: Fix trivial typo in adapter-api
From: Johan Hedberg @ 2013-01-14 14:45 UTC (permalink / raw)
To: Mikel Astiz; +Cc: linux-bluetooth, Mikel Astiz
In-Reply-To: <1358149481-3230-1-git-send-email-mikel.astiz.oss@gmail.com>
Hi Mikel,
On Mon, Jan 14, 2013, Mikel Astiz wrote:
> ---
> While checking the doc changes between 5.0 and 5.1, I came across this line which I believe is a typo.
>
> doc/adapter-api.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%
From: Von Dentz, Luiz @ 2013-01-14 14:21 UTC (permalink / raw)
To: Joao Paulo Rechi Vita
Cc: Luiz Augusto von Dentz, linux-bluetooth@vger.kernel.org,
Vinicius Gomes, Claudio Takahasi
In-Reply-To: <CAAngNMZDdYyNZ4F+3o6sG8rJC=PBCXyFKufMmY_Ud2k8xd3=gw@mail.gmail.com>
Hi Joao,
On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
<jprvita@openbossa.org> wrote:
> On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Joao,
>>
>> On Fri, Jan 11, 2013 at 10:25 PM, João Paulo Rechi Vita
>> <jprvita@openbossa.org> wrote:
>>> ---
>>> profiles/audio/transport.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>>> index a4370a5..6ffa98a 100644
>>> --- a/profiles/audio/transport.c
>>> +++ b/profiles/audio/transport.c
>>> @@ -787,7 +787,7 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
>>> struct a2dp_transport *a2dp;
>>>
>>> a2dp = g_new0(struct a2dp_transport, 1);
>>> - a2dp->volume = -1;
>>> + a2dp->volume = 63;
>>>
>>> transport->resume = resume_a2dp;
>>> transport->suspend = suspend_a2dp;
>>> --
>>> 1.7.11.7
>>
>> Does the spec say anything regarding this? Actually it seems this
>> value must be set by PA if it does support volume notification, which
>> means a new version of PA, then it should set the value when the card
>> is initialized, otherwise if the endpoint doesn't set a value it
>> should remain -1/not available. If volume is not set by the endpoint
>> we should either return and error upon register notification or return
>> maximum volume always and refuse to SetAbsoluteVolume, my guess is
>> that the latter is better for IOP reasons since the remote device may
>> register to volume while the endpoint is setting up the transport so
>> the volume may be set latter.
>>
>>
>
> I agree the right value will come from PA. The problem is that the
> org.bluez.MediaTransport.Volume property doesn't exist when volume is
> < 0 or > 127 and PA will not be able to inform the correct volume
> value. Should we simply remove volume_exists(), or do you have any
> other suggestion?
I guess we could use the role of transport, if it is a sink/controller
transport it should be initialized to max volume otherwise it should
still be initialized with -1 so we can continue to omit in case of
source/target role.
^ permalink raw reply
* [PATCH v2] Bluetooth: Power the device up after a rfkill unblock
From: Vinicius Costa Gomes @ 2013-01-14 14:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Vinicius Costa Gomes
With the HCI_SETUP patches, this is all that is needed to make the
case when a adapter is added with Bluetooth blocked in rfkill to work.
When rfkill is unblocked, the device will be powered on if the device
is in HCI_SETUP state, meaning that it was never properly initialized.
If the device is not used by userspace, the HCI_AUTO_OFF flag will
take care of powering it off.
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
Samuel on IRC caused me to remember this. Thanks :-)
net/bluetooth/hci_core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 596660d..9796c0a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1124,8 +1124,11 @@ static int hci_rfkill_set_block(void *data, bool blocked)
BT_DBG("%p name %s blocked %d", hdev, hdev->name, blocked);
- if (!blocked)
+ if (!blocked) {
+ if (test_bit(HCI_SETUP, &hdev->dev_flags))
+ schedule_work(&hdev->power_on);
return 0;
+ }
hci_dev_do_close(hdev);
--
1.8.1
^ permalink raw reply related
* Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%
From: Joao Paulo Rechi Vita @ 2013-01-14 14:05 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: linux-bluetooth@vger.kernel.org, Vinicius Gomes, Claudio Takahasi,
Luiz Augusto Von Dentz
In-Reply-To: <CABBYNZ+3_KA4Lg2jJ9CJfUNv0jG-n2S5J__SNOMxJenOGeL+GQ@mail.gmail.com>
On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Joao,
>
> On Fri, Jan 11, 2013 at 10:25 PM, João Paulo Rechi Vita
> <jprvita@openbossa.org> wrote:
>> ---
>> profiles/audio/transport.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>> index a4370a5..6ffa98a 100644
>> --- a/profiles/audio/transport.c
>> +++ b/profiles/audio/transport.c
>> @@ -787,7 +787,7 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
>> struct a2dp_transport *a2dp;
>>
>> a2dp = g_new0(struct a2dp_transport, 1);
>> - a2dp->volume = -1;
>> + a2dp->volume = 63;
>>
>> transport->resume = resume_a2dp;
>> transport->suspend = suspend_a2dp;
>> --
>> 1.7.11.7
>
> Does the spec say anything regarding this? Actually it seems this
> value must be set by PA if it does support volume notification, which
> means a new version of PA, then it should set the value when the card
> is initialized, otherwise if the endpoint doesn't set a value it
> should remain -1/not available. If volume is not set by the endpoint
> we should either return and error upon register notification or return
> maximum volume always and refuse to SetAbsoluteVolume, my guess is
> that the latter is better for IOP reasons since the remote device may
> register to volume while the endpoint is setting up the transport so
> the volume may be set latter.
>
>
I agree the right value will come from PA. The problem is that the
org.bluez.MediaTransport.Volume property doesn't exist when volume is
< 0 or > 127 and PA will not be able to inform the correct volume
value. Should we simply remove volume_exists(), or do you have any
other suggestion?
--
João Paulo Rechi Vita
Openbossa Labs - INdT
^ permalink raw reply
* [PATCH v2 1/1] test: Make map script a command line client
From: Christian Fetzer @ 2013-01-14 10:49 UTC (permalink / raw)
To: linux-bluetooth
From: Christian Fetzer <christian.fetzer@bmw-carit.de>
Rework the map-client test script into an interactive command line client.
Now multiple MCE functions can be called in one active session.
The script also allows to specify all filters or optional parameters including
auto completion.
---
test/map-client | 320 ++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 218 insertions(+), 102 deletions(-)
diff --git a/test/map-client b/test/map-client
index 9fb7a5e..c5c899a 100755
--- a/test/map-client
+++ b/test/map-client
@@ -4,10 +4,13 @@ from __future__ import absolute_import, print_function, unicode_literals
import gobject
+import cmd
+import shlex
import sys
import os
import dbus
import dbus.mainloop.glib
+import threading
from optparse import OptionParser
from pprint import pformat
@@ -31,49 +34,39 @@ def unwrap(x):
return tuple(map(unwrap, x))
if isinstance(x, dict):
- return dict([(unwrap(k), unwrap(v)) for k, v in x.iteritems()])
+ return dict([(unwrap(k), unwrap(v)) for k, v in x.items()])
- for t in [unicode, str, long, int, float, bool]:
+ if sys.version_info >= (3, 0):
+ coversion_types = [str, int, float, bool]
+ else:
+ coversion_types = [unicode, str, long, int, float, bool]
+
+ for t in coversion_types:
if isinstance(x, t):
return t(x)
return x
def parse_options():
+ parser.add_option("-s", "--source", dest="source",
+ help="Source / local address to use", metavar="SOURCE")
parser.add_option("-d", "--device", dest="device",
help="Device to connect", metavar="DEVICE")
- parser.add_option("-c", "--chdir", dest="new_dir",
- help="Change current directory to DIR", metavar="DIR")
- parser.add_option("-l", "--lsdir", action="store_true", dest="ls_dir",
- help="List folders in current directory")
- parser.add_option("-v", "--verbose", action="store_true", dest="verbose")
- parser.add_option("-L", "--lsmsg", action="store", dest="ls_msg",
- help="List messages in supplied CWD subdir")
- parser.add_option("-g", "--get", action="store", dest="get_msg",
- help="Get message contents")
- parser.add_option("--get-properties", action="store", dest="get_msg_properties",
- help="Get message properties")
- parser.add_option("--mark-read", action="store", dest="mark_msg_read",
- help="Marks the messages as read")
- parser.add_option("--mark-unread", action="store", dest="mark_msg_unread",
- help="Marks the messages as unread")
- parser.add_option("--mark-deleted", action="store", dest="mark_msg_deleted",
- help="Deletes the message from the folder")
- parser.add_option("--mark-undeleted", action="store", dest="mark_msg_undeleted",
- help="Undeletes the message")
- parser.add_option("-u", "--update-inbox", action="store_true", dest="update_inbox",
- help="Checks for new mails")
+ parser.add_option("-p", "--port", dest="port", default=0,
+ help="RFCOMM port to connect", metavar="PORT")
+ parser.add_option("-v", "--verbose", action="store_true",
+ dest="verbose")
return parser.parse_args()
-def set_folder(session, new_dir):
- session.SetFolder(new_dir)
-
-class MapClient:
+class MapClient(cmd.Cmd):
def __init__(self, session_path, verbose=False):
+ cmd.Cmd.__init__(self)
+ cmd.Cmd.prompt = "MCE> "
self.progress = 0
self.transfer_path = None
self.props = dict()
+ self.dir = dict()
self.verbose = verbose
self.path = session_path
bus = dbus.SessionBus()
@@ -85,8 +78,15 @@ class MapClient:
signal_name="PropertiesChanged",
path_keyword="path")
- def create_transfer_reply(self, reply):
- (path, properties) = reply
+ def create_transfer_reply_get(self, path, properties):
+ self.dir[path] = "in";
+ self.create_transfer_reply(path, properties)
+
+ def create_transfer_reply_put(self, path, properties):
+ self.dir[path] = "out";
+ self.create_transfer_reply(path, properties)
+
+ def create_transfer_reply(self, path, properties):
self.transfer_path = path
self.props[path] = properties
if self.verbose:
@@ -98,22 +98,25 @@ class MapClient:
print("Operation succeeded")
def error(self, err):
- print err
- mainloop.quit()
+ print(err)
def transfer_complete(self, path):
if self.verbose:
print("Transfer finished")
properties = self.props.get(path)
+ print(path)
+ print(self.dir)
if properties == None:
return
- f = open(properties["Filename"], "r")
- os.remove(properties["Filename"])
- print(f.readlines())
+ if self.dir.get(path) == "in":
+ f = open(properties["Filename"], "r")
+ os.remove(properties["Filename"])
+ print(f.readlines())
- def transfer_error(self, path):
- print("Transfer %s error" % path)
- mainloop.quit()
+ def transfer_error(self, code, message, path):
+ if path != self.transfer_path:
+ return
+ print("Transfer finished with error %s: %s" % (code, message))
def properties_changed(self, interface, properties, invalidated, path):
req = self.props.get(path)
@@ -128,46 +131,171 @@ class MapClient:
self.transfer_error(path)
return
- def set_folder(self, new_dir):
- self.map.SetFolder(new_dir)
-
- def list_folders(self):
- for i in self.map.ListFolders(dict()):
- print("%s/" % (i["Name"]))
-
- def list_messages(self, folder):
- ret = self.map.ListMessages(folder, dict())
- print(pformat(unwrap(ret)))
-
- def get_message(self, handle):
- self.map.ListMessages("", dict())
+ def emptyline(self):
+ pass
+
+ def do_EOF(self, args):
+ """ Quit """
+ return True
+
+ def do_exit(self, args):
+ """ Quit """
+ return True
+
+ # SetFolder
+ def do_SetFolder(self, new_dir):
+ """ Set working directory for current session """
+ try:
+ self.map.SetFolder(new_dir)
+ except dbus.exceptions.DBusException as e:
+ print("Failed:", e)
+
+ def do_cd(self, new_dir):
+ self.do_SetFolder(new_dir)
+
+ # ListFolders
+ list_folder_parms = {
+ 'MaxCount': lambda x: dbus.UInt16(x),
+ 'Offset': lambda x: dbus.UInt16(x)
+ }
+
+ def do_ListFolders(self, args):
+ """ List directories in current working directory """
+ parms={}
+ if args:
+ try:
+ for i in args.split(' '):
+ k,v = i.split('=')
+ parms[k] = self.list_folder_parms[k](v)
+ except:
+ print("Syntax error")
+ return
+
+ try:
+ for i in self.map.ListFolders(parms):
+ print("%s/" % (i["Name"]))
+ except dbus.exceptions.DBusException as e:
+ print("Failed:", e)
+
+ def complete_ListFolders(self, text, args, begidx, endidx):
+ if not text:
+ completions = list(self.list_folder_parms.keys())
+ else:
+ completions = [ f + "=" for f in
+ self.list_folder_parms.keys()
+ if f.startswith(text) ]
+ return completions
+
+ # ListFilterFields
+ def do_ListFilterFields(self, args):
+ """ List all available fields that can be used as filters """
+ for i in self.map.ListFilterFields():
+ print(i)
+
+ # ListMessages
+ list_msg_parms = { 'Folder': lambda x: x,
+ 'MaxCount': lambda x: dbus.UInt16(x),
+ 'Offset': lambda x: dbus.UInt16(x),
+ 'SubjectLength': lambda x: dbus.Byte(int(x)),
+ 'Fields': lambda x: x.split(','),
+ 'Types': lambda x: x.split(','),
+ 'PeriodBegin': lambda x: dbus.String(x),
+ 'PeriodEnd': lambda x: dbus.String(x),
+ 'Read': lambda x:
+ dbus.Boolean(x.lower() in
+ ("yes", "true", "1")),
+ 'Recipient': lambda x: dbus.String(x),
+ 'Sender': lambda x: dbus.String(x),
+ 'Priority': lambda x:
+ dbus.Boolean(x.lower() in
+ ("yes", "true", "1"))
+ }
+
+ def do_ListMessages(self, args):
+ """ List messages in current working directory """
+ parms={}
+ if args:
+ try:
+ for i in shlex.split(args):
+ k,v = i.split('=')
+ parms[k] = self.list_msg_parms[k](v)
+ except Exception as e:
+ print("Syntax error", e)
+ return
+
+ try:
+ ret = self.map.ListMessages(parms.pop('Folder', ''),
+ parms)
+ print(pformat(unwrap(ret)))
+ except dbus.exceptions.DBusException as e:
+ print("Failed:", e)
+
+ def complete_ListMessages(self, text, args, begidx, endidx):
+ if not text:
+ completions = list(self.list_msg_parms.keys())
+ else:
+ completions = [ f + "=" for f in
+ self.list_msg_parms.keys()
+ if f.startswith(text) ]
+ return completions
+
+ def do_ls(self, args):
+ print("Folders:")
+ self.do_ListFolders(args)
+ print("Messages:")
+ self.do_ListMessages(args)
+
+ # GetMessage
+ def do_GetMessage(self, handle):
+ """ Download message """
path = self.path + "/message" + handle
obj = bus.get_object(BUS_NAME, path)
msg = dbus.Interface(obj, MESSAGE_INTERFACE)
- msg.Get("", True, reply_handler=self.create_transfer_reply,
+ msg.Get("", True, reply_handler=self.create_transfer_reply_get,
error_handler=self.error)
- def get_message_properties(self, handle):
- self.map.ListMessages("", dict())
- path = self.path + "/message" + handle
- obj = bus.get_object(BUS_NAME, path)
- msg = dbus.Interface(obj, "org.freedesktop.DBus.Properties")
- ret = msg.GetAll(MESSAGE_INTERFACE)
- print(pformat(unwrap(ret)))
-
- def set_message_property(self, handle, prop, flag):
- self.map.ListMessages("", dict())
- path = self.path + "/message" + handle
- obj = bus.get_object(BUS_NAME, path)
- msg = dbus.Interface(obj, MESSAGE_INTERFACE)
- msg.SetProperty (prop, flag);
-
- def update_inbox(self):
- self.map.UpdateInbox()
-
+ # GetMessageProperties
+ def do_GetMessageProperties(self, handle):
+ """ Returns all properties for the message """
+ try:
+ path = self.path + "/message" + handle
+ obj = bus.get_object(BUS_NAME, path)
+ msg = dbus.Interface(obj,
+ "org.freedesktop.DBus.Properties")
+ ret = msg.GetAll(MESSAGE_INTERFACE)
+ print(pformat(unwrap(ret)))
+ except Exception as e:
+ print("Error", e)
+ return
-if __name__ == '__main__':
+ # SetMessageProperties
+ def do_SetMessageProperty(self, args):
+ """Sets value to the mentioned property (Read, Delete) """
+ try:
+ handle, parm_line = args.split(' ')
+ parm, value = parm_line.split('=')
+ except Exception as e:
+ print("Syntax error", e)
+ return
+ try:
+ path = self.path + "/message" + handle
+ obj = bus.get_object(BUS_NAME, path)
+ msg = dbus.Interface(obj, MESSAGE_INTERFACE)
+ msg.SetProperty(parm, value in ("yes", "true", "1"));
+ except dbus.exceptions.DBusException as e:
+ print("Failed:", e)
+
+ # UpdateInbox
+ def do_UpdateInbox(self, line):
+ """ Request remote to update its inbox """
+ try:
+ self.map.UpdateInbox()
+ except dbus.exceptions.DBusException as e:
+ print("Failed:", e)
+
+if __name__ == '__main__':
+ dbus.mainloop.glib.threads_init()
dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
parser = OptionParser()
@@ -179,44 +307,32 @@ if __name__ == '__main__':
exit(0)
bus = dbus.SessionBus()
+ gobject.threads_init()
mainloop = gobject.MainLoop()
- client = dbus.Interface(bus.get_object(BUS_NAME, PATH),
- CLIENT_INTERFACE)
-
- print("Creating Session")
- path = client.CreateSession(options.device, { "Target": "map" })
+ mainloop_thread = threading.Thread(target=mainloop.run)
+ mainloop_thread.start()
- map_client = MapClient(path, options.verbose)
-
- if options.new_dir:
- map_client.set_folder(options.new_dir)
-
- if options.ls_dir:
- map_client.list_folders()
-
- if options.ls_msg is not None:
- map_client.list_messages(options.ls_msg)
-
- if options.get_msg is not None:
- map_client.get_message(options.get_msg)
-
- if options.get_msg_properties is not None:
- map_client.get_message_properties(options.get_msg_properties)
+ try:
+ client = dbus.Interface(bus.get_object(BUS_NAME, PATH),
+ CLIENT_INTERFACE)
- if options.mark_msg_read is not None:
- map_client.set_message_property(options.mark_msg_read, "Read", True)
+ print("Creating Session")
- if options.mark_msg_unread is not None:
- map_client.set_message_property(options.mark_msg_unread, "Read", False)
+ opts = { "Target": "map",
+ "Channel": dbus.Byte(int(options.port)) }
+ if options.source:
+ opts["Source"] = options.source
- if options.mark_msg_deleted is not None:
- map_client.set_message_property(options.mark_msg_deleted, "Deleted", True)
+ path = client.CreateSession(options.device, opts)
- if options.mark_msg_undeleted is not None:
- map_client.set_message_property(options.mark_msg_undeleted, "Deleted", False)
+ map_client = MapClient(path, options.verbose)
+ map_client.cmdloop()
- if options.update_inbox:
- map_client.update_inbox()
+ except (KeyboardInterrupt, SystemExit):
+ pass
+ except Exception as e:
+ print("Error", e)
- mainloop.run()
+ mainloop.quit()
+ mainloop_thread.join()
--
1.8.1
^ permalink raw reply related
* Re: [PATCH 1/1] test: Make map script a command line client
From: Christian Fetzer @ 2013-01-14 10:46 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZJJCW0oUOo_7F0VFcZVEFCVHZJpwGndke6P3T2pR9MqBg@mail.gmail.com>
Hi Luiz,
On 01/13/2013 04:29 PM, Luiz Augusto von Dentz wrote:
> I wonder if we should complicate more the testing scripts adding such
> futures or start working in a proper C tool such as bluetoothctl e.g.
> obexctl? -- Luiz Augusto von Dentz -- To unsubscribe from this list:
> send the line "unsubscribe linux-bluetooth" in the body of a message
> to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
In my opinion it would be nice if the test scripts support most profile
features that are implemented in bluez/obexd.
That's why I extended the map-client. I tried to keep it as simple as
possible while focusing also on good testing usability.
The language used for the test scripts / programs doesn't matter that
much since the scripts are mainly for developers/testers.
Python has some advantages, the scripts can be changed easily while
working on new features.
However, until some script replacement tool is available, it still would
make sense to extend/maintain the scripts that are there.
Regards,
Christian
^ permalink raw reply
* Re: [PATCH 1/8] obexd: Handle empty path name in SetPath
From: Christian Fetzer @ 2013-01-14 10:09 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZKLNngQw6rSUUPJBapZ+rt7C4PsVajegFKwv6j+-8zvrg@mail.gmail.com>
Hi Luiz,
On 01/13/2013 04:55 PM, Luiz Augusto von Dentz wrote:
> But what empty means in this case, I would like to stick with cd
> behavior, so empty should set back to root. Otherwise the patchset
> seems fine. -- Luiz Augusto von Dentz
But this is exactly what the patch is doing.
g_obex_setpath called with an empty path won't set the parent flag,
and thus it requests going back to the root folder.
< ACL data: handle 21 flags 0x00 dlen 21
L2CAP(d): cid 0x0041 len 17 [psm 3]
RFCOMM(d): UIH: cr 1 dlci 32 pf 0 ilen 13 fcs 0xd8
OBEX: SetPath cmd(f): len 13 flags 2 constants 0
Connection ID (0xcb) = 98
Name (0x01) = Unicode length 0
> HCI Event: Number of Completed Packets (0x13) plen 5
handle 21 packets 1
> ACL data: handle 21 flags 0x02 dlen 11
L2CAP(d): cid 0x0041 len 7 [psm 3]
RFCOMM(d): UIH: cr 0 dlci 32 pf 0 ilen 3 fcs 0x2
OBEX: SetPath rsp(f): status 200 len 3
Or did I misunderstand you?
Regards,
Christian
^ permalink raw reply
* [PATCH] doc: Fix trivial typo in adapter-api
From: Mikel Astiz @ 2013-01-14 7:44 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
---
While checking the doc changes between 5.0 and 5.1, I came across this line which I believe is a typo.
doc/adapter-api.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
index cef6cbe..74d235a 100644
--- a/doc/adapter-api.txt
+++ b/doc/adapter-api.txt
@@ -88,7 +88,7 @@ Properties string Address [readonly]
appropriate connectable state of the controller.
The value of this property is not persistent. After
- restart of unplugging of the adapter it will reset
+ restart or unplugging of the adapter it will reset
back to false.
boolean Discoverable [readwrite]
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH BlueZ 1/7] gdbus: Add g_dbus_client_get_proxy
From: Marcel Holtmann @ 2013-01-13 23:03 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZ+=oDRu-x3toCQfWi7wdzk9SCm2Fx1AhS6SNVf+6SEpkw@mail.gmail.com>
Hi Luiz,
> >> >> g_dbus_client_get_proxy gives the possibilitity to just check if a
> >> >> proxy exist for the given path and interface pair instead of using
> >> >> g_dbus_proxy_new which end up creating a proxy if it doesn't exists
> >> >> which is not always necessary.
> >> >
> >> > why would we do that. You get the proxy via the client callbacks for
> >> > proxy created or proxy removed.
> >>
> >> That way we the user don't have to maintain a list of found proxies,
> >> such a list exist and is maintained by GDBusClient.
> >
> > see how bluetoothctl does it. Pretty damn simple. So I am not convinced
> > that this is a good idea.
> >
> >> > The proxy_new method is for dealing with services that do not have
> >> > ObjectManager support.
> >>
> >> Yep, so in one way or the other you are already letting the user
> >> application search in the list of proxies maintained by GDBusClient,
> >> the only thing this does is to make the proxy_lookup public so the
> >> client don't have to maintain its how list only to be able to look
> >> back when a proxy point to another, btw the reason proxy_new is not
> >> enough is because it may lead to extra calls when the user application
> >> may just want to bail out if the proxy is not found.
> >
> > Still not convinced. What kind of application would this. This seems to
> > be all half thought trough. I am really failing to see the real purpose
> > here. Either you go all the way with ObjectManager and do it the right
> > way or you don't have an object manager, then you need to manually
> > trigger the get all properties.
>
> Im actually using these functions in an upcoming patch:
>
> static void register_player(GDBusProxy *proxy)
> {
> ...
> client = g_dbus_proxy_get_client(proxy);
>
> if (!g_dbus_proxy_get_property(proxy, "Device", &iter))
> return;
>
> dbus_message_iter_get_basic(&iter, &path);
>
> device = g_dbus_client_get_proxy(client, path, "org.bluez.Device1");
> if (device == NULL)
> return;
>
> if (!g_dbus_proxy_get_property(device, "Name", &iter))
> return;
> ...
> static void proxy_added(GDBusProxy *proxy, void *user_data)
> {
> ...
> } else if (!strcmp(interface, BLUEZ_MEDIA_PLAYER_INTERFACE)) {
> printf("Bluetooth Player %s found\n", g_dbus_proxy_get_path(proxy));
> register_player(proxy);
> }
> ...
>
> With this I don't have to keep any list of found devices what so ever,
> if I need a proxy I can just query it via g_dbus_client_get_proxy. Now
> compare this to what bluetoothctl does:
>
> static void proxy_added(GDBusProxy *proxy, void *user_data)
> {
> ...
> if (!strcmp(interface, "org.bluez.Device1")) {
> if (device_is_child(proxy, default_ctrl) == TRUE) {
> dev_list = g_list_append(dev_list, proxy);
>
> print_device(proxy, "NEW");
> } else if (!strcmp(interface, "org.bluez.Adapter1")) {
> ctrl_list = g_list_append(ctrl_list, proxy);
>
> if (!default_ctrl)
> default_ctrl = proxy;
>
> print_adapter(proxy, "NEW");
> ...
>
> So you are keeping the list of proxies while GDBusClient keeps them
> too, now it maybe useful for bluetoothctl but for mpris-tool it is
> completely useless, why would I keep a proxy of a device that doesn't
> contain any player, specially while discovering devices this would
> just populate with useless temporary devices.
bluetoothctl only keeps the list of proxies that contain an interface it
is interested in. And the same could be done for mpris-tool. If you get
an object path with a player interface, you remember it, otherwise you
just ignore it.
> Btw, with these functions we could actually have the following changes
> to device_is_child:
>
> diff --git a/client/main.c b/client/main.c
> index 9a927a8..1cd6d10 100644
> --- a/client/main.c
> +++ b/client/main.c
> @@ -224,8 +224,10 @@ static void print_uuids(GDBusProxy *proxy)
>
> static gboolean device_is_child(GDBusProxy *device, GDBusProxy *master)
> {
> + GDBusClient *client;
> + GDBusProxy *proxy;
> DBusMessageIter iter;
> - const char *adapter, *path;
> + const char *path;
>
> if (!master)
> return FALSE;
> @@ -233,13 +235,13 @@ static gboolean device_is_child(GDBusProxy
> *device, GDBusProxy *master)
> if (g_dbus_proxy_get_property(device, "Adapter", &iter) == FALSE)
> return FALSE;
>
> - dbus_message_iter_get_basic(&iter, &adapter);
> - path = g_dbus_proxy_get_path(master);
> + dbus_message_iter_get_basic(&iter, &path);
>
> - if (!strcmp(path, adapter))
> - return TRUE;
> + client = g_dbus_proxy_get_client(device);
And this I have single client, I could just make this global. So I am
still not buying into this one.
> - return FALSE;
> + proxy = g_dbus_client_get_proxy(client, path, "org.bluez.Adapter1");
> +
> + return proxy == master ? TRUE : FALSE;
> }
Don't see how this makes it simpler code. Or ends up in less
instructions for that matter.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH BlueZ 1/7] gdbus: Add g_dbus_client_get_proxy
From: Luiz Augusto von Dentz @ 2013-01-13 22:36 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1358111392.1806.87.camel@aeonflux>
Hi Marcel,
On Sun, Jan 13, 2013 at 11:09 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> >> g_dbus_client_get_proxy gives the possibilitity to just check if a
>> >> proxy exist for the given path and interface pair instead of using
>> >> g_dbus_proxy_new which end up creating a proxy if it doesn't exists
>> >> which is not always necessary.
>> >
>> > why would we do that. You get the proxy via the client callbacks for
>> > proxy created or proxy removed.
>>
>> That way we the user don't have to maintain a list of found proxies,
>> such a list exist and is maintained by GDBusClient.
>
> see how bluetoothctl does it. Pretty damn simple. So I am not convinced
> that this is a good idea.
>
>> > The proxy_new method is for dealing with services that do not have
>> > ObjectManager support.
>>
>> Yep, so in one way or the other you are already letting the user
>> application search in the list of proxies maintained by GDBusClient,
>> the only thing this does is to make the proxy_lookup public so the
>> client don't have to maintain its how list only to be able to look
>> back when a proxy point to another, btw the reason proxy_new is not
>> enough is because it may lead to extra calls when the user application
>> may just want to bail out if the proxy is not found.
>
> Still not convinced. What kind of application would this. This seems to
> be all half thought trough. I am really failing to see the real purpose
> here. Either you go all the way with ObjectManager and do it the right
> way or you don't have an object manager, then you need to manually
> trigger the get all properties.
Im actually using these functions in an upcoming patch:
static void register_player(GDBusProxy *proxy)
{
...
client = g_dbus_proxy_get_client(proxy);
if (!g_dbus_proxy_get_property(proxy, "Device", &iter))
return;
dbus_message_iter_get_basic(&iter, &path);
device = g_dbus_client_get_proxy(client, path, "org.bluez.Device1");
if (device == NULL)
return;
if (!g_dbus_proxy_get_property(device, "Name", &iter))
return;
...
static void proxy_added(GDBusProxy *proxy, void *user_data)
{
...
} else if (!strcmp(interface, BLUEZ_MEDIA_PLAYER_INTERFACE)) {
printf("Bluetooth Player %s found\n", g_dbus_proxy_get_path(proxy));
register_player(proxy);
}
...
With this I don't have to keep any list of found devices what so ever,
if I need a proxy I can just query it via g_dbus_client_get_proxy. Now
compare this to what bluetoothctl does:
static void proxy_added(GDBusProxy *proxy, void *user_data)
{
...
if (!strcmp(interface, "org.bluez.Device1")) {
if (device_is_child(proxy, default_ctrl) == TRUE) {
dev_list = g_list_append(dev_list, proxy);
print_device(proxy, "NEW");
} else if (!strcmp(interface, "org.bluez.Adapter1")) {
ctrl_list = g_list_append(ctrl_list, proxy);
if (!default_ctrl)
default_ctrl = proxy;
print_adapter(proxy, "NEW");
...
So you are keeping the list of proxies while GDBusClient keeps them
too, now it maybe useful for bluetoothctl but for mpris-tool it is
completely useless, why would I keep a proxy of a device that doesn't
contain any player, specially while discovering devices this would
just populate with useless temporary devices.
Btw, with these functions we could actually have the following changes
to device_is_child:
diff --git a/client/main.c b/client/main.c
index 9a927a8..1cd6d10 100644
--- a/client/main.c
+++ b/client/main.c
@@ -224,8 +224,10 @@ static void print_uuids(GDBusProxy *proxy)
static gboolean device_is_child(GDBusProxy *device, GDBusProxy *master)
{
+ GDBusClient *client;
+ GDBusProxy *proxy;
DBusMessageIter iter;
- const char *adapter, *path;
+ const char *path;
if (!master)
return FALSE;
@@ -233,13 +235,13 @@ static gboolean device_is_child(GDBusProxy
*device, GDBusProxy *master)
if (g_dbus_proxy_get_property(device, "Adapter", &iter) == FALSE)
return FALSE;
- dbus_message_iter_get_basic(&iter, &adapter);
- path = g_dbus_proxy_get_path(master);
+ dbus_message_iter_get_basic(&iter, &path);
- if (!strcmp(path, adapter))
- return TRUE;
+ client = g_dbus_proxy_get_client(device);
- return FALSE;
+ proxy = g_dbus_client_get_proxy(client, path, "org.bluez.Adapter1");
+
+ return proxy == master ? TRUE : FALSE;
}
So we compare the proxies not the path alone.
--
Luiz Augusto von Dentz
^ permalink raw reply related
* Re: [PATCH BlueZ 2/7] gdbus: Add g_dbus_proxy_get_client function
From: Marcel Holtmann @ 2013-01-13 21:12 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZKZ1kXbF7E5jJbPhduZcLmFcTT45jyA67KzoJeJVZiz3A@mail.gmail.com>
Hi Luiz,
> >> This is convenient as some callbacks don't provide the client which
> >> the proxy belongs.
> >
> > and I did not add that one on purpose. You should know where your proxy
> > belongs to and worst case hand it over via user_data.
>
> I understand this as the user_data will always be the client pointer,
> or a struct containing it, or you have to use a global variable, is
> that what you really want? The use application has given the client
> when registering the callbacks, so it is already passing it as
> context. Now that being said, Im not saying the design utterly broken,
> quite the contrary it work like a charm, it just could be more
> convenient and that is what Im trying to achieve with this patch.
it is a convenience for one special kind of quick-and-dirty hack that
normal application will never need. I have made these mistakes in the
past and rather have not sneaking up on me again. So lets not do this.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH BlueZ 1/7] gdbus: Add g_dbus_client_get_proxy
From: Marcel Holtmann @ 2013-01-13 21:09 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZJyaBUt+oRSzyYJ0PNLTys8BfODLJFNY5NzC5touoraGA@mail.gmail.com>
Hi Luiz,
> >> g_dbus_client_get_proxy gives the possibilitity to just check if a
> >> proxy exist for the given path and interface pair instead of using
> >> g_dbus_proxy_new which end up creating a proxy if it doesn't exists
> >> which is not always necessary.
> >
> > why would we do that. You get the proxy via the client callbacks for
> > proxy created or proxy removed.
>
> That way we the user don't have to maintain a list of found proxies,
> such a list exist and is maintained by GDBusClient.
see how bluetoothctl does it. Pretty damn simple. So I am not convinced
that this is a good idea.
> > The proxy_new method is for dealing with services that do not have
> > ObjectManager support.
>
> Yep, so in one way or the other you are already letting the user
> application search in the list of proxies maintained by GDBusClient,
> the only thing this does is to make the proxy_lookup public so the
> client don't have to maintain its how list only to be able to look
> back when a proxy point to another, btw the reason proxy_new is not
> enough is because it may lead to extra calls when the user application
> may just want to bail out if the proxy is not found.
Still not convinced. What kind of application would this. This seems to
be all half thought trough. I am really failing to see the real purpose
here. Either you go all the way with ObjectManager and do it the right
way or you don't have an object manager, then you need to manually
trigger the get all properties.
Regards
Marcel
^ permalink raw reply
* Re: Obexd OPP filesend fails with Windows7 stack
From: Syam Sidhardhan @ 2013-01-13 18:35 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: Syam Sidhardhan, linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZKMi572+BUbymbA=sX4s3NKSGcHsrGQWSptj5xVSTkMBw@mail.gmail.com>
Hi Luiz,
On Sun, Jan 13, 2013 at 9:12 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Syam,
>
> On Fri, Jan 11, 2013 at 10:32 AM, Syam Sidhardhan <s.syam@samsung.com> wrote:
>> Hi,
>>
>> We are using the obexd 0.48 version and testing the obexd.
>> During testing OPP file send, we found one interoperability
>> issue with the Windows7 PC stack and some commercialized devices
>> available in the market.
>>
>> The issue is: OBEXD file transfer is getting failed
>> once after the complete file content got transfferd. This is
>> because of, we do a direct RFCOMM disconection rather than
>> doing a proper OBEX disconection before.
>>
>> Does someone can help me a with a fix for this?
>
> We probably need to send a disconnect command before disconnecting
> RFCOMM, iirc there was someone else looking into a similar problem
> with a Nokia phone, the strange part is that the remote stack has
> acknowledged the transfer complete and in theory we could go ahead and
> start another one using the same session.
>
Yes, you are correct. We need to send a OBEX disconnect command
before the RFCOMM disconnection. We have tried with the same and
its working fine.
Regards,
Syam
^ permalink raw reply
* Re: [PATCH BlueZ 2/7] gdbus: Add g_dbus_proxy_get_client function
From: Luiz Augusto von Dentz @ 2013-01-13 16:24 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1357937240.1806.83.camel@aeonflux>
Hi Marcel,
On Fri, Jan 11, 2013 at 10:47 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> This is convenient as some callbacks don't provide the client which
>> the proxy belongs.
>
> and I did not add that one on purpose. You should know where your proxy
> belongs to and worst case hand it over via user_data.
I understand this as the user_data will always be the client pointer,
or a struct containing it, or you have to use a global variable, is
that what you really want? The use application has given the client
when registering the callbacks, so it is already passing it as
context. Now that being said, Im not saying the design utterly broken,
quite the contrary it work like a charm, it just could be more
convenient and that is what Im trying to achieve with this patch.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH BlueZ 1/7] gdbus: Add g_dbus_client_get_proxy
From: Luiz Augusto von Dentz @ 2013-01-13 16:09 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1357937184.1806.82.camel@aeonflux>
Hi Marcel,
On Fri, Jan 11, 2013 at 10:46 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> g_dbus_client_get_proxy gives the possibilitity to just check if a
>> proxy exist for the given path and interface pair instead of using
>> g_dbus_proxy_new which end up creating a proxy if it doesn't exists
>> which is not always necessary.
>
> why would we do that. You get the proxy via the client callbacks for
> proxy created or proxy removed.
That way we the user don't have to maintain a list of found proxies,
such a list exist and is maintained by GDBusClient.
> The proxy_new method is for dealing with services that do not have
> ObjectManager support.
Yep, so in one way or the other you are already letting the user
application search in the list of proxies maintained by GDBusClient,
the only thing this does is to make the proxy_lookup public so the
client don't have to maintain its how list only to be able to look
back when a proxy point to another, btw the reason proxy_new is not
enough is because it may lead to extra calls when the user application
may just want to bail out if the proxy is not found.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH 1/8] obexd: Handle empty path name in SetPath
From: Luiz Augusto von Dentz @ 2013-01-13 15:55 UTC (permalink / raw)
To: Christian Fetzer; +Cc: linux-bluetooth@vger.kernel.org, Christian Fetzer
In-Reply-To: <1357919757-10334-2-git-send-email-christian.fetzer@oss.bmw-carit.de>
Hi Christian,
On Fri, Jan 11, 2013 at 5:55 PM, Christian Fetzer
<christian.fetzer@oss.bmw-carit.de> wrote:
> From: Christian Fetzer <christian.fetzer@bmw-carit.de>
>
> If the empty path is given, an empty name should be sent via OBEX.
> Currently the name field is not set at all and later checks which
> depend on data->index will access invalid memory regions as g_strsplit
> returns NULL when an empty string is given.
>
> 0 0x000000000041a181 in g_obex_setpath (obex=obex@entry=0x662eb0, path=
> 0x20 <Address 0x20 out of bounds>, func=func@entry=0x42d300 <setpath_cb>,
> user_data=user_data@entry=0x668f10, err=err@entry=0x7fffffffda08)
> at gobex/gobex.c:1397
> 1 0x000000000042d395 in setpath_cb (obex=0x662eb0, err=0x0, rsp=<optimized out>,
> user_data=0x668f10) at obexd/client/session.c:902
> 2 0x0000000000418e54 in handle_response (obex=obex@entry=0x662eb0, err=err@entry=0x0,
> rsp=rsp@entry=0x668f40) at gobex/gobex.c:948
> 3 0x0000000000419d7a in incoming_data (io=<optimized out>, cond=<optimized out>,
> user_data=0x662eb0) at gobex/gobex.c:1191
> 4 0x00007ffff703c845 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
> 5 0x00007ffff703cb78 in ?? () from /usr/lib/libglib-2.0.so.0
> 6 0x00007ffff703cf72 in g_main_loop_run () from /usr/lib/libglib-2.0.so.0
> 7 0x000000000040def2 in main (argc=1, argv=0x7fffffffdd88) at obexd/src/main.c:323
> ---
> obexd/client/session.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/obexd/client/session.c b/obexd/client/session.c
> index 9cc824e..db37a86 100644
> --- a/obexd/client/session.c
> +++ b/obexd/client/session.c
> @@ -934,7 +934,7 @@ guint obc_session_setpath(struct obc_session *session, const char *path,
> p = pending_request_new(session, NULL, setpath_complete, data);
>
> /* Relative path */
> - if (path[0] != '/') {
> + if (path[0] != '/' && path[0] != 0) {
> first = data->remaining[data->index];
> data->index++;
> }
> --
> 1.8.1
But what empty means in this case, I would like to stick with cd
behavior, so empty should set back to root. Otherwise the patchset
seems fine.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: Obexd OPP filesend fails with Windows7 stack
From: Luiz Augusto von Dentz @ 2013-01-13 15:42 UTC (permalink / raw)
To: Syam Sidhardhan; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <D43D72D2B7084BA8B750CE7FD0D1C287@sisodomain.com>
Hi Syam,
On Fri, Jan 11, 2013 at 10:32 AM, Syam Sidhardhan <s.syam@samsung.com> wrote:
> Hi,
>
> We are using the obexd 0.48 version and testing the obexd.
> During testing OPP file send, we found one interoperability
> issue with the Windows7 PC stack and some commercialized devices
> available in the market.
>
> The issue is: OBEXD file transfer is getting failed
> once after the complete file content got transfferd. This is
> because of, we do a direct RFCOMM disconection rather than
> doing a proper OBEX disconection before.
>
> Does someone can help me a with a fix for this?
We probably need to send a disconnect command before disconnecting
RFCOMM, iirc there was someone else looking into a similar problem
with a Nokia phone, the strange part is that the remote stack has
acknowledged the transfer complete and in theory we could go ahead and
start another one using the same session.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH 1/1] test: Make map script a command line client
From: Luiz Augusto von Dentz @ 2013-01-13 15:29 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Christian Fetzer, linux-bluetooth@vger.kernel.org,
Christian Fetzer
In-Reply-To: <1357937329.1806.84.camel@aeonflux>
Hi Marcel, Christian,
On Fri, Jan 11, 2013 at 10:48 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Christian,
>
>> Rework the map-client test script into an interactive command line client.
>> Now multiple MCE functions can be called in one active session.
>> The script also allows to specify all filters or optional parameters including
>> auto completion.
>>
>> Change-Id: I9c9ede2bc958009c757384177cf06c081d984c98
>
> no Change-Id crap please.
I wonder if we should complicate more the testing scripts adding such
futures or start working in a proper C tool such as bluetoothctl e.g.
obexctl?
--
Luiz Augusto von Dentz
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox