cpu/samd21: Enable continuous read bit for periph_rtt#17403
Open
dylad wants to merge 4 commits intoRIOT-OS:masterfrom
Open
cpu/samd21: Enable continuous read bit for periph_rtt#17403dylad wants to merge 4 commits intoRIOT-OS:masterfrom
dylad wants to merge 4 commits intoRIOT-OS:masterfrom
Conversation
Contributor
|
With this PR I have missed IRQ And with |
f327486 to
3ba673c
Compare
Contributor
|
Now |
3ba673c to
67aa21c
Compare
Contributor
|
I have some weird results @dylad, I tested once on IoT-LAB and It works: But on local (over ssh) hardware I have it taking ages: Can you push a remove c ommit seting the samples to 10 or something to see the result on the Murdock |
Contributor
|
I re-tested on two samr21-xpro I have locally, and both have very large RTT_MIN_OFFSET=(33..320) maybe something special is going on with the iot-lab samr21-xpro? I tested all 15 and all of them had the same result with RTT_MIN_OFFSET=7, could it be something with the flasher? @aabadie is the anything different with them? |
fjmolinas
reviewed
Mar 10, 2022
sys/ztimer/Makefile.dep
Outdated
| # on ztimer_msec that are higher than > +-1msec. | ||
| # The same goes for the fe310 rtt. | ||
| ifneq (,$(filter sam% fe310,$(CPU))) | ||
| ifneq (,$(filter sam3 fe310,$(CPU))) |
Member
Author
There was a problem hiding this comment.
I have access to SAML10/SAML11, SAML21, a SAMD21 mini clone and a SAME54
I don't have any sam3.
Contributor
There was a problem hiding this comment.
sam3 uses a completely different driver, I wonder if it wasn't caught here by mistake.
Member
Author
There was a problem hiding this comment.
When I started this PR, this line was ifneq (,$(filter sam% fe310,$(CPU)))
Which include both sam0 and sam3.
With this PR, sam0 should work fine but since sam3 were disabled and no tests were run on it. I just leave it there.
Member
Author
There was a problem hiding this comment.
Note that there will be a mismatch with sam3 Kconfig since this feature is also disabled in sam3 Kconfig
Contributor
|
Could you rebase? It would be nice to know if this brings improvement for |
Member
Author
|
Will do tonight. |
1e785f5 to
1abb847
Compare
Member
Author
|
Okay I've rebased this PR (and I really hope I didn't f*ck up something). |
Contributor
|
Ah, Kconfig… |
1abb847 to
f35057e
Compare
f35057e to
6398c5d
Compare
Signed-off-by: Dylan Laduranty <dylan.laduranty@mesotic.com>
Contributor
|
@benpicco Could you check this PR with your |
Contributor
|
With this PR on with this patchdiff --git a/examples/timer_periodic_wakeup/Makefile b/examples/timer_periodic_wakeup/Makefile
index 9168c38eb5..c2ad1913ad 100644
--- a/examples/timer_periodic_wakeup/Makefile
+++ b/examples/timer_periodic_wakeup/Makefile
@@ -1,8 +1,11 @@
APPLICATION = timer_periodic_wakeup
RIOTBASE ?= $(CURDIR)/../..
+
BOARD ?= native
-USEMODULE += xtimer
-QUIET ?= 1
+
+USEMODULE += ztimer_msec
+
+USEMODULE += debug_irq_disable
# Comment this out to disable code in RIOT that does safety checking
# which is not needed in a production environment but helps in the
diff --git a/examples/timer_periodic_wakeup/main.c b/examples/timer_periodic_wakeup/main.c
index a1c91506cf..fca92a0f03 100644
--- a/examples/timer_periodic_wakeup/main.c
+++ b/examples/timer_periodic_wakeup/main.c
@@ -19,19 +19,19 @@
*/
#include <stdio.h>
-#include "xtimer.h"
+#include "ztimer.h"
#include "timex.h"
/* set interval to 1 second */
-#define INTERVAL (1U * US_PER_SEC)
+#define INTERVAL (1U * MS_PER_SEC)
int main(void)
{
- xtimer_ticks32_t last_wakeup = xtimer_now();
+ uint32_t last_wakeup = ztimer_now(ZTIMER_MSEC);
while(1) {
- xtimer_periodic_wakeup(&last_wakeup, INTERVAL);
- printf("slept until %" PRIu32 "\n", xtimer_usec_from_ticks(xtimer_now()));
+ ztimer_periodic_wakeup(ZTIMER_MSEC, &last_wakeup, INTERVAL);
+ printf("slept until %" PRIu32 "\n", ztimer_now(ZTIMER_MSEC));
}
return 0;Still slightly better than With |
This was partially cleans up in RIOT-OS#18423 but it looks like this one was missed. Signed-off-by: Dylan Laduranty <dylan.laduranty@mesotic.com>
Member
Author
|
I failed to understand why CI is complaining only for two |
Contributor
|
How can we move this forward? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contribution description
This PR sets the continuous read bit in
READREQregister for SAMD21. This will allows to continuously sync theCOUNTregister in order to avoid the heavy sync delay that currently exists on master.According to the datasheet, any write attempt on a write synchronized register will clear this bit, so we re-enables it after any
COUNTorALARMwrite for RTT driver. RTC is not affected by this PR.Since
READREQregister is only defined for SAMDxx, others SAM0 families should not be affected by this PR.I don't know if this will have side effects so let the CI tests it.
Testing procedure
Check CI.
Can also be tested on hardware.
Issues/PRs references
Partial fix for #17395