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 optimization - Improve SPI sleep mode #1758

Merged
merged 6 commits into from
Jun 4, 2023
Merged

Conversation

JF002
Copy link
Collaborator

@JF002 JF002 commented May 7, 2023

Ensure that all pins are set to their default configuration during sleep mode. Disable the workaround for FTPAN58 (SPI freezes when transfering a single byte) at the end of the transfer. This disables the resources needed for the workaround. Those changes reduce the power usage by 430-490µA.

Ensure that all pins are set to their default configuration during sleep mode.
Disable the workaround for FTPAN58 (SPI freezes when transfering a single byte) at the end of the transfer. This disables the resources needed for the workaround.
Those changes reduce the power usage by 430-490µA.
@JF002 JF002 added the enhancement Enhancement to an existing app/feature label May 7, 2023
@github-actions
Copy link

github-actions bot commented May 7, 2023

Build size and comparison to main:

Section Size Difference
text 407660B 0B
data 940B 0B
bss 54136B 0B

@JF002 JF002 mentioned this pull request May 7, 2023
mark9064 added a commit to mark9064/InfiniTime that referenced this pull request May 8, 2023
commit 5587ce3
Author: Jean-François Milants <jf@codingfield.com>
Date:   Sun May 7 18:24:34 2023 +0200

    Power optimization - Improve SPI sleep mode

    Ensure that all pins are set to their default configuration during sleep mode.
    Disable the workaround for FTPAN58 (SPI freezes when transfering a single byte) at the end of the transfer. This disables the resources needed for the workaround.
    Those changes reduce the power usage by 430-490µA.
src/drivers/St7789.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@Riksu9000 Riksu9000 left a comment

Choose a reason for hiding this comment

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

How much do each of the changes contribute to reducing the power consumption separately?

src/drivers/SpiNorFlash.cpp Outdated Show resolved Hide resolved
src/drivers/SpiNorFlash.cpp Outdated Show resolved Hide resolved
@JF002
Copy link
Collaborator Author

JF002 commented May 18, 2023

How much do each of the changes contribute to reducing the power consumption separately?

This PR basically contains 2 changes : reset the CS pin configuration during sleep (and restore it at wake up) and disable the "workaround for FTPAN58" (disable the PPI channel).

As I said in responses to your reviews, resetting the CS pin config is probably wrong as CS pins must be kept high whenever we don't want devices to listen to the bus, so I'll remove those change.
The power usage reduction from this change is very small (not easy to measure, something between 1-5µA, 10µA max).

Most of the power usage reduction comes from disabling the PPI channel : from 900µA to 410µA in sleep mode with BLE disabled.

JF002 added 3 commits May 18, 2023 15:18
Calls to Spi::Init() are not needed, pin initialization is already done in ctor().
Remove calls to Spi::Sleep()/Spi::Wakeup() to ensure that SPI CS pins are kept high even in sleep mode.
ST7789 driver : replace the constant '26' with a named constant to specify the pin number of the reset pin of the LCD controller.
Fix formatting issue in St7789.cpp
Comment on lines 180 to 184
void St7789::HardwareReset() {
nrf_gpio_pin_clear(26);
nrf_gpio_pin_clear(pinReset);
nrf_delay_ms(10);
nrf_gpio_pin_set(26);
nrf_gpio_pin_set(pinReset);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we configure the pin only at the beginning of this function, and reset back to default before returning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The pins needs to be kept at a high level to ensure the display controller won't reset unexpectedly, so I guess we can't configure the pins to default (input, no pull up/down) here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's true, wouldn't that mean we shouldn't default configure the pin in the sleep function either or anywhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, you are right, I fixed this in 3c668ca.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, there's still nrf_gpio_cfg_default in the St7789::Sleep() function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, right, I guess we shouldn't default the reset pin!

@JF002 JF002 added this to the 1.13.0 milestone May 20, 2023
mark9064 added a commit to mark9064/InfiniTime that referenced this pull request May 27, 2023
commit 5587ce3
Author: Jean-François Milants <jf@codingfield.com>
Date:   Sun May 7 18:24:34 2023 +0200

    Power optimization - Improve SPI sleep mode

    Ensure that all pins are set to their default configuration during sleep mode.
    Disable the workaround for FTPAN58 (SPI freezes when transfering a single byte) at the end of the transfer. This disables the resources needed for the workaround.
    Those changes reduce the power usage by 430-490µA.
@JF002 JF002 merged commit 8fee341 into main Jun 4, 2023
5 of 7 checks passed
baltevl added a commit to baltevl/InfiniSim that referenced this pull request Jun 12, 2023
As of InfiniTimeOrg/InfiniTime#1758 ST7789 driver constructor has a pinReset argument.
NeroBurner pushed a commit to baltevl/InfiniSim that referenced this pull request Jun 12, 2023
As of InfiniTimeOrg/InfiniTime#1758 ST7789 driver constructor has a
pinReset argument.

Update the InfiniTime submodule to the state after the update of the
ST7789 driver.

Fixes: InfiniTimeOrg#107
@Mindavi
Copy link

Mindavi commented Jul 21, 2023

Thanks for these improvements! It seems like my pinetime is really holding out very long on one charge now, I am very happy about that. 😊

@FintasticMan FintasticMan deleted the power-optimization-spi-sleep branch August 3, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing app/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants