[PATCHv3 1/3] hfpmodem: Retry AT+CLCC request after outgoing callsetup

[PATCHv3 1/3] hfpmodem: Retry AT+CLCC request after outgoing callsetup

[Timo Mueller]

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

From: Timo Mueller <timo.mueller@bmw-carit.de>

Currently the list of current calls is requested right after an
outgoing callsetup has been reported by the AG device. If the AG
device updates the list after reporting the callsetup the +CLCC
response may not contain the new call and it is not registered
with oFono.

In this case AT+CLCC should be requested again once after a small
delay.

AT sequence that exhibited the failure (AG device was a Nokia N9
placing an outgoing call)

< \r\n+CIEV: 5,2\r\n
> AT+CLCC\r
< \r\nOK\r\n
< \r\n+VGS=7\r\n
< \r\n+VGM=7\r\n
< \r\n+CIEV: 5,3\r\n
< \r\n+CIEV: 4,1\r\n
< \r\n+CIEV: 5,0\r\n
---
v3: Splitted clcc_poll_cb to avoid parsing of the clcc result twice
when a dialing call was found in the response.

 drivers/hfpmodem/voicecall.c | 45 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/hfpmodem/voicecall.c b/drivers/hfpmodem/voicecall.c
index 33dd05e..9b05dbc 100644
--- a/drivers/hfpmodem/voicecall.c
+++ b/drivers/hfpmodem/voicecall.c
@@ -203,11 +203,10 @@ static void release_with_status(struct ofono_voicecall *vc, int status)
 	}
 }
 
-static void clcc_poll_cb(gboolean ok, GAtResult *result, gpointer user_data)
+static void handle_clcc_response(gboolean ok, GSList *calls, gpointer user_data)
 {
 	struct ofono_voicecall *vc = user_data;
 	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
-	GSList *calls;
 	GSList *n, *o;
 	struct ofono_call *nc, *oc;
 	unsigned int num_active = 0;
@@ -217,8 +216,6 @@ static void clcc_poll_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	if (!ok)
 		return;
 
-	calls = at_util_parse_clcc(result);
-
 	n = calls;
 	o = vd->calls;
 
@@ -295,6 +292,43 @@ static void clcc_poll_cb(gboolean ok, GAtResult *result, gpointer user_data)
 							vc);
 }
 
+static void clcc_poll_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	GSList *calls;
+
+	if (!ok)
+		return;
+
+	calls = at_util_parse_clcc(result);
+
+	handle_clcc_response(ok, calls, user_data);
+}
+
+static void clcc_poll_dialing_cb(gboolean ok, GAtResult *result,
+			gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GSList *calls, *dialing;
+
+	if (!ok)
+		return;
+
+	calls = at_util_parse_clcc(result);
+	dialing = find_dialing(calls);
+
+	if (dialing == NULL) {
+		if (vd->clcc_source)
+			g_source_remove(vd->clcc_source);
+
+		vd->clcc_source = g_timeout_add(POLL_CLCC_DELAY,
+												poll_clcc, vc);
+		return;
+	}
+
+	handle_clcc_response(ok, calls, user_data);
+}
+
 static gboolean poll_clcc(gpointer user_data)
 {
 	struct ofono_voicecall *vc = user_data;
@@ -977,7 +1011,8 @@ static void ciev_callsetup_notify(struct ofono_voicecall *vc,
 		 * from AG: query and create call.
 		 */
 		g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix,
-				clcc_poll_cb, vc, NULL);
+				clcc_poll_dialing_cb, vc, NULL);
+
 		break;
 
 	case 3:
-- 
1.7.11.7

[PATCHv3 2/3] hfp_hf: Add generic hfp modem vendor

[Timo Mueller]

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

From: Timo Mueller <timo.mueller@bmw-carit.de>

---
 drivers/hfpmodem/vendor.h    | 25 +++++++++++++++++++++++++
 drivers/hfpmodem/voicecall.c |  3 +++
 plugins/hfp_hf_bluez4.c      |  4 +++-
 plugins/hfp_hf_bluez5.c      |  5 ++++-
 4 files changed, 35 insertions(+), 2 deletions(-)
 create mode 100644 drivers/hfpmodem/vendor.h

diff --git a/drivers/hfpmodem/vendor.h b/drivers/hfpmodem/vendor.h
new file mode 100644
index 0000000..fca847a
--- /dev/null
+++ b/drivers/hfpmodem/vendor.h
@@ -0,0 +1,25 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2013  BWM Car IT GmbH. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+enum ofono_vendor {
+	OFONO_VENDOR_GENERIC = 0
+};
diff --git a/drivers/hfpmodem/voicecall.c b/drivers/hfpmodem/voicecall.c
index 9b05dbc..7d2988e 100644
--- a/drivers/hfpmodem/voicecall.c
+++ b/drivers/hfpmodem/voicecall.c
@@ -35,6 +35,7 @@
 #include <ofono/log.h>
 #include <ofono/modem.h>
 #include <ofono/voicecall.h>
+#include "vendor.h"
 
 #include "common.h"
 
@@ -60,6 +61,7 @@ struct voicecall_data {
 	unsigned int clcc_source;
 	unsigned int expect_release_source;
 	unsigned int clip_source;
+	unsigned int vendor;
 };
 
 struct release_id_req {
@@ -1170,6 +1172,7 @@ static int hfp_voicecall_probe(struct ofono_voicecall *vc, unsigned int vendor,
 	vd->chat = g_at_chat_clone(info->chat);
 	vd->ag_features = info->ag_features;
 	vd->ag_mpty_features = info->ag_mpty_features;
+	vd->vendor = vendor;
 
 	memcpy(vd->cind_pos, info->cind_pos, HFP_INDICATOR_LAST);
 	memcpy(vd->cind_val, info->cind_val, HFP_INDICATOR_LAST);
diff --git a/plugins/hfp_hf_bluez4.c b/plugins/hfp_hf_bluez4.c
index 450c183..e90410c 100644
--- a/plugins/hfp_hf_bluez4.c
+++ b/plugins/hfp_hf_bluez4.c
@@ -46,6 +46,7 @@
 #include <ofono/handsfree.h>
 
 #include <drivers/hfpmodem/slc.h>
+#include <drivers/hfpmodem/vendor.h>
 
 #include "bluez4.h"
 
@@ -482,11 +483,12 @@ static int hfp_disable(struct ofono_modem *modem)
 static void hfp_pre_sim(struct ofono_modem *modem)
 {
 	struct hfp_data *data = ofono_modem_get_data(modem);
+	enum ofono_vendor vendor = OFONO_VENDOR_GENERIC;
 
 	DBG("%p", modem);
 
 	ofono_devinfo_create(modem, 0, "hfpmodem", data->handsfree_address);
-	ofono_voicecall_create(modem, 0, "hfpmodem", &data->info);
+	ofono_voicecall_create(modem, vendor, "hfpmodem", &data->info);
 	ofono_netreg_create(modem, 0, "hfpmodem", &data->info);
 	ofono_call_volume_create(modem, 0, "hfpmodem", &data->info);
 	ofono_handsfree_create(modem, 0, "hfpmodem", &data->info);
diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 2f4a89e..c57bb6e 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -49,6 +49,7 @@
 #include <ofono/handsfree.h>
 
 #include <drivers/hfpmodem/slc.h>
+#include <drivers/hfpmodem/vendor.h>
 
 #include "bluez5.h"
 
@@ -237,12 +238,14 @@ static int hfp_disable(struct ofono_modem *modem)
 static void hfp_pre_sim(struct ofono_modem *modem)
 {
 	struct hfp *hfp = ofono_modem_get_data(modem);
+	enum ofono_vendor vendor = OFONO_VENDOR_GENERIC;
+
 	char *address = (char *) ofono_modem_get_string(modem, "Remote");
 
 	DBG("%p", modem);
 
 	ofono_devinfo_create(modem, 0, "hfpmodem", address);
-	ofono_voicecall_create(modem, 0, "hfpmodem", &hfp->info);
+	ofono_voicecall_create(modem, vendor, "hfpmodem", &hfp->info);
 	ofono_netreg_create(modem, 0, "hfpmodem", &hfp->info);
 	ofono_handsfree_create(modem, 0, "hfpmodem", &hfp->info);
 	ofono_call_volume_create(modem, 0, "hfpmodem", &hfp->info);
-- 
1.7.11.7

[PATCHv3 3/3] hfpmodem: Add N9 quirk to run CLCC after callheld=<2 or 3>

[Timo Mueller]

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

From: Timo Mueller <timo.mueller@bmw-carit.de>

On the Nokia N9 with a call being active, placing a second call on the
AG device will lead to the active call being put on hold. During the
transition the phone does not update the list of current calls. This
leads to an +CLCC response that does not contain the new outgoing
call, even though an outgoing callsetup was reported.

The list will be updated once the active call was put on hold and
callheld=2 is reported. In this case the list has to be requested
again (AT+CLCC).

AT sequence that exhibited the failure (AG device was a Nokia N9
having an active outgoing call and placing a second outgoing call)

< \r\n+CIEV: 5,2\r\n
> AT+CLCC\r
< \r\n+CLCC: 1,0,0,0,0,"+49xxx1",145\r\n
< \r\nOK\r\n
< \r\n+CIEV: 6,2\r\n
< \r\n+CIEV: 5,3\r\n
< \r\n+CIEV: 6,1\r\n
> AT+CLCC\r
< \r\n+CIEV: 5,0\r\n
< \r\n+CLCC: 1,0,1,0,0,"+49xxx1",145\r\n
< \r\n+CLCC: 2,0,0,0,0,"+49xxx2",145\r\n
< \r\nOK\r\n
---
 drivers/hfpmodem/vendor.h    |  3 ++-
 drivers/hfpmodem/voicecall.c | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/hfpmodem/vendor.h b/drivers/hfpmodem/vendor.h
index fca847a..597ddee 100644
--- a/drivers/hfpmodem/vendor.h
+++ b/drivers/hfpmodem/vendor.h
@@ -21,5 +21,6 @@
  */
 
 enum ofono_vendor {
-	OFONO_VENDOR_GENERIC = 0
+	OFONO_VENDOR_GENERIC = 0,
+	OFONO_VENDOR_NOKIA_N9
 };
diff --git a/drivers/hfpmodem/voicecall.c b/drivers/hfpmodem/voicecall.c
index 7d2988e..af2aa75 100644
--- a/drivers/hfpmodem/voicecall.c
+++ b/drivers/hfpmodem/voicecall.c
@@ -1041,13 +1041,20 @@ out:
 	vd->cind_val[HFP_INDICATOR_CALLSETUP] = value;
 }
 
+static int needs_callheld_clcc_quirk(int vendor)
+{
+		return vendor == OFONO_VENDOR_NOKIA_N9;
+}
+
 static void ciev_callheld_notify(struct ofono_voicecall *vc,
 					unsigned int value)
 {
 	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
 	GSList *l;
+	GSList *dialing;
 	struct ofono_call *call;
 	unsigned int callheld = vd->cind_val[HFP_INDICATOR_CALLHELD];
+	unsigned int callsetup = vd->cind_val[HFP_INDICATOR_CALLSETUP];
 
 	switch (value) {
 	case 0:
@@ -1083,6 +1090,16 @@ static void ciev_callheld_notify(struct ofono_voicecall *vc,
 				call->status = CALL_STATUS_HELD;
 				ofono_voicecall_notify(vc, call);
 			}
+
+			if (needs_callheld_clcc_quirk(vd->vendor)) {
+				if (callsetup == 2 || callsetup == 3) {
+					dialing = find_dialing(vd->calls);
+
+					if (dialing == NULL)
+						g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix,
+								clcc_poll_cb, vc, NULL);
+				}
+			}
 		} else if (callheld == 1) {
 			if (vd->clcc_source)
 				g_source_remove(vd->clcc_source);
-- 
1.7.11.7

Re: [PATCHv3 3/3] hfpmodem: Add N9 quirk to run CLCC after callheld=<2 or 3>

[Timo =?unknown-8bit?q?M=C3=BCller?=]

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

Timo Mueller wrote, On 13.02.2013 09:36:
> From: Timo Mueller <timo.mueller@bmw-carit.de>
>
> On the Nokia N9 with a call being active, placing a second call on the
> AG device will lead to the active call being put on hold. During the
> transition the phone does not update the list of current calls. This
> leads to an +CLCC response that does not contain the new outgoing
> call, even though an outgoing callsetup was reported.
>
> The list will be updated once the active call was put on hold and
> callheld=2 is reported. In this case the list has to be requested
> again (AT+CLCC).
>
> AT sequence that exhibited the failure (AG device was a Nokia N9
> having an active outgoing call and placing a second outgoing call)
>
> < \r\n+CIEV: 5,2\r\n
>> AT+CLCC\r
> < \r\n+CLCC: 1,0,0,0,0,"+49xxx1",145\r\n
> < \r\nOK\r\n
> < \r\n+CIEV: 6,2\r\n
> < \r\n+CIEV: 5,3\r\n
> < \r\n+CIEV: 6,1\r\n
>> AT+CLCC\r
> < \r\n+CIEV: 5,0\r\n
> < \r\n+CLCC: 1,0,1,0,0,"+49xxx1",145\r\n
> < \r\n+CLCC: 2,0,0,0,0,"+49xxx2",145\r\n
> < \r\nOK\r\n
> ---
>   drivers/hfpmodem/vendor.h    |  3 ++-
>   drivers/hfpmodem/voicecall.c | 17 +++++++++++++++++
>   2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hfpmodem/vendor.h b/drivers/hfpmodem/vendor.h
> index fca847a..597ddee 100644
> --- a/drivers/hfpmodem/vendor.h
> +++ b/drivers/hfpmodem/vendor.h
> @@ -21,5 +21,6 @@
>    */
>
>   enum ofono_vendor {
> -	OFONO_VENDOR_GENERIC = 0
> +	OFONO_VENDOR_GENERIC = 0,
> +	OFONO_VENDOR_NOKIA_N9
>   };
> diff --git a/drivers/hfpmodem/voicecall.c b/drivers/hfpmodem/voicecall.c
> index 7d2988e..af2aa75 100644
> --- a/drivers/hfpmodem/voicecall.c
> +++ b/drivers/hfpmodem/voicecall.c
> @@ -1041,13 +1041,20 @@ out:
>   	vd->cind_val[HFP_INDICATOR_CALLSETUP] = value;
>   }
>
> +static int needs_callheld_clcc_quirk(int vendor)
> +{
> +		return vendor == OFONO_VENDOR_NOKIA_N9;
> +}
> +
>   static void ciev_callheld_notify(struct ofono_voicecall *vc,
>   					unsigned int value)
>   {
>   	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
>   	GSList *l;
> +	GSList *dialing;
>   	struct ofono_call *call;
>   	unsigned int callheld = vd->cind_val[HFP_INDICATOR_CALLHELD];
> +	unsigned int callsetup = vd->cind_val[HFP_INDICATOR_CALLSETUP];
>
>   	switch (value) {
>   	case 0:
> @@ -1083,6 +1090,16 @@ static void ciev_callheld_notify(struct ofono_voicecall *vc,
>   				call->status = CALL_STATUS_HELD;
>   				ofono_voicecall_notify(vc, call);
>   			}
> +
> +			if (needs_callheld_clcc_quirk(vd->vendor)) {
> +				if (callsetup == 2 || callsetup == 3) {
> +					dialing = find_dialing(vd->calls);
> +
> +					if (dialing == NULL)
> +						g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix,
> +								clcc_poll_cb, vc, NULL);
> +				}
> +			}
>   		} else if (callheld == 1) {
>   			if (vd->clcc_source)
>   				g_source_remove(vd->clcc_source);
>

ping

Re: [PATCHv3 1/3] hfpmodem: Retry AT+CLCC request after outgoing callsetup

[Denis Kenzior]

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

Hi Timo,

On 02/13/2013 02:36 AM, Timo Mueller wrote:
> From: Timo Mueller<timo.mueller@bmw-carit.de>
>
> Currently the list of current calls is requested right after an
> outgoing callsetup has been reported by the AG device. If the AG
> device updates the list after reporting the callsetup the +CLCC
> response may not contain the new call and it is not registered
> with oFono.
>
> In this case AT+CLCC should be requested again once after a small
> delay.
>
> AT sequence that exhibited the failure (AG device was a Nokia N9
> placing an outgoing call)
>
> <  \r\n+CIEV: 5,2\r\n
>> AT+CLCC\r
> <  \r\nOK\r\n
> <  \r\n+VGS=7\r\n
> <  \r\n+VGM=7\r\n
> <  \r\n+CIEV: 5,3\r\n
> <  \r\n+CIEV: 4,1\r\n
> <  \r\n+CIEV: 5,0\r\n
> ---
> v3: Splitted clcc_poll_cb to avoid parsing of the clcc result twice
> when a dialing call was found in the response.

I'm fine with this version, however I still would prefer that we hid the 
poll-for-dialing logic behind the N9 vendor quirk.

Regards,
-Denis

Re: [PATCHv3 2/3] hfp_hf: Add generic hfp modem vendor

[Denis Kenzior]

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

Hi Timo,

On 02/13/2013 02:36 AM, Timo Mueller wrote:
> From: Timo Mueller<timo.mueller@bmw-carit.de>
>
> ---
>   drivers/hfpmodem/vendor.h    | 25 +++++++++++++++++++++++++
>   drivers/hfpmodem/voicecall.c |  3 +++
>   plugins/hfp_hf_bluez4.c      |  4 +++-
>   plugins/hfp_hf_bluez5.c      |  5 ++++-
>   4 files changed, 35 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/hfpmodem/vendor.h
>

Please split up this patch in line with our patch submission guidelines. 
  Refer to HACKING, 'Submitting Patches' section.

I prefer this to be broken into four patches:
- vendor.h changes
- voicecall.c changes
- hfp_hf_bluez4 changes
- hfp_hf_bluez5 changes

Re: [PATCHv3 3/3] hfpmodem: Add N9 quirk to run CLCC after callheld=<2 or 3>

[Denis Kenzior]

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

Hi Timo,

On 02/13/2013 02:36 AM, Timo Mueller wrote:
> From: Timo Mueller<timo.mueller@bmw-carit.de>
>
> On the Nokia N9 with a call being active, placing a second call on the
> AG device will lead to the active call being put on hold. During the
> transition the phone does not update the list of current calls. This
> leads to an +CLCC response that does not contain the new outgoing
> call, even though an outgoing callsetup was reported.
>
> The list will be updated once the active call was put on hold and
> callheld=2 is reported. In this case the list has to be requested
> again (AT+CLCC).
>
> AT sequence that exhibited the failure (AG device was a Nokia N9
> having an active outgoing call and placing a second outgoing call)
>
> <  \r\n+CIEV: 5,2\r\n
>> AT+CLCC\r
> <  \r\n+CLCC: 1,0,0,0,0,"+49xxx1",145\r\n
> <  \r\nOK\r\n
> <  \r\n+CIEV: 6,2\r\n
> <  \r\n+CIEV: 5,3\r\n
> <  \r\n+CIEV: 6,1\r\n
>> AT+CLCC\r
> <  \r\n+CIEV: 5,0\r\n
> <  \r\n+CLCC: 1,0,1,0,0,"+49xxx1",145\r\n
> <  \r\n+CLCC: 2,0,0,0,0,"+49xxx2",145\r\n
> <  \r\nOK\r\n
> ---
>   drivers/hfpmodem/vendor.h    |  3 ++-
>   drivers/hfpmodem/voicecall.c | 17 +++++++++++++++++
>   2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hfpmodem/vendor.h b/drivers/hfpmodem/vendor.h
> index fca847a..597ddee 100644
> --- a/drivers/hfpmodem/vendor.h
> +++ b/drivers/hfpmodem/vendor.h
> @@ -21,5 +21,6 @@
>    */
>
>   enum ofono_vendor {
> -	OFONO_VENDOR_GENERIC = 0
> +	OFONO_VENDOR_GENERIC = 0,
> +	OFONO_VENDOR_NOKIA_N9
>   };
> diff --git a/drivers/hfpmodem/voicecall.c b/drivers/hfpmodem/voicecall.c
> index 7d2988e..af2aa75 100644
> --- a/drivers/hfpmodem/voicecall.c
> +++ b/drivers/hfpmodem/voicecall.c
> @@ -1041,13 +1041,20 @@ out:
>   	vd->cind_val[HFP_INDICATOR_CALLSETUP] = value;
>   }
>
> +static int needs_callheld_clcc_quirk(int vendor)
> +{
> +		return vendor == OFONO_VENDOR_NOKIA_N9;
> +}
> +

Please don't do this, a switch/case statement is just as effective and 
easier to read.

>   static void ciev_callheld_notify(struct ofono_voicecall *vc,
>   					unsigned int value)
>   {
>   	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
>   	GSList *l;
> +	GSList *dialing;
>   	struct ofono_call *call;
>   	unsigned int callheld = vd->cind_val[HFP_INDICATOR_CALLHELD];
> +	unsigned int callsetup = vd->cind_val[HFP_INDICATOR_CALLSETUP];
>
>   	switch (value) {
>   	case 0:
> @@ -1083,6 +1090,16 @@ static void ciev_callheld_notify(struct ofono_voicecall *vc,
>   				call->status = CALL_STATUS_HELD;
>   				ofono_voicecall_notify(vc, call);
>   			}
> +
> +			if (needs_callheld_clcc_quirk(vd->vendor)) {

Have you thought of a way to set / detect when a device requires this quirk?

> +				if (callsetup == 2 || callsetup == 3) {
> +					dialing = find_dialing(vd->calls);
> +
> +					if (dialing == NULL)
> +						g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix,
> +								clcc_poll_cb, vc, NULL);
> +				}
> +			}
>   		} else if (callheld == 1) {
>   			if (vd->clcc_source)
>   				g_source_remove(vd->clcc_source);

Regards,
-Denis