Re: [PATCH] Gateway profile

[Marcel Holtmann <marcel@holtmann.org>]

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.

Regards

Marcel