Re: [PATCH 4/6] gemalto: handle sim is inserted or removed URCs

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Gabriel,

On 03/16/2018 08:28 AM, Gabriel Lucas wrote:
> Hi Denis,
> 
> On 15/03/2018 18:26, Denis Kenzior wrote:
>> Hi Gabriel,
>>
>> On 03/15/2018 07:49 AM, Gabriel Lucas wrote:
>>> From: Mariem Cherif <mariem.cherif@ardia.com.tn>
>>>
>>> +    g_at_result_iter_init(&iter, result);
>>> +
>>> +    if (!g_at_result_iter_next(&iter, "+CIEV:"))
>>> +        return;
>>> +
>>
>> So generally +CIEV indication is an <index>,<value> syntax.
> We receive this kind of signal: +CIEV: simstatus,<status>.
> In this code, Mariem is ensuring the index is simstatus.
> Do you prefer us to use g_at_result_iter_next to handle the index ?
> Or even to use "+CIEV: simstatus" as prefix in the register function ?

Okay, that makes sense now.  Gemalto should really fix their firmware as 
this syntax is not really correct, but nothing we can do about it now. 
I would stick with the current approach.

>>> +    if (!g_at_result_iter_next_unquoted_string(&iter, &ind_str))
>>> +        return;
>>
>> Does Gemalto use some other syntax here?  If so, you might want to 
>> document a simple example above.
> I've added a comment
>>> +        ofono_sim_inserted_notify(sim, FALSE);
>>> +    } else if(status == 1) {
>>> +        ofono_sim_inserted_notify(sim, TRUE);
>>> +    }
>>
>> Okay, but simpler written as ofono_sim_inserted_notify(sim, status);
> The thing is we don't only receive the status 0 and 1. We also have 4 
> (ALS3) and
> 5 (ALS3 + PHS8P). Only the first two should be used.

In that case I would use a switch/case statement, add a case label 
/comment for the other possibilities and what they mean and simply use a 
'break' statement for those.

Regards,
-Denis