[PATCH 4/4] Add a MessageWaiting interface to track message waiting indications.

[Andrzej Zaborowski]

2009/7/31 Denis Kenzior <denkenz at gmail.com>:
>> +     if (mw->messages[i] != value) {
>> +             mw->messages[i] = value;
>> +
>> +             if (!mw->pending)
>> +                     mw->pending = g_timeout_add(0, mw_mwis_update, modem);
>> +
>> +             dbus_gsm_signal_property_changed(conn, modem->path,
>> +                             MESSAGE_WAITING_INTERFACE,
>> +                             mw_messages_property_name[i],
>> +                             DBUS_TYPE_BYTE, &value);
>> +     }
>> +
>> +     return dbus_message_new_method_return(msg);
>> +}
>
> Ok, I'm a bit confused here.  Are you sure that MWIs should have the ability
> to be cleared out by the user?  The way I understood it, the user dials the
> voice mail system and the provider sends another MWI message which clears out
> the status.

You're right, there's probably no point to provide this method.  I
haven't found the exact statement in the docs saying that the provider
should send a new MWI but it's logical.  Otherwise I thought the D-Bus
client might be able to tell somehow that the mailbox should now have
1 message less and want to update us.

>
>> +
>> +enum mwi_information_type {
>> +     MWI_UNSPECIFIED = -1,
>> +     MWI_MESSAGES_WAITING = -2,
>> +     MWI_NO_MESSAGES_WAITING = -3,
>> +};
>
> Is there a reason these are negative?

So the caller can give the exact number of messages (zero or positive)
or partial information using one of the constants.

>
>> +static void handle_mwi(struct ofono_modem *modem,
>> +                     struct sms *sms, gboolean *out_discard)
>> +{
>> +     gboolean active, discard;
>> +     enum sms_mwi_type type;
>> +     char *message;
>> +     int profile = 1;
>> +     GSList *sms_list;
>> +
>> +     /* "Store" bits are ORed if multiple MWI types are present
>> +      * but if neither Special SMS Message Indication nor DCS based
>> +      * indication is present, the bit must remain set.  */
>> +     int set = 1, store = 0;
>
> What is this comment about, since I don't see any ORing going on below.  Are
> you trying to accommodate this piece of stupidity from 23.040 9.2.3.24.2?
> "In the event of a conflict between this setting and the setting of the Data
> Coding Scheme (see 3GPP TS 23.038 [9]) then the message shall be stored if
> either the DCS indicates this, or Octet 1 above indicates this."

Yes. :-)

I'm thinking it's better to try to implement what's in the text so
when someone needs to know how ofono will react to something, you can
point at the specification, barring any TODOs in the code.

>
>> +
>> +     /* Check MWI types in the order from lowest to highest priority
>> +      * because they will override one another.  */
>
> Can we instead process the different sources from highest to lowest instead and
> bail out early?  No sense in calling message_waiting_notify several times (and
> possibly emitting spurious signals) when only once is required.

We can but then we won't comply with that 23.040 9.2.3.24.2 above.
There's also a tiny possibility someone might send us updates for the
different mailboxes (out of the 5 types defined) one using DCS and
another one in UDH.  In the attached patch I left this as is and made
sure PropertyChanged is only sent from the time callback (bottom half)
which was my original intention.  I can still change this.

>
>> +     if (sms_dcs_decode(sms->deliver.dcs, NULL, NULL, NULL, NULL) &&
>> +                     sms->deliver.udhi) {
>
> This seems to be incorrect.  An MWI DCS message can still contain enhanced
> IEIs.  Should this check just for UDHI instead?

True, I had wrongly copied the condition from ofono_sms_deliver_notify.

>
>> +final:
>> +     /* Only bother the Message Waiting interface with textual
>> +      * notifications that are not to be stored together with other
>> +      * Short Messages in ME.  The other messages will be delivered
>> +      * to the user through normal SMS store.  */
>
> It sounds to me like we can completely ignore the text of such messages.  The
> network is basically saying: "the text isn't important, the contents are."

Probably, although with the Return Call type of notification there's
no other information beside the text.  I left it in the updated patch.

Regards
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-a-MessageWaiting-interface-to-track-message-wait.patch
Type: text/x-patch
Size: 23067 bytes
Desc: not available
URL: <http://lists.ofono.org/pipermail/ofono/attachments/20090802/4bea867c/attachment-0001.bin>