meshtastic / firmware

Meshtastic device firmware
https://meshtastic.org
GNU General Public License v3.0
3.31k stars 800 forks source link

GPS Power State tidy-up #4161

Closed todd-herbert closed 2 months ago

todd-herbert commented 2 months ago

In https://github.com/meshtastic/firmware/pull/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:

Update: See https://github.com/meshtastic/firmware/pull/4161#issuecomment-2202079700 for latest summary

GPSFan commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

todd-herbert commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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

GPSFan commented 2 months ago

Each receiver generation is a bit different, since going into deep sleep will shutdown all functions, another 1/2 second shouldn't matter. We need to look at how some other receivers do power management, the UC6580 has no software command to put it to sleep, it solely relies on switching VCC and V_IO to control the power state. On v1.0 of the Heltec wireless tracker they could do this, the "improved" V1.1 removed this capability. ;>(

todd-herbert commented 2 months ago

the "improved" V1.1 removed this capability. ;>(

I've definitely got a suspicion that's what's responsible for #4154 NomDeGary from the Discord server sent me a a write-up of a new behavior for dynamically deciding between hard and soft sleep, instead of just using the 15 minute threshold we currently do. I'll take a good look at it in the next day or two; it might help reign-in UC6580 a bit.

Edit: this switching would be made possible with a proposed SharedGPIO class, which would allow the various peripherals relying on that VExt supply (GPS + Display?) to release their claim to it when unused.

GPSFan commented 2 months ago

It would if you didn't want to use thee UC6580's mode that you just switch off V_IO and not VCC. Since Heltec designed that feature out of the new rev, it's academic. And until someone else fields a device with a UC6580 we won't have to worry. Edit: I think that you can remove the WIP and get this code merged. (If that pesky trunk likes it ;>))

todd-herbert commented 2 months ago

Still a few things I'd like to chase up first, but I think we're close! I do want to have a look at Gary's suggestion, and see if it ends up saving power by making better decisions about when to power down instead of standby. I also want to double check that fixed position is still working properly.

GPSFan commented 2 months ago

Latest rev looks good, I need to test with a t-beam next.

GPSFan commented 2 months ago

t-beam seems to perform as expected ;>))

todd-herbert commented 2 months ago

I've caught up with NomDeGary on updating that threshold which determines whether to use soft-sleep (standby) or hard-sleep (poweroff). Currently, this threshold is 900 seconds (15 minutes).

It's actually looking like a fair amount of power is wasted by using a single fixed value here for all hardware. Using values we measured for U-blox Neo-6M and M10050, I've thrown together a quick model which estimates how long the "update interval" needs to be to justify using hard-sleep. You can see that the ideal value for this threshold varies significantly, depending on both the "time-to-get-lock" parameter, and the GPS model.

https://www.desmos.com/calculator/weazisxyml

To calculate this dynamically, the code would need need to know typical values for "current when searching", "standby current", and "warm lock time", for various common GPS models.

I think this all probably goes well beyond the scope of this PR, and might be something to work on separately, in the future. I realize this is also outside your personal use-case, @GPSFan.

It'd be important to make sure that the GPS still makes no attempt to sleep with update intervals <= 10 seconds.


In the short term, it might be worth moving the fixed threshold for hard-sleep forward from 900s (15min.) to 300s (5 min.)?

GPSFan commented 2 months ago

And a lot depends on the antenna's view of the sky and how many constellations are configured/tracked. IMHO no one should be using any Neo-6 or Neo-7 receivers, there are so many newer/better options and folks often pay more for a Neo-6 clone/counterfeit than they should. You are correct, this is beyond the scope of this PR and should be addressed later. While my use case is important to me, I believe that Meshtastic should benefit from GPS as much as possible.

todd-herbert commented 2 months ago

And a lot depends on the antenna's view of the sky and how many constellations are configured/tracked.

Does this have a significant impact on the current draw while searching / "lock-time from standby"? If so, this idea isn't quite so attractive, although maybe still worth considering at some point as an "educated guess" (as compared to the fixed threshold).

GPSFan commented 2 months ago

We must be in vastly different time zones, sorry for the delayed reply. Yes, the acquisition/tracking behavior and consequent power consumption is very compute intensive. Is effected by lots of configuration parameters as well as the signal levels of the sats in view. Meshtastic waits till the NMEA data stream indicates that the GPS has a Lock, then puts the GPS to sleep. Unfortunately, just because the GPS has a Lock doesn't mean that the GPS has finished acquiring all the sats it can. When asleep, the GPS can't update its almanac or ephermeris, so in the default Meshtastic operating mode the GPS will get a low quality fix and not improve it for a long time. DOP is not a quality indicator, it is a measure of how much to degrade the CEP based on the geometry of the sats used in the navigation solution. Bad DOP means bad fix, Good DOP doesn't necessarily mean a good fix. If you are only tracking a few sats your DOP can be poor because the first sats you acquire are the ones with the best signal and they tend to be right overhead, poor geometry --> poor DOP, and when asleep, it doesn't get better for a long time. I think we need to have a longer, more detailed discussion on what Meshtastic wants GPS to do for its various use cases. Trying to satisfy all is a difficult task, to say nothing of supporting many different GNSS reciever types.

todd-herbert commented 2 months ago

I think we need to have a longer, more detailed discussion on what Meshtastic wants GPS to do for its various use cases. Trying to satisfy all is a difficult task, to say nothing of supporting many different GNSS reciever types.

That's probably a very good idea. The technical aspects of GPS are beyond me, so I don't have a whole lot to offer to a big architecture discussion, but the resulting decisions would certainly help guide stuff like this.


In terms of tangible changes for this PR, options are:

Just for consideration, I'm pushing a commit which implements this kind of "dynamic estimate". It uses a simple equation which gives a HARDSLEEP threshold somewhere between the estimated ideals for M10050 and Neo-6M (representing modern and end-of-life tech respectively). Although the model this is fitted against is wildly imprecise, the estimates it gives are hopefully somewhat closer to ideals for power consumption.

If you think it's better to stick with the fixed HARDSLEEP threshold for now, please do let me know if you think that existing 900s value is reasonable.

GPSFan commented 2 months ago

Oh dear... I need to understand what you are really trying to do. Maybe we need to start with a set of definitions, such as HardSleep and HardSleep Threshold. What is it, why does Meshtastic want the GPS to be in that mode, how does Meshtastic put the GPS into that mode, (ie is it a hardware or software function), can the GPS be woken up from that mode? Does the user have any input as to when of if that mode is used? Does that mode save any/some or a lot of power? If a hardware function, what Meshtastic devices are designed to support it? Repeat for others. I assume this is all related to u-blox GPS receivers, but some other receiver types have similar but different power management features, some don't. Excuse my density, I'm a hardware guy, for software problems, I live by 2 things: 1 "Ready, Fire, Aim" 2 "Ugly but effective"

todd-herbert commented 2 months ago

Ah, sorry. I probably have slipped into using a different set of terms without realizing. They're values of the GPSPowerState enum.

GPS_ACTIVE,    // Awake and want a position
GPS_IDLE,      // Awake, but not wanting another position yet
GPS_SOFTSLEEP, // Physically powered on, but soft-sleeping
GPS_HARDSLEEP, // Physically powered off, but scheduled to wake
GPS_OFF        // Powered off indefinitely

SOFTSLEEP, HARDSLEEP, and OFF will look slightly different depending on the hardware configuration. Essentially, SOFTSLEEP is "standby", and HARDSLEEP is "powered off". Maybe a (stripped back) snippet of the code describes these states best:

case GPS_ACTIVE:
case GPS_IDLE:
    // These two states are "awake"
    // Identical physically, but used internally by Meshtastic to keep track of the update schedule
    writePinEN(true);      // Power (EN pin): on
    setPowerPMU(true);     // Power (PMU): on
    writePinStandby(true); // Standby (pin): awake
    setPowerUBLOX(true);   // Standby (UBLOX): awake
    break;

case GPS_SOFTSLEEP:
    // This is a timed sleep!
    writePinEN(true);                // Power (EN pin): on
    setPowerPMU(true);               // Power (PMU): on
    writePinStandby(false);          // Standby (pin): asleep
    setPowerUBLOX(false, sleepTime); // Standby (UBLOX): asleep, timed
    break;

case GPS_HARDSLEEP:
    // This is a timed sleep!
    writePinEN(false);               // Power (EN pin): off
    setPowerPMU(false);              // Power (PMU): off
    writePinStandby(false);          // Standby (pin): asleep
    setPowerUBLOX(false, sleepTime); // Standby (UBLOX): asleep, timed
    break;

case GPS_OFF:
    // This is an indefinite sleep
    writePinEN(false);       // Power (EN pin): off
    setPowerPMU(false);      // Power (PMU): off
    writePinStandby(false);  // Standby (pin): asleep
    setPowerUBLOX(false, 0); // Standby (UBLOX): asleep, indefinitely
    break;

The general thinking (which I inherited from reading the old GPS code) is:

Previously, this "threshold" value was estimated at 900s:

// 15 minutes is probably long enough to make a complete poweroff worth it.

The true "threshold value" which justifies this sort of HARDSLEEP / power-off depends on a few different variables; "how long it takes to get the GPS fix" is a key one of these. Instead of just assuming that "900s is long enough", this proposed equation attempts to estimate "how long is actually long enough" to justify HARDSLEEP. The was no clever logic used in formulating the suggested equation in this commit; I just varied the parameters until it roughly gave values which fit that model linked above.

The model (to which this equation is fitted) shows at approximately what point it makes power-saving sense to use HARDSLEEP instead of SOFTSLEEP, using observations from those two GPS models (M10050 and NEO-6M).

todd-herbert commented 2 months ago

I probably should have answered your questions directly too, sorry about that.

What is it,

You can think of GPS_HARDSLEEP as powered off (or as near as possible, depending on hardware). You can think of GPS_SOFTSLEEP as low-power standby (if at all possible, depending on hardware).

why does Meshtastic want the GPS to be in that mode

To minimize power consumption between GPS updates.

how does Meshtastic put the GPS into that mode, (ie is it a hardware or software function)

A mix of whatever hardware / software options are available: external switching, onboard PMU, GPS standby pins, U-blox PMREQs

can the GPS be woken up from that mode?

Yes

Does the user have any input as to when of if that mode is used?

Does that mode save any/some or a lot of power?

This plot illustrates the average power consumption for U-blox M10050, for different gps_update_interval values. The solid line represents average power consumption when using GPS_HARDSLEEP, the dashed line represents average power consumption when using GPS_SOFTSLEEP. Notice that with long-enough update intervals, it becomes more economical to use HARDSLEEP instead of SOFTSLEEP. Be aware: while this graph is based on measured values, it is still only an approximation.

graph of power consumption for different sleep modes

I assume this is all related to u-blox GPS receivers, but some other receiver types have similar but different power management features, some don't.

GPS_SOFTSLEEP aims to enter a low-power standby using whatever hardware options are available. GPS_HARDSLEEP aims to fully power down the GPS hardware, but this may also not be possible. Hardware which is not capable of any form of low-power sleep should probably use GPS_HARDSLEEP only, even when GPS_SOFTSLEEP is requested by the code. I haven't yet implemented this, but it's on the to-do list before marking this PR as ready for review.

I have only reorganised and reused existing code from the GPS class, which allows for GPS_SOFTSLEEP using either a defined standby pin, or U-blox PMREQ. If anyone does implement some new vendor specific low-power standby code, it should slot into this framework easily.

Excuse my density, I'm a hardware guy, for software problems, I live by 2 things: 1 "Ready, Fire, Aim" 2 "Ugly but effective"

Hey, don't let me fool you, this GPS stuff is all new to me. I'm just doing my best to make sense of it as I go.

GPSFan commented 2 months ago

That's a great set of answers, Thanks! Have you read the 2 Power Management app notes from u-blox? Both are available on the u-blox web site.

GPS.G6-X-10014.pdf For the Neo-6 published 2010-2012 UBX-13005162.pdf For the Neo-7 and M8 published 2014

The latter is outdated and was never updated as newer firmware for the M8 was released. The Receiver protocol descriptions and Hardware Integration manuals also provide a lot of good insight about the inner workings of the modules. Acquisition and Tracking are complex and compute intensive tasks. There are basically 3 states a receiver can start in: Cold Start: The receiver has no Time, Almanac or Epheremis Warm Start: The receiver has Time and a current Almanac but no or outdated Epheremis. Hot Start: The receiver has Time a current Almanac and a valid set of Epheremis of enough sats to get a lock.

This data is stored in the battery backed RAM.

All sats in a constellation broadcast the time and almanac. Each sat broadcasts its own Epheremis.

https://www.spirent.com/blogs/2011-05-12_gps_almanac

https://hitechniques.ie/blog/understanding-gnss-operations-almanac-ephemeris-and-receiver-start-modes/

A cold start with poor signal levels can take a LONG time (if ever) to get a lock, which is why there is a parameter for when to give up looking in the u-blox M8 modules. (UBX-CFG-PM2 maxStartupStateDur... set to disabled in Meshtastic) Once a receiver has acquired and is tracking "enough" sats (4) the receiver can declare that it has a "Lock" But most modern receivers (Neo-6 included) have many more than 4 channels to acquire and track sats. If the receiver is put to sleep as soon as it gets a lock, the quality of the fix can suffer, and it is a waste of the receiver resources (IMHO). On top of that, it can take a lot longer to acquire sats in some constellations ((Galileo) than others, and some dual band receivers must acquire and track a sats L1 signal before they try to acquire the L5 (or L2) signal. Some even must acquire GPS before they look for other constellations. Some of this information is published. some must be inferred by experimentation and observation of receiver behavior.

todd-herbert commented 2 months ago

Have you read the 2 Power Management app notes from u-blox? Both are available on the u-blox web site.

I definitely haven't read through these, but I should definitely make the time and take a close look. It sounds like there's a whole lot more to consider than just power consumption, but whether that's the number one priority for Meshtastic when configured with the longer update intervals, I couldn't say personally!

For what it's worth, I'm not really hoping to reinvent the wheel with this tidy-up PR. For the most part, I've done my best to recreate the existing power-saving behavior. Possibly the only major change considered right now is exactly how long that "hardsleep threshold" is. My honest intention with this particular PR was just to re-organize the current behavior, to make it easier to fix that PMREQ for deep sleep bug, and hopefully to make it easier for someone to maintain in the future.

Is there any "low hanging fruit" that we're missing so far? Any basic tweaks we can make to get the most out of this tidy-up? I'll definitely have a look at those app notes. I'm sure there's a lot of very useful knowledge to pick up from them, although I'm guessing it might all pose "big picture" questions about they way we're using GPS, right?

GPSFan commented 2 months ago

Not for this tidy up, but in the near future I would like to propose a new parameter that gives a time to add after the GPS gets a lock before it is put to sleep, the default for normal Meshtastic use can be 0 but could be set to something like 60 sec to allow the GPS to acquire more sats sooner, that will lead to better fixes. It could even be scaled over time or number of sats tracked.

todd-herbert commented 2 months ago

but in the near future I would like to propose a new parameter that gives a time to add after the GPS gets a lock before it is put to sleep

I do like the sound of that! If everyone else is in favor of adding the setting to the protobufs, it shouldn't be hard to implement either.

GPSFan commented 2 months ago

I'd like to test the concept first, I'm sure I can cobble together something to simulate it.

todd-herbert commented 2 months ago

(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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 https://github.com/meshtastic/firmware/pull/4161/commits/c50b43292c72617500067d172972892272e3510c and https://github.com/meshtastic/firmware/pull/4161/commits/5e69c7e069028da9680f9799c4249b0862702d00.

GPSFan commented 2 months ago

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 commented 2 months ago

Are you switching power with PIN_GPS_EN? It's possible that https://github.com/meshtastic/firmware/pull/4161/commits/5e69c7e069028da9680f9799c4249b0862702d00 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 commented 2 months ago

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 commented 2 months ago

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

GPSFan commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Summary

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

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 commented 2 months ago

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 commented 2 months ago

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

geeksville commented 2 months ago

hey @todd-herbert The shared GPIO class is in. If you want to use it for your GPS fixing please go for it. I'm going to be doing power measurement infrastructure for another weekish (possibly two - IDK). At that point, if you haven't had a chance to look into it I'll ping you and possibly we can tag team it or I can do it.

Using the actual classes should be pretty straightforward. See the comments in the header file and the example usage I did for setLed(). In short: you'll want to have both the GPS and OLED classes use a VirtGpio 'pin' instance for their particular 'gpio'. And then use GpioBinaryTransform (in operator OR mode) to combine both of those 'pins' to power up the one real hw GPIO that controls that rail.

todd-herbert commented 2 months ago

At that point, if you haven't had a chance to look into it I'll ping you and possibly we can tag team it or I can do it.

It'll take some dedicated code-staring on my end to figure it out, but I'm definitely keen to get my head around it. No worries about coordinating it though; the hope was definitely that this PR would make it easier for anyone to drop in changes (like #4211) at any point in the future 👍

todd-herbert commented 2 months ago

@geeksville Had a merge conflict with PowerMon::setState and PowerMon::clearState, just want to check whether their new location (in GPS::setPowerState) is acceptable?

todd-herbert commented 2 months ago

@jp-bennett @GPSFan Have you guys hit any bugs, or got any last minute adjustments? I feel like you guys are key stakeholders here.