Re: [PATCH] Gateway profile

[event <event.riga@gmail.com>]

On Sun, Jan 18, 2009 at 18:07, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi,
>
>> >> I've implemented gateway profile. I've tested basic things, like
>> >> place/cancel/answer call. Others are in development. Some could not be
>> >> tested as my carrier doesn't provide corresponding services (like
>> >> 3-way call, etc.) so any help welcome.
>> >
>> > thanks for the works, but can you please base the patch against the
>> > latest GIT tree. It is kinda hard to review things that might already
>> > have been implemented like sco_listen.
>> >
>> >  audio/audio-api.txt  |   94 +++++
>> >  audio/device.h       |    7
>> >  audio/gateway.c      |  938 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  audio/gateway.h      |   11
>> >  audio/manager.c      |  124 ++++--
>> >  common/glib-helper.c |   85 +++-
>> >  common/glib-helper.h |    1
>> >  7 files changed, 1205 insertions(+), 55 deletions(-)
>> >
>> > So any changes to glib-helper.[ch] have to be in a separate patch and
>> > need to be discussed independent from the gateway implementation.
>> >
>> > Any audio-api.txt stuff should also go separately since that has to be
>> > discussed. Also we can't send PCM data over D-Bus. It just doesn't work
>> > like that. We do have the internal IPC for that and plugins for ALSA,
>> > GStreamer and PulseAudio that should be used.
>> >
>> > However the most important part is that you follow the coding style and
>> > that is the kernel coding style. You make it really hard for us to
>> > review the code like this and it can't be applied. I really want you to
>> > add support for the gateway role to BlueZ, but the overall code in the
>> > project needs to follow the same rules.
>> >
>> > So please fix these issues first and then we do a deep review of it.
>>
>> Here is reworked and improved patches as you suggested with IPC support.
>>
>> But I have some doubts and questions:
>> 1. to distinguish between headset and gateway I've added one more alsa
>> config option "role" which could be master (for gateway and probably
>> a2dp source) or slave (which is default and works for headset and a2dp
>> sink). I don't really like this approach so if you have any other idea
>> it would be great.
>
> we should use the terms "headset", "gateway", "sink" and "source" as
> these are used through the specs.
>

But current configuration contains "type" which is either voice or
hifi. So should I set 4 mentioned above for type field? I mean this
will break backward compatibility.

>> 2. my cell phone closes SCO connection when it doesn't need one,
>> probably others act like this as well. SCO close results in
>> bluetooth_hsp_write returning an error. What would be the best way to
>> overcome this?
>
> No idea what's the problem here. You should already get a notice of the
> IPC that the channel is closed. On closed channels we have to discard
> any kind of PCM data from the PC.
>

That is how it should work with current implementation but it would
not be very nice for application developers as e.g. pause of the
player on the phone will result in snd_pcm_read (or how is it named)
returning an error. I've tested using pyalsaaudio which raises an
exception in this case.
If I would develop an application over such api I would say several bad words :)

>> 3. I've noticed that ipc interface duplicate dbus interface to some
>> extent. Why can't pcm_bluetooth work over dbus directly?
>
> D-Bus can't handle massive amount of PCM data payload. Also the ALSA
> plugins don't really like dealing with a D-Bus mainloop. Hence we do
> have the IPC as an alternate way of dealing with audio. We don't like to
> do it, but we have to.
>

You can add one more DBus call which will create socket and send audio
connection over it.

>> 4. Aren't there any legal issues with implementing bluez? As I noticed
>> in the spec that all the rights belong to Bluetooth SIG so it does
>> look neither "free as in free beer" nor "free as in freedom" :)
>
> BlueZ is a fully qualified host stack that complies with the SIG's
> requirements. We do have to update the listing with a headset gateway
> role at some point, but only once we have it fully working and actually
> used in a product.
>
> Regards
>
> Marcel
>
>
>


Vale,
Leonid Movshovich