GPS Power State tidy-up

[todd-herbert]

In #4070 I said I'd follow up and have a go at tidying the GPS class, with the intention of then hunting down a bug affecting U-blox standby. I'm not at all confident working with GPS hardware, so this submission is a rough first draft. Seeking feedback / contribution / testing!

Initial intentions:

• Encapsulate code which schedules GPS updates
• Tighter interaction with the GPSPowerState enum
• Remove an unused GPS Sleep Observable
• Control sleep with high-level up(), down() methods

Update:
See #4161 (comment) for latest summary

[GPSFan]

Just built an ran your code, there is a corner case that you need to address, that is when the GPS is asleep and you use the user button to put the esp32 into deep sleep, the GPS wakes up and stay's awake while the esp32 is in deep sleep.
I have been looking at this issue and with the current master, added a couple of lines into prepareDeepSleep. first send an 0xff to wake the GPS up if it was asleep, then wait 500ms for it to wake up, then send a PMREQ with 0 duration, to put the GPS into sleep forever, then when the ESP32 shuts down the GPS is already asleep. That was just a quick & dirty test to see if the concept was ok.

[todd-herbert]

Just built an ran your code, there is a corner case that you need to address, that is when the GPS is asleep and you use the user button to put the esp32 into deep sleep, the GPS wakes up and stay's awake while the esp32 is in deep sleep. I have been looking at this issue and with the current master, added a couple of lines into prepareDeepSleep. first send an 0xff to wake the GPS up if it was asleep, then wait 500ms for it to wake up, then send a PMREQ with 0 duration, to put the GPS into sleep forever, then when the ESP32 shuts down the GPS is already asleep. That was just a quick & dirty test to see if the concept was ok.

Perfect! I imagine there's a whole bunch of little things like this, so the more we can spot, the better! Did you notice if we still need to try hold the TX line high or anything too (something we discussed in the old PR, right?), or is this fix enough to keep the GPS asleep? Getting that PMREQ for deep-sleep was one of the key things we needed to sort with all this, so I'm glad it might be this easy to solve.

I think maybe we can tie this into a nice high level off() method, to match up() and down(). Might be some value in this "wake to sleep" behavior when disabling with triple press too?

[GPSFan]

Still testing, I noticed a few oddities with your code when the Update Interval was <= 10 sec. and I have not tested it with anything other than an esp32. I believe that the Tx line to the GPS needs to be held high. My mod did NOT do that, but I think it needs to be, more testing will tell.

[todd-herbert]

Still testing, I noticed a few oddities with your code when the Update Interval was <= 10 sec. and I have not tested it with anything other than an esp32. I believe that the Tx line to the GPS needs to be held high. My mod did NOT do that, but I think it needs to be, more testing will tell.

I'll definitely check those short update intervals tomorrow too. More than anything, I just wanted to make this WIP visible, even though it's still early days (or might get shelved, if there's reason to go in a different direction)

[GPSFan]

Yeah, this whole GPS sleep/awake/idle/etc is very complicated, esp WRT the quality of the fix the GPS has. My use case is in may ways too simple in that the GPS is on all the time and gets a really good fix, Other use cases with power consumption as their primary goal suffer poor GPS fix quality and long acquisition times.

[GPSFan]

I have to run off to DayJob... BBL (I hope)

[todd-herbert]

I noticed a few oddities with your code when the Update Interval was <= 10 sec.

Keen to try fix it, but I can't spot anything super obvious just from the log of my T-Beam V1.2, with a 10 second update interval. What exactly are you seeing?

Edit: actually, there might be some stuff happening when moving between "idle" and "active" which is at best unnecessary. I'll remove it and we'll see if that makes anything better (or worse)

[GPSFan]

Your latest gps-refactor branch still has the problem if the GPS is asleep when you ask for a DeePSleep, that the GPS doesn't wake up to get the PMREQ with duration 0. The earlier problem I was seeing was that you were not sending a PMREQ with duration 0 at all when DeepSleep was requested.

[todd-herbert]

The earlier problem I was seeing was that you were not sending a PMREQ with duration 0 at all when DeepSleep was requested.

Oh yes, no, sorry, I should have been clear: I haven't changed anything yet. I just merged upstream ready to work on it, then paused because I wasn't sure exactly what needed doing. I'll make an attempt at both issues and then let you know here.

[todd-herbert]

@GPSFan Hoping I've got both of those issues sorted now. They seem to be fixed now with my test node, but let me know if I've missed them again.

[GPSFan]

@todd-herbert That seems to work, except you are not waiting long enough for the GPS to wake up after sending the 0xff. I increased the delay from 100 to 500 and that seems to give my M8N enough time to wake up. This is tested on an esp32, I'll try it on my RAK later today.

[todd-herbert]

Hmm, sounds like it does need the full 500ms then, we'll definitely use that. 100ms was long enough on the ESP32 + Neo-6M(?) I was testing with, so I had hoped that maybe you just threw out that 500ms as a ballpark figure. But nope apparently not! No big deal, I just get a bit squeamish adding longer delays. Only place this one will ever come up outside of shutdown is when the user-button is triple pressed to disable GPS, but that happens so rarely there's probably no reason to worry about it impacting LoRa RX

[todd-herbert]

(No significant behavior changes, just tying up loose ends)
I think that's it for this PR, apart from catching any bugs that might pop up.

[GPSFan]

Haven't tested your new code yet, but I agree that we should wrap up this issue/PR.
I see from the comments in your commits that you had issues with determining what GPS was what, I would have much rather the GPS type(s) had been defined in the variant.h file, much like the LORS radios are. Then there could have been a ubx.h, ubx.c, Qucetel.h, Qucetel.c, Allystar.h, Allystar.c ... file for each of the different types with all the unique stuff abstracted. Maybe someday as that would be a huge change.

[todd-herbert]

I would have much rather the GPS type(s) had been defined in the variant.h file, much like the LORS radios are. Then there could have been a ubx.h, ubx.c, Qucetel.h, Qucetel.c, Allystar.h, Allystar.c ... file for each of the different types with all the unique stuff abstracted. Maybe someday as that would be a huge change.

I think that would fit quite nicely with a Firmware Creation Tool, if / when that gets off the ground.

I see from the comments in your commits that you had issues with determining what GPS was what

Ah there was no big issue there, just that the accidentally inverted standby pin logic was totally disrupting the affected hardware, so much that they failed the probe. As soon as HIGH / LOW was swapped, it all came right.

but I agree that we should wrap up this issue/PR.

Definitely no more intended work on this, but I'd definitely like to make sure we merge this early in a release cycle, just because there's such an opportunity for little corner-case bugs to pop up. I'm not sure if 2.3.15 alpha is creeping up already, or if we're still a good distance off.

One thing I'd like to test myself for a day or two is T-Echo battery life. I don't think the device has any hardware switching for the GPS, other that the "standby pin", and I'm pretty sure improper GPS config is know to cause issues, so I'd like to give it a bit more of a run first. Update: tested, fixed.

Other than that, if you're happy that everything is still performing correctly after this tidy up, let me know and we'll mark this ready for review.

[GPSFan]

Just got to test the latest, something isn't right (or the same as it used to be) with Update intervals <= 10 sec's.
Did you change any of the settings for the time pulse output?
In the recent past I would get the once per second time pulse output when the Update Interval was <= 10 sec. Now I don't. Since my Chatter 2 platform has no VCC control for the GPS it should be on in continuous tracking mode. I'll have to poke deeper into what's going on.
Starting to bisect the issue, but each bisect results a complete re-compile.. it will be a while...

[todd-herbert]

In the recent past I would get the once per second time pulse output

Sorry about that! I definitely didn't expect any of these latest commits to impact anything like that. Glad you caught something weird before it was too late!

My ignorance is showing again here: exactly what were you seeing before, and what aren't you seeing now? I do see PIN_GPS_PPS defined, and set to INPUT, but I can't see any place in the codebase where it is actually used, so I'm a little bit lost!

I'm suspicious of commits c50b432 and 5e69c7e.

[GPSFan]

bisect finished, there seems to be a state that the GPS (M8N, maybe it;s only mine) can get in that it is not producing time pulses and not behaving at all like it normally does. This may have been a red herring. as I'm back to your current gpe-refactor head and things seem normal.
I;ll keep testing to see if I can figure out what's going on.

[todd-herbert]

Are you switching power with PIN_GPS_EN? It's possible that 5e69c7e gave some protection against weird states by enforcing a power cycle during init. This code block was only removed because it seemed to be redundant now, but there'd be no issues with leaving it in place, if it does help?

[GPSFan]

No, This config doesn't switch power at all, Vcc is there at all times. I haven't been able to reproduce the issue. But it was wierd as I was getting NMEA data out as if it were locked on and working but no time pulse. it said it had 8 sats, but that didn't change over time. once I went back to an earlier version and disconnected the antenna for a while it came back to "normal" operation, and 12 sats. Then when I went to a later version it was ok, then I went to the latest and it was still ok, and varying from 9 to 12 sats.
It's now at 9 sats... good external antenna with a good view of the sky.

[todd-herbert]

Just for my understanding, are you talking about the time pulse signal from one of the hardware pins?

[GPSFan]

the GPS breakout board I have for the M8N has an LED on the time pulse output when it flashes I know it is locked.
I'll try it with a t-beam. It also has an LED on the time pulse output.

[GPSFan]

t-beam seem to be behaving normally, I'm going to chalk that up to random weirdness...
Quick question, did you mean to do anything about the Heltec Wireless tracker ADC ctrl pin driving the base of a BJT without a series resistor? The consensus was that setting that pin to pullup for a 1 and pulldown for a 0 should eliminate the excessive current drawn by directly driving the base of a BJT..

[todd-herbert]

t-beam seem to be behaving normally, I'm going to chalk that up to random weirdness... Quick question, did you mean to do anything about the Heltec Wireless tracker ADC ctrl pin driving the base of a BJT without a series resistor? The consensus was that setting that pin to pullup for a 1 and pulldown for a 0 should eliminate the excessive current drawn by directly driving the base of a BJT..

I do think it seems like a good idea, but it actually hadn't crossed my mind to change it here. I don't actually have that device to test, so I was hoping someone else would pick up the issue and try it out.

I think @geeksville has plans to implement a SharedGPIO class which will specifically impact that VExt pin for Wireless Tracker V1.1, so might actually be convenient to roll it in when implementing that.

[todd-herbert]

Summary

Intention: to simplify GPS power management, to aid future maintenance (hopefully)

• High level GPS::up() and GPS::down() methods
• GPS power-saving logic reduced to a set of discrete states (enum GPS_PowerState, GPS::setPowerState)
• GPS "update scheduling" code encapsulated as own class
• Consider "predicted time to get lock", when deciding which sleep mode to use
• Bugfix: U-Blox low power during MCU deep-sleep

Even if everything looks good, might be wise to hold off merging this one until right at the start of a release cycle, in case any sneaky bugs slip through.

[GPSFan]

Let's defer to @geeksville for the Tracker & SharedIO stuff.
And let's keep an eye out for more random weirdness.
I have no problem holding off till the beginning of a new release cycle.

[jp-bennett]

At first glance, looks great. I'll try to do some testing as well.