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

fix(swan_r5): 3v3 was enabled but then immediately reset. #6458

Merged
merged 3 commits into from Jun 21, 2022

Conversation

m-mcgowan
Copy link

@m-mcgowan m-mcgowan commented Jun 3, 2022

Problem

The 3v3 output was active only for a brief period.

Solution

Move initialize_discharge_pin() to reset_board which happens after reset_all_pins so that the 3v3 output remains active.

Test Code

import board
import time
from digitalio import DigitalInOut, Direction, Pull, DriveMode

discharge = DigitalInOut(board.DISCHARGE_3V3)
discharge.switch_to_input()

charge = DigitalInOut(board.ENABLE_3V3)
charge.switch_to_output(True, DriveMode.PUSH_PULL)


def disable3v3(drain_duration=0.2):
    discharge.switch_to_output(False, DriveMode.OPEN_DRAIN)
    charge.value = False
    time.sleep(drain_duration)
    discharge.switch_to_input()

def enable3v3():
    discharge.switch_to_input()
    charge.value = True

time.sleep(1)

disable3v3()

while True:
    time.sleep(1)
    enable3v3()
    time.sleep(1)
    disable3v3()

…alization to `reset_board` which happens after `reset_all_pins`.
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Do you want CircuitPython to reset the pin? You can "never_reset" it so that CircuitPython leaves it alone.

@m-mcgowan
Copy link
Author

Do you want CircuitPython to reset the pin? You can "never_reset" it so that CircuitPython leaves it alone.

Thanks for the suggestion! I tried playing with the never_reset functionality, however, the function also claims the pin so it cannot be used in scripts (e.g. with DigitalInOut as in the test script above.). I was thinking of adding a function like never_reset_no_claim but wanted to make this PR as simple as possible.

@m-mcgowan
Copy link
Author

Could you cancel the running checks and restart please? I pushed the pre-commit suggested fixes. I really need to add the precommit hook to my local repo. Doing that now...

@m-mcgowan
Copy link
Author

Just thinking aloud here, the functionality that would be ideal in this case is along these lines

  • a board can set up pins in board_init() and mark them as never_reset but also unclaimed - we can call this state board preset, which is a new combination since never resettable pins are currently always claimed.

  • DigitalInOut recognizes when a pin has been preset by the board (non resettable and not claimed). It claims the pin as usual but does not attempt to change the state of the pin.

  • The script can use switch_to_output etc when state changes are required.

  • deinit only unclaims the pin and makes no state changes when the pin is marked as non resettable.

In essence, that would allow the board to setup some pins while also allowing scriptable changes.

@RetiredWizard
Copy link

RetiredWizard commented Jun 4, 2022

I've done some testing with mat's script and his PR changes and for what it's worth this is what I found:

  • Using the current latest CP build the swan 3v3 output pin discharges slowly from 3.3V after the board powers up.
  • With the changes from this PR when CP loads the 3v3 output goes to 3.3V and stays there
  • When I run the script as is on either the current latest CP or CP with this PR the 3v3 pin swings from 3.3 to 1.3 volts
  • Running the script as is appears to hang the USB REPL interface, the voltages continue to swing
  • I enabled DEBUG_UART and confirmed the board and REPL were still operating normally, just the USB terminal was hung
  • Commenting out the "charge.value" assignments prevented the USB terminal from hanging
  • Placing print statements around the assignments shows that the USB terminal freezes the first time charge.value is set to True in the enable3v3 function

@m-mcgowan
Copy link
Author

m-mcgowan commented Jun 6, 2022

I've done some testing with mat's script and his PR changes and for what it's worth this is what I found:

  • Using the current latest CP build the swan 3v3 output pin discharges slowly from 3.3V after the board powers up.
  • With the changes from this PR when CP loads the 3v3 output goes to 3.3V and stays there
  • When I run the script as is on either the current latest CP or CP with this PR the 3v3 pin swings from 3.3 to 1.3 volts
  • Running the script as is appears to hang the USB REPL interface, the voltages continue to swing
  • I enabled DEBUG_UART and confirmed the board and REPL were still operating normally, just the USB terminal was hung
  • Commenting out the "charge.value" assignments prevented the USB terminal from hanging
  • Placing print statements around the assignments shows that the USB terminal freezes the first time charge.value is set to True in the enable3v3 function

@RetiredWizard Thanks for taking the time to test these changes!

If there's no load on the 3V3 pin then the discharge will take some time. I had a LED hooked up and saw it blink without a slow discharge, although in testing I needed to allow several tenths of a second for full discharge. Your setup may require a larger drain_duration value passed to the disable3v3() call. Not setting the DISCHARGE_3V3 to output (to discharge of the capacitor to ground) produced a much slower discharge.

I didn't see the USB terminal hang completely - Ctrl-C works, as does making a change to code.py to cause a reload. Ctrl-D (reload) did not work with the script, although I've seen this with other scripts too, not just this one. It would be a great help if you could confirm this is the same for you both before and after this PR? If that's the case then it's not a regression in this PR I'll open a new issue for that so it can be investigated independently of this change.

Thanks again for testing!

@tannewt
Copy link
Member

tannewt commented Jun 6, 2022

Do you want CircuitPython to reset the pin? You can "never_reset" it so that CircuitPython leaves it alone.

Thanks for the suggestion! I tried playing with the never_reset functionality, however, the function also claims the pin so it cannot be used in scripts (e.g. with DigitalInOut as in the test script above.). I was thinking of adding a function like never_reset_no_claim but wanted to make this PR as simple as possible.

Ah, I think the claiming may be an STM port quirk. I don't think that's true on other ports.

Just thinking aloud here, the functionality that would be ideal in this case is along these lines

* a board can set up pins in `board_init()` and mark them as `never_reset` but also `unclaimed` - we can call this state `board preset`, which is a new combination since never resettable pins are currently always claimed.

* `DigitalInOut` recognizes when a pin has been preset by the board (non resettable and not claimed).  It claims the pin as usual but _does not attempt to change the state of the pin._

* The script can use `switch_to_output` etc when state changes are required.

* `deinit` only unclaims the pin and makes no state changes when the pin is marked as non resettable.

In essence, that would allow the board to setup some pins while also allowing scriptable changes.

I think the best option is that you have a static DigitalInOut object that you add to the board module via pins.c. It'd be more like board.DISPLAY. You init it in board_init and have it marked as never_reset. Then, user code references that object instead of creating a DigitalInOut itself when the in-use check occurs.

Some, ESP ports do have board specific pin reset state but I don't think that's what you want.

@RetiredWizard
Copy link

@m-mcgowan I'll do some more testing later tonight, but on a hunch I connected up to the microcontroller using a "screen" terminal from Linux rather than the puTTY terminal from Windows 8 and I don't seem to have the terminal window hang. I have to do some more careful testing but when I let the script run through Windows/puTTY the voltages continued to alternate after the terminal froze for a minute or two and then everything seemed to halt. Perhaps filling up the output buffer to the terminal caused that.

In any case, from the screen/Linux connection the script seems to run indefinitely without issues and can be exited with a ctrl-C

@RetiredWizard
Copy link

RetiredWizard commented Jun 7, 2022

@m-mcgowan I've run the script on both the clean 8.0.0 (from a couple days ago) and with these PR changes edited from both puTTY on the PC and a Screen terminal from on Linux. Here are the results:

CLEAN BUILD FROM REPOSITORY:

On PC:

(bootloader issue?)

  • First copy of UF2 file doesn't cause CIRCUITPY drive to show up and no COM port created after reset
  • Second copy CIRCUITPY drive shows up but can't connect to COM port
  • reset the board again and COM port and CIRCUITPY drive work properly

on reset 3v3 pin goes to 3Volts but drops slowly unless an LED attached in which case drops rapidly to about 1.5V and
then continues dropping slowly

Running Script:

  • No print statements
  • Voltage swings for minute or two then stops
  • Ctrl-C/Ctrl-D does nothing
  • Closing terminal window does nothing

reset board and reran script

  • no print statements
  • Voltage swings from 3.3V to 1V for minute or two then stops when it stops Voltage is at 1V
  • Closed terminal window without hitting any other keys or control keys and voltage swings resumed between 3.3 and 1V
  • script runs indefinitely
  • Attaching new terminal window to COM port stops the script with the voltage steady at either 3.3V or 1V depending on timing
  • Closing terminal window causes script to resume
  • Attached new terminal window to COM port and pressed Ctrl-C, voltage switched to from 1V to 3.3V and script stopped running
  • but terminal was unresponsive/hung closing and reattaching terminal screen didn't help

On Linux:

  • First copy of UF2 file doesn't result in mountable drive
  • Double clicking reset for bootloader works, the bootloader drive appears with identifier incremented (sda -> sdb)
  • After second copy board resets normally new drive sdc1 is mountable and COM device works

on reset 3v3 pin goes to 3Volts but drops slowly unless an LED attached in which case drops rapidly to about 1.5V and
then continues dropping slowly

Running Script:

  • First couple print statements appear to have been lost
  • occasionally a newline character is dropped
  • string of unprintable characters is displayed occasionally

image

  • Script runs (with printing/display issues) indefinitely
  • Ctrl-C breaks out of script
  • Breaking out of script with voltage at 3.3 results in steady voltage w/LED attached

BUILD WITH PULL REQUEST INCLUDED:

On PC:

Same bootloader issues requiring copying UF2 file twice.

on reset 3V3 pin goes high and stays there with LED connected

Running Script:

3 to 5 print statements print but other than that script behaves exactly as it did in the clean build version

On Linux:

Same bootloader issue requiring copying the UF2 file twice

On reset 3v3 output goes to 3.3V and stays there with an LED attached.

Running Script:

Initial print statements worked this time but other than that script behaves exactly as it did in the clean build version

@m-mcgowan
Copy link
Author

I think the best option is that you have a static DigitalInOut object that you add to the board module via pins.c. It'd be more like board.DISPLAY. You init it in board_init and have it marked as never_reset. Then, user code references that object instead of creating a DigitalInOut itself when the in-use check occurs.

Thanks for the detailed reply - all makes sense.

I wanted to create a DigitalInOut instance on the board initially but didn't (probably still don't!) know enough about creating python objects in the runtime in C. I recently saw how the rtc.RTC() returned object is created, and thanks for the pointer to board.DISPLAY, these examples of prior art will help me do this in a way that is consistent with other ports.

@tannewt
Copy link
Member

tannewt commented Jun 8, 2022

So what you'd do is have a globally allocated digitalio_digitalinout_obj_t with the base type set to digitalio_digitalinout_type. (The dynamic version of this is in shared-bindings.) Python objects are struct instances where the first it (base.type) is a pointer to the type object/struct. So something like:

// Allocate the global instance struct. Usually this happens on micropython heap but nothing cares
// if it actually is on it. (Though you do need be careful that it doesn't use the heap.)
digitalio_digitalinout_obj_t power_pin;

// when you init
power_pin.base.type = &digitalio_digitalinout_t;

// then use the common_hal with it
common_hal_digitalio_digitalinout_construct(&power_pin, &pin_PE06);
common_hal_digitalio_digitalinout_never_reset(&power_pin);

// in pins.c
{ MP_ROM_QSTR(MP_QSTR_DISCHARGE_3V3), &power_pin },

@m-mcgowan
Copy link
Author

@RetiredWizard from what I understand from your testing, there are issues relating to the bootloader and USB handling which I don't believe are caused by this PR (from A/B testing with and without this PR.) To narrow the focus of this PR to just the 3V3 output, I'll add new issues to track the bootloader and USB behavior.

@RetiredWizard
Copy link

@m-mcgowan I can't seem to recreate the bootloader issue I was having when I did the earlier testing. I would hold off on the new issue unless you see the same behavior. If it pops up again for me, I'll try and figure out what the trigger is and open an issue on it if I can make it repeatable.

@m-mcgowan
Copy link
Author

Here's the updated test script - the main change is that we don't need to wrap the pins in a DigitalInOut instance. Also the discharge pin is kept as open drain while discharging so that discharging can complete fully.

import board
import time
from digitalio import DriveMode

discharge = board.DISCHARGE_3V3
charge = board.ENABLE_3V3


def disable3v3(drain_duration=0.2):
    discharge.switch_to_output(False, DriveMode.OPEN_DRAIN)
    charge.value = False
    time.sleep(drain_duration)

def enable3v3():
    discharge.switch_to_input()
    charge.value = True

time.sleep(1)

disable3v3()

while True:
    time.sleep(1)
    enable3v3()
    time.sleep(1)
    disable3v3()

@RetiredWizard
Copy link

I built the new PR files into the CP 8.0.0-alpha.1 tag and ran Mat's new script and the voltage now swings between 3.3V and 0V, with or without an LED attached to the 3v3 output.

@m-mcgowan
Copy link
Author

Thanks for testing again @RetiredWizard - the PR is ready to merge. 🎉

@m-mcgowan m-mcgowan requested a review from tannewt June 21, 2022 16:27
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you!

@tannewt tannewt merged commit 562c73b into adafruit:main Jun 21, 2022
@m-mcgowan m-mcgowan deleted the swan_r5_3v3_enable branch June 21, 2022 21:57
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.

None yet

3 participants