Are CoreTimer services "atomic"?

Created Sun, 12 Feb 2017 23:47:54 +0000 by rasmadrak

rasmadrak

Sun, 12 Feb 2017 23:47:54 +0000

Quick question - my Google-Fu is lacking and my coffee is running out:

Since it's not true multitasking in the Pic32... If I write to a volatile uint16_t variable in the main loop and read it from inside a callback-core service; Is there any risk of it ever being half written? I.e can the interrupt trigger while the main loop is "creating/writing" the variable or does the writing/current instruction complete before the interrupt is allowed to trigger?

I want to make sure that the main loop always finishes the new value for the variable before the core timer service uses it. Hope I'm not talking nonsense here. :ugeek:

majenko

Mon, 13 Feb 2017 11:54:25 +0000

No, you're not talking nonsense ;)

The only real way of knowing what is going on is to look at the assembly produced.

For instance, the following bit of code:

volatile uint16_t shared = 0;

void setup() {
    attachInterrupt(0, myISR, FALLING);
    shared = 6;
}

void loop() {}

void __USER_ISR myISR() {
    shared = shared + 4;
}

produces, amongst other things, this in the setup function:

9d002a1c:	24020006 	li	v0,6
9d002a20:	a78280e0 	sh	v0,-32544(gp)

That is, load the immediate value 6 into register $v0, then store the value in register $v0 into the upper 16 bits of the memory 32544 bytes down from the Global Pointer $gp.

Every assembly instruction is atomic, but an interrupt can trigger between any two assembly instructions. So if the ISR should clobber $v0 and not save / restore the value stored in shared could be something other than 6.

So let's look at the ISR:

9d002694 <_Z5myISRv>:
9d002694:	415de800 	rdpgpr	sp,sp
9d002698:	401b7000 	mfc0	k1,c0_epc
9d00269c:	401a6002 	mfc0	k0,c0_srsctl
9d0026a0:	27bdffe8 	addiu	sp,sp,-24
9d0026a4:	afbb0014 	sw	k1,20(sp)
9d0026a8:	401b6000 	mfc0	k1,c0_status
9d0026ac:	afba0010 	sw	k0,16(sp)
9d0026b0:	401a6800 	mfc0	k0,c0_cause
9d0026b4:	afbb000c 	sw	k1,12(sp)
9d0026b8:	001ad282 	srl	k0,k0,0xa
9d0026bc:	7f5b7a84 	ins	k1,k0,0xa,0x6
9d0026c0:	7c1b2044 	ins	k1,zero,0x1,0x4
9d0026c4:	409b6000 	mtc0	k1,c0_status
9d0026c8:	afa30004 	sw	v1,4(sp)
9d0026cc:	afa20000 	sw	v0,0(sp)
9d0026d0:	978280e0 	lhu	v0,-32544(gp)
9d0026d4:	24420004 	addiu	v0,v0,4
9d0026d8:	3042ffff 	andi	v0,v0,0xffff
9d0026dc:	a78280e0 	sh	v0,-32544(gp)
9d0026e0:	8fa30004 	lw	v1,4(sp)
9d0026e4:	8fa20000 	lw	v0,0(sp)
9d0026e8:	41606000 	di
9d0026ec:	000000c0 	ehb
9d0026f0:	8fba0014 	lw	k0,20(sp)
9d0026f4:	8fbb000c 	lw	k1,12(sp)
9d0026f8:	409a7000 	mtc0	k0,c0_epc
9d0026fc:	8fba0010 	lw	k0,16(sp)
9d002700:	27bd0018 	addiu	sp,sp,24
9d002704:	409a6002 	mtc0	k0,c0_srsctl
9d002708:	41dde800 	wrpgpr	sp,sp
9d00270c:	409b6000 	mtc0	k1,c0_status
9d002710:	42000018 	eret

There's quite a bit there, much of which you don't really care about. But the important bits to note are what registers get used and what get stored on the stack.

Now the compiler is pretty good. It knows what registers it is using (and tries to mainly use the $k0 and $k1 register, which are reserved for interrupt, or "kernel" use). The core of the routine is this bit though:

9d0026c8:	afa30004 	sw	v1,4(sp)
9d0026cc:	afa20000 	sw	v0,0(sp)
9d0026d0:	978280e0 	lhu	v0,-32544(gp)
9d0026d4:	24420004 	addiu	v0,v0,4
9d0026d8:	3042ffff 	andi	v0,v0,0xffff
9d0026dc:	a78280e0 	sh	v0,-32544(gp)
9d0026e0:	8fa30004 	lw	v1,4(sp)
9d0026e4:	8fa20000 	lw	v0,0(sp)

That is, store the registers $v0 and $v1 on the stack, load the current value of "shared" into $v0. add 4 to it, and write it back to memory. Even though it didn't actually use $v1 at all it stored it on the stack anyway. Yes, it seems to be being overly cautious. However, $v0 and $v1 are the "expression evaluation and return value" registers. It knows it's going to evaluate an expression, and knows those two registers should be used for it (you could actually use any registers you like, but those are the ones the documentation says should be used), so the compiler saves them both, does its evaluation using either one or both of them, and then restores them.

So you can see it's "almost" atomic.

While the interrupt routine takes care not to disrupt the flow of values through the registers, so the 6 doesn't get changed into something else entirely, it's not a single block of code that can't be interrupted. There's a chance the interrupt could fire before 6 gets written into the variable. For instance:

• Put 6 in $v0
• Save $v0 in shared
• Interrupt triggered - shared = shared + 4.
• Shared is 10

Or:

• Put 6 in $v0
• Interrupt triggered - shared = shared + 4.
• Save $v0 in shared
• Shared is 6

Two possible outcomes from the same bit of code depending on when the interrupt triggers.

So really you should disable / enable interrupts, creating a critical section around your assignment of the shared variable. However the disableInterrupt() and restoreInterrupts() routines can be a little heavyweight. Better is to just disable / enable the one specific interrupt that could affect the value you are working with. The most efficient, though the least portable, method is to directly control the IECx register for that interrupt. Kind of goes against the grain of the chipKIT mentality (portability, compatibility, abstraction, etc), however you do want to keep critical sections as short as possible, since your interrupt can't fire until the critical section is over. So you have to consider which is more important in your current scenario - speed and efficiency or portability. If your interrupt only triggers rarely you can get away with the portable abstraction functions. If it's a rapidly triggering one, though, you want to keep it fast and efficient.

One thing to note though is if you were setting shared to 0 and not to 6 in setup. That makes an entirely different bit of code, thanks to the existence of the $zero register which always contains 0. It just ends up as:

9d002a4c:	a78080e0 	sh	zero,-32544(gp)

And that IS completely atomic. So should you need to reset a counter after reading it in your main context you don't need a critical section.

So let's consider reading. Adding this to the loop:

void loop() {
    uint16_t copy = shared;
    shared = 0;
    Serial.println(copy);
    delay(1000);
}

That is, copy the shared value into another variable called copy, set shared back to 0 to reset the counter, and print the copy value. Then delay waiting for the count to build up in shared again.

That produces a loop that has this:

9d0057e0 <.LBB2>:
9d0057e0:	978580e8 	lhu	a1,-32536(gp)

9d0057e4 <.LVL1>:
9d0057e4:	a78080e8 	sh	zero,-32536(gp)
9d0057e8:	27848344 	addiu	a0,gp,-31932
9d0057ec:	30a5ffff 	andi	a1,a1,0xffff
9d0057f0:	0f40159f 	jal	9d00567c <.LFE18>
9d0057f4:	2406000a 	li	a2,10

• Load the upper half of the value at the address of "shared" into $a1.
• Store 0 into shared ($zero).
• Get pointer of the serial class object into $a0.
• Mask the upper 16 bits of $a1 since we are working with a uint16_t type.
• Call the Serial.println() function
• (branch delay slot) : store 10 in register $a2.

The $a registers are the function call parameter registers. When you call Serial.println(val) it actually calls HardwareSerial::println(&Serial, val, 10), and the three parameters are mapped to registers $a0, $a1 and $a2. There's 4 of those registers at most, and any more than 4 parameters cause the stack to be used as well for passing parameters to functions.

So where in there could things go wrong? Well, there's only really one place:

9d0057e0:	978580e8 	lhu	a1,-32536(gp)
9d0057e4:	a78080e8 	sh	zero,-32536(gp)

It's possible that, between capturing the current value and resetting the value to 0 another interrupt could have happened. While not fatal it does mean that you just lost a count. One got thrown away.

• Shared = 3948 (for example)
• Load the upper half of the value at the address of "shared" into $a1. ($a1 = 3948)
• Interrupt triggers (shared = 3952)
• Store 0 into shared ($zero).

The interrupt that triggers in the middle there effectively gets completely wiped out by the immediate "store = 0" afterwards. As I say, not fatal, but just be aware that it may affect your results adversely. It could be worth making a critical section around those two instructions.

I think that just about covers everything. You now know as much about working with interrupts as me ;)

rasmadrak

Mon, 13 Feb 2017 14:37:51 +0000

Now that's a read... :shock:

grabs popcorn

As always, many thanks. Brutally detailed as usual! :D Let me see if I got this correct...

//Ultra pseudo 
volatile unint6_t output_bank = 0x0000;
volatile unint6_t input_bank = 0x0000;

IVR_every_20KHZ()
{
	SPI_out_16bit(output_bank);
	SPI_in_16bit(input_bank);
}

loop() 
{
	every 60hz do:
	{
		uint16_t buffer = input_bank;  //capture current input_bank state.
		for each bit in buffer: //for each input/switch, do something. 
			Do stuff (™) 
			
		output_bank = <new value>;  //depending on various variables, set output bank to a state where each bit is an output. 			
	}
}

In the above code, the worst thing that could happen is that the values sent/received are old(er)? Next refresh would contain the new value?

Is it required to disable interrupts to protect (I think it might be to slow and noticeable in LED/DMD's etc?) or can I use a volatile bool, like so:

volatile bool locked=false;

IVR()
{
	//do main stuff
	
	//do the other stuff unless busy.
	if (locked == false)
	{
		SPI_out_16bit(output_bank);
		SPI_in_16bit(input_bank);	
	}
}

loop()
{
	locked = true;		//soft block
	tempin = input_bank;
	output_bank = temp_out;
	locked = false;		//release soft block
	
	do stuff.
}

rasmadrak

Mon, 13 Feb 2017 14:48:49 +0000

I'm thinking about this part; "While the interrupt routine takes care not to disrupt the flow of values through the registers, so the 6 doesn't get changed into something else entirely, it's not a single block of code that can't be interrupted. There's a chance the interrupt could fire before 6 gets written into the variable."

Since I'm doing many, many, updates I'm not really concerned if one is lost. :) If the old value was say, 0xFFFF and the new should be 0xF0F0 - That would mean the worst thing that could happen is that 0xFFFF would be read as 0xFFFF or perhaps even 0xF0FF / 0xFFF0, and 0xF0F0 the next time the interrupt triggers?

If so, that's perfectly fine. I do not want to end up with 0xF000 or 0x0000 if low-byte wasn't updated before the interrupt kicked in. :)

majenko

Mon, 13 Feb 2017 15:18:23 +0000

Is it required to disable interrupts to protect (I think it might be to slow and noticeable in LED/DMD's etc?) or can I use a volatile bool, like so:

What you have created there is called a mutex, which is a kind of semaphore used specifically for thread synchronisation and resource sharing.

The most simple "spinlock" mutex is simply:

while(locked);
locked = true;
...
locked = false;

which waits for another thread (context, ISR, whatever) to release the "locked" flag then locks it for itself.

Of course, that's not safe to use inside an ISR, since nothing would ever release "locked", so your check for "if (!locked) {...}" is more appropriate as it avoids collisions of the "bank" resources.

rasmadrak

Mon, 13 Feb 2017 22:03:28 +0000

Alright, this is what I got and probably will go with.

I'll simply lock, store copies of the values, unlock. And the interrupt returns right away if the buffer is locked. Shouldn't block for very long, and if my calculations are correct I'll have 2.5 complete switch 8-8 matrices per one needed. I will also be using additive reads in the interrupt, so the interrupt routine will only ever add to input whilst the main loop fetches input and clears the row (i.e "mark handled"). That way no switch pressed will be missed in case the second interrupt would have cleared a valid trigger before the main loop had checked. I do not need sub-millsec accuracy of switch timing, but I figure I could store readCoreTimer() values upon switch hits as well.

That's the plan at least. :)

if (checkSwitches.check())  //~1000hz
  {
       //get a copy of the current switch matrices, built by interrupt routine, 
       // roughly 2.5x per each game loop update (8x8).
        lock_io=true;        //lock to prevent interrupt from interfering. 
        {        
          for (int i=0; i < 8; i++) 
          {
          	switch_states[i] = SWITCH_STATES[i]; //get states 
          	SWITCH_STATES[i] = 0; //Done with this row, clear it. (Switch inputs keeps adding hits until cleared, i.e additive reading)                  	
          }
           /*
           *  switch_buffer = 
           *   {0,0,0,1,0,0,0,0},          
           * ... 
           * {0,1,0,0,0,0,0,0};
           */
          cabinet_io_states = CABINET_IO_STATES;
          CABINET_IO_STATES=0; 
        }
        lock_io=false;    //release lock 
        
        //Process all switches based on result from switch matrix. 
        volatile uint8_t *io_p = switch_states; 
        for (i=0; i < MAX_SWITCHES;)
              switches[i++].update(*io_p++); 

        doCabinetInputs(cabinet_io_states); 

        //done with inputs, do outputs next:
        lock_io=true;        //lock to prevent interrupt from interfering. 
        {
          //set output states to the values built by game loop and switches. 
          SOLENOID_STATES = solenoid_states;            
          GI_FLASHER_STATES = gi_flasher_states;  
        }
        lock_io=false;    //release lock 

        //All done - until next update. 
  }

//Edited because of voodoo-offline-coding.