5

I stumbled upon this piece in digitalWrite. I'm unsure why clearing interrupts and setting/resetting SREG is neccesary here.

Can anyone shed a light on this?

uint8_t oldSREG = SREG;
cli();

if (val == LOW) {
    *out &= ~bit;
} else {
    *out |= bit;
}

SREG = oldSREG;

I figure they don't want an interrupt to possibly change any of the bits in the output register while it executes *out = *out & ~bit?

And what would the SREG have to do with it? Could the code affect SREG and why would that matter?

When speed matters, I often use the direct port manipulation, I'd like to get an idea if it's neccesary for me to include these "precautions".

aaa
  • 2,695
  • 2
  • 24
  • 40
  • If you want to do this kind of interrupt protection yourself, look at simply using ATOMIC_BLOCK from #include <util/atomic.h> http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html – BrettFolkins Jun 05 '16 at 20:30
  • @BrettAM yup, figured that one out as well (see latest comment on Majenko's answer). Though it looks fancier (and more logical) it won't make a difference runtime? – aaa Jun 06 '16 at 11:08

2 Answers2

5

I figure they don't want an interrupt to possibly change any of the bits in the output register while it executes *out = *out & ~bit?

Correct. That is known as a critical section.

And what would the SREG have to do with it? Could the code affect SREG and why would that matter?

SREG is where the flag that says if interrupts are enable or not is stored. Basically that is storing the current state, disabling interrupts, then restoring the current state.

The upshot is:

  • If interrupts are enabled to begin with, they are disabled, then re-enabled.
  • If interrupts are disabled to begin with, they are again disabled, and they remain disabled.

It's a way of restoring the previous state without actually having to check what the state was and set or clear accordingly.

Majenko
  • 105,095
  • 5
  • 79
  • 137
  • Ah! At first it didn't make sense, I thought they could've used 'cli()' and 'sei()'. Since I was thinking of an "interrupts always on scenario". The fun thing is, when interrupts is off, you want them to be off after that piece of code as well. And on when it was on (: – aaa Jun 05 '16 at 20:12
  • What happend when one doesn't use interrupt disabling on that critical part? It may cause the wrong output state when this pin is also changed within an interrupt? – aaa Jun 05 '16 at 20:19
  • It may be that the pin you want to change in your ISR (sometimes) doesn't get changed. Or it could be that the pin you want to change outside the ISR may (sometimes) not get changed. They don't need to be the same pin - just in the same register. – Majenko Jun 05 '16 at 20:21
  • Anything that does a read-modify-write that can also happen in a different context (i.e., in an interrupt) should be protected so that one RMW doesn't happen mid-way through the other RMW. – Majenko Jun 05 '16 at 20:23
  • Ah, got it, the interrupt may editie a pin in the same register, just after reading and before writing. After that it writes, so the interrupt's effect is cancelled – aaa Jun 05 '16 at 20:24
  • 1
    Yep. Spot on. The only times you don't need a critical section are for atomic operations - i.e., things that cannot be interrupted since they are only one machine "op code". Note I don't use "assembly instruction" there, since some assemblers use macro instructions that get assembled into multiple op-codes and thus aren't atomic. – Majenko Jun 05 '16 at 20:25
  • I've indeed heard about atomic blick http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html and read-modify-write cycles. Coudn't directly spot that it was applied in this situation (: – aaa Jun 05 '16 at 20:30
2

Answering only the last question, as the rest has been addressed by Majenko.

I often use the direct port manipulation, I'd like to get an idea if it's neccesary for me to include these "precautions".

If you use constant values, like PORTB |= _BV(2); or the like, there is no need, since this compiles into a single CPU instruction. Even if you write things that take multiple instructions, like PORTB |= _BV(2) | _BV(3);, you probably know whether you are modifying the same register inside an interrupt handler or not. If not, no need for critical section either. The authors of the Arduino core cannot make any assumptions.

Edgar Bonet
  • 43,033
  • 4
  • 38
  • 76
  • Good one. I should check how many instructions some of these take. Instead of PORTB |= _BV(2) | _BV(3); you could use PORTB |= 0b00000110 so it's one instruction again?. But well, it will depend on compiler and settings if that does anything different. – aaa Jun 05 '16 at 20:47
  • 1
    No, that's not one instruction - that's a read-modify-write. It compiles to in r24, 0x05; ori r24 0x06; out 0x05, r24; - i,.e, read the port into register 24, OR the new value with it, then write the new combined value back out to the port. The only reason the other method for a single bit is a single instruction is because of the SBI instruction, (set bit in I/O register), such as sbi 0x05, 1; for PORTB |= 0b0000010; – Majenko Jun 05 '16 at 23:31
  • 1
    Using two separate |= would be two atomic instructions that would be fine: PORTB |= 0b00000010; PORTB |= 0b00000100; = sbi 0x05,1; sbi 0x05, 2; - it's less efficient though - each sbi is 2 clocks, but in, out and ori are one clock each, so 4 clocks vs 3 clocks. – Majenko Jun 06 '16 at 09:44