On Mon, 14 Jan 2019 11:51:29 +0100
Johan Hovold [off-list ref] wrote:

here is a second part of a reply.

[...]

quoted

quoted

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.

quoted

quoted

     - 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?

quoted

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.

quoted

quoted

   - 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