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

test/xtimer hang: separate debug pins #10397

Merged
merged 1 commit into from Mar 18, 2019
Merged

test/xtimer hang: separate debug pins #10397

merged 1 commit into from Mar 18, 2019

Conversation

Josar
Copy link
Contributor

@Josar Josar commented Nov 15, 2018

Previously one pin was used for both slacker threads.
For better debugging seperating the threads to use one pin each is better.

@Josar Josar force-pushed the pr/test/xtimer_hang/separate_debug_pin branch 2 times, most recently from 2264f71 to 468b5a7 Compare November 15, 2018 16:34
@A-Paul A-Paul 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 Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Nov 16, 2018
A-Paul
A-Paul reviewed Nov 16, 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, some questions came up while reading your code, and trying to understand it. :)

tests/xtimer_hang/main.c Outdated
#if defined(WORKER_THREAD_PIN)
gpio_t worker_pin = WORKER_THREAD_PIN;
gpio_init(worker_pin, GPIO_OUT);
#if defined(WORKER_THREAD_PIN_1)
Copy link
Member

Choose a reason for hiding this comment

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

You are using the pins only when both are defined (see line 60). Why don't you require the same condition for initialization?
Also, each gpio is initialized twice.

tests/xtimer_hang/main.c Outdated
gpio_t worker_pin_1 = WORKER_THREAD_PIN_1;
gpio_init(worker_pin_1, GPIO_OUT);
#endif
#if defined(WORKER_THREAD_PIN_2)
Copy link
Member

Choose a reason for hiding this comment

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

Same as line 49

tests/xtimer_hang/main.c
@@ -65,8 +79,8 @@ int main(void)
#endif

LOG_DEBUG("[INIT]\n");
uint32_t sleep_timer1 = 1000;
uint32_t sleep_timer2 = 1100;
uint32_t sleep_timer1 = TEST_SLEEP_TIME_1;
Copy link
Member

Choose a reason for hiding this comment

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

Thumbs up for eliminating literal numbers. :-)

tests/xtimer_hang/main.c Outdated

while (1) {
#if defined(WORKER_THREAD_PIN_1) && defined(WORKER_THREAD_PIN_2)
if (*(uint32_t *)(arg) == TEST_SLEEP_TIME_1) {
Copy link
Member

Choose a reason for hiding this comment

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

I think a switch case might be a cleaner approach here.

@A-Paul
Copy link
Member

A-Paul commented Nov 16, 2018

@Josar, look at this Patch as a suggestion.

@Josar Josar force-pushed the pr/test/xtimer_hang/separate_debug_pin branch from fc59214 to f363896 Compare November 17, 2018 09:55
@Josar
Copy link
Contributor Author

Josar commented Nov 17, 2018

Nice Idea, but does not work. The case has to be in the while. As we overwrite the pin with the second use of the function.

grafik

@Josar Josar force-pushed the pr/test/xtimer_hang/separate_debug_pin branch from f363896 to 43cfc1d Compare November 18, 2018 12:28
@Josar
Copy link
Contributor Author

Josar commented Nov 27, 2018 •
edited

@A-Paul i pushed an updated Version.

@Josar
Copy link
Contributor Author

Josar commented Dec 10, 2018

@A-Paul any time to have a second look?

@Josar Josar mentioned this pull request Dec 18, 2018
Closed
@Josar
Copy link
Contributor Author

Josar commented Jan 30, 2019 •
edited

@A-Paul still no Time? @haukepetersen maybe you?

A-Paul
A-Paul reviewed Feb 26, 2019
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. Happy new year and sorry for you shouting in the desert.
I have looked into your changes give a "pre approval" now. I will do some testing with a scope on HW the next days.

@A-Paul A-Paul added 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 Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Mar 15, 2019
A-Paul
A-Paul approved these changes Mar 15, 2019
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.

Running test on arduino-mega2560 and samd21-xpro with a scope was successful.
Merging when Murdock is O.K.

@A-Paul
Copy link
Member

A-Paul commented Mar 15, 2019

Hi @Josar, please squash.

@Josar Josar force-pushed the pr/test/xtimer_hang/separate_debug_pin branch from 43cfc1d to 9627d65 Compare March 17, 2019 08:13
@Josar
Copy link
Contributor Author

Josar commented Mar 17, 2019

@A-Paul sqaushed and rebased.

@Josar
test/xtimer_hang: separate debug pins
254d25f 
Previously one pin was used for both slacker threads.
For better debugging seperating the threads to use one pin each is better.
@Josar Josar force-pushed the pr/test/xtimer_hang/separate_debug_pin branch from 9627d65 to 254d25f Compare March 17, 2019 08:21
@A-Paul A-Paul added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 17, 2019
@A-Paul A-Paul merged commit fbac2d4 into RIOT-OS:master Mar 18, 2019
@danpetry danpetry added this to the Release 2019.04 milestone Mar 26, 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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer 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 Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2019.04
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Josar @A-Paul @danpetry