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

Power leakage from external flash #1725

Merged
merged 5 commits into from Mar 19, 2019

Conversation

@YutingYou
Copy link
Contributor

commented Mar 14, 2019

Problem

  1. We don't send enter sleep command to external flash before uninitializing it
  2. We don't uninitialize external flash before entering STANDBY mode.

Steps to Test

Test the fix on Xenon-SoM, the power consumption decrease from 8uA to 1uA in STANDBY mode.

Example App

void setup() {
}
void loop() {
    System.sleep(SLEEP_MODE_DEEP, 0);
}

References

[CH29844]
[CH29689]


Completeness

  • [Bugfix] [Gen 3] Fixes a deadlock in system_power_manager and i2c_hal when exiting the sleep mode 1725
  • [Enhancement] [Gen 3] QSPI flash is put into sleep mode and is deinitialized when entering STANDBY or STOP sleep mode 1725

@YutingYou YutingYou requested a review from avtolstoy Mar 14, 2019

@avtolstoy
Copy link
Member

left a comment

Great work.


// Disable external flash
hal_exflash_uninit();
hal_exflash_unlock();

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Mar 14, 2019

Member

I think it should be fine to keep the lock here.

This comment has been minimized.

Copy link
@YutingYou

YutingYou Mar 14, 2019

Author Contributor

Yeah, the MCU won't execute code after entering standby mode. Removed it.

@avtolstoy avtolstoy added this to the 1.1.0-rc.1 milestone Mar 14, 2019

Eugene
@YutingYou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

To be doubly sure, I'll run the EEPROM test in stop mode, in case the sleep command impacts external flash.

@YutingYou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

The external flash on dev board is different from the one on SoM, we need an additional read id command to wake up flash. And we should ensure the flash operation is alright after resuming from stop/standby mode.

Test device: Xenon, Xenon-SoM
Test application:

void setup() {
}

void loop() {
    delay(5000);
    Log.info("about to SLEEP_MODE_DEEP");

    // System.sleep(SLEEP_MODE_DEEP);
    System.sleep(D8, FALLING, 10, SLEEP_NETWORK_OFF);
    static uint32_t value = 1;
    EEPROM.write(5, value++);
    Log.info("data: %d", EEPROM.read(5));
}
@YutingYou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Disabling thread scheduler is also needed in stop/standby mode. The power thread has high priority than system thread, we disable some interrupts in stop mode function, if the thread scheduler occurs before we enable I2C interrupt, then the power thread will get stuck in I2C transaction.

@YutingYou YutingYou added needs review and removed do not merge labels Mar 15, 2019

@@ -938,6 +945,9 @@ int HAL_Core_Execute_Standby_Mode_Ext(uint32_t flags, void* reserved) {
if (flags & HAL_STANDBY_MODE_FLAG_DISABLE_WKP_PIN) {
return SYSTEM_ERROR_NOT_SUPPORTED;
}

// Disable thread scheduling
os_thread_scheduling(false, NULL);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Mar 15, 2019

Member

This shouldn't be done. You are disabling scheduling and later on attempt to acquire the exflash lock which is implemented using a recursive mutex. If by this point it's being held by some other thread, it will never be released and everything will deadlock. sd_nvic_critical_region_enter will also disable thread scheduling, disabling the scheduler is unnecessary.

@@ -541,6 +542,9 @@ int HAL_Core_Enter_Stop_Mode_Ext(const uint16_t* pins, size_t pins_count, const
}
}

// Disable thread scheduling
os_thread_scheduling(false, NULL);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Mar 15, 2019

Member

This shouldn't be done. You are disabling scheduling and later on attempt to acquire the exflash lock which is implemented using a recursive mutex. If by this point it's being held by some other thread, it will never be released and everything will deadlock. sd_nvic_critical_region_enter will also disable thread scheduling, disabling the scheduler is unnecessary.

@YutingYou YutingYou force-pushed the fix/hal/gen3-exflash-enter-sleep branch from 650cb90 to e4f440d Mar 15, 2019

@YutingYou YutingYou force-pushed the fix/hal/gen3-exflash-enter-sleep branch from e4f440d to 5cc4524 Mar 15, 2019

@technobly technobly merged commit 9acb3bf into develop Mar 19, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the fix/hal/gen3-exflash-enter-sleep branch Mar 19, 2019

@avtolstoy avtolstoy referenced this pull request May 29, 2019
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.