1
\$\begingroup\$

I am currently coding a flight controller from scratch and am currently working on the module to decode the incoming PPM signal. Keeping in mind that this is an ISR, I've limited its functionality only to read the channel values from my receiver. It is working totally okay, but I wanted some feedback and improvements on the same (if possible). The ISR is as follows:

    void measureChannel(){
   int elapsedTime = t.read_us();
          //if any pulse shorter than 1000us, discard as garbage
   if(elapsedTime < 900){
        t.reset();
        return;
    }

  else{ 
        //if start/stop sequence, then set currentChannel = 1 and begin to read
        if(elapsedTime > 2100) {
                currentChannel = 1;
                t.reset();
                return;
            }
        //read channel value
        else if(elapsedTime >= 1000 && elapsedTime <= 2000){
                if(currentChannel == 1){
                        channelVal[0] = elapsedTime;
                        currentChannel = 2;
                        t.reset();
                        return;
                    }

                else if(currentChannel == 2){
                        channelVal[1] = elapsedTime;
                        currentChannel = 3;
                        t.reset();
                        return;
                    }

                else if(currentChannel == 3){
                        channelVal[2] = elapsedTime;
                        currentChannel = 4;
                        t.reset();
                        return;
                    }

                else if(currentChannel == 4){
                        channelVal[3] = elapsedTime;
                        currentChannel = 5;
                        t.reset();
                        return;
                    }

                else if(currentChannel == 5){
                        channelVal[4] = elapsedTime;
                        currentChannel = 6;
                        t.reset();
                        return;
                    }

                else if(currentChannel == 6){
                        channelVal[5] = elapsedTime;
                        currentChannel = 0;
                        t.reset();
                        return;
                    }
            }
      } 
}

The relevant global variables and various instances are initialised as follows:

    Timer t;
    InterruptIn ppmPin(PB_5);
    uint16_t channelVal[6];
    uint8_t currentChannel = 1;
\$\endgroup\$
  • 1
    \$\begingroup\$ Assuming those globals are accessed outside of your ISR they should be declared volatile. \$\endgroup\$ – brhans Oct 18 '17 at 19:20
  • 1
    \$\begingroup\$ ... and any of those globals which are only accessed inside the ISR, but need to maintain their state between ISR calls I would declare as static inside the ISR. That way they're inaccessible from outside the ISR, but do maintain their state between calls. \$\endgroup\$ – brhans Oct 18 '17 at 20:05
  • \$\begingroup\$ if any pulse shorter than 1000us not the truth in the real life. \$\endgroup\$ – P__J__ Oct 19 '17 at 8:33
0
\$\begingroup\$
  • the return statements are not needed

  • for clearity it's better to use some arrays to minimize the code

The read channel value part can be written as:

  //read channel value
  else if(elapsedTime >= 1000 && elapsedTime <= 2000)
  {
       if((currentChannel >= 1) && (currentChannel <= 6)) 
       {
             channelVal[currentChannel - 1] = elapsedTime;
             currentChannel = (currentChannel + 1) % 7;
       }
  }
\$\endgroup\$
  • \$\begingroup\$ the return statements are not needed But it is ok to have one. \$\endgroup\$ – P__J__ Oct 19 '17 at 8:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.