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 high idle/sleep current due to flash and use xiao-ble from upstream Zephyr #2091

Merged
merged 1 commit into from Jan 6, 2024

Conversation

ldab
Copy link
Contributor

@ldab ldab commented Jan 2, 2024

I'm working a XIAO-BLE based keyboard and I noticed high idle and sleep currents, bearing in mind that my board:

  • Uses matrix polling;
  • has CA95xx gpio expander;

image

The 500uA current is similar to reported here: https://forum.seeedstudio.com/t/low-power-with-xiao-nrf52840-on-zephyr-rtos/270491/6

Which led me to #1927. Even tho the PR seems to address the issue somehow, I find it strange that it does since the nRF driver puts the flash on Deep Power-Down (DPD) drivers/flash/nrf_qspi_nor.c#L1324, the reason for DPD not properly working I believe to be related to the fact that the ZMK fork of Zephyr use what seems the incorrect boards/arm/seeeduino_xiao_ble/seeeduino_xiao_ble.dts#L97 board configuration (which does not match the upstream Zephyr).

I noticed the seeeduino_xiao_ble was added on zmkfirmware/zephyr@1f19b89 and I do not know the reason for adding since it is already available on boards/arm/xiao_ble and also on Zephyr 3.2 and newer.

With that being said, this PR:

  1. Remove seeeduino_xiao_ble and use the xiao_ble from Zephyr
  2. Put the Flash to Deep Power Down mode and reduce the idle/sleep current. (To add measurement)

Note/Question:

  1. Another PR can be made on zmkfirmware/zephyr to revert zmkfirmware/zephyr@1f19b89
  2. If removing the board is not desireable, I can change everything on the app/boards/seeeduino_xiao_ble.overlay

@ldab ldab requested a review from a team as a code owner January 2, 2024 09:46
@ldab ldab closed this Jan 2, 2024
@ldab ldab changed the title Xiao flash sleep Fix high idle/sleep current due to flash and use xiao-ble from upstream Zephyr Jan 2, 2024
@ldab ldab reopened this Jan 2, 2024
@caksoylar
Copy link
Contributor

I don't have a strong opinion on removing seeeduino_xiao_ble, but from a user/support POV it would be great if some sort of backwards compatibility shim existed.

At the very least, I think it would be helpful to split this cleanly into two commits: one which fixes the battery use and one which removes the seeeduino_xiao_ble board. Then it would be easier to test individually and commit one or both depending on the eventual decision.

@xudongzheng
Copy link
Contributor

xudongzheng commented Jan 4, 2024

Have you measured the power consumption with these changes? Ideally measure and compare with Bluetooth disabled but even figures with Bluetooth could be useful.

Also worth mentioning that upstream Zephyr definition for xiao_ble changed from SPI to QSPI in Zephyr 3.4 so your change will break with the Zephyr 3.5 upgrade in #1995.

I would even try measuring on Zephyr 3.5 without any changes. See zephyrproject-rtos/zephyr#55199 for some related issues. I haven't looked at this closely but one of those issues won't be in upstream Zephyr until 3.6.

@ldab
Copy link
Contributor Author

ldab commented Jan 4, 2024

Have you measured the power consumption with these changes? Ideally measure and compare with Bluetooth disabled but even figures with Bluetooth could be useful.

Also worth mentioning that upstream Zephyr definition for xiao_ble changed from SPI to QSPI in Zephyr 3.4 so your change will break with the Zephyr 3.5 upgrade in #1995.

I would even try measuring on Zephyr 3.5 without any changes. See zephyrproject-rtos/zephyr#55199 for some related issues. I haven't looked at this closely but one of those issues won't be in upstream Zephyr until 3.6.

Measuring with BT off is pointless since the USB controller + Battery charger will drain a lot more power than this fix and it would overshadow the fix.

I do not see how that would break with the upstream QSPI Zephyr, in my view it is the opposite actually. Currently there is ZMK own board definition inside Zephyr repo that could contain flags that are no longer supported. On the upstream, QSPI is defined and configured and enter DPD upon init(), unless I missed what you meant.

@ldab
Copy link
Contributor Author

ldab commented Jan 4, 2024

At the very least, I think it would be helpful to split this cleanly into two commits: one which fixes the battery use and one which removes the seeeduino_xiao_ble board. Then it would be easier to test individually and commit one or both depending on the eventual decision.

ok, I have made a separate commit.

Please find below the difference:

  • from 260uA to 180uA on "idle"
  • from 180uA to 1uA on deep sleep 🎆

Measured at 3.3V as the LDO is undocumented and cant be trusted:

My board before

2024-01-04_13-04

With this commit

2024-01-04_13-12

I have also flashed hummingbird to a bare module for comparison

Before

hummingbird_before

After

hummingbird_after

@xudongzheng
Copy link
Contributor

xudongzheng commented Jan 4, 2024

from 180uA to 1uA on deep sleep 🎆

Thanks for these additional figures. The deep sleep figures were more or less what I was looking for - essentially both BT and USB off as that most accurately isolates the QSPI power consumption.

I do not see how that would break with the upstream QSPI Zephyr, in my view it is the opposite actually. Currently there is ZMK own board definition inside Zephyr repo that could contain flags that are no longer supported. On the upstream, QSPI is defined and configured and enter DPD upon init(), unless I missed what you meant.

Your earlier commit had CONFIG_SPI_NOR_IDLE_IN_DPD. With the upstream xiao_ble board, that would been fine prior to Zephyr 3.4 since it used SPI. It would would not work for Zephyr 3.4 since QSPI is used instead of SPI, as is the case with the board in ZMK's fork.

@caksoylar
Copy link
Contributor

I see just one commit in the PR now, with the qspi node changes. Is that enough or were the older changes dropped by accident?

@ldab
Copy link
Contributor Author

ldab commented Jan 4, 2024

I see just one commit in the PR now, with the qspi node changes. Is that enough or were the older changes dropped by accident?

The 1x commit is enough

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

This all seems reasonable to me. We have our own board definition for historical reasons... Sadly, removing it now would break a lot of folks' existing builds. It would be nice to at least get the upstream board definition fully supported (by adding the necessary config and overlay ZMK specific additions on app/boards) and changing our metadata file to guide folks to that board ID, so we can slowly phase it out.

For now... Does this in fact fix things still for our board definition? If so, happy to merge, and grateful for the fix!

@petejohanson petejohanson self-assigned this Jan 5, 2024
@petejohanson petejohanson added bug Something isn't working board PRs and issues related to boards. labels Jan 5, 2024
@Kikangh
Copy link

Kikangh commented Jan 5, 2024

This all seems reasonable to me. We have our own board definition for historical reasons... Sadly, removing it now would break a lot of folks' existing builds. It would be nice to at least get the upstream board definition fully supported (by adding the necessary config and overlay ZMK specific additions on app/boards) and changing our metadata file to guide folks to that board ID, so we can slowly phase it out.

For now... Does this in fact fix things still for our board definition? If so, happy to merge, and grateful for the fix!

Hi Pete, is this something that could be used with my revxlp v0.1? If so I'd gladly test it to see if I get longer battery life.
Edit: my discord handle is Gillesh ;-)

@ldab
Copy link
Contributor Author

ldab commented Jan 5, 2024

For now... Does this in fact fix things still for our board definition? If so, happy to merge, and grateful for the fix!

Yes, the overlay fix the current board definition with:

  • from 260uA to 180uA on "idle"
  • from 180uA to 1uA on deep slee

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

Flashed to my revxlp here with a XIAO BLE, and runs no problem. I trust OP measurements here, so merging so others can get the fix easily. We can follow up further if folks still report high power draw.

Thanks for digging and for the fix @ldab !

@petejohanson petejohanson merged commit cebf651 into zmkfirmware:main Jan 6, 2024
11 checks passed
@ldab ldab deleted the xiao-flash-sleep branch January 6, 2024 09:03
Kikangh added a commit to Kikangh/zmk that referenced this pull request Jan 7, 2024
…am Zephyr zmkfirmware#2091

 Merged
petejohanson merged 1 commit into zmkfirmware:main from ldab:xiao-flash-sleep 2 days ago

Getting the changes in my repo to benefit from the battery savings.
@KLingO-MS
Copy link

with latest changes to seeeduino_xiao_ble.overlay (instead qspi disabled) my split KB with XIAO BLEs refuse to go to sleep anymore after first sleep-wakeup cycle ... borads need to be powered off-on to get to sleep again

I again built with qspi overridden to disabled and it works again

@caksoylar
Copy link
Contributor

I am also seeing randomness in being able to sleep, sometimes it sleeps fine, sometimes it won't. The symptoms look to be the same as #1901.

@ldab
Copy link
Contributor Author

ldab commented Jan 14, 2024

with latest changes to seeeduino_xiao_ble.overlay (instead qspi disabled) my split KB with XIAO BLEs refuse to go to sleep anymore after first sleep-wakeup cycle ... borads need to be powered off-on to get to sleep again

I again built with qspi overridden to disabled and it works again

On my tests as shown on the measurements above, it worked ok.

To help you, can you please create an issue and include a detailed description of your issue, including which board you use, the schematic, measurements and enable USB logs and attach it?

@ldab
Copy link
Contributor Author

ldab commented Jan 14, 2024

I am also seeing randomness in being able to sleep, sometimes it sleeps fine, sometimes it won't. The symptoms look to be the same as #1901.

The sleep code only looks for VBus detected, no?

It could be related to zephyrproject-rtos/zephyr#55543 can you pull some logs?

Can you try to patch with zephyrproject-rtos/zephyr#64782 and see if it helps?

@KLingO-MS
Copy link

with latest changes to seeeduino_xiao_ble.overlay (instead qspi disabled) my split KB with XIAO BLEs refuse to go to sleep anymore after first sleep-wakeup cycle ... borads need to be powered off-on to get to sleep again
I again built with qspi overridden to disabled and it works again

On my tests as shown on the measurements above, it worked ok.

To help you, can you please create an issue and include a detailed description of your issue, including which board you use, the schematic, measurements and enable USB logs and attach it?

I'm running handwire split (10 keys on each half, directly connected to pins and GND - no matrix) on XIAO BLEs with latest ZMK build
sequence:

  • power on
  • typing, and leave to rest until sleep timer expires
  • enter the sleep (boards disappear in battery widget in MacOS)
  • wakeup (i can see battery percentages in widget again)
  • typing and leave to rest -- not going to sleep anymore (no matter how long I wait)

I have split battery reporting on and also using Brave's RGB widget - that's how I noticed that they are not sleeping .. as after returning to PC they do not blink battery status and connection after pressing keys (what RGB widget always does after wakeup) - this led me to checknig it on battery widget in MacOS and indeed they always stay on after first wakeup

after setting QSPI to disabled behavior want back to normal and boards go to sleep after timeout

my assumption is that when I'll try running logging they will not go to sleep as USB is connected

@ldab
Copy link
Contributor Author

ldab commented Jan 14, 2024

my assumption is that when I'll try running logging they will not go to sleep as USB is connected

Your assumption is correct, but the logs may show something that we are missing, you can change this and it will go to sleep when USB is connected:

if (inactive_time > MAX_SLEEP_MS && !is_usb_power_present()) {

Do you have your keyboard configuration? I can try to reproduce it here.

Can you try to patch with zephyrproject-rtos/zephyr#64782 and see if it helps?

@KLingO-MS
Copy link

I do not have local env .... building via GHA, so editing .c files is out of my reach i guess

my repo is here
https://github.com/KLingO-MS/zmk-config-KgO-BLE28/tree/BLE20

@ldab
Copy link
Contributor Author

ldab commented Jan 14, 2024

It prints

[00:00:14.324,859] <err> pm: Device p25q16h@0 did not enter suspended state (-140)

So I suppose it relates to the patch I mentioned above. If I remove the power (and battery) it works no problem. so there is a point where the QSPI is not handled properly.

@caksoylar
Copy link
Contributor

I am now using a Zephyr 3.5-based branch (#2027, that is based on #1995) and the deep sleep is working OK after waking up from sleep.
I suppose the upgrade from 3.2 to 3.5 might have some fixes related to the flash chip's power management.

I am not sure if the power consumption is normal after waking up since I don't have a power profiler, but I'll track overall battery life.

@ldab
Copy link
Contributor Author

ldab commented Jan 20, 2024

I am not sure if the power consumption is normal after waking up since I don't have a power profiler, but I'll track overall battery life.

I didn't flash the board you mentioned to my keyboard because the GPIOs are different, but I have build based on #1995 my keyboard and the result is:

image

So using #1995 the sleep current is 20uA vs ~1uA I measured not, but it seems to fix the sleep issue after second wake. I guess we can open an issue to track it and investigate more when 3.5.0 is merged, what do you think @caksoylar ?

@caksoylar
Copy link
Contributor

caksoylar commented Jan 21, 2024

That's interesting that the sleep current went up but the usage consumption looks pretty good, thanks for measuring!

I guess we can open an issue to track it and investigate more when 3.5.0 is merged, what do you think @caksoylar ?

I think that would be a good idea indeed. It shouldn't be too long until 1995 is merged, I am guessing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
board PRs and issues related to boards. bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants