Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't reset GPIO4 on the MagTag (used for voltage monitoring) #6246

Merged
merged 2 commits into from
Apr 6, 2022

Conversation

DavePutz
Copy link
Collaborator

@DavePutz DavePutz commented Apr 5, 2022

Fixes #6148. Since the re-doing of Espressif GPIO resets done as part of PR #5892 the voltage monitoring done on the MagTag by pin GPIO4 has been always returning high values. This fix avoids resetting that pin on the MagTag and allows correct readings of the voltage.

dhalbert
dhalbert previously approved these changes Apr 5, 2022
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks!

@jepler
Copy link
Member

jepler commented Apr 5, 2022

I'm happy if this fixes a problem .. but .. it would also be interesting to know what's different about ADC on a pin that has been 'reset', because if we understood that maybe we'd get better ADC behavior on all pins.

@dhalbert
Copy link
Collaborator

dhalbert commented Apr 5, 2022

I think it is that the resets now set the pin to be an output with a pull-up, instead of an input. @tannewt did that in Perhaps the change should instead be that we make sure to undo the pull-up when setting up AnalogIn. #6057. @DavePutz, I think maybe that is the better fix.

@dhalbert
Copy link
Collaborator

dhalbert commented Apr 5, 2022

I am working on a different fix with the same effect.

@dhalbert
Copy link
Collaborator

dhalbert commented Apr 6, 2022

I think there are two problems here:

  1. The default pull-up on the pin pulls the voltage to the wrong value.
  2. The voltage divider circuitry holds the voltage at some value for a very long time, and it can be the wrong value. It's two 1Mohm resistors, with a capacitor across one. If the voltage divider is pulled high, it will take a while for the cap to drain through the resistor. So once the pin is set up for ADC, it takes a while for it to get to the right value.

Here is a test program running on CircuitPython 7.2.3, with no fixes. Notice the voltage is wrong to begin with, moves down, and then stabilizes. But it takes almost a second for that.

import analogio, board, time

elapsed_msecs = 0
a = analogio.AnalogIn(board.BATTERY)
while True:
    print(elapsed_msecs / 1000, a.value / 65535 * 3.3 * 2)
    time.sleep(0.1)
    elapsed_msecs += 100
0.0 5.33599
0.1 4.59799
0.2 4.33998
0.3 4.29798
0.4 4.28791
0.5 4.28791
0.6 4.28599
0.7 4.28197
0.8 4.28398
0.9 4.28599
1.0 4.28599
...

So it is not stuck, but it is wrong for a while. Removing the reset works because it takes a while for CircuitPython to start up from a hard reset (when the pin is initially pulled up to high by the ESP-IDF reset (see gpio_pin_reset()), and by that time the voltage reading is correct.

I tried disabling the pullup in the AnalogIn constructor, but there's not enough time between that happening and doing an ADC read for the pin to stabilize. I also tried pulling the pin down for 10 msecs or so, but that didn't really help. The time constant is really long, as you can see.

So the fix in this PR works because the pin never gets the pull-up set on it. It is a bit accidental. If the ESP-IDF startup reset the pin using its own reset routine (which sets the pullup), we'd have the same problem.

I'm not sure whether to keep this or to simply document that you should wait at least a second after the program starts to read board.BATTERY.

@DavePutz, did you already come to this conclusion? @jepler what do you think?

@DavePutz
Copy link
Collaborator Author

DavePutz commented Apr 6, 2022

@danh, I was wondering if pins other than the GPIO4 used for the voltage monitor would have similar issues when used for ADC readings. If so, then disabling the pullup in the AnalogIn constructor might be the only solution. Also, would boards other than the Espressif boards have the same problem?

@dhalbert
Copy link
Collaborator

dhalbert commented Apr 6, 2022

[btw @ danh on GitHub is someone else, not me; I am @dhalbert]

I was wondering if pins other than the GPIO4 used for the voltage monitor would have similar issues when used for ADC readings. If so, then disabling the pullup in the AnalogIn constructor might be the only solution.

I tested other AnalogIn pins, just floating, and I don't see this problem. Even without disabling the pull-up, their values are near zero. I think maybe the cap on the voltage divider is a big culprit here. I was worrying that AnalogIn was really broken completely now, but that is not the case.

Also, would boards other than the Espressif boards have the same problem?

Espressif is unique in saying that enabling the pull-up causes lower power consumption. On all the other boards, we reset pins to floating with no pulls in either direction.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked with @jepler about this, and we decided to disable the pull-up( I added 99dc402), and we'll also keep your no-reset change, so that one need not wait to read the battery voltage. We can change this in the future, but for now, this is the easiest for the end user. Thanks for your work on this.

@dhalbert dhalbert merged commit dbb6f5f into adafruit:main Apr 6, 2022
@DavePutz DavePutz deleted the issue_6148 branch June 14, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MagTag battery voltage incorrect in 7.2.0
3 participants