Re: [PATCH v2 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Andreas Kemnade <andreas@kemnade.info>]

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

On Mon, 14 Jan 2019 11:51:29 +0100
Johan Hovold <johan@kernel.org> wrote:

here is a second part of a reply.

[...]
> > > In pseudo code we have:
> > > 
> > >   activate:
> > >    - toggle on-off
> > >    - wait(data-received, ACTIVATE_TIMEOUT + REPORT_CYCLE)
> > >      - reception: success   
> > 
> > Note: we can also get the goodbye/shutdown message from the chip here
> > so there are chances of a false success, but since we initially power down,
> > we will rule out wrong state here.  
> 
> Good point. Unless we know the current state, we'd need to sleep for
> HIBERNATE_TIMEOUT before waiting for data reception.
> 

And probably this also magically (together with my
runtime_get/put()-pair) in _probe()) worked around the
problems fixed by the.
gnss: sirf: fix activation retry handling

I was not aware of this problem while writing that code. Just
discovered this fact after the submission.
The current state feels a bit wonky. So I would prefer to bring it
into a stable thing with clear limitations.

> > >      - timeout: failure
> > > 
> > >   hibernate:
> > >    - toggle on-off
> > >    - sleep(HIBERNATE_TIMEOUT)  
> > we could also additionally check here for 
> >    if (last_bytes_received == GOODBYE_MSG)  
> 
> Caching and parsing the stream for this could get messy. And is the
> expected message clearly defined somewhere, or would it be device (and
> firmware) dependent?
> 
> > or alternatively check for
> >    if (!BOOTUP_MSG_RECEIVED)
> >      - return success  
> 
> This seems to suggest the only thing you worry about is the drivers idea
> of the current state being out of sync (which could be addressed
> differently and only once at probe) and not hibernation failing for some
> other reason. And you'd still need to wait for ACTIVATION_TIMEOUT (as
> well as allow the chip time to actually suspend).
> 

States being out of sync is one problem. Hibernation/activation can also
fail. I would try to catch that also. The question was just how to get
the best for the long report cycle time problem and where we are now.

> > >    - wait(data-received, REPORT_CYCLE)
> > >      - reception: failure
> > >      - timeout: success
> > > 
> > > A problem with your implementation, is that you assume a report cycle
> > > of one second, but this need to be the case. Judging from a quick look
> > > at the sirf protocols (sirf nmea and sirf binary) report cycles can be
> > > configured to be as long as 30s (sirf binary) or even 255s (sirf nmea).
> > > [ To make things worse these numbers can be even higher in some
> > > low-power modes it seems. ]
> > >   
> > As far as I see we will only might have a problem if 
> >   - those settings are nonvolatile (or we come in with those
> >     settings on another way)
> >   - or a state change lateron fails which we cannot properly detect  
> 
> So again, you only worry about getting the initial state right?
> 
The point is that we have at least some chances if we get the initial
state right.

> Otherwise, lowering the message rate would at runtime would affect all
> state changes (as currently implemented), regardless of whether these
> changes are stored in NVRAM or not.
> 
Well, with the last patchset and short report cycle we can detect state
changes to off state reliably but state changes to on state
only unreliably (which was, as said, not the intention). If the GPS chip
behaves well enough, we will not see trouble.

Now with long report cycles: Off state detection will always return
success. On state detection (in its current wonky form) will see the
state change messages and will also always return success. If initial
state is correct, this works at least in a wonky way.

I do not like these wonky things too much. So I would rather see
well-defined behavior with well-known limitations.

State change failures are probably not only a theoretical thing,
so it is a good idea to track that.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]