cpu/samd21: Enable continuous read bit for periph_rtt #17403
+26
−16
Conversation
This file contains 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
dylad
added
the
CI: run tests
github-actions
bot
added
Area: CI
dylad
added
the
CI: ready for build
|
With this PR I have missed IRQ And with |
dylad
force-pushed
the
pr/cpu/samd21/rtt_continuous_read
branch
from
f327486 to
3ba673c
Compare
dylad
added
CI: ready for build
|
Now |
dylad
force-pushed
the
pr/cpu/samd21/rtt_continuous_read
branch
from
3ba673c to
67aa21c
Compare
github-actions
bot
added
Area: sys
|
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 |
dylad
requested review from
cgundogan,
jia200x and
leandrolanzieri
as code owners
fjmolinas
added
CI: ready for build
|
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
added
CI: ready for build
fjmolinas
reviewed
Mar 10, 2022
sys/ztimer/Makefile.dep
Outdated
| @@ -84,7 +84,7 @@ endif | |||
| # the counter is read, this propagates and leads to timing errors | |||
| # 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))) | |||
There was a problem hiding this comment.
BTW @dylad what sam% based BOARDs do you have?
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.
There was a problem hiding this comment.
sam3 uses a completely different driver, I wonder if it wasn't caught here by mistake.
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.
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
stale
bot
added
the
State: stale
|
Could you rebase? It would be nice to know if this brings improvement for |
stale
bot
removed
the
State: stale
|
Will do tonight. |
dylad
force-pushed
the
pr/cpu/samd21/rtt_continuous_read
branch
from
1e785f5 to
1abb847
Compare
github-actions
bot
removed
Area: Kconfig
|
Okay I've rebased this PR (and I really hope I didn't f*ck up something). |
|
Ah, Kconfig… |
dylad
force-pushed
the
pr/cpu/samd21/rtt_continuous_read
branch
from
1abb847 to
f35057e
Compare
dylad
force-pushed
the
pr/cpu/samd21/rtt_continuous_read
branch
from
f35057e to
6398c5d
Compare
Signed-off-by: Dylan Laduranty <[email protected]>
|
@benpicco Could you check this PR with your |
|
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 <[email protected]>
dylad
requested review from
smlng,
aabadie and
MichelRottleuthner
as code owners
|
I failed to understand why CI is complaining only for two |
benpicco
added
CI: ready for build
|
How can we move this forward? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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