Re: [PATCH] Gateway profile

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

On Mon, Jan 19, 2009 at 11:20, 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.
>
> you could actually and still have "sink" == "hifi" and then also
> "headset" == "voice" for example.
>
>> >> 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 :)
>
> Obviously the ALSA plugin (or GStreamer or PluseAudio for that matter)
> need to hide that fact and make it return a proper value instead of an
> error. While for playback we can just discard the audio data, for
> capture we might have to produce silence.
>
>> >> 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.
>
> It just doesn't work that way. You will be killing your performance and
> creating memory pressure. Especially on small and embedded systems. Also
> the latency is pretty bad. Trust me here.

I didn't meant to send audio data over dbus. My idea was like this:

     +-------+            +------+
     | bluez |            | alsa |
     +-------+            +------+
         |  get unix socket  |
         | <-----------------|
      create                 |
    unix socket              |
         |                   |
         |    send unix      |
         |   socket name     |
         |------------------>|
         |                 listen
         |                 for fd
         |                   |
         | send sco fd       |
         |------------------>|

first call is over Dbus and socket is sent over domain socket.
>
> Regards
>
> Marcel
>
>
>


Vale,
Leonid Movshovich