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

WKP pin should not be enabled as a wakeup source unconditionally for STOP mode #948

Merged
merged 1 commit into from Jun 16, 2016

Conversation

@avtolstoy
Copy link
Member

commented Apr 8, 2016

Fixes #938

Confirmed that after exiting STOP mode with some other pin specified as a wakeup source, WKP pin retains its configuration (e.g. tone() continues working).

If WKP is specified as wakeup source for System.sleep(), its pinMode will be changed to either INPUT, INPUT_PULLDOWN or INPUT_PULLUP depending on trigger condition. The pin will have to be reconfigured.


Doneness:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • N/A - API tests compiled
  • Run unit/integration/application tests on device
  • N/A Add documentation
  • Add to CHANGELOG.md after merging (add links to docs and issues)

@monkbroc monkbroc added the in progress label Apr 8, 2016

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2016

Could you write a test to validate the behavior?

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

I'm uncertain if we should change the default behavior (so A7 is no longer available as a wakeup source after stop mode sleep.)

While that would be the simplest fix, it may break existing applications, although in the long term I feel this is the best solution. We could provide documentation for when A7 wakeup is required then PWR_WakeUpPinCmd(ENABLE); should be called before putting the device to sleep.

@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

How about we go with the second approach: save the A7 configuration, call PWR_WakeUpPinCmd(DISABLE); on exit from STOP mode and restore A7 configuration? I believe it should work just fine, I'm not sure why we dismissed it.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

ok, if we can preserve the configuration, that's also a good approach, although I do feel using WKP in stop mode is unexpected, and I can imagine situations where this might cause unexpected wakeups, depending upon what else A7 is being used for.

I feel your initial PR is best - remove wakeup by default, and let application developers add this if they need it.

@m-mcgowan m-mcgowan added this to the 0.5.1 milestone Apr 27, 2016

@monkbroc monkbroc assigned monkbroc and unassigned avtolstoy Apr 28, 2016

@monkbroc

This comment has been minimized.

Copy link
Member

commented Apr 28, 2016

Is this changing documented behavior so that WKP will no longer wake a device up from sleep?

https://docs.particle.io/reference/firmware/photon/#backup-ram-sram-

Power Down Method: System.sleep(SLEEP_MODE_DEEP)
Power Up Method : Rising edge on WKP pin, or Hard Reset

https://docs.particle.io/datasheets/photon-datasheet/#pin-description

WKP: Active-high wakeup pin, wakes the module from sleep/standby modes

Update
I see that SLEEP_MODE_DEEP uses STANDBY mode not STOP.

My question still stands: are users depending on the fact that WKP will wake up a device in STOP mode?

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2016

This is stop mode sleep, not deep sleep. As far as I know, being able to wake up stop mode sleep from A7/WKP is undocumented (and imho unepxected) behavior.

Stop mode sleep is sleep(D3, RISING, 30000);

@monkbroc

This comment has been minimized.

Copy link
Member

commented Apr 28, 2016

I appears that the WKP pin doesn't wake the micro in STOP mode System.sleep(D0, RISING); either before or after this change.

So I'd say this is ready to merge.

@monkbroc monkbroc removed the in progress label Apr 28, 2016

@monkbroc monkbroc removed their assignment Apr 28, 2016

@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

Correct, according to the reference manual WKP doesn't wakeup CPU from STOP mode:

If WFI or return from ISR was used for entry:
Any EXTI lines configured in Interrupt mode (the corresponding EXTI
Interrupt vector must be enabled in the NVIC). The interrupt source can
be external interrupts or peripherals with wakeup capability. Refer to
Table 20: Vector table on page 163.

This PR mainly fixes the issue that WKP pin is fixed in INPUT_PULLDOWN configuration after exiting STOP mode and its configuration cannot be changed anymore.

@ScruffR

This comment has been minimized.

Copy link
Contributor

commented May 25, 2016

Just to be awkward 😉

https://docs.particle.io/reference/firmware/photon/#sleep-sleep-
currently does say

wakeUpPin: the wakeup pin number. supports external interrupts on the following pins:
D0, D1, D2, D3, D4, A0, A1, A3, A4, A5, A6, A7

Easily fixed, but contradicts this statement made earlier

As far as I know, being able to wake up stop mode sleep from A7/WKP is undocumented (and imho unepxected) behavior.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented May 25, 2016

What I meant was that the WKP pin isn't documented to function in addition to the specified wakeup pin in stop mode. (Currently the code is enabling WKP as a deep-sleep wakeup source, for stop mode sleep.)

So, sleep(D0, RISING, 60) shouldn't also wake up on A7/WKP. For that, the user should specify A7 as the trigger pin, sleep(A7, RISING, 60);

@technobly technobly self-assigned this Jun 15, 2016

@technobly technobly modified the milestones: 0.6.x, 0.5.1 Jun 16, 2016

@technobly technobly merged commit cfbb226 into develop Jun 16, 2016

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 removed their assignment Jun 16, 2016

@danpe

This comment has been minimized.

Copy link

commented Oct 21, 2016

I'm still having the issue while using the AssetTracker WakeOnMove example.

System.sleep(WKP, RISING, TIME_PUBLISH_BATTERY_SEC, SLEEP_NETWORK_STANDBY);

When I used the 0.5.3 electreon firmware version it stopped waking up the device after an hour.
Now after updating to 0.6.0-RC2 it takes about 3-4 hours of no movement to stop waking up the device.

@technobly technobly deleted the feature/stop-mode-wkp-pin branch Oct 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.