Re: [PATCH] Gateway profile

Marcel Holtmann
2009-01-19T10:11:22Z

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.

then you still have to handle the integration of D-Bus mainloop (or call
it message parsing/handling) into an ALSA plugin. ALSA is just a total
broken concept when it comes to virtual sound cards. It works great for
actual physical devices, but that is it.

Regards

Marcel