[PATCH] sms: Add delay before submitting multiple SMS to modem

[PATCH] sms: Add delay before submitting multiple SMS to modem

[Guillaume Zajac]

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

For 3GPP GSM test case 34.2.9.1 and 34.2.9.1 and WCDMA test
case 16.1.9.1 and 16.1.9.2, we need to transmitmultiple SMS
using same RRC channel.
oFono needs to wait its tx queue to be filled in with the
next SMS before submitting the first one to use +CMMS=1 modem
option.
---
 src/sms.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index acfc39b..443e502 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -50,6 +50,8 @@
 #define TXQ_MAX_RETRIES 4
 #define NETWORK_TIMEOUT 332
 
+#define SMS_TX_NEXT 500
+
 static gboolean tx_next(gpointer user_data);
 
 static GSList *g_drivers = NULL;
@@ -2072,7 +2074,7 @@ int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
 	g_queue_push_tail(sms->txq, entry);
 
 	if (sms->registered && g_queue_get_length(sms->txq) == 1)
-		sms->tx_source = g_timeout_add(0, tx_next, sms);
+		sms->tx_source = g_timeout_add(SMS_TX_NEXT, tx_next, sms);
 
 	if (uuid)
 		memcpy(uuid, &entry->uuid, sizeof(*uuid));
-- 
1.7.5.4

Re: [PATCH] sms: Add delay before submitting multiple SMS to modem

[Denis Kenzior]

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

Hi Guillaume,

On 08/22/2012 04:29 AM, Guillaume Zajac wrote:
> For 3GPP GSM test case 34.2.9.1 and 34.2.9.1 and WCDMA test
> case 16.1.9.1 and 16.1.9.2, we need to transmitmultiple SMS
> using same RRC channel.
> oFono needs to wait its tx queue to be filled in with the
> next SMS before submitting the first one to use +CMMS=1 modem
> option.
> ---
>   src/sms.c |    4 +++-
>   1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/src/sms.c b/src/sms.c
> index acfc39b..443e502 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -50,6 +50,8 @@
>   #define TXQ_MAX_RETRIES 4
>   #define NETWORK_TIMEOUT 332
>
> +#define SMS_TX_NEXT 500
> +
>   static gboolean tx_next(gpointer user_data);
>
>   static GSList *g_drivers = NULL;
> @@ -2072,7 +2074,7 @@ int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>   	g_queue_push_tail(sms->txq, entry);
>
>   	if (sms->registered&&  g_queue_get_length(sms->txq) == 1)
> -		sms->tx_source = g_timeout_add(0, tx_next, sms);
> +		sms->tx_source = g_timeout_add(SMS_TX_NEXT, tx_next, sms);
>

Umm, I don't even know how to respond to this.  Lets just say that this 
isn't going to be accepted upstream ;)  Why don't you use multi-segment 
SMS, or queue the messages offline.

>   	if (uuid)
>   		memcpy(uuid,&entry->uuid, sizeof(*uuid));

Regards,
-Denis

Re: [PATCH] sms: Add delay before submitting multiple SMS to modem

[Guillaume Zajac]

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

Hi Denis,

On 23/08/2012 00:57, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 08/22/2012 04:29 AM, Guillaume Zajac wrote:
>> For 3GPP GSM test case 34.2.9.1 and 34.2.9.1 and WCDMA test
>> case 16.1.9.1 and 16.1.9.2, we need to transmitmultiple SMS
>> using same RRC channel.
>> oFono needs to wait its tx queue to be filled in with the
>> next SMS before submitting the first one to use +CMMS=1 modem
>> option.
>> ---
>>   src/sms.c |    4 +++-
>>   1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/sms.c b/src/sms.c
>> index acfc39b..443e502 100644
>> --- a/src/sms.c
>> +++ b/src/sms.c
>> @@ -50,6 +50,8 @@
>>   #define TXQ_MAX_RETRIES 4
>>   #define NETWORK_TIMEOUT 332
>>
>> +#define SMS_TX_NEXT 500
>> +
>>   static gboolean tx_next(gpointer user_data);
>>
>>   static GSList *g_drivers = NULL;
>> @@ -2072,7 +2074,7 @@ int __ofono_sms_txq_submit(struct ofono_sms 
>> *sms, GSList *list,
>>       g_queue_push_tail(sms->txq, entry);
>>
>>       if (sms->registered&& g_queue_get_length(sms->txq) == 1)
>> -        sms->tx_source = g_timeout_add(0, tx_next, sms);
>> +        sms->tx_source = g_timeout_add(SMS_TX_NEXT, tx_next, sms);
>>
>
> Umm, I don't even know how to respond to this.  Lets just say that 
> this isn't going to be accepted upstream ;)  Why don't you use 
> multi-segment SMS, or queue the messages offline.

We can forget about queuing messages offline because the simulator needs 
to check we are registered to the network before asking to send the 3 
multiple SMS.
So this will break the sequences expected by the test.
Concerning Multi-segment message, we already knew it was working for GSM 
test cases 34.2.9.1 and 34.2.9.1 because message content is not checked.
However for WCDMA test case we have a message specified by the test to 
send and I was expecting the simulator to check the content of each 
multiple messages. That's why I chose to use a delay. Fortunately for 
practical purposes, the test is passing in sending multi-segment 
messages even with a text not corresponding to the one specified by the 
test.
Unfortunately I come to the conclusion that if I want to send one same 
SMS to 3/4/5/... recipients with an application using oFono middleware, 
I will not be able to send them through the same RRC channel, unless I 
queue them offline.

>
>
>>       if (uuid)
>>           memcpy(uuid,&entry->uuid, sizeof(*uuid));
>
> Regards,
> -Denis

Kind regards,
Guillaume

Re: [PATCH] sms: Add delay before submitting multiple SMS to modem

[Denis Kenzior]

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

Hi Guillaume,

<snip>

> Unfortunately I come to the conclusion that if I want to send one same
> SMS to 3/4/5/... recipients with an application using oFono middleware,
> I will not be able to send them through the same RRC channel, unless I
> queue them offline.
>

If such a feature is important, then you do have a point.  However, 
implementing _anything_ by introducing an arbitrary delay is simply 
wrong.  I believe they teach the reasons for that in Operating Systems 
101.  You are introducing a race condition that will break things at the 
most inopportune time.

If we really require multi-recipient SMS to go on the same RRC channel, 
then the best way to do so would be to introduce a new API specifically 
for this use case.

Regards,
-Denis

Re: [PATCH] sms: Add delay before submitting multiple SMS to modem

[Guillaume Zajac]

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

Hi Denis,

On 23/08/2012 15:48, Denis Kenzior wrote:
> Hi Guillaume,
>
> <snip>
>
>> Unfortunately I come to the conclusion that if I want to send one same
>> SMS to 3/4/5/... recipients with an application using oFono middleware,
>> I will not be able to send them through the same RRC channel, unless I
>> queue them offline.
>>
>
> If such a feature is important, then you do have a point. However, 
> implementing _anything_ by introducing an arbitrary delay is simply 
> wrong.  I believe they teach the reasons for that in Operating Systems 
> 101.  You are introducing a race condition that will break things at 
> the most inopportune time.

I agree that it is not the most elegant/cleanest solution.

>
> If we really require multi-recipient SMS to go on the same RRC 
> channel, then the best way to do so would be to introduce a new API 
> specifically for this use case.

Having a new API for multi-recipient SMS would be indeed the best solution.
With current implementation the test is passing with multi-segment SMS 
using script but GCF test description is mentioning 3 different SMS.
During device certification (with applications), the tester will try to 
send an SMS to 3 recipients but it will fail.
Another solution would be to remove multi-SMS support from the PICS.

Kind regards,
Guillaume

Re: [PATCH] sms: Add delay before submitting multiple SMS to modem

[Denis Kenzior]

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

Hi Guillaume,

On 08/23/2012 09:26 AM, Guillaume Zajac wrote:
> Hi Denis,
>
> On 23/08/2012 15:48, Denis Kenzior wrote:
>> Hi Guillaume,
>>
>> <snip>
>>
>>> Unfortunately I come to the conclusion that if I want to send one same
>>> SMS to 3/4/5/... recipients with an application using oFono middleware,
>>> I will not be able to send them through the same RRC channel, unless I
>>> queue them offline.
>>>
>>
>> If such a feature is important, then you do have a point. However,
>> implementing _anything_ by introducing an arbitrary delay is simply
>> wrong. I believe they teach the reasons for that in Operating Systems
>> 101. You are introducing a race condition that will break things at
>> the most inopportune time.
>
> I agree that it is not the most elegant/cleanest solution.
>

Then please refrain from sending patches which contain 'not the most 
elegant/cleanest' solution in the future.  oFono has a very high 
standard of quality, and the standard is even higher in the core than 
the driver code.

You are better off starting a discussion on IRC or describing the 
problem in detail on the mailing list asking for possible solutions.

>>
>> If we really require multi-recipient SMS to go on the same RRC
>> channel, then the best way to do so would be to introduce a new API
>> specifically for this use case.
>
> Having a new API for multi-recipient SMS would be indeed the best solution.
> With current implementation the test is passing with multi-segment SMS
> using script but GCF test description is mentioning 3 different SMS.
> During device certification (with applications), the tester will try to
> send an SMS to 3 recipients but it will fail.
> Another solution would be to remove multi-SMS support from the PICS.

Our current goal is just the baseline.  That means if we're missing a 
feature, then we need to note that down and continue on.

This is where the TODO file comes in.  If you see features that are 
missing, then send a patch to the TODO file describing the feature and 
the preferred approach (after the relevant discussions on IRC/mailing 
list).  The feature priority assignment / implementation can then 
proceed with the well-established workflow.

Regards,
-Denis

Re: [PATCH] sms: Add delay before submitting multiple SMS to modem

[Guillaume Zajac]

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

Hi Denis,

On 23/08/2012 19:33, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 08/23/2012 09:26 AM, Guillaume Zajac wrote:
>> Hi Denis,
>>
>> On 23/08/2012 15:48, Denis Kenzior wrote:
>>> Hi Guillaume,
>>>
>>> <snip>
>>>
>>>> Unfortunately I come to the conclusion that if I want to send one same
>>>> SMS to 3/4/5/... recipients with an application using oFono 
>>>> middleware,
>>>> I will not be able to send them through the same RRC channel, unless I
>>>> queue them offline.
>>>>
>>>
>>> If such a feature is important, then you do have a point. However,
>>> implementing _anything_ by introducing an arbitrary delay is simply
>>> wrong. I believe they teach the reasons for that in Operating Systems
>>> 101. You are introducing a race condition that will break things at
>>> the most inopportune time.
>>
>> I agree that it is not the most elegant/cleanest solution.
>>
>
> Then please refrain from sending patches which contain 'not the most 
> elegant/cleanest' solution in the future.  oFono has a very high 
> standard of quality, and the standard is even higher in the core than 
> the driver code.
>
> You are better off starting a discussion on IRC or describing the 
> problem in detail on the mailing list asking for possible solutions.
>
>>>
>>> If we really require multi-recipient SMS to go on the same RRC
>>> channel, then the best way to do so would be to introduce a new API
>>> specifically for this use case.
>>
>> Having a new API for multi-recipient SMS would be indeed the best 
>> solution.
>> With current implementation the test is passing with multi-segment SMS
>> using script but GCF test description is mentioning 3 different SMS.
>> During device certification (with applications), the tester will try to
>> send an SMS to 3 recipients but it will fail.
>> Another solution would be to remove multi-SMS support from the PICS.
>
> Our current goal is just the baseline.  That means if we're missing a 
> feature, then we need to note that down and continue on.
>
> This is where the TODO file comes in.  If you see features that are 
> missing, then send a patch to the TODO file describing the feature and 
> the preferred approach (after the relevant discussions on IRC/mailing 
> list).  The feature priority assignment / implementation can then 
> proceed with the well-established workflow.
>

Ok I will send a RFC about this new feature, this will be the new thread 
to discuss on it.

Kind regards,
Guillaume