why does ISR not always seem to run on wake from sleep?

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

why does ISR not always seem to run on wake from sleep?

Britton Kerin
On vanilla Arduino Uno Rev3, the ISR always runs on wake from sleep
the first time through in the loop in the below program, but not
always later.  So eventually after lots of quick pin changes it ends
up awake but with got_pbpci false.  I cannot understand why.

I also attached the assembly below.  I looked at it to verify that
sei/cli don't appear to be getting reordered in some unexpected way.

Any help greatly appreciated.

#include <avr/interrupt.h>
#include <avr/sleep.h>
#include <util/delay.h>

// The wake-up pin is PB2

// For showing when we hit a trap:
#define BLINK_PB5_LED_FOREVER(period_ms) \
  do {                                   \
    for ( ; ; ) {                        \
      PORTB |= _BV (PORTB5);             \
      _delay_ms (period_ms / 2.0);       \
      PORTB &= ~(_BV (PORTB5));          \
      _delay_ms (period_ms / 2.0);       \
    }                                    \
  } while ( 0 );


volatile uint8_t got_pbpci = 0;   // Got Port B Pin Change Interrupt

volatile uint8_t inry = 1;   // ISR Note Run Yet

ISR (PCINT0_vect)
{
  inry = 0;
  got_pbpci = 1;
}

int
main (void)
{
  // Initialize PB5 for output, with low as initial value
  PORTB &= ~(_BV (PORTB5));
  DDRB |= _BV (DDB5);

  // Initialize wake-up pin as an input, with internal pull-up
  DDRB &= ~(_BV (DDB2));
  PORTB |= _BV (PORTB2);

  // Enable pin change interrupts on wake-up pin
  PCICR |= _BV (PCIE0);
  PCMSK0 |= _BV (PCINT2);

  while ( 1 ) {
    set_sleep_mode (SLEEP_MODE_IDLE);
    sleep_enable ();
    sei ();
    sleep_cpu ();
    sleep_disable ();
    cli ();

    if ( inry ) {
      BLINK_PB5_LED_FOREVER (100.42);   // Never seems to happen
    }

    if ( ! got_pbpci ) {
      BLINK_PB5_LED_FOREVER (200.42);   // Always happend eventually
    }
    else {
      got_pbpci = 0;
    }
  }
}

And the assembly:

    .file    "alt_main.c"
__SP_H__ = 0x3e
__SP_L__ = 0x3d
__SREG__ = 0x3f
__tmp_reg__ = 0
__zero_reg__ = 1
 ;  GNU C (GCC) version 4.8.1 (avr)
 ;     compiled by GNU C version 4.9.1, GMP version 6.0.0, MPFR
version 3.1.2-p3, MPC version 1.0.2
 ;  GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
 ;  options passed:  -fpreprocessed alt_main.i -mmcu=atmega328p
 ;  -auxbase-strip alt_main.o -Os -Werror -Wall -Wextra -Winline
 ;  -Wmissing-prototypes -Wredundant-decls -Winit-self -Wstrict-prototypes
 ;  -std=gnu11 -fshort-enums -fverbose-asm
 ;  options enabled:  -faggressive-loop-optimizations -fauto-inc-dec
 ;  -fbranch-count-reg -fcaller-saves -fcombine-stack-adjustments -fcommon
 ;  -fcompare-elim -fcprop-registers -fcrossjumping -fcse-follow-jumps
 ;  -fdefer-pop -fdevirtualize -fdwarf2-cfi-asm -fearly-inlining
 ;  -feliminate-unused-debug-types -fexpensive-optimizations
 ;  -fforward-propagate -ffunction-cse -fgcse -fgcse-lm -fgnu-runtime
 ;  -fguess-branch-probability -fhoist-adjacent-loads -fident
 ;  -fif-conversion -fif-conversion2 -findirect-inlining -finline
 ;  -finline-atomics -finline-functions -finline-functions-called-once
 ;  -finline-small-functions -fipa-cp -fipa-profile -fipa-pure-const
 ;  -fipa-reference -fipa-sra -fira-hoist-pressure -fira-share-save-slots
 ;  -fira-share-spill-slots -fivopts -fkeep-static-consts
 ;  -fleading-underscore -fmath-errno -fmerge-constants
 ;  -fmerge-debug-strings -fmove-loop-invariants -fomit-frame-pointer
 ;  -foptimize-register-move -foptimize-sibling-calls -fpartial-inlining
 ;  -fpeephole -fpeephole2 -fprefetch-loop-arrays -freg-struct-return
 ;  -fregmove -freorder-blocks -freorder-functions -frerun-cse-after-loop
 ;  -fsched-critical-path-heuristic -fsched-dep-count-heuristic
 ;  -fsched-group-heuristic -fsched-interblock -fsched-last-insn-heuristic
 ;  -fsched-rank-heuristic -fsched-spec -fsched-spec-insn-heuristic
 ;  -fsched-stalled-insns-dep -fshow-column -fshrink-wrap -fsigned-zeros
 ;  -fsplit-ivs-in-unroller -fsplit-wide-types -fstrict-aliasing
 ;  -fstrict-overflow -fstrict-volatile-bitfields -fsync-libcalls
 ;  -fthread-jumps -ftoplevel-reorder -ftrapping-math -ftree-bit-ccp
 ;  -ftree-builtin-call-dce -ftree-ccp -ftree-ch -ftree-coalesce-vars
 ;  -ftree-copy-prop -ftree-copyrename -ftree-dce -ftree-dominator-opts
 ;  -ftree-dse -ftree-forwprop -ftree-fre -ftree-loop-if-convert
 ;  -ftree-loop-im -ftree-loop-ivcanon -ftree-loop-optimize
 ;  -ftree-parallelize-loops= -ftree-phiprop -ftree-pre -ftree-pta
 ;  -ftree-reassoc -ftree-scev-cprop -ftree-sink -ftree-slp-vectorize
 ;  -ftree-slsr -ftree-sra -ftree-switch-conversion -ftree-tail-merge
 ;  -ftree-ter -ftree-vect-loop-version -ftree-vrp -funit-at-a-time
 ;  -fverbose-asm -fzero-initialized-in-bss

    .text
.global    __vector_3
    .type    __vector_3, @function
__vector_3:
    push r1     ;
    push r0     ;
    in r0,__SREG__     ; ,
    push r0     ;
    clr __zero_reg__     ;
    push r24     ;
/* prologue: Signal */
/* frame size = 0 */
/* stack size = 4 */
.L__stack_usage = 4
    sts inry,__zero_reg__     ;  inry,
    ldi r24,lo8(1)     ;  tmp42,
    sts got_pbpci,r24     ;  got_pbpci, tmp42
/* epilogue start */
    pop r24     ;
    pop r0     ;
    out __SREG__,r0     ; ,
    pop r0     ;
    pop r1     ;
    reti
    .size    __vector_3, .-__vector_3
    .section    .text.startup,"ax",@progbits
.global    main
    .type    main, @function
main:
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
    cbi 0x5,5     ; ,
    sbi 0x4,5     ; ,
    cbi 0x4,2     ; ,
    sbi 0x5,2     ; ,
    lds r24,104     ;  D.1697, MEM[(volatile uint8_t *)104B]
    ori r24,lo8(1)     ;  D.1697,
    sts 104,r24     ;  MEM[(volatile uint8_t *)104B], D.1697
    lds r24,107     ;  D.1697, MEM[(volatile uint8_t *)107B]
    ori r24,lo8(4)     ;  D.1697,
    sts 107,r24     ;  MEM[(volatile uint8_t *)107B], D.1697
.L7:
    in r24,0x33     ;  D.1697, MEM[(volatile uint8_t *)83B]
    andi r24,lo8(-15)     ;  D.1697,
    out 0x33,r24     ;  MEM[(volatile uint8_t *)83B], D.1697
    in r24,0x33     ;  D.1697, MEM[(volatile uint8_t *)83B]
    ori r24,lo8(1)     ;  D.1697,
    out 0x33,r24     ;  MEM[(volatile uint8_t *)83B], D.1697
/* #APP */
 ;  48 "alt_main.c" 1
    sei
 ;  0 "" 2
 ;  49 "alt_main.c" 1
    sleep

 ;  0 "" 2
/* #NOAPP */
    in r24,0x33     ;  D.1697, MEM[(volatile uint8_t *)83B]
    andi r24,lo8(-2)     ;  D.1697,
    out 0x33,r24     ;  MEM[(volatile uint8_t *)83B], D.1697
/* #APP */
 ;  51 "alt_main.c" 1
    cli
 ;  0 "" 2
/* #NOAPP */
    lds r24,inry     ;  inry.0, inry
    tst r24     ;  inry.0
    breq .L3     ; ,
.L8:
    sbi 0x5,5     ; ,
    ldi r18,lo8(160671)     ; ,
    ldi r24,hi8(160671)     ; ,
    ldi r25,hlo8(160671)     ; ,
    1: subi r18,1     ;
    sbci r24,0     ;
    sbci r25,0     ;
    brne 1b
    rjmp .
    nop
    cbi 0x5,5     ; ,
    ldi r18,lo8(160671)     ; ,
    ldi r24,hi8(160671)     ; ,
    ldi r25,hlo8(160671)     ; ,
    1: subi r18,1     ;
    sbci r24,0     ;
    sbci r25,0     ;
    brne 1b
    rjmp .
    nop
    rjmp .L8     ;
.L3:
    lds r24,got_pbpci     ;  got_pbpci.1, got_pbpci
    cpse r24,__zero_reg__     ;  got_pbpci.1,
    rjmp .L5     ;
.L9:
    sbi 0x5,5     ; ,
    ldi r18,lo8(320671)     ; ,
    ldi r24,hi8(320671)     ; ,
    ldi r25,hlo8(320671)     ; ,
    1: subi r18,1     ;
    sbci r24,0     ;
    sbci r25,0     ;
    brne 1b
    rjmp .
    nop
    cbi 0x5,5     ; ,
    ldi r18,lo8(320671)     ; ,
    ldi r24,hi8(320671)     ; ,
    ldi r25,hlo8(320671)     ; ,
    1: subi r18,1     ;
    sbci r24,0     ;
    sbci r25,0     ;
    brne 1b
    rjmp .
    nop
    rjmp .L9     ;
.L5:
    sts got_pbpci,__zero_reg__     ;  got_pbpci,
    rjmp .L7     ;
    .size    main, .-main
.global    inry
    .data
    .type    inry, @object
    .size    inry, 1
inry:
    .byte    1
.global    got_pbpci
    .section .bss
    .type    got_pbpci, @object
    .size    got_pbpci, 1
got_pbpci:
    .zero    1
    .ident    "GCC: (GNU) 4.8.1"
.global __do_copy_data
.global __do_clear_bss

_______________________________________________
AVR-chat mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avr-chat
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: why does ISR not always seem to run on wake from sleep?

Rolf Pfister-2
Am 08.11.2016 um 01:56 schrieb Britton Kerin:

> #define BLINK_PB5_LED_FOREVER(period_ms) \
>   do {                                   \
>     for ( ; ; ) {                        \
>       PORTB |= _BV (PORTB5);             \
>       _delay_ms (period_ms / 2.0);       \
>       PORTB &= ~(_BV (PORTB5));          \
>       _delay_ms (period_ms / 2.0);       \
>     }                                    \
>   } while ( 0 );
>

This do while dont make any sense, with while(0) it will
run only once. while(1) would do it forever.
Anyway the for(;;) will do the loop for ever.

>
>   while ( 1 ) {
>     set_sleep_mode (SLEEP_MODE_IDLE);
>     sleep_enable ();
>     sei ();
>     sleep_cpu ();
>     sleep_disable ();
>     cli ();
>
>     if ( inry ) {
>       BLINK_PB5_LED_FOREVER (100.42);   // Never seems to happen
>     }
>
>     if ( ! got_pbpci ) {
>       BLINK_PB5_LED_FOREVER (200.42);   // Always happend eventually
>     }
>     else {
>       got_pbpci = 0;
>     }
>   }

I think after reaching BLINK_PB5_LED_FOREVER it will never again
go to sleep mode. And interrupts will also not happen because
of the cli().

Rolf


_______________________________________________
AVR-chat mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avr-chat
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: why does ISR not always seem to run on wake from sleep?

Britton Kerin
On Mon, Nov 7, 2016 at 10:58 PM, Rolf Pfister <[hidden email]> wrote:

> Am 08.11.2016 um 01:56 schrieb Britton Kerin:
>>
>> #define BLINK_PB5_LED_FOREVER(period_ms) \
>>   do {                                   \
>>     for ( ; ; ) {                        \
>>       PORTB |= _BV (PORTB5);             \
>>       _delay_ms (period_ms / 2.0);       \
>>       PORTB &= ~(_BV (PORTB5));          \
>>       _delay_ms (period_ms / 2.0);       \
>>     }                                    \
>>   } while ( 0 );
>>
>
> This do while dont make any sense, with while(0) it will
> run only once. while(1) would do it forever.
> Anyway the for(;;) will do the loop for ever.

Yes, it's just a syntactic habit for functionesque macros,
you're right that the outer do-while is pointless here

>>
>>   while ( 1 ) {
>>     set_sleep_mode (SLEEP_MODE_IDLE);
>>     sleep_enable ();
>>     sei ();
>>     sleep_cpu ();
>>     sleep_disable ();
>>     cli ();
>>
>>     if ( inry ) {
>>       BLINK_PB5_LED_FOREVER (100.42);   // Never seems to happen
>>     }
>>
>>     if ( ! got_pbpci ) {
>>       BLINK_PB5_LED_FOREVER (200.42);   // Always happend eventually
>>     }
>>     else {
>>       got_pbpci = 0;
>>     }
>>   }
>
>
> I think after reaching BLINK_PB5_LED_FOREVER it will never again
> go to sleep mode. And interrupts will also not happen because
> of the cli().

Yes, those are intended as traps to show when we've ended up in
what should be an impossible situation (according to the datasheet).
The program isn't supposed to recover once it gets to them.

The problem is it should never get to them in the first place, but it does.

Having thought about this more I believe the root of the problem is
shown in Fig-1 of the ATMega328P datasheet.  When sleeping the
clock is stopped, and when idle I think clk is shut off to the input
circuit of 17-1, but in either case pin change detection can occur,
so obviously it isn't happening due to that circuit.  So wake up can
occur independently, and the only question is whether that circuit
always triggers an interrupt correctly once the clock is going.  I
don't see how it possibly can, because the state of the input that
drives the XOR cannot be known.  So the datasheet is wrong and
an ISR is not guaranteed in this situation.  It has a caveat for
level-triggered interrupts on wake, and it should have one for
this situation as well.

Britton


>
> Rolf
>
>
> _______________________________________________
> AVR-chat mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/avr-chat

_______________________________________________
AVR-chat mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avr-chat
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: why does ISR not always seem to run on wake from sleep?

Erik Christiansen-2
On 08.11.16 15:55, Britton Kerin wrote:
> Having thought about this more I believe the root of the problem is
> shown in Fig-1 of the ATMega328P datasheet.

Fig 17-1 on p87, I guess.

> When sleeping the clock is stopped, and when idle I think clk is shut
> off to the input circuit of 17-1, but in either case pin change
> detection can occur, so obviously it isn't happening due to that
> circuit.

According to Table 14-1, clkIO is active in Idle mode, and pin change
wake-up is effective if unmasked. OK, Fig 17-1 only shows the clock as
"clk", so the two do not corroborate well.

The first paragraph of the "Interrupts" introduction, above figure 17-1
states:

>>>
Pin change interrupts on PCINT are detected asynchronously.
This implies that these interrupts can be used for waking the part also
from sleep modes other than Idle mode.
<<<

The first sentence confirms the pcint_in_(0) --- (x) pin change
interrupts are taken from the AND gates inputting to the OR gate, i.e. a
pin change would be detected even if the clock were stopped.

There is a 3-clock delay before capture in a PCIFR flag, but IIRC your code
does not rely on PCIFR, and the interrupt has already been generated by
then, AIUI.

Have you tried filling all unused vector table slots with a vector to a
third LED blinker, so that if (inexplicably) another interrupt is
enabled and occasionally waking the system, you'll know? (It is nearly
20 years now since that scenario caused me some debugging effort.)

Erik

_______________________________________________
AVR-chat mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avr-chat
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: why does ISR not always seem to run on wake from sleep?

Britton Kerin
On Wed, Nov 9, 2016 at 12:20 AM, Erik Christiansen
<[hidden email]> wrote:

> On 08.11.16 15:55, Britton Kerin wrote:
>> Having thought about this more I believe the root of the problem is
>> shown in Fig-1 of the ATMega328P datasheet.
>
> Fig 17-1 on p87, I guess.
>
>> When sleeping the clock is stopped, and when idle I think clk is shut
>> off to the input circuit of 17-1, but in either case pin change
>> detection can occur, so obviously it isn't happening due to that
>> circuit.
>
> According to Table 14-1, clkIO is active in Idle mode, and pin change
> wake-up is effective if unmasked. OK, Fig 17-1 only shows the clock as
> "clk", so the two do not corroborate well.

True, but clkIO is not on in e.g. power down mode, so either the wake up
mechanism for the modes is different (which is not documented and seems
highly weird and unlikely), or the wake-up mechanism is independent of
all clocks, because according to section 14.6 power-down "basically halts
all generated clocks, allowing operation of asynchronous modules only".

I posted more details about this problem here:

http://www.avrfreaks.net/forum/atmega328p-datasheet-bug-pin-change-isrs-arent-always-called-wake

[snip]

> Have you tried filling all unused vector table slots with a vector to a
> third LED blinker, so that if (inexplicably) another interrupt is
> enabled and occasionally waking the system, you'll know? (It is nearly
> 20 years now since that scenario caused me some debugging effort.)

Great idea, I'll try this.

Britton

_______________________________________________
AVR-chat mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avr-chat
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: why does ISR not always seem to run on wake from sleep?

Rogier Wolff
In reply to this post by Britton Kerin
On Tue, Nov 08, 2016 at 03:55:52PM -0900, Britton Kerin wrote:

> On Mon, Nov 7, 2016 at 10:58 PM, Rolf Pfister <[hidden email]> wrote:
> > Am 08.11.2016 um 01:56 schrieb Britton Kerin:
> >>
> >> #define BLINK_PB5_LED_FOREVER(period_ms) \
> >>   do {                                   \
> >>     for ( ; ; ) {                        \
> >>       PORTB |= _BV (PORTB5);             \
> >>       _delay_ms (period_ms / 2.0);       \
> >>       PORTB &= ~(_BV (PORTB5));          \
> >>       _delay_ms (period_ms / 2.0);       \
> >>     }                                    \
> >>   } while ( 0 );
> >>
> >
> > This do while dont make any sense, with while(0) it will
> > run only once. while(1) would do it forever.
> > Anyway the for(;;) will do the loop for ever.
>
> Yes, it's just a syntactic habit for functionesque macros,
> you're right that the outer do-while is pointless here

The "syntactic habit" as done here is normally a good idea:
with the do { ... } while (0) you can make a multi-statement
macro behave exactly like you'd expect from a single statement.

Consider:

// On <somemachine> we need to call flush after each debug print.
#define dprint_int(str, i) \
    printf ("%s: %d.\n", str, i); \
    fflush (stdout);

This will have funny effects if you do:

    if (a > 10) // Odd, but not critical yet.
        dprint_int("odd, a > 10", a);

which will suddenly call the "fflush" all the time, even when a <= 10.
(As the flush does "almost nothing", it won't be that bad in my
"quick" example, but if for example the print happens to be the second
instruction you'd have unexpected results..... )

With the "do { ... } while (0)" around the two statements, the
statements will be executed once as intended, and "wrap" correctly
around the "if" statement.

As far as I know, this trick does not allow your semicolon after the
while (0). Maybe often it IS allowed, but in some corner case, you'd
get a syntax error for the double semicolon.Ah:
  for (something_that_is_now_a_define(); i < 10;i))
     somethingelse ();

That's where an extra semicolon wouldn't work.

        Roger.

--
** [hidden email] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
**    Delftechpark 26 2628 XH  Delft, The Netherlands. KVK: 27239233    **
*-- BitWizard writes Linux device drivers for any device you may have! --*
The plan was simple, like my brother-in-law Phil. But unlike
Phil, this plan just might work.

_______________________________________________
AVR-chat mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avr-chat
Loading...