Forum Rule: Always post complete source code & details to reproduce any issue!
Results 1 to 8 of 8

Thread: Faster way to do if() compares in ISR?

  1. #1

    Faster way to do if() compares in ISR?

    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

  2. #2
    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

  3. #3
    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

  4. #4
    Junior Member
    Join Date
    Oct 2019
    Location
    Germany Maintal
    Posts
    19
    Try to make it with:
    Switch last_current
    And remove the variables blsStart ...

    Do you need the first byte twice?

  5. #5
    Try removing "volatile" from those variable definitions.

  6. #6
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    10,918
    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)
                {
                    Timer1.stop(); 
                    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()

  7. #7
    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

  8. #8
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    10,918
    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.

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •