Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added support for HFP from Ofono #172

Closed
wants to merge 2 commits into from
Closed

Added support for HFP from Ofono #172

wants to merge 2 commits into from

Conversation

tbultel
Copy link
Contributor

@tbultel tbultel commented Nov 22, 2018 •
edited
Loading

This brings HFP support over Ofono, using its DBus interface.

HFP-HF has been tested and both source and sink work. HFP-AG has not been tested yet.
The "transport" code has been slightly modified, in order to be able to have SCO without RFCOMM

The backend is robust to bluez-alsa restarts, in regard to Ofono, and is also robust to Ofono restarts.

@arkq
Copy link
Owner

arkq commented Nov 22, 2018

Many tanks for this PR. It indeed looks promising. I'll try to review it during this weekend, however right now I can say few things:

  1. The usage of __func__ in debug() calls. If the name of debugged function is desired, it should be added to the debug() macro definition, not to the call site. It generates a lot of boilerplate code.
  2. Help message (in the main.c) lacks ofono version number (if it was desired, and I'm not saying that it should be), so the argument list is shifted.
  3. The introduction of TRANSPORT_TYPE_OFONO_SCO. I do not know exactly why, but it seems too much for me (in almost every place it equals to TRANSPORT_TYPE_SCO) :D Maybe after thorough review it will make sense.

Extra thing to keep in mind:
I'd like the ofono code to be conditional (via preprocessor), however I will talk about this later, because I'm not sure yet whether ofono code or everything related to HFP/HSP.

Thierry Bultel added 2 commits November 23, 2018 16:11
log: added __func__ in debug macro
afb865a
Added support for HFP from Ofono
be05f7c 
Adds support for HFP-HF & HFP-AG over Ofono.
Namely, when Ofono is running, it registers as HFP profile to
bluez, preventing bluez-alsa to do so.
Fortunately, it has a DBus interface to get the connections
events and the audio file descriptor.
To enable Ofono support, bluez-alsa must be compiled with
--enable-ofono

Signed-off-by: Thierry Bultel <thierry.bultel@iot.bzh>
@tbultel
Copy link
Contributor Author

tbultel commented Nov 23, 2018 via email

@arkq
Copy link
Owner

arkq commented Nov 27, 2018

Sorry for the delay, however I'm rather busy lately. I will try to make a proper review in the next few days. I hope you don't mind.

@tbultel
Copy link
Contributor Author

tbultel commented Nov 28, 2018 via email

@arkq arkq closed this in 53bb612 Dec 2, 2018
@arkq
Copy link
Owner

arkq commented Dec 2, 2018

Hi,

I've took the liberty to make same changes by myself within this PR in order to speed up merging process (otherwise it would take many ping-pong-like conversations). In general this PR was OK, however there was some things to be fixed:

  • general code cleanup (it has looked more like PoC, than merge-ready commit - badly formatted docstrings, mixed indentation, function names taken from other places without renaming, etc.)
  • function reorganization within the ofono.c file in order to remove forward declarations
  • I've placed all oFono DBus related strings and values in the iface.h file
  • fixed many memory leakages (not freed glib error structs, not unrefed DBus replies, not initialized char* causing segfault, etc.)
  • fixed SCO file descriptor leakage when there is no PCM client connected
  • I've also prepared some generic framework for acquire/release handlers (in other commit), so this PR could be simplified

I hope you don't mind :)

Unfortunately, I was not able to test whether HFP-AG works... It seems, that it is required to configure modem within oFono in order to get AG profile. If you've got setup with AG configured, please test whether it works after my changes. Also, I've removed connect_pending flag, so right now there are warning messages that the operation is already in progress. I will think about some more elegant way of deciding whether to call "Connect" method, so for now I'm OK with this warning.

Once again may thanks for your work and making bluez-alsa better software!

@tbultel
Copy link
Contributor Author

tbultel commented Dec 2, 2018 via email

@arkq
Copy link
Owner

arkq commented Dec 2, 2018

I am just curious why you removed the ofono_unregister()

General idea is very simple. When DBus connection is terminated, every object registered in DBus is dropped as well (or should be dropped). That's exactly the case for pkill ofonod you've implemented. One cannot rely on others for cleanup procedures. This forces "us" (e.g. DBus, kernel itself, glibc) to implement cleanup actions upon e.g. connection lost or process termination (file descriptors are closed, heap memory is freed, mutexes are unlocked, etc.). So, most the time I'm exploiting this requirement by intentionally omitting cleanup upon main() return. One exception is filesystem, where cleanup is required (temporary files, named sockets are not unlinked). This eliminates code redundancy, hence saves some bytes.

All the functions in ctl_client.h are of interest for an application wanting to monitor the available transports (of both SCO and A2DP), in order to start or stop PCM loops. I hope this kind of thing would also be of interest to you.

Yes, this kind of work is desired :) See this PR: #95 However, I'm not very eager to merge such PR right now, because providing library adds some extra responsibility. There will be some API and ABI which should be maintained then. I will merge such a feature when the ctl-proto.h and ctl-client.h will be mature enough. Also keep in mind that there is a possibility for #160 to be merged in the near future.

@tbultel
Copy link
Contributor Author

tbultel commented Dec 3, 2018 via email

@tbultel
Copy link
Contributor Author

tbultel commented Dec 3, 2018 via email

@arkq
Copy link
Owner

arkq commented Dec 3, 2018 via email

@arkq
Copy link
Owner

arkq commented Dec 3, 2018 via email

@tbultel
Copy link
Contributor Author

tbultel commented Dec 3, 2018 via email

@arkq
Copy link
Owner

arkq commented Dec 3, 2018

I do not understand why, if you wakeup the io thread it will immediately close and forget the SCO transport. There is maybe something I am missing in the logic.

The only place where the SCO file descriptor can be closed is io_thread_sco(). When the new transport is created in the ofono_card_add(), the bt_fd is initialized to -1 and the thread is started (call to set_state(ACTIVE)). This "locks" thread_sco in the poll() until some event arrives (e.g. connection from the client). When poll() returns, there is a logic which adds new FDs to the poll (pfds[].fd = t->bt_fd) structure. When new SCO connection is initialized in ofono_agent_new_connection() it is required to wake thread_sco from the poll() call, so it could add SCO fd to the poll structure, which will allow proper SCO closing (prevents SCO fd leakage). The actual name of the signal might be indeed misleading (TRANSPORT_PCM_OPEN) - I will change that.

Sooo, finally the problem is, that when poll() wakes, thread_sco doesn't know that the PCM speaker/microphone is opened and closes SCO.

And now the question is, how bluealsa should behave when there is no client for reading SCO data, and why in your case, thread_sco sees spk_pcm.fd == -1?

Regarding the case where there is indeed PCM client, how bluealsa should act when new SCO connection arrives. I think that the most appropriate way is to let oFono know, that we are not able to handle audio, so it could release SCO connection - phone will not forward audio to the bluetooth. Or maybe it is OK to grab phone audio even though it will go straight to /dev/null so to speak. What do you think?

@arkq
Copy link
Owner

arkq commented Dec 3, 2018 •
edited
Loading

Hmmm... indeed it does not work.... It seems, that I had to break it just before making commit :|

EDIT:
OK, definitely I've made a mess with this transport_send_signal() call. It won't work that way, unfortunately this signal is required... I'll fix it for sure because it is my (double) fault: this bug and what's more embarrassing, pushing code without testing...

EDIT2:
Feel free to comment out this transport_send_signal(t, TRANSPORT_PCM_OPEN) call for now, so it will not block your work. I'll try to fix it tomorrow, because it seems that some thread_sco refactoring will be required.

arkq added a commit that referenced this pull request Dec 4, 2018
@arkq
Fix oFono SCO premature release
56fcc32 
In order to properly handle SCO socket file descriptor obtained from
the oFono, it is required to omit our internal acquire/release actions
executed during event handling. Also, it is not possible not to read
data from the socket when there is no PCM client connected, because it
will be not possible to release SCO (we have to read from it in order
to detect hangup).

Fixes #172
@arkq
Copy link
Owner

arkq commented Dec 4, 2018

I think, that I've fixed this premature SCO release caused by the introduction of the transport_send_signal(). Please, check if it will work for you (I've checked it with my phone, and I was able to send/receive audio to/from connected phone). Sorry, for the trouble caused by this bug.

@tbultel
Copy link
Contributor Author

tbultel commented Dec 4, 2018 via email

This was referenced Dec 15, 2018
Closed
Closed
@xavi-b
Copy link

xavi-b commented Oct 7, 2020

Hi,
It seems that @tbultel succeeded in making phone calls using bluealsa and ofono, do you have any information related to your ofono configuration, asoundrc configuration or anything else that could help me with my issue #385 ?

@xavi-b
Copy link

xavi-b commented Oct 15, 2020

@arkq, do you happen to have any ofono/bluealsa/asound configuration files/commands that could help me solve my issue ?

@dr-ni
Copy link

dr-ni commented Oct 25, 2020

yes, some more info woul be nice. I could use both streams like this:

sudo su
export LIBASOUND_THREAD_SAFE=0
bluealsa -p hfp-ofono

and in another terminal:
arecord -D bluealsa | aplay
to receive stream

and in another terminal:
arecord | aplay -D bluealsa
to send stream

works, but huge delay 20s :-(

cat ~/.asoundrc
defaults.bluealsa.service "org.bluealsa"
defaults.bluealsa.device "XX:XX:XX:XX:XX:XX"
defaults.bluealsa.profile "sco"
defaults.bluealsa.delay 10000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tbultel @arkq @xavi-b @dr-ni