Re: [RFC PATCH 2/4] modem: add EmergencyMode property

[Andras Domokos <andras.domokos@nokia.com>]

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

Hi Denis,

On 11/23/2010 10:46 AM, ext Denis Kenzior wrote:
> Hi Andras,
>
> On 11/15/2010 10:57 AM, Andras Domokos wrote:
>    
>> From: Andras Domokos<andras.domokos@nokia.com>
>>
>> ---
>>   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.
>    
I made changes based on your comments and I am going to
resend the patch.

> Regards,
> -Denis
>    
Regards,
Andras