[RFC 0/2] Add hardware monitor interface to gemalto plugin

[RFC 0/2] Add hardware monitor interface to gemalto plugin

[Vincent Cesson]

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

Gemalto Cinterion P-family modems expose hardware related commands to monitor
temperature and voltage. The following patches add a custom interface in
Gemalto plugin. 

Implementation is based on ril_intel and smart-messaging plugins for DBus
interface creation and callback handling. Create DBus interface 
org.ofono.HardwareMonitor with two methods GetTemperature and GetVoltage.

First patch registers the DBus interface and create the DBus method table.
Second patch implements the DBus methods with AT command calls and DBus
message handling.

Vincent Cesson (2):
  gemalto: Prepare new interface for hardware monitoring
  gemalto: Implement HardwareMonitor methods

 plugins/gemalto.c | 194 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 194 insertions(+)

-- 
1.9.1

[RFC 1/2] gemalto: Prepare new interface for hardware monitoring

[Vincent Cesson]

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

Gemalto modems have hardware related commands, allowing to monitor voltage
and temperature. These parameters will be accessible on DBus interface:
org.ofono.HardwareMonitor.

 - Create the DBus method table with two entries:
GetTemperature and GetVoltage.
 - Create a dedicated structure to handle the DBus methods.
 - Create enable/disable functions to handle DBus interface registration.
---
 plugins/gemalto.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 2870ce8..985dfd8 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -30,7 +30,11 @@
 #include <gatchat.h>
 #include <gattty.h>
 
+#include "gdbus.h"
+#include "ofono.h"
+
 #define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/dbus.h>
 #include <ofono/plugin.h>
 #include <ofono/log.h>
 #include <ofono/modem.h>
@@ -46,14 +50,24 @@
 #include <drivers/atmodem/atutil.h>
 #include <drivers/atmodem/vendor.h>
 
+#define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".HardwareMonitor"
+
 static const char *none_prefix[] = { NULL };
 
+struct gemalto_hardware_monitor {
+	DBusMessage *msg;
+	struct ofono_modem *modem;
+	int32_t temperature;
+	int32_t voltage;
+};
+
 struct gemalto_data {
 	GAtChat *app;
 	GAtChat *mdm;
 	struct ofono_sim *sim;
 	gboolean have_sim;
 	struct at_util_sim_state_query *sim_state_query;
+	struct gemalto_hardware_monitor *hm;
 };
 
 static int gemalto_probe(struct ofono_modem *modem)
@@ -142,6 +156,73 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 						NULL);
 }
 
+static DBusMessage *hardware_monitor_get_temperature(DBusConnection *conn,
+							DBusMessage *msg,
+							void *user_data)
+{
+	DBG("");
+
+	return __ofono_error_not_implemented(msg);
+}
+
+static DBusMessage *hardware_monitor_get_voltage(DBusConnection *conn,
+							DBusMessage *msg,
+							void *user_data)
+{
+	DBG("");
+
+	return __ofono_error_not_implemented(msg);
+}
+
+static const GDBusMethodTable hardware_monitor_methods[] = {
+	{ GDBUS_ASYNC_METHOD("GetTemperature",
+			NULL, GDBUS_ARGS({ "temperature", "i" }),
+			hardware_monitor_get_temperature) },
+	{ GDBUS_ASYNC_METHOD("GetVoltage",
+			NULL, GDBUS_ARGS({ "voltage", "i" }),
+			hardware_monitor_get_voltage) },
+	{}
+};
+
+static void hardware_monitor_cleanup(void *user_data)
+{
+	struct gemalto_data *data = user_data;
+	struct gemalto_hardware_monitor *hm = data->hm;
+
+	g_free(hm);
+}
+
+static int gemalto_hardware_monitor_enable(struct ofono_modem *modem)
+{
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = ofono_modem_get_path(modem);
+
+	DBG("");
+
+	/* Enable temperature output */
+	g_at_chat_send(data->app, "AT^SCTM=0,1", none_prefix, NULL, NULL, NULL);
+
+	/* Create Hardware Monitor DBus interface */
+	data->hm = g_try_new0(struct gemalto_hardware_monitor, 1);
+	if (data->hm == NULL)
+		return -EIO;
+
+	data->hm->modem = modem;
+
+	if (!g_dbus_register_interface(conn, path, HARDWARE_MONITOR_INTERFACE,
+					hardware_monitor_methods, NULL, NULL,
+					data, hardware_monitor_cleanup)) {
+		ofono_error("Could not register %s interface under %s",
+					HARDWARE_MONITOR_INTERFACE, path);
+		g_free(data->hm);
+		return -EIO;
+	}
+
+	ofono_modem_add_interface(modem, HARDWARE_MONITOR_INTERFACE);
+	return 0;
+}
+
 static int gemalto_enable(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
@@ -181,6 +262,8 @@ static int gemalto_enable(struct ofono_modem *modem)
 	g_at_chat_send(data->app, "AT+CFUN=4", none_prefix,
 			cfun_enable, modem, NULL);
 
+	gemalto_hardware_monitor_enable(modem);
+
 	return -EINPROGRESS;
 }
 
@@ -203,12 +286,19 @@ static void gemalto_smso_cb(gboolean ok, GAtResult *result, gpointer user_data)
 static int gemalto_disable(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = ofono_modem_get_path(modem);
 
 	DBG("%p", modem);
 
 	g_at_chat_cancel_all(data->app);
 	g_at_chat_unregister_all(data->app);
 
+	if (g_dbus_unregister_interface(conn, path,
+				HARDWARE_MONITOR_INTERFACE))
+		ofono_modem_remove_interface(modem,
+				HARDWARE_MONITOR_INTERFACE);
+
 	/* Shutdown the modem */
 	g_at_chat_send(data->app, "AT^SMSO", none_prefix, gemalto_smso_cb,
 			modem, NULL);
-- 
1.9.1

[RFC 2/2] gemalto: Implement HardwareMonitor methods

[Vincent Cesson]

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

Use Gemalto commands ^SCTM and ^SBV to monitor temperature and voltage.
DBus reply is formed in AT callbacks.
---
 plugins/gemalto.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 2 deletions(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 985dfd8..edfaa5c 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -53,6 +53,8 @@
 #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".HardwareMonitor"
 
 static const char *none_prefix[] = { NULL };
+static const char *sctm_prefix[] = { "^SCTM:", NULL };
+static const char *sbv_prefix[] = { "^SBV:", NULL };
 
 struct gemalto_hardware_monitor {
 	DBusMessage *msg;
@@ -156,22 +158,124 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 						NULL);
 }
 
+static void gemalto_sctm_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct gemalto_data *data = user_data;
+	GAtResultIter iter;
+	DBusMessageIter dbus_iter;
+	DBusMessageIter dbus_dict;
+
+	if (!ok)
+		goto error;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "^SCTM:"))
+		goto error;
+
+	if (!g_at_result_iter_skip_next(&iter))
+		goto error;
+
+	if (!g_at_result_iter_skip_next(&iter))
+		goto error;
+
+	if (!g_at_result_iter_next_number(&iter, &(data->hm->temperature)))
+		goto error;
+
+	dbus_message_iter_init_append(data->hm->msg, &dbus_iter);
+
+	dbus_message_iter_open_container(&dbus_iter, DBUS_TYPE_ARRAY,
+					OFONO_PROPERTIES_ARRAY_SIGNATURE,
+					&dbus_dict);
+
+	ofono_dbus_dict_append(&dbus_dict, "Temperature", DBUS_TYPE_INT32,
+				&(data->hm->temperature));
+
+	dbus_message_iter_close_container(&dbus_iter, &dbus_dict);
+
+	g_dbus_send_reply(conn, data->hm->msg, DBUS_TYPE_INT32,
+			&data->hm->temperature, DBUS_TYPE_INVALID);
+
+	return;
+
+error:
+	g_dbus_send_error(conn, data->hm->msg, DBUS_ERROR_FAILED,
+			"Unable to read temperature");
+}
+
 static DBusMessage *hardware_monitor_get_temperature(DBusConnection *conn,
 							DBusMessage *msg,
 							void *user_data)
 {
+	struct gemalto_data *data = user_data;
+
 	DBG("");
 
-	return __ofono_error_not_implemented(msg);
+	if (!g_at_chat_send(data->app, "AT^SCTM?", sctm_prefix, gemalto_sctm_cb,
+			data, NULL))
+		return __ofono_error_failed(msg);
+
+	data->hm->msg = dbus_message_ref(msg);
+
+	return NULL;
+}
+
+static void gemalto_sbv_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct gemalto_data *data = user_data;
+	GAtResultIter iter;
+	DBusMessageIter dbus_iter;
+	DBusMessageIter dbus_dict;
+
+	if (!ok)
+		goto error;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "^SBV:"))
+		goto error;
+
+	if (!g_at_result_iter_next_number(&iter, &(data->hm->voltage)))
+		goto error;
+
+	dbus_message_iter_init_append(data->hm->msg, &dbus_iter);
+
+	dbus_message_iter_open_container(&dbus_iter, DBUS_TYPE_ARRAY,
+					OFONO_PROPERTIES_ARRAY_SIGNATURE,
+					&dbus_dict);
+
+	ofono_dbus_dict_append(&dbus_dict, "Voltage", DBUS_TYPE_INT32,
+				&(data->hm->voltage));
+
+	dbus_message_iter_close_container(&dbus_iter, &dbus_dict);
+
+	g_dbus_send_reply(conn, data->hm->msg, DBUS_TYPE_INT32,
+			&data->hm->voltage, DBUS_TYPE_INVALID);
+
+	return;
+
+error:
+	g_dbus_send_error(conn, data->hm->msg, DBUS_ERROR_FAILED,
+			"Unable to read voltage");
 }
 
 static DBusMessage *hardware_monitor_get_voltage(DBusConnection *conn,
 							DBusMessage *msg,
 							void *user_data)
 {
+	struct gemalto_data *data = user_data;
+
 	DBG("");
 
-	return __ofono_error_not_implemented(msg);
+	if (!g_at_chat_send(data->app, "AT^SBV", sbv_prefix, gemalto_sbv_cb,
+			data, NULL))
+		return __ofono_error_failed(msg);
+
+	data->hm->msg = dbus_message_ref(msg);
+
+	return NULL;
 }
 
 static const GDBusMethodTable hardware_monitor_methods[] = {
-- 
1.9.1

Re: [RFC 1/2] gemalto: Prepare new interface for hardware monitoring

[Denis Kenzior]

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

Hi Vincent,

On 05/11/2017 09:48 AM, Vincent Cesson wrote:
> Gemalto modems have hardware related commands, allowing to monitor voltage
> and temperature. These parameters will be accessible on DBus interface:
> org.ofono.HardwareMonitor.

It should be clear that this is a vendor specific interface, so 
org.ofono.cinterion.HardwareMonitor.

>
>  - Create the DBus method table with two entries:
> GetTemperature and GetVoltage.

So the applications would need to poll these values.  Does the modem 
enable some sort of unsolicited notifications of these?

>  - Create a dedicated structure to handle the DBus methods.
>  - Create enable/disable functions to handle DBus interface registration.
> ---
>  plugins/gemalto.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index 2870ce8..985dfd8 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c
> @@ -30,7 +30,11 @@
>  #include <gatchat.h>
>  #include <gattty.h>
>
> +#include "gdbus.h"

By our convention, please use <gdbus.h>

> +#include "ofono.h"
> +
>  #define OFONO_API_SUBJECT_TO_CHANGE
> +#include <ofono/dbus.h>
>  #include <ofono/plugin.h>
>  #include <ofono/log.h>
>  #include <ofono/modem.h>
> @@ -46,14 +50,24 @@
>  #include <drivers/atmodem/atutil.h>
>  #include <drivers/atmodem/vendor.h>
>
> +#define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".HardwareMonitor"
> +
>  static const char *none_prefix[] = { NULL };
>
> +struct gemalto_hardware_monitor {
> +	DBusMessage *msg;
> +	struct ofono_modem *modem;
> +	int32_t temperature;
> +	int32_t voltage;
> +};
> +
>  struct gemalto_data {
>  	GAtChat *app;
>  	GAtChat *mdm;
>  	struct ofono_sim *sim;
>  	gboolean have_sim;
>  	struct at_util_sim_state_query *sim_state_query;
> +	struct gemalto_hardware_monitor *hm;
>  };
>
>  static int gemalto_probe(struct ofono_modem *modem)
> @@ -142,6 +156,73 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>  						NULL);
>  }
>
> +static DBusMessage *hardware_monitor_get_temperature(DBusConnection *conn,
> +							DBusMessage *msg,
> +							void *user_data)
> +{
> +	DBG("");
> +
> +	return __ofono_error_not_implemented(msg);
> +}
> +
> +static DBusMessage *hardware_monitor_get_voltage(DBusConnection *conn,
> +							DBusMessage *msg,
> +							void *user_data)
> +{
> +	DBG("");
> +
> +	return __ofono_error_not_implemented(msg);
> +}
> +
> +static const GDBusMethodTable hardware_monitor_methods[] = {
> +	{ GDBUS_ASYNC_METHOD("GetTemperature",
> +			NULL, GDBUS_ARGS({ "temperature", "i" }),
> +			hardware_monitor_get_temperature) },
> +	{ GDBUS_ASYNC_METHOD("GetVoltage",
> +			NULL, GDBUS_ARGS({ "voltage", "i" }),
> +			hardware_monitor_get_voltage) },
> +	{}
> +};
> +

If the modem reports these via unsolicited notifications, then it might 
be better to model Temperature & Voltage as properties instead.

> +static void hardware_monitor_cleanup(void *user_data)
> +{
> +	struct gemalto_data *data = user_data;
> +	struct gemalto_hardware_monitor *hm = data->hm;
> +
> +	g_free(hm);
> +}
> +
> +static int gemalto_hardware_monitor_enable(struct ofono_modem *modem)
> +{
> +	struct gemalto_data *data = ofono_modem_get_data(modem);
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = ofono_modem_get_path(modem);
> +
> +	DBG("");
> +
> +	/* Enable temperature output */
> +	g_at_chat_send(data->app, "AT^SCTM=0,1", none_prefix, NULL, NULL, NULL);
> +
> +	/* Create Hardware Monitor DBus interface */
> +	data->hm = g_try_new0(struct gemalto_hardware_monitor, 1);
> +	if (data->hm == NULL)
> +		return -EIO;
> +
> +	data->hm->modem = modem;
> +
> +	if (!g_dbus_register_interface(conn, path, HARDWARE_MONITOR_INTERFACE,
> +					hardware_monitor_methods, NULL, NULL,
> +					data, hardware_monitor_cleanup)) {
> +		ofono_error("Could not register %s interface under %s",
> +					HARDWARE_MONITOR_INTERFACE, path);
> +		g_free(data->hm);
> +		return -EIO;
> +	}
> +
> +	ofono_modem_add_interface(modem, HARDWARE_MONITOR_INTERFACE);
> +	return 0;
> +}
> +
>  static int gemalto_enable(struct ofono_modem *modem)
>  {
>  	struct gemalto_data *data = ofono_modem_get_data(modem);
> @@ -181,6 +262,8 @@ static int gemalto_enable(struct ofono_modem *modem)
>  	g_at_chat_send(data->app, "AT+CFUN=4", none_prefix,
>  			cfun_enable, modem, NULL);
>
> +	gemalto_hardware_monitor_enable(modem);
> +

Might want to do this only after the device is 'enabled'.  E.g. in 
cfun_enable

>  	return -EINPROGRESS;
>  }
>
> @@ -203,12 +286,19 @@ static void gemalto_smso_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  static int gemalto_disable(struct ofono_modem *modem)
>  {
>  	struct gemalto_data *data = ofono_modem_get_data(modem);
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = ofono_modem_get_path(modem);
>
>  	DBG("%p", modem);
>
>  	g_at_chat_cancel_all(data->app);
>  	g_at_chat_unregister_all(data->app);
>
> +	if (g_dbus_unregister_interface(conn, path,
> +				HARDWARE_MONITOR_INTERFACE))
> +		ofono_modem_remove_interface(modem,
> +				HARDWARE_MONITOR_INTERFACE);
> +
>  	/* Shutdown the modem */
>  	g_at_chat_send(data->app, "AT^SMSO", none_prefix, gemalto_smso_cb,
>  			modem, NULL);
>

Regards,
-Denis

Re: [RFC 2/2] gemalto: Implement HardwareMonitor methods

[Denis Kenzior]

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

Hi Vincent,

On 05/11/2017 09:48 AM, Vincent Cesson wrote:
> Use Gemalto commands ^SCTM and ^SBV to monitor temperature and voltage.
> DBus reply is formed in AT callbacks.
> ---
>  plugins/gemalto.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 106 insertions(+), 2 deletions(-)
>
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index 985dfd8..edfaa5c 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c
> @@ -53,6 +53,8 @@
>  #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".HardwareMonitor"
>
>  static const char *none_prefix[] = { NULL };
> +static const char *sctm_prefix[] = { "^SCTM:", NULL };
> +static const char *sbv_prefix[] = { "^SBV:", NULL };
>
>  struct gemalto_hardware_monitor {
>  	DBusMessage *msg;
> @@ -156,22 +158,124 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>  						NULL);
>  }
>
> +static void gemalto_sctm_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct gemalto_data *data = user_data;
> +	GAtResultIter iter;
> +	DBusMessageIter dbus_iter;
> +	DBusMessageIter dbus_dict;
> +
> +	if (!ok)
> +		goto error;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "^SCTM:"))
> +		goto error;
> +
> +	if (!g_at_result_iter_skip_next(&iter))
> +		goto error;
> +
> +	if (!g_at_result_iter_skip_next(&iter))
> +		goto error;
> +
> +	if (!g_at_result_iter_next_number(&iter, &(data->hm->temperature)))
> +		goto error;
> +
> +	dbus_message_iter_init_append(data->hm->msg, &dbus_iter);
> +
> +	dbus_message_iter_open_container(&dbus_iter, DBUS_TYPE_ARRAY,
> +					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +					&dbus_dict);
> +
> +	ofono_dbus_dict_append(&dbus_dict, "Temperature", DBUS_TYPE_INT32,
> +				&(data->hm->temperature));

For a simple GetTemperature call, you're generating an a{sv} return 
signature.  E.g. a dictionary.  It might be easier to simply return an 
int32.

> +
> +	dbus_message_iter_close_container(&dbus_iter, &dbus_dict);
> +
> +	g_dbus_send_reply(conn, data->hm->msg, DBUS_TYPE_INT32,
> +			&data->hm->temperature, DBUS_TYPE_INVALID);
> +
> +	return;
> +
> +error:
> +	g_dbus_send_error(conn, data->hm->msg, DBUS_ERROR_FAILED,
> +			"Unable to read temperature");
> +}
> +
>  static DBusMessage *hardware_monitor_get_temperature(DBusConnection *conn,
>  							DBusMessage *msg,
>  							void *user_data)
>  {
> +	struct gemalto_data *data = user_data;
> +
>  	DBG("");
>
> -	return __ofono_error_not_implemented(msg);
> +	if (!g_at_chat_send(data->app, "AT^SCTM?", sctm_prefix, gemalto_sctm_cb,
> +			data, NULL))
> +		return __ofono_error_failed(msg);
> +
> +	data->hm->msg = dbus_message_ref(msg);

What if a GetVoltage call is already in progress?

> +
> +	return NULL;
> +}
> +
> +static void gemalto_sbv_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct gemalto_data *data = user_data;
> +	GAtResultIter iter;
> +	DBusMessageIter dbus_iter;
> +	DBusMessageIter dbus_dict;
> +
> +	if (!ok)
> +		goto error;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "^SBV:"))
> +		goto error;
> +
> +	if (!g_at_result_iter_next_number(&iter, &(data->hm->voltage)))
> +		goto error;
> +
> +	dbus_message_iter_init_append(data->hm->msg, &dbus_iter);
> +
> +	dbus_message_iter_open_container(&dbus_iter, DBUS_TYPE_ARRAY,
> +					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +					&dbus_dict);
> +
> +	ofono_dbus_dict_append(&dbus_dict, "Voltage", DBUS_TYPE_INT32,
> +				&(data->hm->voltage));
> +
> +	dbus_message_iter_close_container(&dbus_iter, &dbus_dict);
> +
> +	g_dbus_send_reply(conn, data->hm->msg, DBUS_TYPE_INT32,
> +			&data->hm->voltage, DBUS_TYPE_INVALID);
> +
> +	return;
> +
> +error:
> +	g_dbus_send_error(conn, data->hm->msg, DBUS_ERROR_FAILED,
> +			"Unable to read voltage");
>  }
>
>  static DBusMessage *hardware_monitor_get_voltage(DBusConnection *conn,
>  							DBusMessage *msg,
>  							void *user_data)
>  {
> +	struct gemalto_data *data = user_data;
> +
>  	DBG("");
>
> -	return __ofono_error_not_implemented(msg);
> +	if (!g_at_chat_send(data->app, "AT^SBV", sbv_prefix, gemalto_sbv_cb,
> +			data, NULL))
> +		return __ofono_error_failed(msg);
> +
> +	data->hm->msg = dbus_message_ref(msg);
> +
> +	return NULL;
>  }
>
>  static const GDBusMethodTable hardware_monitor_methods[] = {
>

Regards,
-Denis

Re: [RFC 1/2] gemalto: Prepare new interface for hardware monitoring

[Vincent CESSON]

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

Hi Denis,


Le 2017-05-11 21:51, Denis Kenzior a écrit :
> Hi Vincent,
> 
> On 05/11/2017 09:48 AM, Vincent Cesson wrote:
>> Gemalto modems have hardware related commands, allowing to monitor 
>> voltage
>> and temperature. These parameters will be accessible on DBus 
>> interface:
>> org.ofono.HardwareMonitor.
> 
> It should be clear that this is a vendor specific interface, so
> org.ofono.cinterion.HardwareMonitor.
> 
>> 
>>  - Create the DBus method table with two entries:
>> GetTemperature and GetVoltage.
> 
> So the applications would need to poll these values.  Does the modem
> enable some sort of unsolicited notifications of these?
> 

No unsolicited notifications for voltage.
There are notifications for over/under temperature warning,
but exact value must be polled.

How would you implement the notification? With a property and a signal
PropertyChanged? Is it ok to keep the methods as I propose, and add
the notification later?

I will propose patch v2 with your comments soon.

>>  - Create a dedicated structure to handle the DBus methods.
>>  - Create enable/disable functions to handle DBus interface 
>> registration.
>> ---
>>  plugins/gemalto.c | 90 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 90 insertions(+)
>> 
>> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
>> index 2870ce8..985dfd8 100644
>> --- a/plugins/gemalto.c
>> +++ b/plugins/gemalto.c
>> @@ -30,7 +30,11 @@
>>  #include <gatchat.h>
>>  #include <gattty.h>
>> 
>> +#include "gdbus.h"
> 
> By our convention, please use <gdbus.h>
> 
>> +#include "ofono.h"
>> +
>>  #define OFONO_API_SUBJECT_TO_CHANGE
>> +#include <ofono/dbus.h>
>>  #include <ofono/plugin.h>
>>  #include <ofono/log.h>
>>  #include <ofono/modem.h>
>> @@ -46,14 +50,24 @@
>>  #include <drivers/atmodem/atutil.h>
>>  #include <drivers/atmodem/vendor.h>
>> 
>> +#define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".HardwareMonitor"
>> +
>>  static const char *none_prefix[] = { NULL };
>> 
>> +struct gemalto_hardware_monitor {
>> +	DBusMessage *msg;
>> +	struct ofono_modem *modem;
>> +	int32_t temperature;
>> +	int32_t voltage;
>> +};
>> +
>>  struct gemalto_data {
>>  	GAtChat *app;
>>  	GAtChat *mdm;
>>  	struct ofono_sim *sim;
>>  	gboolean have_sim;
>>  	struct at_util_sim_state_query *sim_state_query;
>> +	struct gemalto_hardware_monitor *hm;
>>  };
>> 
>>  static int gemalto_probe(struct ofono_modem *modem)
>> @@ -142,6 +156,73 @@ static void cfun_enable(gboolean ok, GAtResult 
>> *result, gpointer user_data)
>>  						NULL);
>>  }
>> 
>> +static DBusMessage *hardware_monitor_get_temperature(DBusConnection 
>> *conn,
>> +							DBusMessage *msg,
>> +							void *user_data)
>> +{
>> +	DBG("");
>> +
>> +	return __ofono_error_not_implemented(msg);
>> +}
>> +
>> +static DBusMessage *hardware_monitor_get_voltage(DBusConnection 
>> *conn,
>> +							DBusMessage *msg,
>> +							void *user_data)
>> +{
>> +	DBG("");
>> +
>> +	return __ofono_error_not_implemented(msg);
>> +}
>> +
>> +static const GDBusMethodTable hardware_monitor_methods[] = {
>> +	{ GDBUS_ASYNC_METHOD("GetTemperature",
>> +			NULL, GDBUS_ARGS({ "temperature", "i" }),
>> +			hardware_monitor_get_temperature) },
>> +	{ GDBUS_ASYNC_METHOD("GetVoltage",
>> +			NULL, GDBUS_ARGS({ "voltage", "i" }),
>> +			hardware_monitor_get_voltage) },
>> +	{}
>> +};
>> +
> 
> If the modem reports these via unsolicited notifications, then it
> might be better to model Temperature & Voltage as properties instead.
> 
>> +static void hardware_monitor_cleanup(void *user_data)
>> +{
>> +	struct gemalto_data *data = user_data;
>> +	struct gemalto_hardware_monitor *hm = data->hm;
>> +
>> +	g_free(hm);
>> +}
>> +
>> +static int gemalto_hardware_monitor_enable(struct ofono_modem 
>> *modem)
>> +{
>> +	struct gemalto_data *data = ofono_modem_get_data(modem);
>> +	DBusConnection *conn = ofono_dbus_get_connection();
>> +	const char *path = ofono_modem_get_path(modem);
>> +
>> +	DBG("");
>> +
>> +	/* Enable temperature output */
>> +	g_at_chat_send(data->app, "AT^SCTM=0,1", none_prefix, NULL, NULL, 
>> NULL);
>> +
>> +	/* Create Hardware Monitor DBus interface */
>> +	data->hm = g_try_new0(struct gemalto_hardware_monitor, 1);
>> +	if (data->hm == NULL)
>> +		return -EIO;
>> +
>> +	data->hm->modem = modem;
>> +
>> +	if (!g_dbus_register_interface(conn, path, 
>> HARDWARE_MONITOR_INTERFACE,
>> +					hardware_monitor_methods, NULL, NULL,
>> +					data, hardware_monitor_cleanup)) {
>> +		ofono_error("Could not register %s interface under %s",
>> +					HARDWARE_MONITOR_INTERFACE, path);
>> +		g_free(data->hm);
>> +		return -EIO;
>> +	}
>> +
>> +	ofono_modem_add_interface(modem, HARDWARE_MONITOR_INTERFACE);
>> +	return 0;
>> +}
>> +
>>  static int gemalto_enable(struct ofono_modem *modem)
>>  {
>>  	struct gemalto_data *data = ofono_modem_get_data(modem);
>> @@ -181,6 +262,8 @@ static int gemalto_enable(struct ofono_modem 
>> *modem)
>>  	g_at_chat_send(data->app, "AT+CFUN=4", none_prefix,
>>  			cfun_enable, modem, NULL);
>> 
>> +	gemalto_hardware_monitor_enable(modem);
>> +
> 
> Might want to do this only after the device is 'enabled'.  E.g. in 
> cfun_enable
> 

Hardware related commands are available whatever CFUN value.
I think it is ok to keep it here.

>>  	return -EINPROGRESS;
>>  }
>> 
>> @@ -203,12 +286,19 @@ static void gemalto_smso_cb(gboolean ok, 
>> GAtResult *result, gpointer user_data)
>>  static int gemalto_disable(struct ofono_modem *modem)
>>  {
>>  	struct gemalto_data *data = ofono_modem_get_data(modem);
>> +	DBusConnection *conn = ofono_dbus_get_connection();
>> +	const char *path = ofono_modem_get_path(modem);
>> 
>>  	DBG("%p", modem);
>> 
>>  	g_at_chat_cancel_all(data->app);
>>  	g_at_chat_unregister_all(data->app);
>> 
>> +	if (g_dbus_unregister_interface(conn, path,
>> +				HARDWARE_MONITOR_INTERFACE))
>> +		ofono_modem_remove_interface(modem,
>> +				HARDWARE_MONITOR_INTERFACE);
>> +
>>  	/* Shutdown the modem */
>>  	g_at_chat_send(data->app, "AT^SMSO", none_prefix, gemalto_smso_cb,
>>  			modem, NULL);
>> 
> 
> Regards,
> -Denis

Regards,
-Vincent

Re: [RFC 1/2] gemalto: Prepare new interface for hardware monitoring

[Denis Kenzior]

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

Hi Vincent,

>>
>> So the applications would need to poll these values.  Does the modem
>> enable some sort of unsolicited notifications of these?
>>
>
> No unsolicited notifications for voltage.
> There are notifications for over/under temperature warning,
> but exact value must be polled.

Okay.  In that case a method call is probably the best way to do this. 
I would combine the two method calls into one.  e.g. GetStatistics() 
that would return a dictionary of values.  Something like:

a{sv} GetStatistics()

where s is the key:
	"Voltage" - uint32 (I assume this can't be negative)
	"Temperature" - int32

This keeps your round-trips to a minimum.  It takes ages to go over 
D-Bus, so it should be considerably faster to have oFono query both 
values at once.  Especially if you can combine the query into a single 
AT commmand.

Might want to mention the units as well.  F vs C, etc.

>
> How would you implement the notification? With a property and a signal
> PropertyChanged? Is it ok to keep the methods as I propose, and add
> the notification later?
>

If it is just a plain notification that e.g. a temperature exceeds the 
threshold, then I would model it as a signal.  Something like:
	TemperatureThresholdExceeded()

Is there a way to set the threshold?

Regards,
-Denis

Re: [RFC 1/2] gemalto: Prepare new interface for hardware monitoring

[Vincent CESSON]

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

Hi Denis,


Le 2017-05-12 16:13, Denis Kenzior a écrit :
> Hi Vincent,
> 
>>> 
>>> So the applications would need to poll these values.  Does the modem
>>> enable some sort of unsolicited notifications of these?
>>> 
>> 
>> No unsolicited notifications for voltage.
>> There are notifications for over/under temperature warning,
>> but exact value must be polled.
> 
> Okay.  In that case a method call is probably the best way to do
> this. I would combine the two method calls into one.  e.g.
> GetStatistics() that would return a dictionary of values.  Something
> like:
> 
> a{sv} GetStatistics()
> 
> where s is the key:
> "Voltage" - uint32 (I assume this can't be negative)
> "Temperature" - int32
> 
> This keeps your round-trips to a minimum.  It takes ages to go over
> D-Bus, so it should be considerably faster to have oFono query both
> values at once.  Especially if you can combine the query into a single
> AT commmand.
> 
> Might want to mention the units as well.  F vs C, etc.
> 

Ok to squash the methods. Why GetStatistics? I suggest simply 
GetProperties instead.
How would you mention the units? In the key name? "Temperature (C)" and 
"Voltage (mV)"?
Or in a new entry?
It's details but I prefer to have your opinion.

>> 
>> How would you implement the notification? With a property and a 
>> signal
>> PropertyChanged? Is it ok to keep the methods as I propose, and add
>> the notification later?
>> 
> 
> If it is just a plain notification that e.g. a temperature exceeds
> the threshold, then I would model it as a signal.  Something like:
> TemperatureThresholdExceeded()
> 
> Is there a way to set the threshold?

There is no way to set the thresholds. In fact there are 5 levels:

Below lowest temperature limit (causes switch-off after 5 s time).
Below low temperature alert limit.
Normal operating temperature.
Above upper temperature alert limit.
Above uppermost temperature limit (causes switch-off after 5 s time).

> 
> Regards,
> -Denis

Regards,
-Vincent

Re: [RFC 1/2] gemalto: Prepare new interface for hardware monitoring

[Denis Kenzior]

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

Hi Vincent,

On 05/15/2017 10:53 AM, Vincent CESSON wrote:
> Hi Denis,
>
>
> Le 2017-05-12 16:13, Denis Kenzior a écrit :
>> Hi Vincent,
>>
>>>>
>>>> So the applications would need to poll these values.  Does the modem
>>>> enable some sort of unsolicited notifications of these?
>>>>
>>>
>>> No unsolicited notifications for voltage.
>>> There are notifications for over/under temperature warning,
>>> but exact value must be polled.
>>
>> Okay.  In that case a method call is probably the best way to do
>> this. I would combine the two method calls into one.  e.g.
>> GetStatistics() that would return a dictionary of values.  Something
>> like:
>>
>> a{sv} GetStatistics()
>>
>> where s is the key:
>> "Voltage" - uint32 (I assume this can't be negative)
>> "Temperature" - int32
>>
>> This keeps your round-trips to a minimum.  It takes ages to go over
>> D-Bus, so it should be considerably faster to have oFono query both
>> values at once.  Especially if you can combine the query into a single
>> AT commmand.
>>
>> Might want to mention the units as well.  F vs C, etc.
>>
>
> Ok to squash the methods. Why GetStatistics? I suggest simply
> GetProperties instead.

If you use GetProperties then it would be expected that you implement 
PropertyChanged signal as well, and maybe SetProperty().  Don't think it 
makes sense to have a signal if the caller is responsible for polling 
the values.  Hence a separate method call.

> How would you mention the units? In the key name? "Temperature (C)" and
> "Voltage (mV)"?
> Or in a new entry?
> It's details but I prefer to have your opinion.

I assume you would provide a doc file for this.  E.g. 
doc/cinterion-hardware-monitor-api.txt  The API description would be the 
place to mention the units, ranges, etc.

>
>>>
>>> How would you implement the notification? With a property and a signal
>>> PropertyChanged? Is it ok to keep the methods as I propose, and add
>>> the notification later?
>>>
>>
>> If it is just a plain notification that e.g. a temperature exceeds
>> the threshold, then I would model it as a signal.  Something like:
>> TemperatureThresholdExceeded()
>>
>> Is there a way to set the threshold?
>
> There is no way to set the thresholds. In fact there are 5 levels:
>
> Below lowest temperature limit (causes switch-off after 5 s time).
> Below low temperature alert limit.
> Normal operating temperature.
> Above upper temperature alert limit.
> Above uppermost temperature limit (causes switch-off after 5 s time).
>

Okay, this probably should be described in the doc file as well.

Regards,
-Denis

[RFC v2 1/3] gemalto: Prepare new interface for hardware monitoring

[Vincent Cesson]

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

Gemalto modems have hardware related commands, allowing to monitor voltage
and temperature. These parameters will be accessible on DBus interface:
org.ofono.HardwareMonitor.

 - Create the DBus method table with one entry: GetStatistics. This method
would return temperature and voltage values.
 - Create a dedicated structure to handle the DBus methods.
 - Create enable/disable functions to handle DBus interface registration.
---
 plugins/gemalto.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 2870ce8..3798e09 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -29,8 +29,12 @@
 #include <glib.h>
 #include <gatchat.h>
 #include <gattty.h>
+#include <gdbus.h>
+
+#include "ofono.h"
 
 #define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/dbus.h>
 #include <ofono/plugin.h>
 #include <ofono/log.h>
 #include <ofono/modem.h>
@@ -46,14 +50,23 @@
 #include <drivers/atmodem/atutil.h>
 #include <drivers/atmodem/vendor.h>
 
+#define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".cinterion.HardwareMonitor"
+
 static const char *none_prefix[] = { NULL };
 
+struct gemalto_hardware_monitor {
+	DBusMessage *msg;
+	int32_t temperature;
+	int32_t voltage;
+};
+
 struct gemalto_data {
 	GAtChat *app;
 	GAtChat *mdm;
 	struct ofono_sim *sim;
 	gboolean have_sim;
 	struct at_util_sim_state_query *sim_state_query;
+	struct gemalto_hardware_monitor *hm;
 };
 
 static int gemalto_probe(struct ofono_modem *modem)
@@ -142,6 +155,59 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 						NULL);
 }
 
+static DBusMessage *hardware_monitor_get_statistics(DBusConnection *conn,
+							DBusMessage *msg,
+							void *user_data)
+{
+	DBG("");
+
+	return __ofono_error_not_implemented(msg);
+}
+
+static const GDBusMethodTable hardware_monitor_methods[] = {
+	{ GDBUS_ASYNC_METHOD("GetStatistics",
+			NULL, GDBUS_ARGS({ "Statistics", "a{sv}" }),
+			hardware_monitor_get_statistics) },
+	{}
+};
+
+static void hardware_monitor_cleanup(void *user_data)
+{
+	struct gemalto_data *data = user_data;
+	struct gemalto_hardware_monitor *hm = data->hm;
+
+	g_free(hm);
+}
+
+static int gemalto_hardware_monitor_enable(struct ofono_modem *modem)
+{
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = ofono_modem_get_path(modem);
+
+	DBG("");
+
+	/* Enable temperature output */
+	g_at_chat_send(data->app, "AT^SCTM=0,1", none_prefix, NULL, NULL, NULL);
+
+	/* Create Hardware Monitor DBus interface */
+	data->hm = g_try_new0(struct gemalto_hardware_monitor, 1);
+	if (data->hm == NULL)
+		return -EIO;
+
+	if (!g_dbus_register_interface(conn, path, HARDWARE_MONITOR_INTERFACE,
+					hardware_monitor_methods, NULL, NULL,
+					data, hardware_monitor_cleanup)) {
+		ofono_error("Could not register %s interface under %s",
+					HARDWARE_MONITOR_INTERFACE, path);
+		g_free(data->hm);
+		return -EIO;
+	}
+
+	ofono_modem_add_interface(modem, HARDWARE_MONITOR_INTERFACE);
+	return 0;
+}
+
 static int gemalto_enable(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
@@ -181,6 +247,8 @@ static int gemalto_enable(struct ofono_modem *modem)
 	g_at_chat_send(data->app, "AT+CFUN=4", none_prefix,
 			cfun_enable, modem, NULL);
 
+	gemalto_hardware_monitor_enable(modem);
+
 	return -EINPROGRESS;
 }
 
@@ -203,12 +271,19 @@ static void gemalto_smso_cb(gboolean ok, GAtResult *result, gpointer user_data)
 static int gemalto_disable(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = ofono_modem_get_path(modem);
 
 	DBG("%p", modem);
 
 	g_at_chat_cancel_all(data->app);
 	g_at_chat_unregister_all(data->app);
 
+	if (g_dbus_unregister_interface(conn, path,
+				HARDWARE_MONITOR_INTERFACE))
+		ofono_modem_remove_interface(modem,
+				HARDWARE_MONITOR_INTERFACE);
+
 	/* Shutdown the modem */
 	g_at_chat_send(data->app, "AT^SMSO", none_prefix, gemalto_smso_cb,
 			modem, NULL);
-- 
1.9.1

[RFC v2 2/3] gemalto: Implement HardwareMonitor method

[Vincent Cesson]

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

Use Gemalto commands ^SCTM and ^SBV to monitor temperature and voltage.
Use a single method GetStatistics to read and return both values.
---
 plugins/gemalto.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 1 deletion(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 3798e09..9f67f17 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -53,6 +53,8 @@
 #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".cinterion.HardwareMonitor"
 
 static const char *none_prefix[] = { NULL };
+static const char *sctm_prefix[] = { "^SCTM:", NULL };
+static const char *sbv_prefix[] = { "^SBV:", NULL };
 
 struct gemalto_hardware_monitor {
 	DBusMessage *msg;
@@ -155,13 +157,103 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 						NULL);
 }
 
+static void gemalto_sctm_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct gemalto_data *data = user_data;
+	GAtResultIter iter;
+	DBusMessageIter dbus_iter;
+	DBusMessageIter dbus_dict;
+
+	if (!ok)
+		goto error;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "^SCTM:"))
+		goto error;
+
+	if (!g_at_result_iter_skip_next(&iter))
+		goto error;
+
+	if (!g_at_result_iter_skip_next(&iter))
+		goto error;
+
+	if (!g_at_result_iter_next_number(&iter, &(data->hm->temperature)))
+		goto error;
+
+	dbus_message_iter_init_append(data->hm->msg, &dbus_iter);
+
+	dbus_message_iter_open_container(&dbus_iter, DBUS_TYPE_ARRAY,
+			OFONO_PROPERTIES_ARRAY_SIGNATURE,
+			&dbus_dict);
+
+	ofono_dbus_dict_append(&dbus_dict, "Temperature",
+			DBUS_TYPE_INT32, &data->hm->temperature);
+
+	ofono_dbus_dict_append(&dbus_dict, "Voltage",
+			DBUS_TYPE_UINT32, &data->hm->voltage);
+
+	dbus_message_iter_close_container(&dbus_iter, &dbus_dict);
+
+	g_dbus_send_message(conn, data->hm->msg);
+
+	data->hm->msg = NULL;
+
+	return;
+
+error:
+	g_dbus_send_error(conn, data->hm->msg, DBUS_ERROR_FAILED,
+			"Unable to read temperature");
+}
+
+static void gemalto_sbv_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct gemalto_data *data = user_data;
+	GAtResultIter iter;
+
+	if (!ok)
+		goto error;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "^SBV:"))
+		goto error;
+
+	if (!g_at_result_iter_next_number(&iter, &(data->hm->voltage)))
+		goto error;
+
+	if (!g_at_chat_send(data->app, "AT^SCTM?", sctm_prefix, gemalto_sctm_cb,
+			data, NULL))
+		g_dbus_send_error(conn, data->hm->msg, DBUS_ERROR_FAILED,
+				"Unable to read temperature");
+
+	return;
+
+error:
+	g_dbus_send_error(conn, data->hm->msg, DBUS_ERROR_FAILED,
+			"Unable to read voltage");
+}
+
 static DBusMessage *hardware_monitor_get_statistics(DBusConnection *conn,
 							DBusMessage *msg,
 							void *user_data)
 {
+	struct gemalto_data *data = user_data;
+
 	DBG("");
 
-	return __ofono_error_not_implemented(msg);
+	if (data->hm->msg != NULL)
+		return __ofono_error_busy(msg);
+
+	if (!g_at_chat_send(data->app, "AT^SBV", sbv_prefix, gemalto_sbv_cb,
+			data, NULL))
+		return __ofono_error_failed(msg);
+
+	data->hm->msg = dbus_message_new_method_return(msg);
+
+	return NULL;
 }
 
 static const GDBusMethodTable hardware_monitor_methods[] = {
-- 
1.9.1

[RFC v2 3/3] doc: gemalto: add documentation about interface cinterion.HardwareMonitor.

[Vincent Cesson]

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

This interface exposes a single method to access temperature and supply
voltage of the modem.
---
 doc/cinterion-hardware-monitor-api.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 doc/cinterion-hardware-monitor-api.txt

diff --git a/doc/cinterion-hardware-monitor-api.txt b/doc/cinterion-hardware-monitor-api.txt
new file mode 100644
index 0000000..50adca3
--- /dev/null
+++ b/doc/cinterion-hardware-monitor-api.txt
@@ -0,0 +1,17 @@
+
+HardwareMonitor hierarchy
+=========================
+
+Service		org.ofono.cinterion
+Interface	org.ofono.cinterion.HardwareMonitor
+Object path	/{device0,device1,...}
+
+Methods		array{string,variant} GetStatistics
+
+			Returns an array of dict entries representing the
+			current temperature and supply voltage of the modem.
+
+			Units:
+			Temperature: Celsius
+			Voltage: mV
+
-- 
1.9.1

Re: [RFC v2 1/3] gemalto: Prepare new interface for hardware monitoring

[Denis Kenzior]

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

Hi Vincent,

On 05/16/2017 03:38 AM, Vincent Cesson wrote:
> Gemalto modems have hardware related commands, allowing to monitor voltage
> and temperature. These parameters will be accessible on DBus interface:
> org.ofono.HardwareMonitor.
>
>  - Create the DBus method table with one entry: GetStatistics. This method
> would return temperature and voltage values.
>  - Create a dedicated structure to handle the DBus methods.
>  - Create enable/disable functions to handle DBus interface registration.
> ---
>  plugins/gemalto.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>

Applied, thanks.

Regards,
-Denis

Re: [RFC v2 2/3] gemalto: Implement HardwareMonitor method

[Denis Kenzior]

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

Hi Vincent,

On 05/16/2017 03:38 AM, Vincent Cesson wrote:
> Use Gemalto commands ^SCTM and ^SBV to monitor temperature and voltage.
> Use a single method GetStatistics to read and return both values.
> ---
>  plugins/gemalto.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 93 insertions(+), 1 deletion(-)
>
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index 3798e09..9f67f17 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c
> @@ -53,6 +53,8 @@
>  #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".cinterion.HardwareMonitor"
>
>  static const char *none_prefix[] = { NULL };
> +static const char *sctm_prefix[] = { "^SCTM:", NULL };
> +static const char *sbv_prefix[] = { "^SBV:", NULL };
>
>  struct gemalto_hardware_monitor {
>  	DBusMessage *msg;
> @@ -155,13 +157,103 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>  						NULL);
>  }
>
> +static void gemalto_sctm_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct gemalto_data *data = user_data;
> +	GAtResultIter iter;
> +	DBusMessageIter dbus_iter;
> +	DBusMessageIter dbus_dict;
> +
> +	if (!ok)
> +		goto error;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "^SCTM:"))
> +		goto error;
> +
> +	if (!g_at_result_iter_skip_next(&iter))
> +		goto error;
> +
> +	if (!g_at_result_iter_skip_next(&iter))
> +		goto error;
> +
> +	if (!g_at_result_iter_next_number(&iter, &(data->hm->temperature)))

'()' are not needed around data->hm->temperature

> +		goto error;
> +
> +	dbus_message_iter_init_append(data->hm->msg, &dbus_iter);
> +
> +	dbus_message_iter_open_container(&dbus_iter, DBUS_TYPE_ARRAY,
> +			OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +			&dbus_dict);
> +
> +	ofono_dbus_dict_append(&dbus_dict, "Temperature",
> +			DBUS_TYPE_INT32, &data->hm->temperature);
> +
> +	ofono_dbus_dict_append(&dbus_dict, "Voltage",
> +			DBUS_TYPE_UINT32, &data->hm->voltage);
> +
> +	dbus_message_iter_close_container(&dbus_iter, &dbus_dict);
> +
> +	g_dbus_send_message(conn, data->hm->msg);
> +
> +	data->hm->msg = NULL;

or use __ofono_dbus_pending_reply to save a few steps.

> +
> +	return;
> +
> +error:
> +	g_dbus_send_error(conn, data->hm->msg, DBUS_ERROR_FAILED,
> +			"Unable to read temperature");
> +}
> +
> +static void gemalto_sbv_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct gemalto_data *data = user_data;
> +	GAtResultIter iter;
> +
> +	if (!ok)
> +		goto error;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "^SBV:"))
> +		goto error;
> +
> +	if (!g_at_result_iter_next_number(&iter, &(data->hm->voltage)))

'()' around data->hm->voltage are not needed

> +		goto error;
> +
> +	if (!g_at_chat_send(data->app, "AT^SCTM?", sctm_prefix, gemalto_sctm_cb,
> +			data, NULL))
> +		g_dbus_send_error(conn, data->hm->msg, DBUS_ERROR_FAILED,
> +				"Unable to read temperature");
> +
> +	return;
> +

I'm not sure distinguishing between voltage and temperature errors is 
useful.  So why why not:

if (g_at_chat_send(...) > 0)
	return;

> +error:
> +	g_dbus_send_error(conn, data->hm->msg, DBUS_ERROR_FAILED,
> +			"Unable to read voltage");

and change this to a generic failed message.  You can use 
__ofono_dbus_pending_reply.  This will have the benefit of setting 
hm->msg to NULL so that subsequent GetStatistics calls don't return 'busy'.

> +}
> +
>  static DBusMessage *hardware_monitor_get_statistics(DBusConnection *conn,
>  							DBusMessage *msg,
>  							void *user_data)
>  {
> +	struct gemalto_data *data = user_data;
> +
>  	DBG("");
>
> -	return __ofono_error_not_implemented(msg);
> +	if (data->hm->msg != NULL)
> +		return __ofono_error_busy(msg);
> +
> +	if (!g_at_chat_send(data->app, "AT^SBV", sbv_prefix, gemalto_sbv_cb,
> +			data, NULL))
> +		return __ofono_error_failed(msg);
> +
> +	data->hm->msg = dbus_message_new_method_return(msg);
> +
> +	return NULL;
>  }
>
>  static const GDBusMethodTable hardware_monitor_methods[] = {
>

Regards,
-Denis

Re: [RFC v2 3/3] doc: gemalto: add documentation about interface cinterion.HardwareMonitor.

[Denis Kenzior]

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

Hi Vincent,

On 05/16/2017 03:38 AM, Vincent Cesson wrote:
> This interface exposes a single method to access temperature and supply
> voltage of the modem.
> ---
>  doc/cinterion-hardware-monitor-api.txt | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>  create mode 100644 doc/cinterion-hardware-monitor-api.txt
>

I reworded the commit slightly..

> diff --git a/doc/cinterion-hardware-monitor-api.txt b/doc/cinterion-hardware-monitor-api.txt
> new file mode 100644
> index 0000000..50adca3
> --- /dev/null
> +++ b/doc/cinterion-hardware-monitor-api.txt
> @@ -0,0 +1,17 @@
> +
> +HardwareMonitor hierarchy
> +=========================
> +
> +Service		org.ofono.cinterion

You're still running inside the org.ofono service.  Only the interface 
has a different prefix.  So I fixed this as well and applied the patch.

Regards,
-Denis

[PATCH v3] gemalto: Implement HardwareMonitor method

[Vincent Cesson]

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

Use Gemalto commands ^SCTM and ^SBV to monitor temperature and voltage.
Use a single method GetStatistics to read and return both values.
---
 plugins/gemalto.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 3798e09..011713e 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -53,6 +53,8 @@
 #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".cinterion.HardwareMonitor"
 
 static const char *none_prefix[] = { NULL };
+static const char *sctm_prefix[] = { "^SCTM:", NULL };
+static const char *sbv_prefix[] = { "^SBV:", NULL };
 
 struct gemalto_hardware_monitor {
 	DBusMessage *msg;
@@ -155,13 +157,102 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 						NULL);
 }
 
+static void gemalto_sctm_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct gemalto_data *data = user_data;
+	DBusMessage *reply;
+	GAtResultIter iter;
+	DBusMessageIter dbus_iter;
+	DBusMessageIter dbus_dict;
+
+	if (data->hm->msg == NULL)
+		return;
+
+	if (!ok)
+		goto error;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "^SCTM:"))
+		goto error;
+
+	if (!g_at_result_iter_skip_next(&iter))
+		goto error;
+
+	if (!g_at_result_iter_skip_next(&iter))
+		goto error;
+
+	if (!g_at_result_iter_next_number(&iter, &data->hm->temperature))
+		goto error;
+
+	reply = dbus_message_new_method_return(data->hm->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);
+
+	ofono_dbus_dict_append(&dbus_dict, "Temperature",
+			DBUS_TYPE_INT32, &data->hm->temperature);
+
+	ofono_dbus_dict_append(&dbus_dict, "Voltage",
+			DBUS_TYPE_UINT32, &data->hm->voltage);
+
+	dbus_message_iter_close_container(&dbus_iter, &dbus_dict);
+
+	__ofono_dbus_pending_reply(&data->hm->msg, reply);
+
+	return;
+
+error:
+	__ofono_dbus_pending_reply(&data->hm->msg,
+			__ofono_error_failed(data->hm->msg));
+}
+
+static void gemalto_sbv_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct gemalto_data *data = user_data;
+	GAtResultIter iter;
+
+	if (!ok)
+		goto error;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "^SBV:"))
+		goto error;
+
+	if (!g_at_result_iter_next_number(&iter, &data->hm->voltage))
+		goto error;
+
+	if (g_at_chat_send(data->app, "AT^SCTM?", sctm_prefix, gemalto_sctm_cb,
+				data, NULL) > 0)
+		return;
+
+error:
+	__ofono_dbus_pending_reply(&data->hm->msg,
+			__ofono_error_failed(data->hm->msg));
+}
+
 static DBusMessage *hardware_monitor_get_statistics(DBusConnection *conn,
 							DBusMessage *msg,
 							void *user_data)
 {
+	struct gemalto_data *data = user_data;
+
 	DBG("");
 
-	return __ofono_error_not_implemented(msg);
+	if (data->hm->msg != NULL)
+		return __ofono_error_busy(msg);
+
+	if (!g_at_chat_send(data->app, "AT^SBV", sbv_prefix, gemalto_sbv_cb,
+			data, NULL))
+		return __ofono_error_failed(msg);
+
+	data->hm->msg = dbus_message_ref(msg);
+
+	return NULL;
 }
 
 static const GDBusMethodTable hardware_monitor_methods[] = {
-- 
1.9.1

Re: [PATCH v3] gemalto: Implement HardwareMonitor method

[Denis Kenzior]

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

Hi Vincent,

On 05/17/2017 04:12 AM, Vincent Cesson wrote:
> Use Gemalto commands ^SCTM and ^SBV to monitor temperature and voltage.
> Use a single method GetStatistics to read and return both values.
> ---
>  plugins/gemalto.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 92 insertions(+), 1 deletion(-)
>

Applied, thanks.

Regards,
-Denis