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

[photon2] fix GPIOs sleep configuration. #2666

Merged
merged 3 commits into from Jul 26, 2023
Merged

[photon2] fix GPIOs sleep configuration. #2666

merged 3 commits into from Jul 26, 2023

Conversation

XuGuohui
Copy link
Member

@XuGuohui XuGuohui commented Jul 3, 2023

Problem

  1. All GPIOs cannot disable pull resistors when they are configured as output.
  2. All GPIOs cannot remain HIGH in hibernate mode if they are configured as output HIGH. All GPIOs are actually shutdown in hibernate mode.
  3. SWD DAT that is configured as GPIO cannot remain LOW in hibernate mode if it is configured as output LOW.
  4. SWD DAT that is configured as SWD actives high in hibernate/stop/ulp mode, which results in power leakage [Photon2 only].
  5. SWD CLK that is configured as GPIO makes SWD DAT actives high in hibernate mode, which results in power leakage [Photon2 only].

Solution

  1. Disable pull resistors when a pin is configured as output.
  2. Enable pull-up when a pin is configured as output HIGH and restore default when output LOW.
  3. Resolved by #1.
  4. Disable SWD functionality in sleep modes and configure the SWD DAT pin as input pull-down.
  5. Configure the another SWD pin as GPIO with input no-pull.

Example

#include "application.h"

SYSTEM_MODE(MANUAL);

uint8_t pin1 = D4; // replace with D6
uint8_t pin2 = D5; // replace with D7
bool initValue = HIGH;

void setup() {
    pinMode(pin1, OUTPUT);
    digitalWrite(pin1, initValue);
    pinMode(pin2, OUTPUT);
    digitalWrite(pin2, initValue);

    delay(5s);

    for (uint8_t i = 0; i < 3; i++) {
        digitalWrite(pin1, !initValue);
        digitalWrite(pin2, !initValue);
        delay(500);
        digitalWrite(pin1, initValue);
        digitalWrite(pin2, initValue);
        delay(500);
    }

    System.sleep(SystemSleepConfiguration().mode(SystemSleepMode::HIBERNATE).duration(10s));
}

References

N/A


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@scott-brust
Copy link
Member

scott-brust commented Jul 17, 2023

I used the following test app to set D7 as OUTPUT and wrote it LOW. However, I couldnt get D7 to stay low through Hibernate. It is pulled up when sleeping. See the attached capture as well.

I tried configuring D7 as PIN_MODE_SWD as well, and I could see that it was not pulled down either. It remained high through sleep as well.

I made sure to include the SDK changes and rebuilt + reflashed prebootloader-part, bootloader and system-part1 binaries to make sure the pin configuration was correct. Not sure what I am missing...

Test app (just modified from the RTC PR)

#include "application.h"

SYSTEM_MODE(MANUAL);

Serial1LogHandler l(115200, LOG_LEVEL_ALL);

static time32_t year, month, day, weekday, hour, minute, second;

void updateTime() {
    year = Time.year();
    month = Time.month();
    day = Time.day();
    weekday = Time.weekday();
    hour = Time.hour();
    minute = Time.minute();
    second = Time.second();
}

STARTUP(updateTime());

void setup() {
    Log.info("%d-%d-%d, %d:%d:%d, %d, test: %ld", year, month, day, hour, minute, second, weekday);

    pinMode(D7, OUTPUT);
    digitalWrite(D7, LOW);
    // Or
    // pinMode(D7, PIN_MODE_SWD);

    delay(5s);

    updateTime();
    Log.info("%d-%d-%d, %d:%d:%d, %d, test: %ld", year, month, day, hour, minute, second, weekday);

    System.sleep(SystemSleepConfiguration().mode(SystemSleepMode::HIBERNATE).duration(10s));
}
d7_output_low

@@ -249,8 +249,31 @@ void configureDeepSleepWakeupSource(const hal_sleep_config_t* config) {

// Copy and paste from SOCPS_DeepSleep_RAM()
void enterDeepSleep() {
#if PLATFORM_ID == PLATFORM_P2
// dirty-hack for Photon2 D7: PA27 (SWD-DIO)
uint32_t bitMask = 1 << 27;
Copy link
Member

Choose a reason for hiding this comment

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

Are there other pins at risk of this behavior or is this just because of how the SWD pin is configured in the SDK?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested other normal GPIOs and they behave as expected. But when configuring D6 as GPIO, which is originally SWD-CLK, the D7 is also affected, because SWD is disabled and its pin state is not defined, which results in power leakage as well. I'll make some changes to fix it.

@XuGuohui
Copy link
Member Author

@scott-brust I re-tested the PR against the app you posted above and it worked fine.

Below is when the D7 is configured as output and output low (5s delay + 10s sleep):
Screen Shot 2023-07-19 at 15 41 29

Below is when D7 is configured as SWD (5s delay + 10s sleep):
Screen Shot 2023-07-19 at 15 42 32

@scott-brust
Copy link
Member

scott-brust commented Jul 19, 2023

@scott-brust I re-tested the PR against the app you posted above and it worked fine.

Below is when the D7 is configured as output and output low (5s delay + 10s sleep):

Below is when D7 is configured as SWD (5s delay + 10s sleep):

I re-tested today, and everything seems to work 🤷‍♂️ , here is D7 remaining low while Hibernating. I added a 1s pulse just to make sure the IO was actually working:
Screen Shot 2023-07-19 at 12 38 18 PM

When configured as SWD, it remained low through Hibernate as well. I also confirmed I was able to attach SWD when the device waked, so the pin re-configuration to SWD works as well.

I also checked with the OTII and before in Hibernate I was measuring 115 uA. With these changes its down to 108 uA. Does this match with what you would expect?

@XuGuohui
Copy link
Member Author

XuGuohui commented Jul 20, 2023

Instresting finding, when GPIO is configured as OUTPUT, the pull up/down resistor is not set as expected and it remains the previous setting:

0000001746 [hal] TRACE: pin: 0, expected: 00000100, PADCTR: 00000100 // INPUT_PULLUP
0000001752 [hal] TRACE: pin: 0, expected: 00000000, PADCTR: 00000000 // INPUT_PULLDOWN
0000001757 [hal] TRACE: pin: 0, expected: 00000200, PADCTR: 00000200 // INPUT
0000001764 [hal] TRACE: pin: 0, expected: 00000000, PADCTR: 00000200 // OUTPUT
0000001770 [hal] TRACE: pin: 1, expected: 00000100, PADCTR: 00000100
0000001777 [hal] TRACE: pin: 1, expected: 00000000, PADCTR: 00000000
0000001782 [hal] TRACE: pin: 1, expected: 00000200, PADCTR: 00000200
0000001788 [hal] TRACE: pin: 1, expected: 00000000, PADCTR: 00000200
0000001795 [hal] TRACE: pin: 2, expected: 00000100, PADCTR: 00000100
0000001801 [hal] TRACE: pin: 2, expected: 00000000, PADCTR: 00000000
0000001807 [hal] TRACE: pin: 2, expected: 00000200, PADCTR: 00000200
0000001813 [hal] TRACE: pin: 2, expected: 00000000, PADCTR: 00000200
0000001819 [hal] TRACE: pin: 3, expected: 00000100, PADCTR: 00000100
0000001834 [hal] TRACE: pin: 3, expected: 00000000, PADCTR: 00000000
0000001844 [hal] TRACE: pin: 3, expected: 00000200, PADCTR: 00000200
0000001849 [hal] TRACE: pin: 3, expected: 00000000, PADCTR: 00000200
0000001856 [hal] TRACE: pin: 4, expected: 00000100, PADCTR: 00000100
0000001862 [hal] TRACE: pin: 4, expected: 00000000, PADCTR: 00000000
0000001867 [hal] TRACE: pin: 4, expected: 00000200, PADCTR: 00000200
0000001874 [hal] TRACE: pin: 4, expected: 00000000, PADCTR: 00000200
0000001880 [hal] TRACE: pin: 5, expected: 00000100, PADCTR: 00000100
0000001887 [hal] TRACE: pin: 5, expected: 00000000, PADCTR: 00000000
0000001892 [hal] TRACE: pin: 5, expected: 00000200, PADCTR: 00000200
0000001898 [hal] TRACE: pin: 5, expected: 00000000, PADCTR: 00000200
0000001904 [hal] TRACE: pin: 6, expected: 00000100, PADCTR: 00000100
0000001910 [hal] TRACE: pin: 6, expected: 00000000, PADCTR: 00000000
0000001916 [hal] TRACE: pin: 6, expected: 00000200, PADCTR: 00000200
0000001923 [hal] TRACE: pin: 6, expected: 00000000, PADCTR: 00000200
0000001928 [hal] TRACE: pin: 7, expected: 00000100, PADCTR: 00000100
0000001935 [hal] TRACE: pin: 7, expected: 00000000, PADCTR: 00000000
0000001941 [hal] TRACE: pin: 7, expected: 00000200, PADCTR: 00000200
0000001946 [hal] TRACE: pin: 7, expected: 00000000, PADCTR: 00000200
0000001954 [hal] TRACE: pin: 10, expected: 00000100, PADCTR: 00000100
0000001959 [hal] TRACE: pin: 10, expected: 00000000, PADCTR: 00000000
0000001966 [hal] TRACE: pin: 10, expected: 00000200, PADCTR: 00000200
0000001972 [hal] TRACE: pin: 10, expected: 00000000, PADCTR: 00000200
0000001977 [hal] TRACE: pin: 11, expected: 00000100, PADCTR: 00000100
0000001984 [hal] TRACE: pin: 11, expected: 00000000, PADCTR: 00000000
0000001990 [hal] TRACE: pin: 11, expected: 00000200, PADCTR: 00000200
0000001996 [hal] TRACE: pin: 11, expected: 00000000, PADCTR: 00000200
0000002002 [hal] TRACE: pin: 12, expected: 00000100, PADCTR: 00000100
0000002009 [hal] TRACE: pin: 12, expected: 00000000, PADCTR: 00000000
0000002015 [hal] TRACE: pin: 12, expected: 00000200, PADCTR: 00000200
0000002020 [hal] TRACE: pin: 12, expected: 00000000, PADCTR: 00000200
0000002027 [hal] TRACE: pin: 13, expected: 00000100, PADCTR: 00000100
0000002033 [hal] TRACE: pin: 13, expected: 00000000, PADCTR: 00000000
0000002040 [hal] TRACE: pin: 13, expected: 00000200, PADCTR: 00000200
0000002045 [hal] TRACE: pin: 13, expected: 00000000, PADCTR: 00000200
0000002052 [hal] TRACE: pin: 14, expected: 00000100, PADCTR: 00000100
0000002058 [hal] TRACE: pin: 14, expected: 00000000, PADCTR: 00000000
0000002065 [hal] TRACE: pin: 14, expected: 00000200, PADCTR: 00000200
0000002070 [hal] TRACE: pin: 14, expected: 00000000, PADCTR: 00000200
0000002077 [hal] TRACE: pin: 15, expected: 00000100, PADCTR: 00000100
0000002083 [hal] TRACE: pin: 15, expected: 00000000, PADCTR: 00000000
0000002089 [hal] TRACE: pin: 15, expected: 00000200, PADCTR: 00000200
0000002096 [hal] TRACE: pin: 15, expected: 00000000, PADCTR: 00000200
0000002101 [hal] TRACE: pin: 16, expected: 00000100, PADCTR: 00000100
0000002108 [hal] TRACE: pin: 16, expected: 00000000, PADCTR: 00000000
0000002114 [hal] TRACE: pin: 16, expected: 00000200, PADCTR: 00000200
0000002120 [hal] TRACE: pin: 16, expected: 00000000, PADCTR: 00000200
0000002126 [hal] TRACE: pin: 17, expected: 00000100, PADCTR: 00000100
0000002132 [hal] TRACE: pin: 17, expected: 00000000, PADCTR: 00000000
0000002139 [hal] TRACE: pin: 17, expected: 00000200, PADCTR: 00000200
0000002145 [hal] TRACE: pin: 17, expected: 00000000, PADCTR: 00000200
0000002151 [hal] TRACE: pin: 18, expected: 00000100, PADCTR: 00000100
0000002157 [hal] TRACE: pin: 18, expected: 00000000, PADCTR: 00000000
0000002164 [hal] TRACE: pin: 18, expected: 00000200, PADCTR: 00000200
0000002169 [hal] TRACE: pin: 18, expected: 00000000, PADCTR: 00000200

@XuGuohui XuGuohui changed the title [photon2] fix D7 sleep configuration. [photon2] fix GPIOs sleep configuration. Jul 20, 2023
@XuGuohui
Copy link
Member Author

After digging deeper, this seams to be a common problem, so I updated the title and the description of the PR.

Running the example that is attached in the description (featuring D4 and D5 as normal GPIOs) before applying the diff,

  1. Normal GPIOs, STOP/ULP mode, output HIGH before sleep: ✅
D4_D5_HIGH_STOP
  1. Normal GPIOs, STOP/ULP mode, output LOW before sleep: ✅
D4_D5_LOW_STOP
  1. Normal GPIOs, HIBERNATE mode, output HIGH before sleep: ❌
D4_D5_HIGH_HIB
  1. Normal GPIOs, HIBERNATE mode, output LOW before sleep: ✅
D4_D5_LOW_HIB
  1. SWD pins, STOP/ULP mode, output HIGH before sleep: ✅
D7_D6_HIGH_STOP
  1. SWD pins, STOP/ULP mode, output LOW before sleep: ✅
D7_D6_LOW_STOP
  1. SWD pins, HIBERNATE mode, output HIGH before sleep: ❌ (D6/SWD-CLK)
D7_D6_HIGH_HIB
  1. SWD pins, HIBERNATE mode, output LOW before sleep: ❌ (D7/SWD-DAT)
D7_D6_LOW_HIB

@@ -103,3 +103,7 @@

// Read-only charge indicator pin for Photon2
#define CHG S5

// Set it to PIN_INVALID if not present
Copy link
Member Author

Choose a reason for hiding this comment

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

@scott-brust need to define these pins for msom.

@XuGuohui
Copy link
Member Author

Will do the same tests on MSoM and see if problems are producable without the patch.

@XuGuohui
Copy link
Member Author

Unfortunately when GPIOs are configured as output, the internal pad is actually shutdown during hibernate mode. Configuring pull-up/down resitor just maintain the level on the pin without any load. If there is load on the pin, then it won't work as expected. I have confirmed with Realtek FAE and requested them to provide a patch if possible.

@XuGuohui XuGuohui merged commit 37a6c25 into develop Jul 26, 2023
12 checks passed
@XuGuohui XuGuohui deleted the fix/photon2_D7 branch July 26, 2023 08:10
@scott-brust scott-brust modified the milestones: 5.4.1, 5.5.0-rc.1 Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants