[PATCH] atmodem: Enable network time for AT modem

[PATCH] atmodem: Enable network time for AT modem

[Antti Paila]

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

---
 drivers/atmodem/network-registration.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
index 4913611..2d589f0 100644
--- a/drivers/atmodem/network-registration.c
+++ b/drivers/atmodem/network-registration.c
@@ -722,6 +722,8 @@ static void ifx_ctzv_notify(GAtResult *result, gpointer user_data)
 	nd->time.mday = mday;
 	nd->time.mon = mon;
 	nd->time.year = 2000 + year;
+
+	ofono_netreg_time_notify(netreg, &nd->time);
 }
 
 static void ifx_ctzdst_notify(GAtResult *result, gpointer user_data)
-- 
1.7.1

RE: [PATCH] atmodem: Enable network time for AT modem

[Jeevaka.Badrappan]

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

Hi Antti,

ofono-bounces(a)ofono.org wrote:
> ---
>  drivers/atmodem/network-registration.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/atmodem/network-registration.c
> b/drivers/atmodem/network-registration.c
> index 4913611..2d589f0 100644
> --- a/drivers/atmodem/network-registration.c
> +++ b/drivers/atmodem/network-registration.c
> @@ -722,6 +722,8 @@ static void ifx_ctzv_notify(GAtResult *result,
>  	gpointer user_data) nd->time.mday = mday;
>  	nd->time.mon = mon;
>  	nd->time.year = 2000 + year;
> +
> +	ofono_netreg_time_notify(netreg, &nd->time);
>  }
> 
>  static void ifx_ctzdst_notify(GAtResult *result, gpointer user_data)

Since the fix is more to do with the ifx modem, it would be nice if we
have the
commit message changed to reflect that.

Whenever there is a timezone change, CTZV, CTZDST and XNITZINFO URCs
will be reported. That is the main
reason behind calling the ofono_netreg_time_notify from CTZDST. Are you
pretty sure
this is not the case?

Regards,
Jeevaka

RE: [PATCH] atmodem: Enable network time for AT modem

[Antti Paila]

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

Hi Jeevaka,

On Tue, 2011-03-08 at 10:59 +0200, ext Jeevaka.Badrappan(a)elektrobit.com
wrote:
> Hi Antti,
> 
> ofono-bounces(a)ofono.org wrote:
> > ---
> >  drivers/atmodem/network-registration.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/atmodem/network-registration.c
> > b/drivers/atmodem/network-registration.c
> > index 4913611..2d589f0 100644
> > --- a/drivers/atmodem/network-registration.c
> > +++ b/drivers/atmodem/network-registration.c
> > @@ -722,6 +722,8 @@ static void ifx_ctzv_notify(GAtResult *result,
> >  	gpointer user_data) nd->time.mday = mday;
> >  	nd->time.mon = mon;
> >  	nd->time.year = 2000 + year;
> > +
> > +	ofono_netreg_time_notify(netreg, &nd->time);
> >  }
> > 
> >  static void ifx_ctzdst_notify(GAtResult *result, gpointer user_data)
> 
> Since the fix is more to do with the ifx modem, it would be nice if we
> have the
> commit message changed to reflect that.

I suppose the person applying the patch can do the modifications to the commit message if needed. 

> Whenever there is a timezone change, CTZV, CTZDST and XNITZINFO URCs
> will be reported. That is the main
> reason behind calling the ofono_netreg_time_notify from CTZDST. Are you
> pretty sure
> this is not the case?

I don't know the details of ifx modem internals. If the time changes, is
it always guaranteed that CTZDST is sent and that CTZDST comes after the
CTZV? All I know that when testing NITZ with ifx modem, the time
notification is not emitted without my fix.

BR,
  Antti

RE: [PATCH] atmodem: Enable network time for AT modem

[Jeevaka.Badrappan]

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

Hi Antti,

ofono-bounces(a)ofono.org wrote:
> Hi Jeevaka,
> 
> On Tue, 2011-03-08 at 10:59 +0200, ext
> Jeevaka.Badrappan(a)elektrobit.com
> wrote:
>> Hi Antti,
>> 
>> ofono-bounces(a)ofono.org wrote:
>>> ---
>>>  drivers/atmodem/network-registration.c |    2 ++
>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/drivers/atmodem/network-registration.c
>>> b/drivers/atmodem/network-registration.c
>>> index 4913611..2d589f0 100644
>>> --- a/drivers/atmodem/network-registration.c
>>> +++ b/drivers/atmodem/network-registration.c
>>> @@ -722,6 +722,8 @@ static void ifx_ctzv_notify(GAtResult *result,
>>>  	gpointer user_data) nd->time.mday = mday;
>>>  	nd->time.mon = mon;
>>>  	nd->time.year = 2000 + year;
>>> +
>>> +	ofono_netreg_time_notify(netreg, &nd->time);
>>>  }
>>> 
>>>  static void ifx_ctzdst_notify(GAtResult *result, gpointer
>>> user_data)
> 
>> Whenever there is a timezone change, CTZV, CTZDST and XNITZINFO URCs
>> will be reported. That is the main reason behind calling the
>> ofono_netreg_time_notify from CTZDST. Are you pretty sure this is not
>> the case?
> 
> I don't know the details of ifx modem internals. If the time
> changes, is it always guaranteed that CTZDST is sent and that
> CTZDST comes after the CTZV? All I know that when testing
> NITZ with ifx modem, the time notification is not emitted without my
> fix. 

ofonod[454]: Net: < \r\n+XNITZINFO:
"GMT+00:00","08/01/01,13:00:00"\r\n\r\n+CTZDST: 0\r\n
ofonod[454]: drivers/atmodem/network-registration.c:ifx_ctzdst_notify()
dst 0
ofonod[454]: examples/nettime.c:example_nettime_info_received() Received
a network time notification on modem: 0x83fc058
ofonod[454]: examples/nettime.c:example_nettime_info_received() Time:
2008-01-01 13:00:00-00:00 (DST=0)
ofonod[454]: Net: < \r\n+CTZV: +00,"11/01/27,14:24:43"\r\n
ofonod[454]: drivers/atmodem/network-registration.c:ifx_ctzv_notify() tz
+00 time 11/01/27,14:24:43
ofonod[454]: Net: < \r\n+XNITZINFO:
"GMT+00:00","11/01/27,14:24:43"\r\n\r\n+CTZDST: 0\r\n
ofonod[454]: drivers/atmodem/network-registration.c:ifx_ctzdst_notify()
dst 0
ofonod[454]: examples/nettime.c:example_nettime_info_received() Received
a network time notification on modem: 0x83fc058
ofonod[454]: examples/nettime.c:example_nettime_info_received() Time:
2011-01-27 14:24:43-00:00 (DST=0)
......
ofonod[454]: Net: < \r\n+CTZV: +04,"11/01/27,15:24:43"\r\n
ofonod[454]: drivers/atmodem/network-registration.c:ifx_ctzv_notify() tz
+04 time 11/01/27,15:24:43
ofonod[454]: Net: < \r\n+XNITZINFO:
"GMT+00:00","11/01/27,15:24:43"\r\n\r\n+CTZDST: 1\r\n
ofonod[454]: drivers/atmodem/network-registration.c:ifx_ctzdst_notify()
dst 1
ofonod[454]: examples/nettime.c:example_nettime_info_received() Received
a network time notification on modem: 0x83fc058
ofonod[454]: examples/nettime.c:example_nettime_info_received() Time:
2011-01-27 15:24:43-00:00 (DST=0)

From the logs, it can be seen that we receive CTZV, XNITZINFO and CTZDST
when the time changes.

This log is taken some time back. Possible that Network Daylight Saving
Time is received as part of the MM information message whereas
it may not be in your case.

Regards,
Jeevaka

RE: [PATCH] atmodem: Enable network time for AT modem

[Aki Niemi]

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

Hi Jeevaka,

On Tue, 2011-03-08 at 12:35 +0200, ext Jeevaka.Badrappan(a)elektrobit.com
wrote:
> This log is taken some time back. Possible that Network Daylight Saving
> Time is received as part of the MM information message whereas
> it may not be in your case.

If my memory serves, DST is optional in NITZ; only the UTC offset is
mandatory content.

Cheers,
Aki

RE: [PATCH] atmodem: Enable network time for AT modem

[Jeevaka.Badrappan]

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

Hi Aki,

ofono-bounces(a)ofono.org wrote:
> Hi Jeevaka,
> 
> On Tue, 2011-03-08 at 12:35 +0200, ext
> Jeevaka.Badrappan(a)elektrobit.com
> wrote:
>> This log is taken some time back. Possible that Network Daylight
>> Saving Time is received as part of the MM information message whereas
>> it may not be in your case.
> 
> If my memory serves, DST is optional in NITZ; only the UTC offset is
> mandatory content. 
> 

Thats what I was referring to in my previous mail indirectly.

"Possible that Network Daylight Saving Time is received as part of the
MM information message whereas
 it may not be in your case." This is due to the fact that DST is
optional element.

CTZDST will be reported only when the Network Daylight Saving Time is
received as part of the
MM information message.

So, the fix seems to be correct.

Regards,
Jeevaka

Re: [PATCH] atmodem: Enable network time for AT modem

[Marcel Holtmann]

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

Hi Antti,

>  drivers/atmodem/network-registration.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
> index 4913611..2d589f0 100644
> --- a/drivers/atmodem/network-registration.c
> +++ b/drivers/atmodem/network-registration.c
> @@ -722,6 +722,8 @@ static void ifx_ctzv_notify(GAtResult *result, gpointer user_data)
>  	nd->time.mday = mday;
>  	nd->time.mon = mon;
>  	nd->time.year = 2000 + year;
> +
> +	ofono_netreg_time_notify(netreg, &nd->time);
>  }
>  
>  static void ifx_ctzdst_notify(GAtResult *result, gpointer user_data)

actually this time notification is a bad idea since you will notify
twice now. With the firmware that I tested this with, it is guaranteed
to always get CTZV and CTZDST and in that order.

Regards

Marcel

RE: [PATCH] atmodem: Enable network time for AT modem

[Jeevaka.Badrappan]

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

Hi Marcel,

ofono-bounces(a)ofono.org wrote:
> Hi Antti,
> 
>>  drivers/atmodem/network-registration.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/atmodem/network-registration.c
>> b/drivers/atmodem/network-registration.c
>> index 4913611..2d589f0 100644
>> --- a/drivers/atmodem/network-registration.c
>> +++ b/drivers/atmodem/network-registration.c
>> @@ -722,6 +722,8 @@ static void ifx_ctzv_notify(GAtResult *result,
>>  	gpointer user_data)  	nd->time.mday = mday; nd->time.mon =
mon;
>>  	nd->time.year = 2000 + year;
>> +
>> +	ofono_netreg_time_notify(netreg, &nd->time);
>>  }
>> 
>>  static void ifx_ctzdst_notify(GAtResult *result, gpointer user_data)
> 
> actually this time notification is a bad idea since you will
> notify twice now. With the firmware that I tested this with,
> it is guaranteed to always get CTZV and CTZDST and in that order.
> 

As said in the other mail, CTZDST will be reported only when
the Network Daylight Saving Time information element is received as
part of the MM information message.

If DST information element is received, then the CTZDST will be received
after the CTZV.

I suppose in your case, DST information element is received with the MM
information message.

Regards,
Jeevaka  

Re: [PATCH] atmodem: Enable network time for AT modem

[Antti Paila]

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

Hi Marcel,

On Tue, 2011-03-08 at 09:12 -0800, ext Marcel Holtmann wrote:
> Hi Antti,
> 
> >  drivers/atmodem/network-registration.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
> > index 4913611..2d589f0 100644
> > --- a/drivers/atmodem/network-registration.c
> > +++ b/drivers/atmodem/network-registration.c
> > @@ -722,6 +722,8 @@ static void ifx_ctzv_notify(GAtResult *result, gpointer user_data)
> >  	nd->time.mday = mday;
> >  	nd->time.mon = mon;
> >  	nd->time.year = 2000 + year;
> > +
> > +	ofono_netreg_time_notify(netreg, &nd->time);
> >  }
> >  
> >  static void ifx_ctzdst_notify(GAtResult *result, gpointer user_data)
> 
> actually this time notification is a bad idea since you will notify
> twice now. With the firmware that I tested this with, it is guaranteed
> to always get CTZV and CTZDST and in that order.

Of course there will be two notifications since there are two distinct
event CTZV and CTDST. My ifx modem documentation doesn't say that these
are always bound together so we should not make any implicit assumption.
Anyway I believe that it is the next level i.e. time plug-in whose job
it is to check whether it has all the bits and pieces needed to inform
the Timed or some other client.

BR,
  Antti 

Re: [PATCH] atmodem: Enable network time for AT modem

[Marcel Holtmann]

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

Hi Antti,

> > >  drivers/atmodem/network-registration.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
> > > index 4913611..2d589f0 100644
> > > --- a/drivers/atmodem/network-registration.c
> > > +++ b/drivers/atmodem/network-registration.c
> > > @@ -722,6 +722,8 @@ static void ifx_ctzv_notify(GAtResult *result, gpointer user_data)
> > >  	nd->time.mday = mday;
> > >  	nd->time.mon = mon;
> > >  	nd->time.year = 2000 + year;
> > > +
> > > +	ofono_netreg_time_notify(netreg, &nd->time);
> > >  }
> > >  
> > >  static void ifx_ctzdst_notify(GAtResult *result, gpointer user_data)
> > 
> > actually this time notification is a bad idea since you will notify
> > twice now. With the firmware that I tested this with, it is guaranteed
> > to always get CTZV and CTZDST and in that order.
> 
> Of course there will be two notifications since there are two distinct
> event CTZV and CTDST. My ifx modem documentation doesn't say that these
> are always bound together so we should not make any implicit assumption.
> Anyway I believe that it is the next level i.e. time plug-in whose job
> it is to check whether it has all the bits and pieces needed to inform
> the Timed or some other client.

that is a clear no here. We are not making every single nettime plugin
responsible for handling this. This needs to be handled either in the
core or in the drivers.

If you are afraid of the IFX modem not sending CTZV + CTZDST all the
time, then you need to handle this with a timeout to trigger notifying
nettime core.

However, I have not seen that yet on any network I have tested this on.
Also asking IFX to confirm the current assumption would just work.

Regards

Marcel