atmega timer: Interrupt Pin by Josar · Pull Request #9025 · RIOT-OS/RIOT

Josar · 2018-04-25T15:21:42Z

Debug pin can be used to probe timer interrupts. Thus, determine when an interrupt occurs and how long the ISR takes.

The timer interrupt times can be probed with a oscilloscope if pin DEBUG_TIMER_PIN, the respective DEBUG_TIMER_PORT and DDEBUG_TIMER_DDR are defined in the makefile as follows.

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

Josar · 2018-05-07T10:45:52Z

@aabadie could you have a look?

A-Paul

Hi @Josar. I'm not @aabadie, but I try to be somewhat usefull. :)
One question turned up while reading your code.

A-Paul · 2018-06-18T03:52:54Z

cpu/atmega_common/periph/timer.c

 {
+#if defined(DEBUG_TIMER_PORT)
+    DEBUG_TIMER_PORT |= (1 << DEBUG_TIMER_PIN);
+    DEBUG_TIMER_PORT &= ~(1 << DEBUG_TIMER_PIN);


You you stated in the initial comment to measure the duration of the ISR. I'm imagining a pulse that is high until the CPU has left the ISR. Shouldn't this line be placed after __exit_isr() to achieve that?

@A-Paul thank you for that. Your asumption is correct and it was implemented like this at the beginning.

As the context swap of the atmegas was changed to also execute in interrupts setting the pin to low would now happen after the context comes back the context which was active when the interrupt was triggered so it would not executed in the right time. I changed the debug pin to only signilizes the start of the timer interrupt. And forgott to update the documentation.

I change the pin toggle to be executed before the context swap, thus the documentation fits the function, only that the context swap is not included in the high period anymore.

Josar · 2018-07-03T16:02:51Z

Squash and rebase?

A-Paul

Hi @Josar, there is somthing i missed in my first review.

A-Paul · 2018-07-05T02:45:37Z

cpu/atmega_common/periph/timer.c

+ * CFLAGS += -DDEBUG_TIMER_DDR=DDRF
+ * CFLAGS += -DDEBUG_TIMER_PIN=PORTF4
+ */
+#if defined(DEBUG_TIMER_PORT)


"board.h" is already included in line 25.

PeterKietzmann · 2018-07-05T07:11:03Z

@Josar please rebase. @A-Paul comments addressed, mind to approve?

Josar · 2018-07-05T07:52:53Z

@PeterKietzmann done

A-Paul · 2018-07-10T14:58:24Z

I wiil do a HW test tomorrow.

A-Paul · 2018-07-12T18:20:49Z

Hi @Josar , I have built and run some of the tests/xtimer_* on a arduino-mega2560 and connected to an oscilloscope. Works like it is supposed.
However, I have gotten a little concerned, because this is a very particular approach for a functionality that might be helpful on a more general level.
@PeterKietzmann mentioned a PR (#7352) that has a similar direction. Do you mind having a look at it and maybe combine your efforts with @haukepetersen?

Josar · 2018-07-12T20:29:00Z

@A-Paul i had a quick view over the approach from #7352.

And yes it seems more generalized, but it is also on halt for quite a while.

One other thing is that if it is used like this it is not flexible anymore. Means if i use e.g. DBGPIN1 for the timer and in a seperate debug atempt also use it for e.g. context swap then only one or both can be enabled at the same time on the same pin. I can not choose the pin seperatly anymore.

This Approach defines a well defined PIN for a single action e.g. Timer, This can be freely set to a pin.
The same could then be done for context swap with e.g -DDEBUG_CONTEXT_SWAP_PIN=PORTF5 and also the pin can be choosen freely.

I am not in favor of the generalized approach shown in #7352 for a low level driver debug pin solution.

The generelized approach could be something which could be used in application level as shown here https://github.com/RIOT-OS/RIOT/pull/8865/files.
This would increase pin toggle speed and would benefit from generalization as each platform could use the same test and configure the debug pin at the debugpin_cpu.h

A-Paul · 2018-07-18T15:37:39Z

@Josar thank you for you investigation. I will follow you reasoning and merge, but I have a last change request. I'll place it in a review comment.

A-Paul

The last code change left the config explanation (lines 35-47) standing alone. Putting it above the initialization as a replacement of line 99 would be more sensible.
It also requires a lot space and has some redundant infos.
I propose a more dense text as follows:

/*
 * A debug pin can be used to probe timer interrupts with an oscilloscope or
 * other time measurement equipment. Thus, determine when an interrupt occurs
 * and how long the ISR takes.
 * The pin should be defined in the makefile as follows:
 * CFLAGS += -DDEBUG_TIMER_PORT=PORTF -DDEBUG_TIMER_DDR=DDRF \
 *           -DDEBUG_TIMER_PIN=PORTF4
 */

A debug pin can be used to probe timer interrupts with an oscilloscope or other time measurement equipment. Thus, determine when an interrupt occurs and how long the timer ISR takes. The pin should be defined in the makefile as follows: CFLAGS += -DDEBUG_TIMER_PORT=PORTF -DDEBUG_TIMER_DDR=DDRF \ -DDEBUG_TIMER_PIN=PORTF4

Josar · 2018-07-18T18:49:18Z

@A-Paul done.

A-Paul · 2018-07-19T16:34:26Z

@Josar, thank you for addressing all change requests. CI is happy. ACK and go.

Josar force-pushed the pr/timer_debug_pin branch from 355453e to dc507e8 Compare May 9, 2018 12:48

Josar mentioned this pull request May 15, 2018

xtimer_core: reduce idle time #8990

Closed

Josar mentioned this pull request May 28, 2018

xtimer: timer & target overflow, hang resolved. #9211

Merged

A-Paul reviewed Jun 18, 2018

View reviewed changes

A-Paul added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: AVR Platform: This PR/issue effects AVR-based platforms labels Jun 18, 2018

A-Paul suggested changes Jul 5, 2018

View reviewed changes

Josar force-pushed the pr/timer_debug_pin branch from ac676db to 2ae2100 Compare July 5, 2018 07:47

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 Jul 10, 2018

A-Paul suggested changes Jul 18, 2018

View reviewed changes

Josar force-pushed the pr/timer_debug_pin branch from 2ae2100 to d6c0398 Compare July 18, 2018 18:48

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 19, 2018

A-Paul approved these changes Jul 19, 2018

View reviewed changes

A-Paul self-assigned this Jul 19, 2018

A-Paul merged commit bf3ce68 into RIOT-OS:master Jul 19, 2018

A-Paul mentioned this pull request Nov 8, 2018

test/xtimer_drift: DEBUG_PINS #10173

Closed

Conversation

Josar commented Apr 25, 2018

Uh oh!

Josar commented May 7, 2018

Uh oh!

A-Paul left a comment

Choose a reason for hiding this comment

Uh oh!

A-Paul Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Josar commented Jul 3, 2018

Uh oh!

A-Paul left a comment

Choose a reason for hiding this comment

Uh oh!

A-Paul Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Jul 5, 2018

Uh oh!

Josar commented Jul 5, 2018

Uh oh!

A-Paul commented Jul 10, 2018

Uh oh!

A-Paul commented Jul 12, 2018

Uh oh!

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

Uh oh!

Uh oh!

A-Paul commented Jul 18, 2018

Uh oh!

A-Paul left a comment

Choose a reason for hiding this comment

Uh oh!

Josar commented Jul 18, 2018

Uh oh!

A-Paul commented Jul 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Josar Jun 18, 2018 •

edited

Loading

Josar commented Jul 12, 2018 •

edited

Loading