[PATCH 1/7] gatresult: strip trailing spaces from unquoted strings

[PATCH 1/7] gatresult: strip trailing spaces from unquoted strings

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

Some vendors might print trailing spaces after unsolicited result codes.
Avoid duplicating and stripping the string after calling
g_at_result_iter_next_unquoted_string() by stripping the spaces in
gatresult instead.
---
 gatchat/gatresult.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/gatchat/gatresult.c b/gatchat/gatresult.c
index 2659db28..883b4105 100644
--- a/gatchat/gatresult.c
+++ b/gatchat/gatresult.c
@@ -111,6 +111,7 @@ gboolean g_at_result_iter_next_unquoted_string(GAtResultIter *iter,
 	unsigned int pos;
 	unsigned int end;
 	unsigned int len;
+	unsigned int stripped;
 	char *line;
 
 	if (iter == NULL)
@@ -139,7 +140,12 @@ gboolean g_at_result_iter_next_unquoted_string(GAtResultIter *iter,
 	while (end < len && line[end] != ',' && line[end] != ')')
 		end += 1;
 
-	iter->buf[end] = '\0';
+	stripped = end;
+
+	while (line[stripped - 1] == ' ')
+		stripped -= 1;
+
+	iter->buf[stripped] = '\0';
 
 out:
 	iter->line_pos = skip_to_next_field(line, end, len);
-- 
2.22.0

[PATCH 2/7] atmodem: rename OFONO_VENDOR_QUECTEL_M95 to OFONO_VENDOR_QUECTEL_SERIAL

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

Other serial connected modems (i.e the MC60 model) from is AT-compatible
with the M95 model, so rename the M95 vendor id to be common for both.
---
 drivers/atmodem/sim.c    | 4 ++--
 drivers/atmodem/sms.c    | 4 ++--
 drivers/atmodem/vendor.h | 2 +-
 plugins/quectel.c        | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index 520b3dbf..dd42cac4 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -1217,7 +1217,7 @@ static void at_pin_retries_query(struct ofono_sim *sim,
 					at_qpinc_cb, cbd, g_free) > 0)
 			return;
 		break;
-	case OFONO_VENDOR_QUECTEL_M95:
+	case OFONO_VENDOR_QUECTEL_SERIAL:
 		if (g_at_chat_send(sd->chat, "AT+QTRPIN", qtrpin_prefix,
 					at_qtrpin_cb, cbd, g_free) > 0)
 			return;
@@ -1354,7 +1354,7 @@ static void at_pin_send_cb(gboolean ok, GAtResult *result,
 	case OFONO_VENDOR_HUAWEI:
 	case OFONO_VENDOR_SIMCOM:
 	case OFONO_VENDOR_SIERRA:
-	case OFONO_VENDOR_QUECTEL_M95:
+	case OFONO_VENDOR_QUECTEL_SERIAL:
 		/*
 		 * On ZTE modems, after pin is entered, SIM state is checked
 		 * by polling CPIN as their modem doesn't provide unsolicited
diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index c3c82afb..442cfc58 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -339,7 +339,7 @@ static inline void at_ack_delivery(struct ofono_sms *sms)
 		case OFONO_VENDOR_GEMALTO:
 			snprintf(buf, sizeof(buf), "AT+CNMA=1");
 			break;
-		case OFONO_VENDOR_QUECTEL_M95:
+		case OFONO_VENDOR_QUECTEL_SERIAL:
 			snprintf(buf, sizeof(buf), "AT+CNMA");
 			break;
 		default:
@@ -1289,7 +1289,7 @@ static void at_csms_query_cb(gboolean ok, GAtResult *result,
 		goto out;
 
 	switch (data->vendor) {
-	case OFONO_VENDOR_QUECTEL_M95:
+	case OFONO_VENDOR_QUECTEL_SERIAL:
 		g_at_result_iter_next_number(&iter, &status_min);
 		g_at_result_iter_next_number(&iter, &status_max);
 		if (status_min <= 1 && 1 <= status_max)
diff --git a/drivers/atmodem/vendor.h b/drivers/atmodem/vendor.h
index 10c04315..d839d1e0 100644
--- a/drivers/atmodem/vendor.h
+++ b/drivers/atmodem/vendor.h
@@ -44,7 +44,7 @@ enum ofono_vendor {
 	OFONO_VENDOR_WAVECOM_Q2XXX,
 	OFONO_VENDOR_ALCATEL,
 	OFONO_VENDOR_QUECTEL,
-	OFONO_VENDOR_QUECTEL_M95,
+	OFONO_VENDOR_QUECTEL_SERIAL,
 	OFONO_VENDOR_UBLOX,
 	OFONO_VENDOR_XMM,
 	OFONO_VENDOR_GEMALTO,
diff --git a/plugins/quectel.c b/plugins/quectel.c
index 11b864bc..f2b765b6 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -330,7 +330,7 @@ static void cgmm_cb(int ok, GAtResult *result, void *user_data)
 		data->vendor = OFONO_VENDOR_QUECTEL;
 	} else if (strcmp(model, "Quectel_M95") == 0) {
 		DBG("%p model M95", modem);
-		data->vendor = OFONO_VENDOR_QUECTEL_M95;
+		data->vendor = OFONO_VENDOR_QUECTEL_SERIAL;
 	} else {
 		ofono_warn("%p unknown model: '%s'", modem, model);
 		data->vendor = OFONO_VENDOR_QUECTEL;
-- 
2.22.0

[PATCH 3/7] quectel: enable call volume settings

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

---
 plugins/quectel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index f2b765b6..61b82744 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -45,6 +45,7 @@
 #include <ofono/sms.h>
 #include <ofono/phonebook.h>
 #include <ofono/voicecall.h>
+#include <ofono/call-volume.h>
 #include <ofono/gprs.h>
 #include <ofono/gprs-context.h>
 #include <ofono/log.h>
@@ -398,6 +399,7 @@ static void call_ready_notify(GAtResult *result, void *user_data)
 
 	ofono_phonebook_create(modem, 0, "atmodem", data->aux);
 	ofono_voicecall_create(modem, 0, "atmodem", data->aux);
+	ofono_call_volume_create(modem, 0, "atmodem", data->aux);
 }
 
 static int open_ttys(struct ofono_modem *modem)
-- 
2.22.0

[PATCH 4/7] quectel: store model id in private data

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

Some Quectel models supports different features such has GNSS or
different URC strings. Add a field in the quectel data structure to be
used when adding support for said features.
---
 plugins/quectel.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index 61b82744..e84e3cf1 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -70,6 +70,13 @@ static const uint8_t gsm0710_terminate[] = {
 	0xf9, /* close flag */
 };
 
+enum quectel_model {
+	QUECTEL_UNKNOWN,
+	QUECTEL_UC15,
+	QUECTEL_M95,
+	QUECTEL_MC60,
+};
+
 struct quectel_data {
 	GAtChat *modem;
 	GAtChat *aux;
@@ -77,6 +84,7 @@ struct quectel_data {
 	guint call_ready;
 	bool have_sim;
 	enum ofono_vendor vendor;
+	enum quectel_model model;
 	struct l_timeout *sms_ready_timer;
 
 	/* used by quectel uart driver */
@@ -329,12 +337,15 @@ static void cgmm_cb(int ok, GAtResult *result, void *user_data)
 	if (strcmp(model, "UC15") == 0) {
 		DBG("%p model UC15", modem);
 		data->vendor = OFONO_VENDOR_QUECTEL;
+		data->model = QUECTEL_UC15;
 	} else if (strcmp(model, "Quectel_M95") == 0) {
 		DBG("%p model M95", modem);
 		data->vendor = OFONO_VENDOR_QUECTEL_SERIAL;
+		data->model = QUECTEL_M95;
 	} else {
 		ofono_warn("%p unknown model: '%s'", modem, model);
 		data->vendor = OFONO_VENDOR_QUECTEL;
+		data->model = QUECTEL_UNKNOWN;
 	}
 
 	g_at_chat_send(data->aux, "AT+CFUN?", cfun_prefix, cfun_query, modem,
-- 
2.22.0

[PATCH 5/7] quectel: add support for the Quectel MC60 modem

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

The modem is AT-compatible with the Quectel M95 modem, but also features
a GNSS module.
---
 plugins/quectel.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index e84e3cf1..9cac92fa 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -56,7 +56,8 @@
 static const char *cfun_prefix[] = { "+CFUN:", NULL };
 static const char *cpin_prefix[] = { "+CPIN:", NULL };
 static const char *qinistat_prefix[] = { "+QINISTAT:", NULL };
-static const char *cgmm_prefix[] = { "UC15", "Quectel_M95", NULL };
+static const char *cgmm_prefix[] = { "UC15", "Quectel_M95", "Quectel_MC60",
+					NULL };
 static const char *none_prefix[] = { NULL };
 
 static const uint8_t gsm0710_terminate[] = {
@@ -342,6 +343,10 @@ static void cgmm_cb(int ok, GAtResult *result, void *user_data)
 		DBG("%p model M95", modem);
 		data->vendor = OFONO_VENDOR_QUECTEL_SERIAL;
 		data->model = QUECTEL_M95;
+	} else if (strcmp(model, "Quectel_MC60") == 0) {
+		DBG("%p model MC60", modem);
+		data->vendor = OFONO_VENDOR_QUECTEL_SERIAL;
+		data->model = QUECTEL_MC60;
 	} else {
 		ofono_warn("%p unknown model: '%s'", modem, model);
 		data->vendor = OFONO_VENDOR_QUECTEL;
-- 
2.22.0

[PATCH 6/7] quectel: add dbus hardware interface

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

For now the interface only exposes the modem supply voltage, but is
added as a preparation for signaling power events.
---
 doc/quectel-hardware-api.txt |  15 ++++
 plugins/quectel.c            | 154 +++++++++++++++++++++++++++++++++++
 2 files changed, 169 insertions(+)
 create mode 100644 doc/quectel-hardware-api.txt

diff --git a/doc/quectel-hardware-api.txt b/doc/quectel-hardware-api.txt
new file mode 100644
index 00000000..f54ea8c7
--- /dev/null
+++ b/doc/quectel-hardware-api.txt
@@ -0,0 +1,15 @@
+Hardware hierarchy
+==================
+
+Service		org.ofono
+Interface	org.ofono.quectel.Hardware
+Object path	/{device0,device1,...}
+
+Methods		array{string,variant} GetProperties
+
+			Returns hardware properties for the modem object. See
+			the properties section for available properties.
+
+Properties	uint32 Voltage [readonly]
+
+			Integer with the modem supply voltage in mV.
diff --git a/plugins/quectel.c b/plugins/quectel.c
index 9cac92fa..3c1d49cd 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -37,6 +37,7 @@
 #include <gattty.h>
 
 #define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono.h>
 #include <ofono/plugin.h>
 #include <ofono/modem.h>
 #include <ofono/devinfo.h>
@@ -49,12 +50,16 @@
 #include <ofono/gprs.h>
 #include <ofono/gprs-context.h>
 #include <ofono/log.h>
+#include <ofono/dbus.h>
+
+#include <gdbus/gdbus.h>
 
 #include <drivers/atmodem/atutil.h>
 #include <drivers/atmodem/vendor.h>
 
 static const char *cfun_prefix[] = { "+CFUN:", NULL };
 static const char *cpin_prefix[] = { "+CPIN:", NULL };
+static const char *cbc_prefix[] = { "+CBC:", NULL };
 static const char *qinistat_prefix[] = { "+QINISTAT:", NULL };
 static const char *cgmm_prefix[] = { "UC15", "Quectel_M95", "Quectel_MC60",
 					NULL };
@@ -95,6 +100,148 @@ struct quectel_data {
 	struct l_gpio_writer *gpio;
 };
 
+struct dbus_hw {
+	DBusMessage *msg;
+	struct ofono_modem *modem;
+	gint charge_status;
+	gint charge_level;
+	gint voltage;
+};
+
+static const char dbus_hw_interface[] = OFONO_SERVICE ".quectel.Hardware";
+
+static void dbus_hw_reply_properties(struct dbus_hw *hw)
+{
+	struct quectel_data *data = ofono_modem_get_data(hw->modem);
+	DBusMessage *reply;
+	DBusMessageIter dbus_iter;
+	DBusMessageIter dbus_dict;
+
+	DBG("%p", hw->modem);
+
+	reply = dbus_message_new_method_return(hw->msg);
+	dbus_message_iter_init_append(reply, &dbus_iter);
+	dbus_message_iter_open_container(&dbus_iter, DBUS_TYPE_ARRAY,
+			OFONO_PROPERTIES_ARRAY_SIGNATURE,
+			&dbus_dict);
+
+	if (data->model == QUECTEL_UC15) {
+		ofono_dbus_dict_append(&dbus_dict, "ChargeStatus",
+					DBUS_TYPE_INT32, &hw->charge_status);
+
+		ofono_dbus_dict_append(&dbus_dict, "ChargeLevel",
+					DBUS_TYPE_INT32, &hw->charge_level);
+	}
+
+	ofono_dbus_dict_append(&dbus_dict, "Voltage", DBUS_TYPE_UINT32,
+				&hw->voltage);
+
+	dbus_message_iter_close_container(&dbus_iter, &dbus_dict);
+
+	__ofono_dbus_pending_reply(&hw->msg, reply);
+}
+
+static void cbc_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct dbus_hw *hw = user_data;
+	GAtResultIter iter;
+
+	DBG("%p", hw->modem);
+
+	if (!hw->msg)
+		return;
+
+	if (!ok)
+		goto error;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CBC:"))
+		goto error;
+
+	/* only uc15 returns valid charge status */
+	if (!g_at_result_iter_next_number(&iter, &hw->charge_status))
+		goto error;
+
+	/* only uc15 returns valid charge level */
+	if (!g_at_result_iter_next_number(&iter, &hw->charge_level))
+		goto error;
+
+	/* now comes the millivolts */
+	if (!g_at_result_iter_next_number(&iter, &hw->voltage))
+		goto error;
+
+	dbus_hw_reply_properties(hw);
+
+	return;
+
+error:
+	__ofono_dbus_pending_reply(&hw->msg, __ofono_error_failed(hw->msg));
+}
+
+static DBusMessage *dbus_hw_get_properties(DBusConnection *conn,
+						DBusMessage *msg,
+						void *user_data)
+{
+	struct dbus_hw *hw = user_data;
+	struct quectel_data *data = ofono_modem_get_data(hw->modem);
+
+	DBG("%p", hw->modem);
+
+	if (hw->msg != NULL)
+		return __ofono_error_busy(msg);
+
+	if (!g_at_chat_send(data->aux, "AT+CBC", cbc_prefix, cbc_cb, hw, NULL))
+		return __ofono_error_failed(msg);
+
+	hw->msg = dbus_message_ref(msg);
+
+	return NULL;
+}
+
+static const GDBusMethodTable dbus_hw_methods[] = {
+	{ GDBUS_ASYNC_METHOD("GetProperties",
+				NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
+				dbus_hw_get_properties) },
+	{}
+};
+
+static void dbus_hw_cleanup(void *data)
+{
+	struct dbus_hw *hw = data;
+
+	DBG("%p", hw->modem);
+
+	if (hw->msg)
+		__ofono_dbus_pending_reply(&hw->msg,
+					__ofono_error_canceled(hw->msg));
+
+	l_free(hw);
+}
+
+static void dbus_hw_enable(struct ofono_modem *modem)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = ofono_modem_get_path(modem);
+	struct dbus_hw *hw;
+
+	DBG("%p", modem);
+
+	hw = l_new(struct dbus_hw, 1);
+	hw->modem = modem;
+
+	if (!g_dbus_register_interface(conn, path, dbus_hw_interface,
+					dbus_hw_methods, NULL, NULL,
+					hw, dbus_hw_cleanup)) {
+		ofono_error("Could not register %s interface under %s",
+				dbus_hw_interface, path);
+		l_free(hw);
+		return;
+	}
+
+	ofono_modem_add_interface(modem, dbus_hw_interface);
+}
+
 static void quectel_debug(const char *str, void *user_data)
 {
 	const char *prefix = user_data;
@@ -253,6 +400,8 @@ static void cpin_notify(GAtResult *result, gpointer user_data)
 
 	g_at_chat_unregister(data->aux, data->cpin_ready);
 	data->cpin_ready = 0;
+
+	dbus_hw_enable(modem);
 }
 
 static void cpin_query(gboolean ok, GAtResult *result, gpointer user_data)
@@ -650,9 +799,14 @@ static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
 static int quectel_disable(struct ofono_modem *modem)
 {
 	struct quectel_data *data = ofono_modem_get_data(modem);
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = ofono_modem_get_path(modem);
 
 	DBG("%p", modem);
 
+	if (g_dbus_unregister_interface(conn, path, dbus_hw_interface))
+		ofono_modem_remove_interface(modem, dbus_hw_interface);
+
 	g_at_chat_cancel_all(data->modem);
 	g_at_chat_unregister_all(data->modem);
 
-- 
2.22.0

Re: [PATCH 6/7] quectel: add dbus hardware interface

[Denis Kenzior]

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

Hi Martin,

On 7/16/19 2:10 PM, Martin Hundebøll wrote:
> For now the interface only exposes the modem supply voltage, but is
> added as a preparation for signaling power events.
> ---
>   doc/quectel-hardware-api.txt |  15 ++++
>   plugins/quectel.c            | 154 +++++++++++++++++++++++++++++++++++
>   2 files changed, 169 insertions(+)
>   create mode 100644 doc/quectel-hardware-api.txt

Can you please separate this per 'HACKING', 'Submitting Patches' section?

> 
> diff --git a/doc/quectel-hardware-api.txt b/doc/quectel-hardware-api.txt
> new file mode 100644
> index 00000000..f54ea8c7
> --- /dev/null
> +++ b/doc/quectel-hardware-api.txt
> @@ -0,0 +1,15 @@
> +Hardware hierarchy
> +==================
> +
> +Service		org.ofono
> +Interface	org.ofono.quectel.Hardware
> +Object path	/{device0,device1,...}
> +
> +Methods		array{string,variant} GetProperties
> +
> +			Returns hardware properties for the modem object. See
> +			the properties section for available properties.
> +
> +Properties	uint32 Voltage [readonly]
> +
> +			Integer with the modem supply voltage in mV.
> diff --git a/plugins/quectel.c b/plugins/quectel.c
> index 9cac92fa..3c1d49cd 100644
> --- a/plugins/quectel.c
> +++ b/plugins/quectel.c
> @@ -37,6 +37,7 @@
>   #include <gattty.h>
>   
>   #define OFONO_API_SUBJECT_TO_CHANGE
> +#include <ofono.h>
>   #include <ofono/plugin.h>
>   #include <ofono/modem.h>
>   #include <ofono/devinfo.h>
> @@ -49,12 +50,16 @@
>   #include <ofono/gprs.h>
>   #include <ofono/gprs-context.h>
>   #include <ofono/log.h>
> +#include <ofono/dbus.h>
> +
> +#include <gdbus/gdbus.h>
>   
>   #include <drivers/atmodem/atutil.h>
>   #include <drivers/atmodem/vendor.h>
>   
>   static const char *cfun_prefix[] = { "+CFUN:", NULL };
>   static const char *cpin_prefix[] = { "+CPIN:", NULL };
> +static const char *cbc_prefix[] = { "+CBC:", NULL };
>   static const char *qinistat_prefix[] = { "+QINISTAT:", NULL };
>   static const char *cgmm_prefix[] = { "UC15", "Quectel_M95", "Quectel_MC60",
>   					NULL };
> @@ -95,6 +100,148 @@ struct quectel_data {
>   	struct l_gpio_writer *gpio;
>   };
>   
> +struct dbus_hw {
> +	DBusMessage *msg;
> +	struct ofono_modem *modem;
> +	gint charge_status;
> +	gint charge_level;
> +	gint voltage;

You export the voltage as unsigned, but it is a signed type here...

Also, you may want to just start using standard types here instead of 
glib ones.

> +};
> +
> +static const char dbus_hw_interface[] = OFONO_SERVICE ".quectel.Hardware";
> +
> +static void dbus_hw_reply_properties(struct dbus_hw *hw)
> +{
> +	struct quectel_data *data = ofono_modem_get_data(hw->modem);
> +	DBusMessage *reply;
> +	DBusMessageIter dbus_iter;
> +	DBusMessageIter dbus_dict;
> +
> +	DBG("%p", hw->modem);
> +
> +	reply = dbus_message_new_method_return(hw->msg);
> +	dbus_message_iter_init_append(reply, &dbus_iter);
> +	dbus_message_iter_open_container(&dbus_iter, DBUS_TYPE_ARRAY,
> +			OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +			&dbus_dict);
> +
> +	if (data->model == QUECTEL_UC15) {
> +		ofono_dbus_dict_append(&dbus_dict, "ChargeStatus",
> +					DBUS_TYPE_INT32, &hw->charge_status);
> +
> +		ofono_dbus_dict_append(&dbus_dict, "ChargeLevel",
> +					DBUS_TYPE_INT32, &hw->charge_level);
> +	}

These are undocumented...

> +
> +	ofono_dbus_dict_append(&dbus_dict, "Voltage", DBUS_TYPE_UINT32,
> +				&hw->voltage);
> +
> +	dbus_message_iter_close_container(&dbus_iter, &dbus_dict);
> +
> +	__ofono_dbus_pending_reply(&hw->msg, reply);
> +}
> +
> +static void cbc_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct dbus_hw *hw = user_data;
> +	GAtResultIter iter;
> +
> +	DBG("%p", hw->modem);
> +
> +	if (!hw->msg)
> +		return;
> +
> +	if (!ok)
> +		goto error;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CBC:"))
> +		goto error;
> +
> +	/* only uc15 returns valid charge status */
> +	if (!g_at_result_iter_next_number(&iter, &hw->charge_status))
> +		goto error;
> +

So the comment is a bit confusing.  Is it that every modem returns all 3 
values, but only for UC15 they're valid; or CBC returns a different set 
of values for each modem?  I assume it is the latter since this doesn't 
match the traditional 27.007 CBC syntax?

> +	/* only uc15 returns valid charge level */
> +	if (!g_at_result_iter_next_number(&iter, &hw->charge_level))
> +		goto error;
> +
> +	/* now comes the millivolts */
> +	if (!g_at_result_iter_next_number(&iter, &hw->voltage))
> +		goto error;
> +
> +	dbus_hw_reply_properties(hw);
> +
> +	return;
> +
> +error:
> +	__ofono_dbus_pending_reply(&hw->msg, __ofono_error_failed(hw->msg));
> +}
> +
> +static DBusMessage *dbus_hw_get_properties(DBusConnection *conn,
> +						DBusMessage *msg,
> +						void *user_data)
> +{
> +	struct dbus_hw *hw = user_data;
> +	struct quectel_data *data = ofono_modem_get_data(hw->modem);
> +
> +	DBG("%p", hw->modem);
> +
> +	if (hw->msg != NULL)
> +		return __ofono_error_busy(msg);
> +
> +	if (!g_at_chat_send(data->aux, "AT+CBC", cbc_prefix, cbc_cb, hw, NULL))
> +		return __ofono_error_failed(msg);
> +
> +	hw->msg = dbus_message_ref(msg);
> +
> +	return NULL;
> +}
> +
> +static const GDBusMethodTable dbus_hw_methods[] = {
> +	{ GDBUS_ASYNC_METHOD("GetProperties",
> +				NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
> +				dbus_hw_get_properties) },
> +	{}
> +};
> +
> +static void dbus_hw_cleanup(void *data)
> +{
> +	struct dbus_hw *hw = data;
> +
> +	DBG("%p", hw->modem);
> +
> +	if (hw->msg)
> +		__ofono_dbus_pending_reply(&hw->msg,
> +					__ofono_error_canceled(hw->msg));
> +
> +	l_free(hw);
> +}
> +
> +static void dbus_hw_enable(struct ofono_modem *modem)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = ofono_modem_get_path(modem);
> +	struct dbus_hw *hw;
> +
> +	DBG("%p", modem);
> +
> +	hw = l_new(struct dbus_hw, 1);
> +	hw->modem = modem;
> +
> +	if (!g_dbus_register_interface(conn, path, dbus_hw_interface,
> +					dbus_hw_methods, NULL, NULL,
> +					hw, dbus_hw_cleanup)) {
> +		ofono_error("Could not register %s interface under %s",
> +				dbus_hw_interface, path);
> +		l_free(hw);
> +		return;
> +	}
> +
> +	ofono_modem_add_interface(modem, dbus_hw_interface);
> +}
> +
>   static void quectel_debug(const char *str, void *user_data)
>   {
>   	const char *prefix = user_data;
> @@ -253,6 +400,8 @@ static void cpin_notify(GAtResult *result, gpointer user_data)
>   
>   	g_at_chat_unregister(data->aux, data->cpin_ready);
>   	data->cpin_ready = 0;
> +
> +	dbus_hw_enable(modem);
>   }
>   
>   static void cpin_query(gboolean ok, GAtResult *result, gpointer user_data)
> @@ -650,9 +799,14 @@ static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
>   static int quectel_disable(struct ofono_modem *modem)
>   {
>   	struct quectel_data *data = ofono_modem_get_data(modem);
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = ofono_modem_get_path(modem);
>   
>   	DBG("%p", modem);
>   
> +	if (g_dbus_unregister_interface(conn, path, dbus_hw_interface))
> +		ofono_modem_remove_interface(modem, dbus_hw_interface);
> +

If you want to be paranoid, you might want to either issue a 
g_at_chat_cancel_all / g_at_chat_unregister_all here to prevent 
g_dbus_unregister_interface from freeing a structure with pending 
commands.  Or alternatively use a dedicated chat for dbus_hw_interface 
stuff (e.g. g_at_chat_clone a version)

>   	g_at_chat_cancel_all(data->modem);
>   	g_at_chat_unregister_all(data->modem);
>   
> 

Regards,
-Denis

[PATCH 7/7] quectel: implement dbus signals for modem power notifications

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

The Quectel modems issues unsolicited strings in case of power related
events. The UC15 uses +QIND: for the events, while M95 and MC60 uses
descriptive strings. (UC15 also uses a string for normal power down).

Register listeners for these strings/codes. The handler emits an
appropriate dbus signal, and closes down the modem if needed.
---
 doc/quectel-hardware-api.txt |  19 +++++
 plugins/quectel.c            | 148 ++++++++++++++++++++++++++++++++++-
 2 files changed, 166 insertions(+), 1 deletion(-)

diff --git a/doc/quectel-hardware-api.txt b/doc/quectel-hardware-api.txt
index f54ea8c7..c9e93926 100644
--- a/doc/quectel-hardware-api.txt
+++ b/doc/quectel-hardware-api.txt
@@ -10,6 +10,25 @@ Methods		array{string,variant} GetProperties
 			Returns hardware properties for the modem object. See
 			the properties section for available properties.
 
+Signals		PowerDown(string reason)
+
+			This signal is emitted on gracefull shutdowns initiated
+			by the modem.
+
+			Possible reasons:
+				"LowPower"	The supply voltage is too low
+				"Normal"	The PWRKEY pin was asserted
+				"HighPower"	The supply voltage is too high
+
+		PowerWarning(string reason)
+
+			This signal is emitted when the modem detects its supply
+			voltage is close to its supported limits.
+
+			Possible reasons:
+				"LowPower"	The supply voltage is low
+				"HighPower"	The supply voltage is high
+
 Properties	uint32 Voltage [readonly]
 
 			Integer with the modem supply voltage in mV.
diff --git a/plugins/quectel.c b/plugins/quectel.c
index 3c1d49cd..3c74fc41 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -108,8 +108,122 @@ struct dbus_hw {
 	gint voltage;
 };
 
+enum quectel_power_event {
+	LOW_POWER_DOWN    = -2,
+	LOW_WARNING       = -1,
+	NORMAL_POWER_DOWN =  0,
+	HIGH_WARNING      =  1,
+	HIGH_POWER_DOWN   =  2,
+};
+
 static const char dbus_hw_interface[] = OFONO_SERVICE ".quectel.Hardware";
 
+static void close_serial(struct ofono_modem *modem);
+
+static void power_handle(struct ofono_modem *modem,
+				enum quectel_power_event event)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	DBusMessage *signal;
+	DBusMessageIter iter;
+	const char *path = ofono_modem_get_path(modem);
+	const char *name;
+	const char *reason;
+	bool close;
+
+	DBG("%p", modem);
+
+	switch (event) {
+	case LOW_POWER_DOWN:
+		close = true;
+		name = "PowerDown";
+		reason = "LowPower";
+		break;
+	case LOW_WARNING:
+		close = false;
+		name = "PowerWarning";
+		reason = "LowPower";
+		break;
+	case NORMAL_POWER_DOWN:
+		close = true;
+		name = "PowerDown";
+		reason = "Normal";
+		break;
+	case HIGH_WARNING:
+		close = false;
+		name = "PowerWarning";
+		reason = "HighPower";
+		break;
+	case HIGH_POWER_DOWN:
+		close = true;
+		name = "PowerDown";
+		reason = "HighPower";
+		break;
+	default:
+		return;
+	}
+
+	signal = dbus_message_new_signal(path, dbus_hw_interface, name);
+	if (signal) {
+		dbus_message_iter_init_append(signal, &iter);
+		dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING,
+						&reason);
+		g_dbus_send_message(conn, signal);
+	}
+
+	if (close)
+		close_serial(modem);
+}
+
+static void qind_notify(GAtResult *result, void *user_data)
+{
+	struct dbus_hw *hw = user_data;
+	GAtResultIter iter;
+	enum quectel_power_event event;
+	const char *type;
+
+	DBG("%p", hw->modem);
+
+	g_at_result_iter_init(&iter, result);
+	g_at_result_iter_next(&iter, "+QIND:");
+
+	if (!g_at_result_iter_next_string(&iter, &type))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &event))
+		return;
+
+	power_handle(hw->modem, event);
+}
+
+static void power_notify(GAtResult *result, void *user_data)
+{
+	struct dbus_hw *hw = user_data;
+	GAtResultIter iter;
+	const char *event;
+
+	DBG("%p", hw->modem);
+
+	g_at_result_iter_init(&iter, result);
+	g_at_result_iter_next(&iter, NULL);
+
+	if (!g_at_result_iter_next_unquoted_string(&iter, &event))
+		return;
+
+	DBG("event: %s", event);
+
+	if (g_strcmp0(event, "UNDER_VOLTAGE POWER DOWN") == 0)
+		power_handle(hw->modem, LOW_POWER_DOWN);
+	else if (g_strcmp0(event, "UNDER_VOLTAGE WARNING") == 0)
+		power_handle(hw->modem, LOW_WARNING);
+	else if (g_strcmp0(event, "NORMAL POWER DOWN") == 0)
+		power_handle(hw->modem, NORMAL_POWER_DOWN);
+	else if (g_strcmp0(event, "OVER_VOLTAGE WARNING") == 0)
+		power_handle(hw->modem, HIGH_WARNING);
+	else if (g_strcmp0(event, "OVER_VOLTAGE POWER DOWN") == 0)
+		power_handle(hw->modem, HIGH_POWER_DOWN);
+}
+
 static void dbus_hw_reply_properties(struct dbus_hw *hw)
 {
 	struct quectel_data *data = ofono_modem_get_data(hw->modem);
@@ -206,6 +320,14 @@ static const GDBusMethodTable dbus_hw_methods[] = {
 	{}
 };
 
+static const GDBusSignalTable dbus_hw_signals[] = {
+	{ GDBUS_SIGNAL("PowerDown",
+			GDBUS_ARGS({ "reason", "s" })) },
+	{ GDBUS_SIGNAL("PowerWarning",
+			GDBUS_ARGS({ "reason", "s" })) },
+	{ }
+};
+
 static void dbus_hw_cleanup(void *data)
 {
 	struct dbus_hw *hw = data;
@@ -222,6 +344,7 @@ static void dbus_hw_cleanup(void *data)
 static void dbus_hw_enable(struct ofono_modem *modem)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
+	struct quectel_data *data = ofono_modem_get_data(modem);
 	const char *path = ofono_modem_get_path(modem);
 	struct dbus_hw *hw;
 
@@ -231,7 +354,7 @@ static void dbus_hw_enable(struct ofono_modem *modem)
 	hw->modem = modem;
 
 	if (!g_dbus_register_interface(conn, path, dbus_hw_interface,
-					dbus_hw_methods, NULL, NULL,
+					dbus_hw_methods, dbus_hw_signals, NULL,
 					hw, dbus_hw_cleanup)) {
 		ofono_error("Could not register %s interface under %s",
 				dbus_hw_interface, path);
@@ -239,6 +362,29 @@ static void dbus_hw_enable(struct ofono_modem *modem)
 		return;
 	}
 
+	g_at_chat_register(data->aux, "NORMAL POWER DOWN", power_notify, FALSE,
+				hw, NULL);
+
+	switch (data->model) {
+	case QUECTEL_UC15:
+		g_at_chat_register(data->aux, "+QIND",  qind_notify, FALSE, hw,
+					NULL);
+		break;
+	case QUECTEL_M95:
+	case QUECTEL_MC60:
+		g_at_chat_register(data->aux, "OVER_VOLTAGE POWER DOWN",
+					power_notify, FALSE, hw, NULL);
+		g_at_chat_register(data->aux, "UNDER_VOLTAGE POWER DOWN",
+					power_notify, FALSE, hw, NULL);
+		g_at_chat_register(data->aux, "OVER_VOLTAGE WARNING",
+					power_notify, FALSE, hw, NULL);
+		g_at_chat_register(data->aux, "UNDER_VOLTAGE WARNING",
+					power_notify, FALSE, hw, NULL);
+		break;
+	case QUECTEL_UNKNOWN:
+		break;
+	}
+
 	ofono_modem_add_interface(modem, dbus_hw_interface);
 }
 
-- 
2.22.0

Re: [PATCH 7/7] quectel: implement dbus signals for modem power notifications

[Denis Kenzior]

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

Hi Martin,

On 7/16/19 2:10 PM, Martin Hundebøll wrote:
> The Quectel modems issues unsolicited strings in case of power related
> events. The UC15 uses +QIND: for the events, while M95 and MC60 uses
> descriptive strings. (UC15 also uses a string for normal power down).
> 
> Register listeners for these strings/codes. The handler emits an
> appropriate dbus signal, and closes down the modem if needed.
> ---
>   doc/quectel-hardware-api.txt |  19 +++++
>   plugins/quectel.c            | 148 ++++++++++++++++++++++++++++++++++-
>   2 files changed, 166 insertions(+), 1 deletion(-)
> 

2 separate commits please per HACKING, 'Submitting Patches'

<snip>

> diff --git a/plugins/quectel.c b/plugins/quectel.c
> index 3c1d49cd..3c74fc41 100644
> --- a/plugins/quectel.c
> +++ b/plugins/quectel.c
> @@ -108,8 +108,122 @@ struct dbus_hw {
>   	gint voltage;
>   };
>   
> +enum quectel_power_event {
> +	LOW_POWER_DOWN    = -2,
> +	LOW_WARNING       = -1,
> +	NORMAL_POWER_DOWN =  0,
> +	HIGH_WARNING      =  1,
> +	HIGH_POWER_DOWN   =  2,
> +};
> +
>   static const char dbus_hw_interface[] = OFONO_SERVICE ".quectel.Hardware";
>   
> +static void close_serial(struct ofono_modem *modem);
> +

We generally frown upon forward declaration of statics...

Regards,
-Denis

Re: [PATCH 1/7] gatresult: strip trailing spaces from unquoted strings

[Denis Kenzior]

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

Hi Martin,

On 7/16/19 2:10 PM, Martin Hundebøll wrote:
> Some vendors might print trailing spaces after unsolicited result codes.
> Avoid duplicating and stripping the string after calling
> g_at_result_iter_next_unquoted_string() by stripping the spaces in
> gatresult instead.
> ---
>   gatchat/gatresult.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 

Patches 1 - 5 applied, thanks.

Regards,
-Denis