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

Thread: Help with too many if statements... there must be a better way!

  1. #1
    Junior Member
    Join Date
    Jul 2020
    Posts
    8

    Help with too many if statements... there must be a better way!

    Hello!

    There must be a better way to do this! Let me explain as quickly as possible:

    I have a system with an optical encoder, where I need to accomplish two things dependent on the encoder position (output):

    1: I need to output a 3v3 “square wave” that is directly related to the position of the encoder. From encoder output 0 to 30, I write a pin LOW, from 30 to 60 I write that pin high, and so on, 48 times, alternating every 30 encoder counts. There's one exception, which makes sure the last 30 counts and the first 30 counts are the same state (LOW), which gives me a "missing tooth" indicator for a separate system. (The encoder count returns to zero every 360 degrees.)

    Currently I read the encoder position, then write the pin30 HIGH or LOW via 48 lines of "if" statements. The speed of the encoder will vary between 0 to 50 rotations per second. I've tried using "else if" after the first if statement, but that doesn't work well (seems some counts are missed and stalls). This all seems super clunky to me.

    2: At particular encoder positions I need to hold a second (and third) pin high or low. Currently that operation is happening as a separate function (void Move ()) which reads the encoder position and writes HIGH or LOW to the appropriate pin(s). Ideally I'd like to integrate them to save time and operations.

    My highest concern at the moment is my large ugly 48 lines of if statements. I have to believe there’s a much better way to do this.

    Thanks in advance for any thoughts you may have! The code below works, but ultimately I want to do other things and I'm worried this will bite me later on. I'm using the teensy 4.1.

    Scott

    HTML Code:
    #include <SPI.h>
     int EPin = 6;* //setup of direction (DR) variable for EVAM
     int IPin = 7;* //setup of direction (DR) variable for IVAM
     int uiPot =38;
     volatile int uiAdj = 0;
    
    // three lines below are for LS7366R encoder init
     int CS1 = 10;
     long EncoderCount1;
     char buff[4];
    
    void QEC_Configure(void) //ls6366r configure
     {
    * SPI.begin();
    * delay(5);
    * digitalWrite(CS1, LOW);
    * SPI.transfer(0x88);
    * SPI.transfer(0x23);
    * digitalWrite(CS1, HIGH);
     } //END QEC_Configure()
    
    void ReadEncoder(void) {
    * long buff[4];
    * digitalWrite(CS1, LOW);
    * SPI.transfer(0x60); // Request count
    * buff[0] = SPI.transfer(0x00); // most significant byte
    * buff[1] = SPI.transfer(0x00);
    * buff[2] = SPI.transfer(0x00);
    * buff[3] = SPI.transfer(0x00); // least significant byte
    * digitalWrite(CS1, HIGH);
    * EncoderCount1 = (long)(buff[0] << 24) + (long)(buff[1] << 16) + (long)(buff[2] << 8) + (long)buff[3];
     } //end ReadEncoder()
    
    void CAPosition() {
    * if (EncoderCount1 >= 0 && EncoderCount1 < 30 )digitalWrite (30, LOW);
    * if (EncoderCount1 >= 30 && EncoderCount1 < 60 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 60* && EncoderCount1 < 90 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 90* && EncoderCount1 < 120 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 120* && EncoderCount1 < 150 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 150* && EncoderCount1 < 180 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 180* && EncoderCount1 < 210 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 210* && EncoderCount1 < 240 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 240* && EncoderCount1 < 270 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 270* && EncoderCount1 < 300 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 300* && EncoderCount1 < 330 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 330* && EncoderCount1 < 360 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 360* && EncoderCount1 < 390) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 390* && EncoderCount1 < 420 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 420* && EncoderCount1 < 450 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 450* && EncoderCount1 < 480 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 480* && EncoderCount1 < 510 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 510* && EncoderCount1 < 540 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 540* && EncoderCount1 < 570 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 570* && EncoderCount1 < 600 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 600* && EncoderCount1 < 630 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 630* && EncoderCount1 < 660 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 660* && EncoderCount1 < 690 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 690* && EncoderCount1 < 720 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 720* && EncoderCount1 < 750 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 750* && EncoderCount1 < 780 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 780* && EncoderCount1 < 810 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 810* && EncoderCount1 < 840 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 840* && EncoderCount1 < 870 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 870* && EncoderCount1 < 900 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 900* && EncoderCount1 < 930 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 930* && EncoderCount1 < 960 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 960* && EncoderCount1 < 990 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 990* && EncoderCount1 < 1020 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 1020* && EncoderCount1 < 1050 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 1050* && EncoderCount1 < 1080 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 1080* && EncoderCount1 < 1110 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 1110* && EncoderCount1 < 1140 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 1140* && EncoderCount1 < 1170 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 1170* && EncoderCount1 < 1200 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 1200* && EncoderCount1 < 1230 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 1230* && EncoderCount1 < 1260 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 1260 && EncoderCount1 < 1290 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 1290* && EncoderCount1 < 1320 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 1320* && EncoderCount1 < 1350 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 1350* && EncoderCount1 < 1380 ) digitalWrite (30, HIGH);
    * if (EncoderCount1 >= 1380* && EncoderCount1 < 1410 ) digitalWrite (30, LOW);
    * if (EncoderCount1 >= 1410* && EncoderCount1 < 1440 ) digitalWrite (30, LOW);
     }//END CAPosition()
    
    void serial () {
    * Serial.print("Encoder1 = ");
    * Serial.print(EncoderCount1);
    * Serial.println ();
     }
    
    void Move() {
    * if (EncoderCount1 >= 0 && EncoderCount1 <= 360) {
    * * digitalWrite (IPin, LOW);} //O
    * else
    * {
    * * digitalWrite (IPin, HIGH); //C
    * } //
    
    * if (EncoderCount1 >= 1080 && EncoderCount1 <= 1440) {
    * * digitalWrite (EPin, LOW); //O
    * }
    * else
    * {
    * * digitalWrite (EPin, HIGH); //C
    * } //
    
    
    }// end move
    
    void UI(){
    * uiAdj = analogRead(uiPot);
    * uiAdj = map(uiAdj, 0, 1024, 0, 200);
    *
     }
     void setup() {
    * // LS7366R Encoder setup
    * Serial.begin(9600);
    * pinMode(CS1, OUTPUT);
    * digitalWrite(CS1, HIGH);
    * QEC_Configure(); // QEC = Quadrature Encoder Counter
    * // End encoder setup
    
    * //===========================
    * //PWM TIMER FOR MOTOR CONTROL
    * //===========================
    * //analogWrite(5, 128); //pin, duty cycle (duty 1 to 255, 128 is 50% duty cycle).
    * analogWriteFrequency(5, 30621.09);* //sets driver PWM pin 5 to 36.621kHz 36621.09*
    *
     }/// end of setup
    
    void loop() {
    * ReadEncoder();
    * //serial(); //serial print for diagnostics
    * Move();
    * CAPosition();
    * UI();
    * analogWrite(5, uiAdj); //pin, duty cycle (duty 1 to 255, 128 is 50% duty cycle).
    
    }//end loop

  2. #2
    It appears that all of the ranges are multiples of 30. So divide EncoderCount by 30 and use the result to index into a table.

    digitalWrite(30, encodertable[EncoderCount/30]);

  3. #3
    Junior Member
    Join Date
    Jul 2020
    Posts
    8
    Ah, interesting. Thank you, I'll have a look into tables!!!

  4. #4
    Senior Member
    Join Date
    May 2015
    Location
    USA
    Posts
    1,071
    I'd probably

    a) handle the special cases with "if"
    b) divide by 30 and switch on or off depending on even or odd

  5. #5
    Junior Member
    Join Date
    Jul 2020
    Posts
    8
    Another great idea. I’ve used odd and even detection before... thank you.

  6. #6
    Senior Member
    Join Date
    Jul 2020
    Posts
    1,235
    Its just a simple formula for that pattern:
    Code:
    digitalWrite (30, EncoderCount1 % 60 / 30);
    The repeat is every 60, not every 30 - not sure people are paying attention

    BTW if you ever find yourself writing code that repititious you can be _certain_ you are
    not doing it the smart way.

    Only the last exception needs special coding:
    Code:
     if (EncoderCount1 >= 1410) digitalWrite (30, LOW);
    Last edited by MarkT; 08-31-2021 at 11:47 AM.

  7. #7
    Senior Member
    Join Date
    May 2015
    Location
    USA
    Posts
    1,071
    > Only the last exception needs special coding:

    Pay attention to request 2.

  8. #8
    Senior Member
    Join Date
    Jul 2020
    Posts
    1,235
    Quote Originally Posted by jonr View Post
    > Only the last exception needs special coding:

    Pay attention to request 2.
    Oh yes, I missed that - so there is one exception for pin 30, and some other pins to handle - not sure there's anything to
    gain integrating them into a single lookup though - reading the ADC will dominate the time taken I suspect.

  9. #9
    Senior Member
    Join Date
    May 2015
    Location
    USA
    Posts
    1,071
    A couple more comments:

    A switch() statement is often clearer than many if statements.

    You almost always want to code for clarity and maintainability, not speed or minimal lines of code.

  10. #10
    Junior Member
    Join Date
    Jul 2020
    Posts
    8
    Thanks for all of the recommendations here.

    This worked, MarkT! I replaced the entire CAPosition() with two lines of code. Now to figure out why it worked, because " % 60 / 30" doesn't make much sense to me! I'm excited!

    void CAPosition() {
    digitalWrite (30, EncoderCount1 % 60 / 30);
    if (EncoderCount1 >= 1410) digitalWrite (30, LOW);
    }

    Thanks again!

  11. #11
    Senior Member
    Join Date
    Jul 2020
    Posts
    1,235
    % 60 means modulo 60 - maps to a 60 count cycle
    / 30 - integer division by 30 splits the range 0..59 into 0..29 (as 0) and 30..59 (as 1).

Posting Permissions

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