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

[Denis Kenzior <denkenz@gmail.com>]

[-- 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