Re: [PATCH v4 1/3] huawei: detect possible secondary device

Re: [PATCH v4 1/3] huawei: detect possible secondary device

[Denis Kenzior]

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

Hi Kalle,

>  struct huawei_data {
> -	GAtChat *chat;
> +	GAtChat *chat, *event;
>  };
> 

Minor nitpick, but we prefer:
GAtChat *chat;
GAtChat *event;

in structures.

> @@ -80,6 +81,11 @@ static void huawei_debug(const char *str, void
>  *user_data) ofono_info("%s", str);
>  }
> 
> +static void huawei_event_debug(const char *str, void *user_data)
> +{
> +	ofono_info("* %s", str);
> +}
> +

There is no need for a separate function here.  user_data can be used for 
this:

const char *prefix = user_data;
ofono_info("%s: %s", prefix, str);

> +	modem_device = ofono_modem_get_string(modem, "Device");
> +	event_device = ofono_modem_get_string(modem, "SecondaryDevice");
> +
> +	if (modem_device == NULL || event_device == NULL)
> +		return -EINVAL;
> +
> +	data->chat = create_port(modem_device);
> +
> +	if (data->chat == NULL)
>  		return -EIO;
> 
>  	g_at_chat_add_terminator(data->chat, "COMMAND NOT SUPPORT", -1, FALSE);
> @@ -120,6 +140,20 @@ static int huawei_enable(struct ofono_modem *modem)
>  	if (getenv("OFONO_AT_DEBUG"))
>  		g_at_chat_set_debug(data->chat, huawei_debug, NULL);
> 
> +	data->event = create_port(modem_device);
> +

Event port using the modem device again?

> +	if (data->event) {
> +		g_at_chat_cancel_all(data->event);
> +		g_at_chat_unregister_all(data->event);
> +	}
> +

You might want to unref event here instead of cfun_disable

Regards,
-Denis

Re: [PATCH v4 2/3] Move report_signal_strength to atutil

[Denis Kenzior]

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

Hi Kalle,

> 
>  static void csq_notify(GAtResult *result, gpointer user_data)
> @@ -490,7 +489,8 @@ static void csq_notify(GAtResult *result, gpointer
>  user_data) if (!g_at_result_iter_next_number(&iter, &strength))
>  		return;
> 
> -	report_signal_strength(netreg, strength);
> +	ofono_netreg_strength_notify(netreg,
> +				at_util_convert_signal_strength(strength));
>  }

How about fixing all other instances of report_signal_strength and removing it 
from this file as well? ;)

Regards,
-Denis

[PATCH v4 0/3] Huawei GPRS support

[Kalle Valo]

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

Hello,

here's v4 of Huawei GPRS support. Lots of whitespace cleanup, a memory leak
fixed and a new function at_util_convert_signal_strength(). All based on
comments from Denis.

---

Kalle Valo (3):
      huawei: add gprs context
      Move report_signal_strength to atutil
      huawei: detect possible secondary device


 drivers/atmodem/atutil.h               |   12 +++
 drivers/atmodem/network-registration.c |    8 +-
 plugins/huawei.c                       |  129 +++++++++++++++++++++++++++++---
 plugins/udev.c                         |   61 +++++++++++++--
 4 files changed, 186 insertions(+), 24 deletions(-)

[PATCH v4 1/3] huawei: detect possible secondary device

[Kalle Valo]

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


---

 plugins/huawei.c |   68 ++++++++++++++++++++++++++++++++++++++++++++----------
 plugins/udev.c   |   61 ++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 108 insertions(+), 21 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index df4d177..2a95cf6 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -45,7 +45,7 @@
 #include <drivers/atmodem/vendor.h>
 
 struct huawei_data {
-	GAtChat *chat;
+	GAtChat *chat, *event;
 };
 
 static int huawei_probe(struct ofono_modem *modem)
@@ -72,6 +72,7 @@ static void huawei_remove(struct ofono_modem *modem)
 	ofono_modem_set_data(modem, NULL);
 
 	g_at_chat_unref(data->chat);
+	g_at_chat_unref(data->event);
 	g_free(data);
 }
 
@@ -80,6 +81,11 @@ static void huawei_debug(const char *str, void *user_data)
 	ofono_info("%s", str);
 }
 
+static void huawei_event_debug(const char *str, void *user_data)
+{
+	ofono_info("* %s", str);
+}
+
 static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct ofono_modem *modem = user_data;
@@ -90,29 +96,43 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 		ofono_modem_set_powered(modem, TRUE);
 }
 
-static int huawei_enable(struct ofono_modem *modem)
+static GAtChat *create_port(const char *device)
 {
-	struct huawei_data *data = ofono_modem_get_data(modem);
 	GAtSyntax *syntax;
 	GIOChannel *channel;
-	const char *device;
-
-	DBG("%p", modem);
-
-	device = ofono_modem_get_string(modem, "Device");
-	if (!device)
-			return -EINVAL;
+	GAtChat *chat;
 
 	channel = g_at_tty_open(device, NULL);
 	if (!channel)
-		return -EIO;
+		return NULL;
 
 	syntax = g_at_syntax_new_gsm_permissive();
-	data->chat = g_at_chat_new(channel, syntax);
+	chat = g_at_chat_new(channel, syntax);
 	g_at_syntax_unref(syntax);
 	g_io_channel_unref(channel);
 
-	if (!data->chat)
+	if (!chat)
+		return NULL;
+
+	return chat;
+}
+
+static int huawei_enable(struct ofono_modem *modem)
+{
+	struct huawei_data *data = ofono_modem_get_data(modem);
+	const char *modem_device, *event_device;
+
+	DBG("%p", modem);
+
+	modem_device = ofono_modem_get_string(modem, "Device");
+	event_device = ofono_modem_get_string(modem, "SecondaryDevice");
+
+	if (modem_device == NULL || event_device == NULL)
+		return -EINVAL;
+
+	data->chat = create_port(modem_device);
+
+	if (data->chat == NULL)
 		return -EIO;
 
 	g_at_chat_add_terminator(data->chat, "COMMAND NOT SUPPORT", -1, FALSE);
@@ -120,6 +140,20 @@ static int huawei_enable(struct ofono_modem *modem)
 	if (getenv("OFONO_AT_DEBUG"))
 		g_at_chat_set_debug(data->chat, huawei_debug, NULL);
 
+	data->event = create_port(modem_device);
+
+	if (data->event == NULL) {
+		g_at_chat_unref(data->chat);
+		data->chat = NULL;
+		return -EIO;
+	}
+
+	g_at_chat_add_terminator(data->event, "COMMAND NOT SUPPORT", -1,
+					FALSE);
+
+	if (getenv("OFONO_AT_DEBUG"))
+		g_at_chat_set_debug(data->event, huawei_event_debug, NULL);
+
 	g_at_chat_send(data->chat, "ATE0", NULL, NULL, NULL, NULL);
 
 	g_at_chat_send(data->chat, "AT+CFUN=1", NULL,
@@ -138,6 +172,9 @@ static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
 	g_at_chat_unref(data->chat);
 	data->chat = NULL;
 
+	g_at_chat_unref(data->event);
+	data->event = NULL;
+
 	if (ok)
 		ofono_modem_set_powered(modem, FALSE);
 }
@@ -148,6 +185,11 @@ static int huawei_disable(struct ofono_modem *modem)
 
 	DBG("%p", modem);
 
+	if (data->event) {
+		g_at_chat_cancel_all(data->event);
+		g_at_chat_unregister_all(data->event);
+	}
+
 	if (!data->chat)
 		return 0;
 
diff --git a/plugins/udev.c b/plugins/udev.c
index 3a6ea28..bdac4fd 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -89,6 +89,24 @@ static const char *get_serial(struct udev_device *udev_device)
 	return serial;
 }
 
+static const char *get_usb_num(struct udev_device *udev_device)
+{
+	struct udev_list_entry *entry;
+	const char *num = NULL;
+
+	entry = udev_device_get_properties_list_entry(udev_device);
+	while (entry) {
+		const char *name = udev_list_entry_get_name(entry);
+
+		if (g_strcmp0(name, "ID_USB_INTERFACE_NUM") == 0)
+			num = udev_list_entry_get_value(entry);
+
+		entry = udev_list_entry_get_next(entry);
+	}
+
+	return num;
+}
+
 #define MODEM_DEVICE		"ModemDevice"
 #define DATA_DEVICE		"DataDevice"
 #define GPS_DEVICE		"GPSDevice"
@@ -201,18 +219,45 @@ static void add_hso(struct ofono_modem *modem,
 static void add_huawei(struct ofono_modem *modem,
 					struct udev_device *udev_device)
 {
-	const char *devnode;
-	int registered;
+	const char *devnode, *num;
+	int primary, secondary;
 
-	registered = ofono_modem_get_integer(modem, "Registered");
-	if (registered != 0)
+	primary = ofono_modem_get_integer(modem, "PrimaryRegistered");
+	secondary = ofono_modem_get_integer(modem, "SecondaryRegistered");
+
+	if (primary && secondary)
 		return;
 
-	devnode = udev_device_get_devnode(udev_device);
-	ofono_modem_set_string(modem, "Device", devnode);
+	num = get_usb_num(udev_device);
 
-	ofono_modem_set_integer(modem, "Registered", 1);
-	ofono_modem_register(modem);
+	/*
+	 * Here is is assumed that that usb port number 0 is the control
+	 * port and port 2 is the event port. This assumption will surely
+	 * be false with some devices and better heuristics is needed.
+	 */
+	if (g_strcmp0(num, "00") == 0) {
+		if (primary != 0)
+			return;
+
+		devnode = udev_device_get_devnode(udev_device);
+		ofono_modem_set_string(modem, "Device", devnode);
+
+		primary = 1;
+		ofono_modem_set_integer(modem, "PrimaryRegistered", primary);
+	} else if (g_strcmp0(num, "02") == 0) {
+		if (secondary != 0)
+			return;
+
+		devnode = udev_device_get_devnode(udev_device);
+		ofono_modem_set_string(modem, "SecondaryDevice", devnode);
+
+		secondary = 1;
+		ofono_modem_set_integer(modem, "SecondaryRegistered",
+					secondary);
+	}
+
+	if (primary && secondary)
+		ofono_modem_register(modem);
 }
 
 static void add_em770(struct ofono_modem *modem,

[PATCH v4 2/3] Move report_signal_strength to atutil

[Kalle Valo]

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

The function is needed in two different places, better to move it
to atutil.h.
---

 drivers/atmodem/atutil.h               |   12 ++++++++++++
 drivers/atmodem/network-registration.c |    4 ++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/atmodem/atutil.h b/drivers/atmodem/atutil.h
index 9303ed4..265a5ad 100644
--- a/drivers/atmodem/atutil.h
+++ b/drivers/atmodem/atutil.h
@@ -55,6 +55,18 @@ static inline struct cb_data *cb_data_new(void *cb, void *data)
 	return ret;
 }
 
+static inline int at_util_convert_signal_strength(int strength)
+{
+	int result;
+
+	if (strength == 99)
+		result = -1;
+	else
+		result = (strength * 100) / 31;
+
+	return result;
+}
+
 #define DECLARE_FAILURE(e) 			\
 	struct ofono_error e;			\
 	e.type = OFONO_ERROR_TYPE_FAILURE;	\
diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
index f7aafbe..ff94160 100644
--- a/drivers/atmodem/network-registration.c
+++ b/drivers/atmodem/network-registration.c
@@ -473,7 +473,6 @@ static inline void report_signal_strength(struct ofono_netreg *netreg,
 	else
 		strength = (strength * 100) / 31;
 
-	ofono_netreg_strength_notify(netreg, strength);
 }
 
 static void csq_notify(GAtResult *result, gpointer user_data)
@@ -490,7 +489,8 @@ static void csq_notify(GAtResult *result, gpointer user_data)
 	if (!g_at_result_iter_next_number(&iter, &strength))
 		return;
 
-	report_signal_strength(netreg, strength);
+	ofono_netreg_strength_notify(netreg,
+				at_util_convert_signal_strength(strength));
 }
 
 static void calypso_csq_notify(GAtResult *result, gpointer user_data)

[PATCH v4 3/3] huawei: add gprs context

[Kalle Valo]

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

Tested with Huawei E1552 HSDPA USB stick using a finnish Saunalahti prepaid
SIM.
---

 drivers/atmodem/network-registration.c |    4 ++
 plugins/huawei.c                       |   63 +++++++++++++++++++++++++++++++-
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
index ff94160..777b3ee 100644
--- a/drivers/atmodem/network-registration.c
+++ b/drivers/atmodem/network-registration.c
@@ -916,6 +916,10 @@ static void at_creg_set_cb(gboolean ok, GAtResult *result, gpointer user_data)
 				cind_support_cb, netreg, NULL);
 
 		break;
+	case OFONO_VENDOR_HUAWEI:
+		/* huawei doesn't support CIND */
+		ofono_netreg_register(netreg);
+		break;
 	default:
 		g_at_chat_send(nd->chat, "AT+CIND=?", cind_prefix,
 				cind_support_cb, netreg, NULL);
diff --git a/plugins/huawei.c b/plugins/huawei.c
index 2a95cf6..07e043d 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -41,7 +41,10 @@
 #include <ofono/gprs.h>
 #include <ofono/voicecall.h>
 #include <ofono/log.h>
+#include <ofono/gprs.h>
+#include <ofono/gprs-context.h>
 
+#include <drivers/atmodem/atutil.h>
 #include <drivers/atmodem/vendor.h>
 
 struct huawei_data {
@@ -140,7 +143,7 @@ static int huawei_enable(struct ofono_modem *modem)
 	if (getenv("OFONO_AT_DEBUG"))
 		g_at_chat_set_debug(data->chat, huawei_debug, NULL);
 
-	data->event = create_port(modem_device);
+	data->event = create_port(event_device);
 
 	if (data->event == NULL) {
 		g_at_chat_unref(data->chat);
@@ -216,14 +219,70 @@ static void huawei_pre_sim(struct ofono_modem *modem)
 		ofono_sim_inserted_notify(sim, TRUE);
 }
 
+static void huawei_cgreg_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs *gprs = user_data;
+	gboolean ret;
+	int status;
+
+	DBG("");
+
+	ret = at_util_parse_reg_unsolicited(result, "+CGREG:", &status,
+						NULL, NULL, NULL,
+						OFONO_VENDOR_HUAWEI);
+
+	if (ret == FALSE)
+		return;
+
+	ofono_gprs_status_notify(gprs, status);
+}
+
+static void huawei_rssi_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	GAtResultIter iter;
+	int strength;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "^RSSI:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &strength))
+		return;
+
+	ofono_netreg_strength_notify(netreg,
+				at_util_convert_signal_strength(strength));
+}
+
 static void huawei_post_sim(struct ofono_modem *modem)
 {
 	struct huawei_data *data = ofono_modem_get_data(modem);
+	struct ofono_gprs_context *gc;
+	struct ofono_netreg *netreg;
+	struct ofono_gprs *gprs;
 
 	DBG("%p", modem);
 
-	ofono_netreg_create(modem, OFONO_VENDOR_HUAWEI, "atmodem", data->chat);
+	netreg = ofono_netreg_create(modem, OFONO_VENDOR_HUAWEI, "atmodem",
+					data->chat);
 	ofono_sms_create(modem, OFONO_VENDOR_QUALCOMM_MSM, "atmodem", data->chat);
+
+	gprs = ofono_gprs_create(modem, OFONO_VENDOR_HUAWEI, "atmodem",
+					data->chat);
+	gc = ofono_gprs_context_create(modem, 0, "atmodem", data->chat);
+
+	if (gprs && gc) {
+		ofono_gprs_add_context(gprs, gc);
+
+		/* huawei has a separate channel for CGREG notifications */
+		g_at_chat_register(data->event, "+CGREG:",
+				huawei_cgreg_notify, FALSE, gprs, NULL);
+
+		/* huawei uses non-standard "^RSSI:18" strings */
+		g_at_chat_register(data->event, "^RSSI:",
+				huawei_rssi_notify, FALSE, netreg, NULL);
+	}
 }
 
 static struct ofono_modem_driver huawei_driver = {

Re: [PATCH v4 2/3] Move report_signal_strength to atutil

[Kalle Valo]

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

Denis Kenzior <denkenz@gmail.com> writes:

> Hi Kalle,
>
>> 
>>  static void csq_notify(GAtResult *result, gpointer user_data)
>> @@ -490,7 +489,8 @@ static void csq_notify(GAtResult *result, gpointer
>>  user_data) if (!g_at_result_iter_next_number(&iter, &strength))
>>  		return;
>> 
>> -	report_signal_strength(netreg, strength);
>> +	ofono_netreg_strength_notify(netreg,
>> +				at_util_convert_signal_strength(strength));
>>  }
>
> How about fixing all other instances of report_signal_strength and removing it 
> from this file as well? ;)

Oops, I totally forgot to remove the function network-registration.c.
Fixed now.

-- 
Kalle Valo

Re: [PATCH v4 1/3] huawei: detect possible secondary device

[Kalle Valo]

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

Denis Kenzior <denkenz@gmail.com> writes:

> Hi Kalle,

Hi Denis,

>>  struct huawei_data {
>> -	GAtChat *chat;
>> +	GAtChat *chat, *event;
>>  };
>> 
>
> Minor nitpick, but we prefer:
> GAtChat *chat;
> GAtChat *event;
>
> in structures.

Changed.

>> @@ -80,6 +81,11 @@ static void huawei_debug(const char *str, void
>>  *user_data) ofono_info("%s", str);
>>  }
>> 
>> +static void huawei_event_debug(const char *str, void *user_data)
>> +{
>> +	ofono_info("* %s", str);
>> +}
>> +
>
> There is no need for a separate function here.  user_data can be used for 
> this:
>
> const char *prefix = user_data;
> ofono_info("%s: %s", prefix, str);

I did it like this:

g_at_chat_set_debug(data->chat, huawei_debug, "");

g_at_chat_set_debug(data->event, huawei_debug, "EventChannel: ");

>> @@ -120,6 +140,20 @@ static int huawei_enable(struct ofono_modem *modem)
>>  	if (getenv("OFONO_AT_DEBUG"))
>>  		g_at_chat_set_debug(data->chat, huawei_debug, NULL);
>> 
>> +	data->event = create_port(modem_device);
>> +
>
> Event port using the modem device again?

Good catch. I actually fixed this, but commited to the wrong patch. I
moved that part now from patch 3 to patch 1.

>> +	if (data->event) {
>> +		g_at_chat_cancel_all(data->event);
>> +		g_at_chat_unregister_all(data->event);
>> +	}
>> +
>
> You might want to unref event here instead of cfun_disable

Done.

-- 
Kalle Valo