Hi Andras,
On 11/15/2010 10:57 AM, Andras Domokos wrote:
> From: Andras Domokos <[email protected]>
>
> ---
> src/modem.c | 134
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> src/ofono.h | 4 ++
> 2 files changed, 138 insertions(+), 0 deletions(-)
>
> diff --git a/src/modem.c b/src/modem.c
> index f73cc1d..4307914 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -61,6 +61,7 @@ enum modem_state {
> struct ofono_modem {
> char *path;
> enum modem_state modem_state;
> + enum modem_state modem_state_pre_emergency;
> GSList *atoms;
> struct ofono_watchlist *atom_watches;
> GSList *interface_list;
> @@ -72,6 +73,7 @@ struct ofono_modem {
> ofono_bool_t powered_pending;
> guint timeout;
> ofono_bool_t online;
> + unsigned int emergency_mode;
I really prefer this to be called 'emergency'
> struct ofono_watchlist *online_watches;
> GHashTable *properties;
> struct ofono_sim *sim;
> @@ -514,6 +516,127 @@ static void offline_cb(const struct ofono_error *error,
> void *data)
> modem_change_state(modem, MODEM_STATE_OFFLINE);
> }
>
> +ofono_bool_t ofono_modem_get_emergency_mode(struct ofono_modem *modem)
> +{
> + if (modem == NULL)
> + return FALSE;
> +
> + return modem->emergency_mode != 0;
> +}
> +
> +static void modem_change_online(struct ofono_modem *modem,
> + enum modem_state new_state)
> +{
> + DBusConnection *conn = ofono_dbus_get_connection();
> + enum modem_state old_state = modem->modem_state;
> + ofono_bool_t new_online = new_state == MODEM_STATE_ONLINE;
> +
> + DBG("old state: %d, new state: %d", old_state, new_state);
> +
> + if (new_online == modem->online)
> + return;
> +
> + modem->modem_state = new_state;
> + modem->online = new_online;
> +
> + ofono_dbus_signal_property_changed(conn, modem->path,
> + OFONO_MODEM_INTERFACE, "Online",
> + DBUS_TYPE_BOOLEAN, &modem->online);
> +
> + notify_online_watches(modem);
> +}
> +
> +static void emergency_online_cb(const struct ofono_error *error, void *data)
> +{
> + struct ofono_modem *modem = data;
> +
> + DBG("modem: %p", modem);
> +
> + if (error->type == OFONO_ERROR_TYPE_NO_ERROR &&
> + modem->modem_state != MODEM_STATE_ONLINE)
> + modem_change_online(modem, MODEM_STATE_ONLINE);
> +}
> +
> +static void emergency_offline_cb(const struct ofono_error *error, void *data)
> +{
> + struct ofono_modem *modem = data;
> + DBusConnection *conn = ofono_dbus_get_connection();
> + const char *path = ofono_modem_get_path(modem);
> + gboolean state = FALSE;
> +
> + DBG("modem: %p", modem);
> +
> + if (error->type == OFONO_ERROR_TYPE_NO_ERROR &&
> + modem->modem_state == MODEM_STATE_ONLINE)
> + modem_change_online(modem, modem->modem_state_pre_emergency);
> +
> + modem->emergency_mode--;
> +
> + ofono_dbus_signal_property_changed(conn, path,
> + OFONO_MODEM_INTERFACE,
> + "EmergencyMode",
The property should really be called 'Emergency' to be in line with the
current API proposal (doc/modem-api.txt & TODO)
> + DBUS_TYPE_BOOLEAN,
> + &state);
> +}
> +
> +void ofono_modem_inc_emergency_mode(struct ofono_modem *modem)
> +{
> + DBusConnection *conn = ofono_dbus_get_connection();
> + const char *path = ofono_modem_get_path(modem);
> + gboolean state = TRUE;
> +
> + DBG("modem: %p", modem);
> +
> + modem->emergency_mode++;
> + if (modem->emergency_mode > 1)
> + return;
> +
> + ofono_dbus_signal_property_changed(conn, path,
> + OFONO_MODEM_INTERFACE,
> + "EmergencyMode",
Again, 'Emergency' here
> + DBUS_TYPE_BOOLEAN,
> + &state);
> +
> + modem->modem_state_pre_emergency = modem->modem_state;
> +
> + if (modem->modem_state == MODEM_STATE_ONLINE)
> + return;
> +
> + modem->driver->set_online(modem, TRUE, emergency_online_cb, modem);
> +}
> +
> +void ofono_modem_dec_emergency_mode(struct ofono_modem *modem)
> +{
> + DBusConnection *conn = ofono_dbus_get_connection();
> + const char *path = ofono_modem_get_path(modem);
> + gboolean state = FALSE;
> +
> + DBG("modem: %p", modem);
> +
> + if (modem->emergency_mode == 0)
> + return;
Can you be a bit more pedantic and send an ofono_error for this case?
We probably want to track whether some plugin abuses the reference counting.
> +
> + if (modem->emergency_mode > 1) {
> + modem->emergency_mode--;
> + return;
> + }
> +
> + if (modem->modem_state == MODEM_STATE_ONLINE &&
> + modem->modem_state_pre_emergency != MODEM_STATE_ONLINE) {
Please indent one more, item M4 in coding-style.txt
> + modem->driver->set_online(modem, FALSE,
> + emergency_offline_cb, modem);
> + return;
> + }
> +
> + modem->emergency_mode--;
> +
> + ofono_dbus_signal_property_changed(conn, path,
> + OFONO_MODEM_INTERFACE,
> + "EmergencyMode",
And again, 'Emergency' here.
> + DBUS_TYPE_BOOLEAN,
> + &state);
> +}
> +
> static DBusMessage *set_property_online(struct ofono_modem *modem,
> DBusMessage *msg,
> DBusMessageIter *var)
> @@ -535,6 +658,9 @@ static DBusMessage *set_property_online(struct
> ofono_modem *modem,
> if (modem->modem_state < MODEM_STATE_OFFLINE)
> return __ofono_error_not_available(msg);
>
> + if (modem->emergency_mode)
> + return __ofono_error_failed(msg);
> +
Is it worth to create a new dbus error for this case? Perhaps
ofono_error_emergency, or emergency_active?
> if (modem->online == online)
> return dbus_message_new_method_return(msg);
>
> @@ -562,6 +688,7 @@ void __ofono_modem_append_properties(struct ofono_modem
> *modem,
> int i;
> GSList *l;
> struct ofono_atom *devinfo_atom;
> + ofono_bool_t state;
Please rename this variable to 'val' or 'value'
>
> ofono_dbus_dict_append(dict, "Online", DBUS_TYPE_BOOLEAN,
> &modem->online);
> @@ -569,6 +696,10 @@ void __ofono_modem_append_properties(struct ofono_modem
> *modem,
> ofono_dbus_dict_append(dict, "Powered", DBUS_TYPE_BOOLEAN,
> &modem->powered);
>
> + state = ofono_modem_get_emergency_mode(modem);
> + ofono_dbus_dict_append(dict, "EmergencyMode",
And 'Emergency' here
> + DBUS_TYPE_BOOLEAN, &state);
> +
> devinfo_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_DEVINFO);
>
> /* We cheat a little here and don't check the registered status */
> @@ -647,6 +778,9 @@ static int set_powered(struct ofono_modem *modem,
> ofono_bool_t powered)
> if (modem->powered_pending == powered)
> return -EALREADY;
>
> + if (modem->emergency_mode)
I really would prefer modem->emergency > 0 here
> + return -EINVAL;
> +
If we introduce a new ofono error, then returning something like EPERM
or EACCESS for the case where emergency mode is active might be a good idea.
> /* Remove the atoms even if the driver is no longer available */
> if (powered == FALSE)
> modem_change_state(modem, MODEM_STATE_POWER_OFF);
> diff --git a/src/ofono.h b/src/ofono.h
> index 01cd6c0..ac56a85 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -185,6 +185,10 @@ unsigned int __ofono_modem_add_online_watch(struct
> ofono_modem *modem,
> void __ofono_modem_remove_online_watch(struct ofono_modem *modem,
> unsigned int id);
>
> +ofono_bool_t ofono_modem_get_emergency_mode(struct ofono_modem *modem);
> +void ofono_modem_inc_emergency_mode(struct ofono_modem *modem);
> +void ofono_modem_dec_emergency_mode(struct ofono_modem *modem);
> +
> #include <ofono/call-barring.h>
>
> gboolean __ofono_call_barring_is_busy(struct ofono_call_barring *cb);
Otherwise this one looks good to me.
Regards,
-Denis
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
