RFC: Add support for last call dialing

RFC: Add support for last call dialing

[Philippe De Swert]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

Hello all,

This is my first ofono patch and I have tried to follow the coding guidelines etc.
I did not find support to actually dial the last dialled call that I could use for
HFP with bluetooth. I did find some references but no method to do so. Thus I added
one myself. I took existing code as a guideline to implement this. 
This has been tested with several phone models and with the Bluetooth SIG testing tool.

Comments and corrections welcome.

Regards,

Philippe

[PATCH 1/4] include/voicecall : update ofono_voicecall_driver to support call last dialled number

[Philippe De Swert]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

Add new DBUS method to enable calling the last dialled number.
---
 include/voicecall.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/voicecall.h b/include/voicecall.h
index 4d0b3886..55beb980 100644
--- a/include/voicecall.h
+++ b/include/voicecall.h
@@ -60,6 +60,10 @@ struct ofono_voicecall_driver {
 			const struct ofono_phone_number *number,
 			enum ofono_clir_option clir, ofono_voicecall_cb_t cb,
 			void *data);
+	/* Dials the last number again, this handles the hfp profile last number
+         * dialing with the +BLDN AT command
+         */
+	void (*dial_last)(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb, void *data);
 	/* Answers an incoming call, this usually corresponds to ATA */
 	void (*answer)(struct ofono_voicecall *vc,
 			ofono_voicecall_cb_t cb, void *data);
-- 
2.11.0

[PATCH 2/4] voicecallmanager: Handle last number dialled DBUS call

[Philippe De Swert]

[-- Attachment #1: Type: text/plain, Size: 3141 bytes --]

Handle the new DialLast method on the voicecallmanager interface
---
 src/voicecall.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/src/voicecall.c b/src/voicecall.c
index 2ff9aa15..4eeb6621 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -1595,6 +1595,94 @@ static DBusMessage *manager_dial(DBusConnection *conn,
 	return __ofono_error_failed(msg);
 }
 
+static void manager_dial_last_callback(const struct ofono_error *error, void *data)
+{
+	struct ofono_voicecall *vc = data;
+	DBusMessage *reply;
+	gboolean need_to_emit;
+	struct voicecall *v;
+
+	v = dial_handle_result(vc, error, NULL, &need_to_emit);
+
+	if (v) {
+		const char *path = voicecall_build_path(vc, v->call);
+
+		reply = dbus_message_new_method_return(vc->pending);
+
+		dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH, &path,
+						DBUS_TYPE_INVALID);
+	} else {
+		reply = __ofono_error_failed(vc->pending);
+	}
+
+	__ofono_dbus_pending_reply(&vc->pending, reply);
+
+	if (need_to_emit)
+		voicecalls_emit_call_added(vc, v);
+}
+
+static int voicecall_dial_last(struct ofono_voicecall *vc,
+					ofono_voicecall_cb_t cb, void *data)
+{
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
+
+	if (g_slist_length(vc->call_list) >= MAX_VOICE_CALLS)
+		return -EPERM;
+
+	if (ofono_modem_get_online(modem) == FALSE)
+		return -ENETDOWN;
+
+	if (vc->driver->dial_last == NULL)
+		return -ENOTSUP;
+
+	if (voicecalls_have_incoming(vc))
+		return -EBUSY;
+
+	/* We can't have two dialing/alerting calls, reject outright */
+	if (voicecalls_num_connecting(vc) > 0)
+		return -EBUSY;
+
+	if (voicecalls_have_active(vc) && voicecalls_have_held(vc))
+		return -EBUSY;
+
+	vc->driver->dial_last(vc, cb, vc);
+
+	return 0;
+}
+
+static DBusMessage *manager_dial_last(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct ofono_voicecall *vc = data;
+	int err;
+
+	if (vc->pending || vc->dial_req || vc->pending_em)
+		return __ofono_error_busy(msg);
+
+	vc->pending = dbus_message_ref(msg);
+
+	err = voicecall_dial_last(vc, manager_dial_last_callback, vc);
+
+	if (err >= 0)
+		return NULL;
+
+	vc->pending = NULL;
+	dbus_message_unref(msg);
+
+	switch (err) {
+	case -EINVAL:
+		return __ofono_error_invalid_format(msg);
+
+	case -ENETDOWN:
+		return __ofono_error_not_available(msg);
+
+	case -ENOTSUP:
+		return __ofono_error_not_implemented(msg);
+	}
+
+	return __ofono_error_failed(msg);
+}
+
 static DBusMessage *manager_transfer(DBusConnection *conn,
 					DBusMessage *msg, void *data)
 {
@@ -2151,6 +2239,7 @@ static const GDBusMethodTable manager_methods[] = {
 		GDBUS_ARGS({ "number", "s" }, { "hide_callerid", "s" }),
 		GDBUS_ARGS({ "path", "o" }),
 		manager_dial) },
+	{ GDBUS_ASYNC_METHOD("DialLast", NULL, NULL, manager_dial_last)},
 	{ GDBUS_ASYNC_METHOD("Transfer", NULL, NULL, manager_transfer) },
 	{ GDBUS_ASYNC_METHOD("SwapCalls",  NULL, NULL, manager_swap_calls) },
 	{ GDBUS_ASYNC_METHOD("ReleaseAndAnswer", NULL, NULL,
-- 
2.11.0

[PATCH 3/4] hfpmodem: Send last call dialled request

[Philippe De Swert]

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

Handle the last call dialled request and send the required AT+BDLN command
for bluetooth HFP profile.
---
 drivers/hfpmodem/voicecall.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/hfpmodem/voicecall.c b/drivers/hfpmodem/voicecall.c
index ffdf4b7b..6dc03574 100644
--- a/drivers/hfpmodem/voicecall.c
+++ b/drivers/hfpmodem/voicecall.c
@@ -404,6 +404,27 @@ static void hfp_dial(struct ofono_voicecall *vc,
 	CALLBACK_WITH_FAILURE(cb, data);
 }
 
+static void hfp_dial_last(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb,
+			void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct cb_data *cbd = cb_data_new(cb, data);
+	char buf[256];
+
+	cbd->user = vc;
+	snprintf(buf, sizeof(buf), "AT+BLDN");
+
+	strcat(buf, ";");
+
+	if (g_at_chat_send(vd->chat, buf, none_prefix,
+				atd_cb, cbd, g_free) > 0)
+		return;
+
+	g_free(cbd);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+
+}
 static void hfp_template(const char *cmd, struct ofono_voicecall *vc,
 			GAtResultFunc result_cb, unsigned int affected_types,
 			ofono_voicecall_cb_t cb, void *data)
@@ -1268,6 +1289,7 @@ static struct ofono_voicecall_driver driver = {
 	.probe			= hfp_voicecall_probe,
 	.remove			= hfp_voicecall_remove,
 	.dial			= hfp_dial,
+	.dial_last		= hfp_dial_last,
 	.answer			= hfp_answer,
 	.hangup_active		= hfp_hangup,
 	.hold_all_active	= hfp_hold_all_active,
-- 
2.11.0

[PATCH 4/4] doc: Document the new DialLast voicecallmanager API addition

[Philippe De Swert]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

The new DialLast method to call the last dialled number for HFP needs
to be added to the documentation.
---
 doc/voicecallmanager-api.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/doc/voicecallmanager-api.txt b/doc/voicecallmanager-api.txt
index 34a9b25c..7aeb81f7 100644
--- a/doc/voicecallmanager-api.txt
+++ b/doc/voicecallmanager-api.txt
@@ -59,6 +59,16 @@ Methods		dict GetProperties()
 					 [service].Error.NotImplemented
 					 [service].Error.Failed
 
+		object DialLast()
+
+			Initiates a new outgoing call to the last dialled number.
+
+			Possible Errors: [service].Error.InProgress
+					 [service].Error.InvalidArguments
+					 [service].Error.InvalidFormat
+					 [service].Error.NotImplemented
+					 [service].Error.Failed
+
 		void Transfer()
 
 			Joins the currently Active (or Outgoing, depending
-- 
2.11.0

Re: [PATCH 2/4] voicecallmanager: Handle last number dialled DBUS call

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 4109 bytes --]

Hi Philippe,

On 10/18/2017 09:04 AM, Philippe De Swert wrote:
> Handle the new DialLast method on the voicecallmanager interface
> ---
>   src/voicecall.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 89 insertions(+)
> 
> diff --git a/src/voicecall.c b/src/voicecall.c
> index 2ff9aa15..4eeb6621 100644
> --- a/src/voicecall.c
> +++ b/src/voicecall.c
> @@ -1595,6 +1595,94 @@ static DBusMessage *manager_dial(DBusConnection *conn,
>   	return __ofono_error_failed(msg);
>   }
>   
> +static void manager_dial_last_callback(const struct ofono_error *error, void *data)
> +{
> +	struct ofono_voicecall *vc = data;
> +	DBusMessage *reply;
> +	gboolean need_to_emit;
> +	struct voicecall *v;
> +
> +	v = dial_handle_result(vc, error, NULL, &need_to_emit);

dial_handle_result is a bit tricky because it handles situations where 
ATD returns immediately and then the call is notified via an unsolicited 
notification, or ATD returns first.

In the case of +BLDN, I believe HFP spec mandates that BLDN always 
returns first, which means that much of the logic in dial_handle_result 
is not applicable.  It may be better to just perform the necessary logic 
directly here.

In the current proposal, we will always generate a new call with an 
empty phone number which will (hopefully) eventually be updated.  I'm 
not so sure that this is a great approach, but I can't think of anything 
better...

> +
> +	if (v) {
> +		const char *path = voicecall_build_path(vc, v->call);
> +
> +		reply = dbus_message_new_method_return(vc->pending);
> +
> +		dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH, &path,
> +						DBUS_TYPE_INVALID);
> +	} else {
> +		reply = __ofono_error_failed(vc->pending);
> +	}
> +
> +	__ofono_dbus_pending_reply(&vc->pending, reply);
> +
> +	if (need_to_emit)
> +		voicecalls_emit_call_added(vc, v);
> +}
> +
> +static int voicecall_dial_last(struct ofono_voicecall *vc,
> +					ofono_voicecall_cb_t cb, void *data)
> +{
> +	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
> +
> +	if (g_slist_length(vc->call_list) >= MAX_VOICE_CALLS)
> +		return -EPERM;
> +
> +	if (ofono_modem_get_online(modem) == FALSE)
> +		return -ENETDOWN;
> +
> +	if (vc->driver->dial_last == NULL)
> +		return -ENOTSUP;
> +
> +	if (voicecalls_have_incoming(vc))
> +		return -EBUSY;
> +
> +	/* We can't have two dialing/alerting calls, reject outright */
> +	if (voicecalls_num_connecting(vc) > 0)
> +		return -EBUSY;
> +
> +	if (voicecalls_have_active(vc) && voicecalls_have_held(vc))
> +		return -EBUSY;
> +
> +	vc->driver->dial_last(vc, cb, vc);
> +
> +	return 0;
> +}
> +
> +static DBusMessage *manager_dial_last(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_voicecall *vc = data;
> +	int err;
> +
> +	if (vc->pending || vc->dial_req || vc->pending_em)
> +		return __ofono_error_busy(msg);
> +
> +	vc->pending = dbus_message_ref(msg);
> +
> +	err = voicecall_dial_last(vc, manager_dial_last_callback, vc);
> +
> +	if (err >= 0)
> +		return NULL;
> +
> +	vc->pending = NULL;
> +	dbus_message_unref(msg);
> +
> +	switch (err) {
> +	case -EINVAL:
> +		return __ofono_error_invalid_format(msg);
> +
> +	case -ENETDOWN:
> +		return __ofono_error_not_available(msg);
> +
> +	case -ENOTSUP:
> +		return __ofono_error_not_implemented(msg);
> +	}
> +
> +	return __ofono_error_failed(msg);
> +}
> +
>   static DBusMessage *manager_transfer(DBusConnection *conn,
>   					DBusMessage *msg, void *data)
>   {
> @@ -2151,6 +2239,7 @@ static const GDBusMethodTable manager_methods[] = {
>   		GDBUS_ARGS({ "number", "s" }, { "hide_callerid", "s" }),
>   		GDBUS_ARGS({ "path", "o" }),
>   		manager_dial) },
> +	{ GDBUS_ASYNC_METHOD("DialLast", NULL, NULL, manager_dial_last)},
>   	{ GDBUS_ASYNC_METHOD("Transfer", NULL, NULL, manager_transfer) },
>   	{ GDBUS_ASYNC_METHOD("SwapCalls",  NULL, NULL, manager_swap_calls) },
>   	{ GDBUS_ASYNC_METHOD("ReleaseAndAnswer", NULL, NULL,
> 

Regards,
-Denis

Re: [PATCH 3/4] hfpmodem: Send last call dialled request

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

Hi Philippe,

On 10/18/2017 09:04 AM, Philippe De Swert wrote:
> Handle the last call dialled request and send the required AT+BDLN command
> for bluetooth HFP profile.
> ---
>   drivers/hfpmodem/voicecall.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/hfpmodem/voicecall.c b/drivers/hfpmodem/voicecall.c
> index ffdf4b7b..6dc03574 100644
> --- a/drivers/hfpmodem/voicecall.c
> +++ b/drivers/hfpmodem/voicecall.c
> @@ -404,6 +404,27 @@ static void hfp_dial(struct ofono_voicecall *vc,
>   	CALLBACK_WITH_FAILURE(cb, data);
>   }
>   
> +static void hfp_dial_last(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb,
> +			void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +	char buf[256];
> +
> +	cbd->user = vc;
> +	snprintf(buf, sizeof(buf), "AT+BLDN");
> +
> +	strcat(buf, ";");

Why not include the ';' in the snprintf?  Also, I don't think this is 
needed?

> +
> +	if (g_at_chat_send(vd->chat, buf, none_prefix,
> +				atd_cb, cbd, g_free) > 0)
> +		return;
> +
> +	g_free(cbd);
> +
> +	CALLBACK_WITH_FAILURE(cb, data);
> +
> +}
>   static void hfp_template(const char *cmd, struct ofono_voicecall *vc,
>   			GAtResultFunc result_cb, unsigned int affected_types,
>   			ofono_voicecall_cb_t cb, void *data)
> @@ -1268,6 +1289,7 @@ static struct ofono_voicecall_driver driver = {
>   	.probe			= hfp_voicecall_probe,
>   	.remove			= hfp_voicecall_remove,
>   	.dial			= hfp_dial,
> +	.dial_last		= hfp_dial_last,
>   	.answer			= hfp_answer,
>   	.hangup_active		= hfp_hangup,
>   	.hold_all_active	= hfp_hold_all_active,
> 

Regards,
-Denis

Re: [PATCH 2/4] voicecallmanager: Handle last number dialled DBUS call

[Philippe De Swert]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]

Hello Denis,

Thank you for the quick review!

On 18/10/17 20:03, Denis Kenzior wrote:
> Hi Philippe,
>
> On 10/18/2017 09:04 AM, Philippe De Swert wrote:
>> Handle the new DialLast method on the voicecallmanager interface
>> ---
>>   src/voicecall.c | 89 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 89 insertions(+)
>>
>> diff --git a/src/voicecall.c b/src/voicecall.c
>> index 2ff9aa15..4eeb6621 100644
>> --- a/src/voicecall.c
>> +++ b/src/voicecall.c
>> @@ -1595,6 +1595,94 @@ static DBusMessage 
>> *manager_dial(DBusConnection *conn,
>>       return __ofono_error_failed(msg);
>>   }
>>   +static void manager_dial_last_callback(const struct ofono_error 
>> *error, void *data)
>> +{
>> +    struct ofono_voicecall *vc = data;
>> +    DBusMessage *reply;
>> +    gboolean need_to_emit;
>> +    struct voicecall *v;
>> +
>> +    v = dial_handle_result(vc, error, NULL, &need_to_emit);
>
> dial_handle_result is a bit tricky because it handles situations where 
> ATD returns immediately and then the call is notified via an 
> unsolicited notification, or ATD returns first.
>
> In the case of +BLDN, I believe HFP spec mandates that BLDN always 
> returns first, which means that much of the logic in 
> dial_handle_result is not applicable.  It may be better to just 
> perform the necessary logic directly here.
>
> In the current proposal, we will always generate a new call with an 
> empty phone number which will (hopefully) eventually be updated.  I'm 
> not so sure that this is a great approach, but I can't think of 
> anything better...

Not sure I understand your comment completely here. So I don't use 
dial_handle_result and just setup a voicecall from the callback 
(voicecall_create)? Or do you mean it is not ideal but acceptable for now?

Regarding your second comment, yes the extra addition of ';' at the end 
should go. Once I know how to deal with your first comment I'll update 
the patch and send an extra one for memory dialling which is similar to 
these.

Thanks,

Philippe.

Re: [PATCH 2/4] voicecallmanager: Handle last number dialled DBUS call

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]

Hi Philippe,

>>> +    v = dial_handle_result(vc, error, NULL, &need_to_emit);
>>
>> dial_handle_result is a bit tricky because it handles situations where 
>> ATD returns immediately and then the call is notified via an 
>> unsolicited notification, or ATD returns first.
>>
>> In the case of +BLDN, I believe HFP spec mandates that BLDN always 
>> returns first, which means that much of the logic in 
>> dial_handle_result is not applicable.  It may be better to just 
>> perform the necessary logic directly here.
>>
>> In the current proposal, we will always generate a new call with an 
>> empty phone number which will (hopefully) eventually be updated.  I'm 
>> not so sure that this is a great approach, but I can't think of 
>> anything better...
> 
> Not sure I understand your comment completely here. So I don't use 
> dial_handle_result and just setup a voicecall from the callback 
> (voicecall_create)? Or do you mean it is not ideal but acceptable for now?

So in dial_last_callback, instead of calling dial_handle_result, just 
add in the necessary logic from dial_handle_result assuming that the 
BLDN callback is called first.  Commit 
3b951d20f4ae2d6d11b6e7a7485b013aadde050c should make that easier for 
you.  E.g. just call synthethize_outgoing_call inside 
dial_last_callback, reply to the dbus message and emit the signal.

> 
> Regarding your second comment, yes the extra addition of ';' at the end 
> should go. Once I know how to deal with your first comment I'll update 
> the patch and send an extra one for memory dialling which is similar to 
> these.
> 

Sounds good.

Regards,
-Denis