Teensy 3.x multithreading library first release

[mjs513]

@tni

Made the changes to threads.cpp and the sketch but now getting the following errors. Did you make any changes to threads.h? I did add void ctx_switch_timer_isr(void); to threads.h since you renamed the function. (just for ref I downloaded the intervalTimer lib from https://github.com/loglow/IntervalTimer.git)

C:\Users\CyberPalin\Documents\Arduino\libraries\TeensyThreads-master\Threads.h:190:7: error: 'int Threads::thread_active' is protected

   int thread_active;

       ^

C:\Users\CyberPalin\Documents\Arduino\libraries\TeensyThreads-master\Threads.cpp:102:15: error: within this context

   if (threads.thread_active == Threads::STARTED) { // switch only if active

               ^

 

[tni]

Even the original TeensyThreads didn't build for me with the current Teensyduino beta toolchain, which includes a 'thread.h'. I renamed the TeensyThreads 'Thread.h' to 'TeensyThreads.h' and changed the include in the sketch and 'Threads.cpp'.

Edit: That's not your problem. I made another change, in the Thread.h header file Thread class definition, there is:
"friend void systick_isr(void);"
change that to:
"friend void ctx_switch_timer_isr(void);"

 

[ftrias]

@tni, I presume you also remove this line in systick_isr():

  systick_millis_count++;

When I looked at this originally, the problem I found was that if the context switch happens while running an interrupt isr other that systick_isr, it can crash. This is especially true if it's usb_isr. So the context_switch looks for this and avoids switching when it is interrupting an interrupt. This information is preserved in the LR register and preserving this is the reason the function is defined as "naked". However, when using IntervalTimer, it's impossible to know if the context switch happened during an interrupt or not. Preserving that information would require changing IntervalTimer. But perhaps there is something I'm missing?

 

[mjs513]

@tni and @ftrias

I just implemented the changes and tested with my sketch and no joy. All sensor values are 0. I tried changing the timeslices as well but no luck. Also, it runs for less than a second and then hangs. Must be missing something.

 

[tni]

@tni, I presume you also remove this line in systick_isr():

  systick_millis_count++;

Yes.

When I looked at this originally, the problem I found was that if the context switch happens while running an interrupt isr other that systick_isr, it can crash. This is especially true if it's usb_isr. So the context_switch looks for this and avoids switching when it is interrupting an interrupt. This information is preserved in the LR register and preserving this is the reason the function is defined as "naked". However, when using IntervalTimer, it's impossible to know if the context switch happened during an interrupt or not. Preserving that information would require changing IntervalTimer. But perhaps there is something I'm missing?

This is the PIT ISR I get with IntervalTimer:

000019f0 <pit0_isr>:
    19f0:	4a02      	ldr	r2, [pc, #8]	; (19fc <pit0_isr+0xc>)
    19f2:	4b03      	ldr	r3, [pc, #12]	; (1a00 <pit0_isr+0x10>)
    19f4:	2101      	movs	r1, #1
    19f6:	6011      	str	r1, [r2, #0]
    19f8:	681b      	ldr	r3, [r3, #0]
    19fa:	4718      	bx	r3
    19fc:	4003710c 	.word	0x4003710c
    1a00:	1fff0738 	.word	0x1fff0738

It doesn't change 'lr', so your 'inside interrupt detection' should work the same as for the systick ISR. But I guess it can't necessarily be guaranteed that GCC generates code this way.

In any case, it would probably make sense to move the context switch to a lowest priority interrupt that's triggered by the timer interrupt. Overhead should be minimal, since interrupt chaining will be used.

 

[tni]

@tni and @ftrias

I just implemented the changes and tested with my sketch and no joy. All sensor values are 0. I tried changing the timeslices as well but no luck. Also, it runs for less than a second and then hangs. Must be missing something.

Can you disassemble your binary and post pit0_isr?

E.g.:
arm-none-eabi-objdump.exe -d --demangle -C YOUR_COMPILED_SKETCH.elf > assembly.txt

 

[defragster]

Can you disassemble your binary and post pit0_isr?

E.g.:
arm-none-eabi-objdump.exe -d --demangle -C YOUR_COMPILED_SKETCH.elf > assembly.txt

@tni - possibly already there - but I just linked this post to the Sticky - Wiki coming thread

 

[mjs513]

@tni

Sorry for not getting back to you sooner. I have attached the sketch and the assembly.txt file. Thanks for taking the time for checking this. Here is pit0_isr:

0000364c <pit0_isr>:
364c: b508 push {r3, lr}
364e: 2201 movs r2, #1
3650: 4b02 ldr r3, [pc, #8] ; (365c <pit0_isr+0x10>)
3652: 601a str r2, [r3, #0]
3654: 4b02 ldr r3, [pc, #8] ; (3660 <pit0_isr+0x14>)
3656: 681b ldr r3, [r3, #0]
3658: 4798 blx r3
365a: bd08 pop {r3, pc}
365c: 4003710c .word 0x4003710c
3660: 1fff0a08 .word 0x1fff0a08

 

[tni]

@tni

Sorry for not getting back to you sooner. I have attached the sketch and the assembly.txt file. Thanks for taking the time for checking this. Here is pit0_isr:

0000364c <pit0_isr>:
364c: b508 push {r3, lr}
364e: 2201 movs r2, #1
3650: 4b02 ldr r3, [pc, #8] ; (365c <pit0_isr+0x10>)
3652: 601a str r2, [r3, #0]
3654: 4b02 ldr r3, [pc, #8] ; (3660 <pit0_isr+0x14>)
3656: 681b ldr r3, [r3, #0]
3658: 4798 blx r3
365a: bd08 pop {r3, pc}
365c: 4003710c .word 0x4003710c
3660: 1fff0a08 .word 0x1fff0a08

Your code has "blx r3", so it modifies 'lr'. This means the 'inside ISR detection' doesn't work and there are potential context switches during other ISRs.

 

[ftrias]

To test this theory further, I remove pit0_isr() from IntervalTimer.cpp and added it to Threads.cpp as follows:

void __attribute((naked)) context_switch_isr(void)
{
  if (threads.thread_active == Threads::STARTED) { // switch only if active
    // we branch in order to preserve LR and the stack
    __asm volatile("b context_switch");
  }
  __asm volatile("bx lr");
}
void __attribute((naked)) pit0_isr()
{
  PIT_TFLG0 = 1;
  __asm volatile("b context_switch_isr");
}

I removed systick_isr() from Threads.cpp. I added the global variable:

IntervalTimer context_timer;

And then added this to setup():

  context_timer.priority(254);
  context_timer.begin(context_switch_isr, 100);

This seemed to do the trick. All tests run successfully.

So at least it seems that with a very small change in IntervalTimer to make the isr naked and then simply branch ("B") to the callback function, it might work.

 

[mjs513]

I think I did everything correct and I keep getting this error

Sonar_Multithread_Test2: In function 'void setup()':
Sonar_Multithread_Test2:124: error: 'context_switch_isr' was not declared in this scope
   context_timer.begin(context_switch_isr, 100);

I put in threads.h where it says extern "C"

  void ctx_switch_timer_isr(void);
  void pit0_isr();

I think I did everything else correct

 

[tni]

I named my ISR 'ctx_switch_timer_isr', ftrias used 'context_switch_isr'. Just pick either one consistently for everything.

 

[ftrias]

Sounds like Arduino didn't recompile everything. You may need to quit and restart Arduino or perhaps change the CPU or something else.

But I have an even simpler solution if you want to try it. Remove pit0_isr() from Threads.cpp and change the one in IntervalTimer.cpp to:

void __attribute((naked)) pit0_isr()
{
        PIT_TFLG0 = 1;
        __asm__ ("bx %0" : : "r"(funct_table[0]));
}

 

[mjs513]

@ftrias. No was my fault. Was editing the wrong IntervalTimer file. Didn't read the compiler message that it was using the one in the teensy 3 core. I edited the correct file and adjusted the time slice for the sonar sensors and it works like a charm.

Just as a note. The last change to the IntervalTimer library might be better because if it doesn't affect usage the IntervalTimer can be changed for the core. But I am talking out of my hat here. If you want I can test the second change and let you know how it works.

 

[mjs513]

@ftrias. Went ahead and gave it a try and it worked just as well. Your choice on how to implement. I will go with the preferred way. You might what to do a pull request to the IntervalTimer library?

 

[mjs513]

@ftrias.

Almost forgot, if you use the Teensy 3.x you would change this section as well, correct?

#elif defined(KINETISL)
void pit_isr() {
	if (PIT_TFLG0) {
		PIT_TFLG0 = 1;
		funct_table[0]();
	}
	if (PIT_TFLG1) {
		PIT_TFLG1 = 1;
		funct_table[1]();
	}
}

 

[tni]

No changes to IntervalTimer are necessary. Here is a version that uses IntervalTimer instead of the systick timer:
https://github.com/tni/TeensyThreads

The priority for the timer interrupt is the lowest, so no 'inside ISR detection' is required. The entry points in the assembly file are now properly marked as thumb functions and can be used directly without indirection.

 

[tni]

@ftrias.

Almost forgot, if you use the Teensy 3.x you would change this section as well, correct?

#elif defined(KINETISL)
void pit_isr() {
	if (PIT_TFLG0) {
		PIT_TFLG0 = 1;
		funct_table[0]();
	}
	if (PIT_TFLG1) {
		PIT_TFLG1 = 1;
		funct_table[1]();
	}
}

No. This code is for Teensy LC and the library doesn't work for LC in the first place. (Teensy LC is Cortex M0 as opposed to M4 and the low level code would be different.)

 

[mjs513]

@tni. Thanks for clearing that up. So kinetisk is the K series for the Cortex M4 while the kinetisl is the L-series which is the Cortex M0+.

Just gave the updated library a try and it worked no problems with same code base I used before. Probably should merge the two libraries for consistency.

 

[ftrias]

No changes to IntervalTimer are necessary. Here is a version that uses IntervalTimer instead of the systick timer:
https://github.com/tni/TeensyThreads

The priority for the timer interrupt is the lowest, so no 'inside ISR detection' is required. The entry points in the assembly file are now properly marked as thumb functions and can be used directly without indirection.

Your solution is very clever. If you don't mind, I will integrate the changes into my library and also rename the Thread.h file. There are a few other minor changes that I'll throw in and commit it all later today.

The only missing piece from your code is checking the "threads.thread_active" flag which is used by the mutex and locking mechanism. It should be easy enough to add in. But for the sensor reading application, this should not make any difference.

 

[tni]

Your solution is very clever. If you don't mind, I will integrate the changes into my library and also rename the Thread.h file.

Go ahead. It would be nice, if you would add a BSD or MIT style license to the library... Consider my code BSD 2-clause licensed.

 

[ftrias]

I committed a new update to GIT (https://github.com/ftrias/TeensyThreads) that includes:

1. Ability to have time slices smaller than 1 millisecond (see below)
2. Rename Threads.* to TeensyThreads.* to avoid conflicts
3. New functions setDefaultTimeSlice() and setDefaultStackSize()
4. Default time slices are now 10 milliseconds instead of 100
5. Various internal improvements

As discussed earlier in this thread, including the suggestions of @tni, I added the ability to use IntervalTimer to create time slices smaller than 1 millisecond. The default behavior is still to use the SysTick timer to generate ticks, as recommended by the Cortex reference manual. However, a new function setMicroTimer(...) will change this, disabling the hook on SysTick, and replacing it with a hook on IntervalTimer. For example:

#include <Arduino.h>
#include "TeensyThreads.h"

void backgroundCheck(int x) { /* do something */ }

void setup() {
  if (threads.setMicroTimer(10)==0) { // ticks every 10 microseconds!
    Serial.println("Failed to set timer!");
  }
  threads.addThread(backgroundCheck, 0);
}

void loop() {
/* do something */
}

In the above example, each tick will be 10 microseconds long. The default time slice is 10 tick counts per slice. So that makes each thread's time slice 100 microseconds.

If this works well, then in the future I may remove the SysTick dependency and add a function to set time slices based on time, rather than the combination of ticks and counts.

 

[mjs513]

@ftrias. Thanks for updating the library. Will give it a try later and let you know how it goes.

UPDATE: 2:54 EST. Installed the new library and used the your example setMicroTimer of 10, 5 and 1 and everything seemed to work fine.

One interesting thing I noticed is the output rate is that when obstacles are very close to sensors (sonar or IR) the output rate increases noticeably. Using just setSliceTime for each thread as before I don't really see that same effect. Using setMicroTimer and setting the timeslices to 1 I don't see that at all.

 

[ftrias]

One interesting thing I noticed is the output rate is that when obstacles are very close to sensors (sonar or IR) the output rate increases noticeably. Using just setSliceTime for each thread as before I don't really see that same effect. Using setMicroTimer and setting the timeslices to 1 I don't see that at all.

There's really no point to having setSliceTime to anything other that 1 if you are using setMicroTimer, since you are able to control the timer precisely. I have to optimize that a little bit on the next update.

As far as the "output rate", are you referring to the frequency at which you are able to get sensor readings? I would think this is expected because the sensor just shoots off the sound and waits for it to come back. The shorter the distance, the less it has to wait and the faster it goes on to the next one. Or are you referring to something else?

 

[mjs513]

@ftrias. thanks for info on the setSliceTime. You are probably right about the sensors "output" rate. Its just interesting that you can see the effect using threads. It is probably actually better using the sensors with threads because you don't have to wait for a delay to finish.3

UPDATE 3/29: Decided to adapt the NewPing 15 sensor example using the sonar thread. Low and behold it worked out fine without any of the timing issues that I saw before. Thanks for all the help everyone. Now back to the 9250 project.