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.

Sign up for GitHub

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

Jump to bottom

tests/xtimer_drift: added test #9596

Closed
wants to merge 1 commit into from
Closed

tests/xtimer_drift: added test #9596

wants to merge 1 commit into from

Conversation

Josar
Copy link
Contributor

@Josar Josar commented Jul 18, 2018

Added a test to be able to investigate problems like stated in #5103.

Problems arising with this PR can be solved with
#9211
#9595

This PRs have to be merged before this problem is solved.

Additional #8990 can be applied.

This test compares the timing between received messages with the host clock and prints when an error occured.

2018-07-18 13:41:16,388 - INFO # main(): This is RIOT! (Version: 2018.10)
2018-07-18 13:41:16,389 - INFO # [START]
2018-07-18 13:41:16,389 - INFO # 
2018-07-18 13:41:17,394 - INFO # now=1.046264 (0x0001fedf ticks), drift=552 us, jitter=552 us
2018-07-18 13:41:19,967 - INFO # now=2.045584 (0x0003e6d2 ticks), drift=-128 us, jitter=-680 us
2.045584: Invalid timebetween messages, expected 0.9 < 1.9108633995056152 < 1.1
2018-07-18 13:41:23,067 - INFO # now=3.045520 (0x0005cf12 ticks), drift=-192 us, jitter=-64 us
3.04552: Invalid timebetween messages, expected 0.9 < 3.1008362770080566 < 1.1
2018-07-18 13:41:25,642 - INFO # now=4.045456 (0x0007b752 ticks), drift=-256 us, jitter=-64 us
4.045456: Invalid timebetween messages, expected 0.9 < 2.5776400566101074 < 1.1
2018-07-18 13:41:27,164 - INFO # now=5.045392 (0x00099f92 ticks), drift=-320 us, jitter=-64 us
5.045392: Invalid timebetween messages, expected 0.9 < 1.517474889755249 < 1.1
2018-07-18 13:41:29,741 - INFO # now=6.045328 (0x000b87d2 ticks), drift=-384 us, jitter=-64 us
6.045328: Invalid timebetween messages, expected 0.9 < 2.6143975257873535 < 1.1
2018-07-18 13:41:31,791 - INFO # now=7.045264 (0x000d7012 ticks), drift=-448 us, jitter=-64 us

This was referenced Jul 18, 2018
Closed
Merged
@A-Paul A-Paul added the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 19, 2018
A-Paul
A-Paul suggested changes Jul 20, 2018
View reviewed changes
Copy link
Member

@A-Paul A-Paul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Josar, here is a formatting issue and some head scratchers.

tests/xtimer_drift/Makefile
USEMODULE += xtimer

include $(RIOTBASE)/Makefile.include

test:
tests/01-run.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline

tests/xtimer_drift/main.c Outdated
@@ -109,6 +111,8 @@ void *worker_thread(void *arg)
last = start;
}
else if ((loop_counter % TEST_HZ) == 0) {
PORTF |= (1 << 5);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my understanding this is specific for a avr MCUs.

tests/xtimer_drift/main.c Outdated
@@ -109,6 +111,8 @@ void *worker_thread(void *arg)
last = start;
}
else if ((loop_counter % TEST_HZ) == 0) {
PORTF |= (1 << 5);
PORTF &= ~(1 << 5);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

tests/xtimer_drift/main.c Outdated
@@ -127,6 +131,11 @@ void *worker_thread(void *arg)

int main(void)
{
gpio_init(GPIO_PIN(PORT_F, 4), GPIO_OUT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not obvious to me where you are using this, and the following three ports.

@Josar Josar force-pushed the pr/test/xtimer_drift branch 3 times, most recently from 64d8322 to a8b0c83 Compare July 21, 2018 10:22
@Josar Josar mentioned this pull request Jul 24, 2018
Merged
@Josar
Copy link
Contributor Author

Josar commented Jul 25, 2018

@A-Paul could i resolve the headscratches?

Here i would have loved to have the debug pin solution, btw.

A-Paul
A-Paul suggested changes Aug 1, 2018
View reviewed changes
Copy link
Member

@A-Paul A-Paul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Josar, thanks for addressing the last comments.
After successful build I found another issue. "tests/01-run.py" has no executable bit set (permissions 644 instead of 755) after checking out. So, make test end up with an error.

@Josar
Copy link
Contributor Author

Josar commented Aug 1, 2018

@A-Paul not on my PC. Tried to change the file pemission. Please try one more time.

A-Paul
A-Paul approved these changes Aug 2, 2018
View reviewed changes
Copy link
Member

@A-Paul A-Paul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the last change make test is running now.

@A-Paul not on my PC. ...

If your system is executing text files regardless of the x flag, you better fix that. For your own safety.

@Josar
Copy link
Contributor Author

Josar commented Aug 3, 2018

File permissions is as expected and correct on my PC. But git was not set to inherit the file permissions when the file is checked out.

@A-Paul A-Paul added the Area: tests Area: tests and testing framework label Aug 3, 2018
@Josar
Copy link
Contributor Author

Josar commented Aug 20, 2018

@A-Paul further remarks?

@A-Paul
Copy link
Member

A-Paul commented Aug 20, 2018

@Josar

@A-Paul further remarks?

No, just waiting for the other PRs.

@miri64
Copy link
Member

miri64 commented Aug 24, 2018

Which PR is this based on?

@Josar
Copy link
Contributor Author

Josar commented Aug 24, 2018

Imho none. Only that there could be some errors getting visible which are solve by the two mentioned PRs in the first post.

Or it needs a TEST_ON_CI_WHITELIST += none ?

@miri64
Copy link
Member

miri64 commented Aug 24, 2018

Imho none. Only that there could be some errors getting visible which are solve by the two mentioned PRs in the first post.

Then it is depending on those PRs.

@Josar
Copy link
Contributor Author

Josar commented Aug 24, 2018

This somehow feels like if we can not test for an error there is no error.

@miri64
Copy link
Member

miri64 commented Aug 24, 2018

No, but deactivating the tests so we can fix the errors first, seems like the wrong way around to do it. So yes, this PR is good to demonstrate there is an issue, so that the fixes you did in the related PRs are testable. But we should only merge it after those fixes are in, so we don't introduce other errors because the test is deactivated (because people might trust on it being active for their coding) ;-).

@Josar Josar force-pushed the pr/test/xtimer_drift branch from 26f7e68 to 02a8580 Compare August 28, 2018 20:28
@Josar Josar mentioned this pull request Oct 18, 2018
Closed
@A-Paul A-Paul added Area: timers Area: timer subsystems Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines labels Nov 16, 2018
@A-Paul A-Paul added Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Nov 16, 2018
@stale
Copy link

stale bot commented Aug 10, 2019

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 stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@stale stale bot closed this Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@A-Paul A-Paul A-Paul approved these changes

Assignees
No one assigned
Labels
Area: tests Area: tests and testing framework Area: timers Area: timer subsystems Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines State: stale State: The issue / PR has no activity for >185 days State: waiting for other PR State: The PR requires another PR to be merged first
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Josar @A-Paul @miri64