Faster way to do if() compares in ISR?

Status
Not open for further replies.

paynterf

Well-known member
Hi,

I'm working on a Teensy 3.2 I2C Sniffer project, and I have an ISR that triggers every 1 uSec using Timer1. I have instrumented the ISR with 'digitalWriteFast()' calls so that I can monitor activity on an O'scope. Here's the code:

Code:
/*
    Name:       Teensy_I2C_Sniffer_V10.ino
    Created:	1/15/2020 8:30:09 AM
    Author:     FRANKNEWXPS15\Frank
*/

#include <TimerOne.h> //needed for ISR


const uint16_t CAPTURE_ARRAY_SIZE = 2048;
uint8_t raw_data[CAPTURE_ARRAY_SIZE]; //holds data captured from I2C bus

#define MONITOR_OUT1 2 //so can monitor ISR activity with O'scope
#define MONITOR_OUT2 3 //so can monitor ISR activity with O'scope
#define MONITOR_OUT3 4 //so can monitor ISR activity with O'scope
#define SDA_PIN 18 
#define SCL_PIN 19


#pragma region ISR_SUPPORT
volatile uint16_t  write_idx = 0;
//volatile uint16_t  BurstLength = 0;
volatile uint8_t   current_portb = 0xFF;
volatile uint8_t   last_portb = 0xFF;
#pragma endregion ISR Support

volatile bool bDone = false;
volatile bool bWaitingForStart = true;
volatile bool bIsData = true;
volatile bool bIsStart = false;
volatile bool bIsStop = false;
//volatile bool bFirstTime = true;

void setup()
{
    unsigned long now = millis();
    Serial.begin(1); //rate value ignored
    int idx = 0;
    while (!Serial && (millis() - now) < 3000)
    {
        delay(500);
        idx++;
    }
    Serial.printf("Serial available after %lu mSec\n", millis() - now);

    pinMode(MONITOR_OUT1, OUTPUT);
    digitalWrite(MONITOR_OUT1, LOW);
    pinMode(MONITOR_OUT2, OUTPUT);
    digitalWrite(MONITOR_OUT2, LOW);
    pinMode(MONITOR_OUT3, OUTPUT);
    digitalWrite(MONITOR_OUT3, LOW);

    pinMode(SCL_PIN, INPUT);
    pinMode(SDA_PIN, INPUT);

    //reset port byte vars & start timer
    last_portb = current_portb = 0;
    write_idx = 0;
    memset(raw_data, 255, CAPTURE_ARRAY_SIZE);
    PrintNextArrayBytes(raw_data, 255, 20);
    Timer1.initialize(1); // run every mico second
    Timer1.attachInterrupt(capture_data);
}
//-------------------------------------------------------------------------------
//--------------------------------    ISR    ------------------------------------
//-------------------------------------------------------------------------------
void capture_data()
{
    //digitalWriteFast(MONITOR_OUT1, HIGH);
    last_portb = current_portb;
    current_portb = GPIOB_PDIR & 12; //reads state of SDA (18) & SCL (19) at same time
    if (!bDone && last_portb != current_portb)
    {
        digitalWriteFast(MONITOR_OUT1, HIGH);

        //01/16/20 experiment
        bIsStart = last_portb == 0xC && current_portb == 0x4; 
        bIsStop = last_portb == 0x4 && current_portb == 0xC;
        bIsData = (last_portb == 0x0 && current_portb == 0x4) || (last_portb == 0x8 && current_portb == 0xC);

        if (bIsStart) //START  
        {
            digitalWriteFast(MONITOR_OUT2, HIGH);
            if (bWaitingForStart)
            {
                raw_data[write_idx++] = last_portb; //capture the first byte
                bWaitingForStart = false;
            }
        }
        else if (bIsStop) //STOP
        {
            digitalWriteFast(MONITOR_OUT2, LOW);
        }

        if (!bWaitingForStart && ( bIsData || bIsStart || bIsStop))
        {
            digitalWriteFast(MONITOR_OUT3, HIGH);
            raw_data[write_idx] = last_portb;
            write_idx++;
            raw_data[write_idx] = current_portb;
            write_idx++;
            if (write_idx >= CAPTURE_ARRAY_SIZE)
            {
                //bDone = true;
                write_idx = 0;
            }
            digitalWriteFast(MONITOR_OUT3, LOW);
        }
        digitalWriteFast(MONITOR_OUT1, LOW);
    }
    //digitalWriteFast(MONITOR_OUT1, LOW);
}
//-------------------------------------------------------------------------------
//-------------------------------- END ISR    ---------------------------------
//-------------------------------------------------------------------------------

void loop()
{

    if (bDone)
    {
        PrintNextArrayBytes(raw_data, 0, 150);

        while (true)
        {

        }
    }
}

void PrintNextArrayBytes(uint8_t* data, uint16_t startidx, uint16_t numbytes)
{
    Serial.printf("%d bytes starting at %d: ", numbytes, startidx);
    for (uint16_t i = 0; i < numbytes; i++)
    {
        Serial.printf("%x\n", data[i + startidx]);
    }
    Serial.printf("%x\n", data[numbytes + startidx - 1]);
}

If I include the three compares shown here:


Code:
        //01/16/20 experiment
        bIsStart = last_portb == 0xC && current_portb == 0x4; 
        bIsStop = last_portb == 0x4 && current_portb == 0xC;
        bIsData = (last_portb == 0x0 && current_portb == 0x4) || (last_portb == 0x8 && current_portb == 0xC);

Then the ISR takes about 1.4 uSec to complete, too long by 0.4 uSec or so. If I remove the compares, the time required to complete drops to about 0.425 uSec.

If necessary, I can make everything work without the compares, but including them makes post-capture processing a lot easier. Is there a way to do the compares in a way that doesn't take so much time, other than going to a Teensy 3.6 or a T4?

TIA,

Frank
 
You can reduce the number of comparisons from 8 to 4 by combining the two portb values into one variable

uint8_t last_current = (last_portb << 4) | (current_portb);
bIsStart = (last_current==0xC4);
bIsStop = (last_current==0x4C);
bIsData = (last_current==0x04) || (last_current==0x8C);

and it's probably a little faster to avoid unnecessary comparisons and assignments. I didn't review your code to see when these values are set to false.

if (last_current==0xC4)
bIsStart = true;
else if (last_current==0x4C)
bIsStop = true;
else if (last_current==0x04)
bIsData = true;
else if (last_current==0x8C)
bIsData = true;

Joe
 
Joe,

Thanks for the quick reply - that's a great idea combining the two values into the same 8-bit word! Your first suggestion,

uint8_t last_current = (last_portb << 4) | (current_portb);
bIsStart = (last_current==0xC4);
bIsStop = (last_current==0x4C);
bIsData = (last_current==0x04) || (last_current==0x8C);

got the time down to around 1.2 uSec - a nice improvement, but not quite enough. And the other option you described,

if (last_current==0xC4)
bIsStart = true;
else if (last_current==0x4C)
bIsStop = true;
else if (last_current==0x04)
bIsData = true;
else if (last_current==0x8C)
bIsData = true;

Is actually slower than my original code, clocking in at around 1.7 uSec.

Frank
 
Try to make it with:
Switch last_current
And remove the variables blsStart ...

Do you need the first byte twice?
 
Not sure what current code looks like ...
If the _isr() did this stop() on end then bDone test is not needed at top of _isr() :: if (!bDone && last_portb != current_portb)
Code:
            if (write_idx >= CAPTURE_ARRAY_SIZE)
            {
                [B]Timer1.stop(); [/B]
                bDone = true;
                // write_idx = 0; // not needed
            }

bDone could be repurposed, and loop() could see when it was done.

Only bDone would need to be volatile as nothing else in the _isr() is used outside the _isr() while it may be changing.

Might help to do :: FASTRUN void capture_data()
 
Defragster,


wow - FASTRUN did the trick; went from 1.45 uSec to 0.89 uSec - thanks!

If anyone ever had any doubts about the value of Paul's forum, this thread shows it; I had never heard of FASTRUN before this post. A few quick Google searches, and BAM - I'm an expert! ;-).

Frank
 
Awesome - glad I was reading and thought to mention it and it made such a dramatic difference! I've spent the last year with the T4 beta to release and it runs code from RAM by default.

Just read the other post and blog :: Teensy-I2C-Sniffer-for-MPU6050- nice work getting it done.
 
Status
Not open for further replies.
Back
Top