test/xtimer_drift: DEBUG_PINS by Josar · Pull Request #10173 · RIOT-OS/RIOT

Josar · 2018-10-16T21:39:17Z

This PR adds debug pins to probe this test with an oscilloscope.
This helps debugging #9595

Before the merge of #9211 (comment) we can observe the hanging behaviour of the system.

After #9211 (comment) is merged a pariodic a race condition appears which can be seen in the prints of the test #9596.

When #9595 is merged this will result in an stable System.

PeterKietzmann · 2018-10-17T07:06:15Z

@leandrolanzieri is this helpful? Mind to have a look?

Josar · 2018-11-01T12:28:48Z

Set generic pin definition as in #10174

PeterKietzmann · 2018-11-05T10:36:20Z

@MichelRottleuthner, @leandrolanzieri you took care of #10174, how about this PR?

A-Paul

Code looks good to me. Just one location where we could save some space.

A-Paul · 2018-11-08T18:36:16Z

tests/xtimer_drift/Makefile

+#CFLAGS += -DDEBUG_TIMER_PORT=PORTF
+#CFLAGS += -DDEBUG_TIMER_DDR=DDRF
+#CFLAGS += -DDEBUG_TIMER_PIN=PORTF4
+


Lines 8-14: This description is already in <cpu/atmega_common/periph/timer.c> since #9025.
Just place a reference, please.

Then you will have to copy paste them from there everytime you want to use this debug pins.
As i understand the tests also as testing tools, having such info in place is imho more helpfull then the space saved.

@A-Paul A-Paul i am willing to remove the comments if this is the showstopper here.

This test should be independent of the atmega implementation. Ideally, we would have a generic debug pin for this purpose configured for every platform. Now that this is out of scope for your PR, I'd say we remove it.

Josar · 2019-03-17T08:36:51Z

@A-Paul can we continue here?

Josar · 2019-04-17T21:13:46Z

@A-Paul @PeterKietzmann @kYc0o is it possible to get this merged as this is a part of the xtimer improvement i started a long time ago.

I somehow get the feeling that there is no interrest from the maintainers to get xtimer working proprerly.

Now old and known issues are coming up again #11149.

kYc0o · 2019-04-18T12:06:23Z

So far the only thing missing is the testing part. Unfortunately I don't have the tools anymore to proceed with a good testing here, however I also think that this PR has waited too much and would be beneficial to merge it.

@A-Paul would you have time for the testing?

PeterKietzmann

@Josar I ran the application on a samr21-xpro with

CFLAGS += -DSLACKER_THREAD_PIN=GPIO_PIN\(0,22\)
CFLAGS += -DMAIN_THREAD_PIN=GPIO_PIN\(1,3\)
CFLAGS += -DPRINT_THREAD_PIN=GPIO_PIN\(1,22\)

Results look slightly different in comparison to yours, but they seem reasonable to me.

In a nutshell:

print occurs every second
main occurs every ~15.6ms and takes a little longer when print happens around the same time
slacker occurs in (non constant) intervals between 1...10ms

PeterKietzmann · 2019-05-21T12:11:38Z

tests/xtimer_drift/Makefile

+#CFLAGS += -DDEBUG_TIMER_PORT=PORTF
+#CFLAGS += -DDEBUG_TIMER_DDR=DDRF
+#CFLAGS += -DDEBUG_TIMER_PIN=PORTF4
+


This test should be independent of the atmega implementation. Ideally, we would have a generic debug pin for this purpose configured for every platform. Now that this is out of scope for your PR, I'd say we remove it.

PeterKietzmann · 2019-05-21T12:17:01Z

tests/xtimer_drift/Makefile

+
+# Define test probing pins GPIO API based.
+# Port number should be found in port enum e.g in cpu/include/periph_cpu.h
+#FEATURES_REQUIRED += periph_gpio


Is there a single platform that has no GPIO driver but xtimer support? In my opinion it should be fine to always include that feature (otherwise we would see what Murdoch says later)

PeterKietzmann · 2019-05-21T12:18:04Z

tests/xtimer_drift/Makefile

+# Define test probing pins GPIO API based.
+# Port number should be found in port enum e.g in cpu/include/periph_cpu.h
+#FEATURES_REQUIRED += periph_gpio
+# Jiminy probing Pins


If you really wanna have the jiminy config here, please define the CFLAGS in

ifneq (,$(filter jiminy,$(BOARD))) endif

PeterKietzmann · 2019-05-21T12:18:54Z

tests/xtimer_drift/main.c


+#if defined(SLACKER_THREAD_PIN)
+    gpio_t slacker_pin = SLACKER_THREAD_PIN;
+    gpio_init(slacker_pin, GPIO_OUT);


Why not directly gpio_init(SLACKER_THREAD_PIN, GPIO_OUT); here and following?

PeterKietzmann · 2019-05-21T12:19:39Z

tests/xtimer_drift/main.c

-
+#if defined(SLACKER_THREAD_PIN)
+        gpio_set(slacker_pin);
+        gpio_clear(slacker_pin);


Probably a matter of taste, but wouldn't a single toggle be simpler and easier to read on the scope?

Josar · 2019-05-22T14:59:23Z

@PeterKietzmann i think its a 32 bit timer which is used for xtimer, waiting for that to overflow takes some time. Maybe you could configure it to use a 16bit timer.

PeterKietzmann · 2019-05-22T15:30:48Z

waiting for that to overflow takes some time

I wasn't waiting for an overflow :-). Are you saying this because I mentioned that my results look slightly different to yours? I'm fine with the concept of this PR and IMO it only needs some small polish in order to be merged
Edit: With "polish" I meant addressing my comments

stale · 2019-11-23T16:15:54Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

stale · 2020-10-11T11:01:19Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

test/xtimer_drift: DEBUG_PINS

e50ab80

PeterKietzmann added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tests Area: tests and testing framework Area: timers Area: timer subsystems labels Oct 17, 2018

PeterKietzmann requested a review from kYc0o October 17, 2018 07:06

Josar added 2 commits October 18, 2018 14:11

fixup! test/xtimer_drift: DEBUG_PINS

d402d7a

fixup! fixup! test/xtimer_drift: DEBUG_PINS

54f84d3

A-Paul suggested changes Nov 8, 2018

View reviewed changes

Josar mentioned this pull request Dec 18, 2018

Improve and debug Xtimer #10393

Closed

PeterKietzmann reviewed May 21, 2019

View reviewed changes

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Nov 23, 2019

MichelRottleuthner removed the State: stale State: The issue / PR has no activity for >185 days label Nov 27, 2019

PeterKietzmann requested a review from MichelRottleuthner April 9, 2020 08:20

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Oct 11, 2020

stale bot closed this Nov 11, 2020

Conversation

Josar commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterKietzmann commented Oct 17, 2018

Uh oh!

Josar commented Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterKietzmann commented Nov 5, 2018

Uh oh!

A-Paul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Josar commented Mar 17, 2019

Uh oh!

Josar commented Apr 17, 2019

Uh oh!

kYc0o commented Apr 18, 2019

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Josar commented May 22, 2019

Uh oh!

PeterKietzmann commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Nov 23, 2019

Uh oh!

stale bot commented Oct 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Josar commented Oct 16, 2018 •

edited

Loading

Josar commented Nov 1, 2018 •

edited

Loading

PeterKietzmann commented May 22, 2019 •

edited

Loading