-
Notifications
You must be signed in to change notification settings - Fork 2k
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
cpu/samd21: implement low power modes #2309
Conversation
start_lpm(); | ||
break; | ||
case LPM_OFF: /* Standby Mode - Potential Wake Up sources: Asynchronous */ | ||
current_mode = LPM_POWERDOWN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake sorry! Should be
current_mode = LPM_OFF;
Just tried with the diff below and couldn't see any difference on our energy consumption test equipment. diff --git a/core/kernel_init.c b/core/kernel_init.c
index a57f119..c18b6e6 100644
--- a/core/kernel_init.c
+++ b/core/kernel_init.c
@@ -64,8 +64,8 @@ static void *idle_thread(void *arg)
lpm_set(LPM_IDLE);
}
else {
- lpm_set(LPM_IDLE);
- /* lpm_set(LPM_SLEEP); */
+ //lpm_set(LPM_IDLE);
+ lpm_set(LPM_SLEEP);
/* lpm_set(LPM_POWERDOWN); */
} |
I'll have access to my lab again next week. I'll give it a go then. Thanks |
@saurabh984 I addressed your comments. |
@@ -14,40 +15,112 @@ | |||
* @brief Implementation of the kernels power management interface | |||
* | |||
* @author Thomas Eichinger <thomas.eichinger@fu-berlin.de> | |||
* @author Saurabh Singh <email-missing@example.com> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saurabh984 your email is missing here ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LudwigOrtmann saurabh@cezy.co Haven't had a chance to try it yet :(
@LudwigOrtmann I tried to test this PR with:
I couldn't awake the board after |
@PeterKietzmann Try commenting out the cpu_init() (shown below) code from
|
@saurabh984, I commented this line out. With this change I could also awake from |
@saurabh984 did you experience the same? |
9f184dd
to
45554bf
Compare
@PeterKietzmann, from p1048 to 1051/1138 (samr21 datasheet), you can find the power consumption of the samr21. There is also a schematic about how to use the measurement pin. |
Here are my results with @PeterKietzmann source code (above):
Here I've got 0,39mA but I can't wake up, Interruption doesn't work anymore. If I keep By the way, @PeterKietzmann, you should use a |
FYI concerning |
@bapclenet what do you mean by the SCB is not mentioned for power management? edit: I am speaking from experience with CM3/4 CPUs, I did not look up CM0 docs on this. |
Sorry, my bad, I didn't know that before, I've just seen it. |
@bapclenet your results seem somehow reasonable even if they appear to be kind of small, comparing to the datasheet (worse are mine :-) ). But honestly I didn't compare the clock setup etc. . Now the question is: is there anybody out who has a deeper knowledge with this MCU and with (low) power modes? Otherwise I need to spend some time in going into detail. |
@bapclenet can you explain in detail how you build your setup? Looking at the datasheet on page 1054 it feels like I can't directly access the needed pins or have to use an external power supply not from USB. Sorry for all these questions. I don't want to spend too much time in this and you already did these steps. |
@PeterKietzmann, I use a Multimeter from Agilent.
There are the same pins as in the datasheet. What I'm doing is removing the header (Datasheet p4 - CURRENT MEASUREMENT HEADER) and plug them to my multimeter. I think (not sure about that), whatever the source is, there is a regulator which transforms 5V to 3.3V, and after the transformation, there are those two pins where you can measure the current before going to the MCU. Does it make sens? |
Yes and no. Measuring the current through the "current measurement header"-pins is reasonable and the way I already did (some time ago). What I don't understand is the hint you gave me with the schematic in the datasheet (p. 1054). Anyway, if I see it right and understand correctly you just power your board by the pins and not by an USB connector and you measure the current through the measurement extension header. |
I think this PR will need a lot of attention so I don't think it will make it for the release. What's the status BTW? |
@MichelRottleuthner did you already give it a try? |
@PeterKietzmann Nope. It's on my todo list but I think it will take a few weeks before I have time for that. |
Maybe @aabadie wants to take a look ;) |
Hey all. Can I help get this merged in for the upcoming release? I have used it, along with a few other changes, and have a SAMR21 running at 5.2uA idle and 15uA average sampling temperature at 1Hz |
@immesys, you're welcome to help us merging this PR :-) |
So, the good news is yes, it is waking up fine from lpm off, the bad news is that the code is a little messy (as things get when you are just trying to make it work). I am busy splitting things up into independent PRs, so things like adding radio sleep to netdev, switching the xtimer source to ULP OSC32. I could really use a shepherd, as some of these changes are in core code and change default behavior (I use FLL instead of PLL saving a bit of energy, and I source it from the ULP instead of the 8mhz RC, which costs accuracy but saves energy). I am busy running experiments to quantify the benefit of each change individually so that the riot core devs can decide if each one is worth it. |
@kaspar030, @gebart, would you be willing to act as a shepherd? I would also volunteer but have only time in about two weeks from now. |
@immesys it's great to see your efforts improving the sam* support in RIOT! Would it help you to merge this PR "as is" (or with slight adoptions which you would need to propose ;-) )? |
I use this file as-is, so it would definitely help to merge it. The slight changes elsewhere are too invasive to just merge, I will need to discuss them with the core maintainers to work out the best way to do them. |
@LudwigKnuepfer would you rebase this PR once more? If not @immesys can you take it over (closing this one and opening an updated PR) |
I would not mind rebasing, but it didn't actually have any changes, I merge it as-is with no conflicts. In fact at the moment the idle thread (in kernel_init.c) doesn't actually put anything to sleep so merging this file shouldn't even change the behavior of existing code (which is either a feature or a problem depending on how you look at things) |
@immesys I do agree with you but regardless of that I'd like to see Murdock pass, so the PR should be updated. (It's still based on Travis, so kind of old :-) ) |
I will rebase. |
4cf9261
to
aa599bf
Compare
Should I also squash and mark as ready for CI? |
Yes please! But probably I'm just back on Monday. |
@LudwigKnuepfer, squash? |
aa599bf
to
cabb200
Compare
@OlegHahm I did. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK and go
Congrats for this PR! |
TODO: needs testing