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

Status
Not open for further replies.
5

512

Member
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: 
#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
 
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]);
 
I'd probably

a) handle the special cases with "if"
b) divide by 30 and switch on or off depending on even or odd
 
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:
jonr said:
> Only the last exception needs special coding:

Pay attention to request 2.
Click to expand...

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.
 
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.
 
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!
 
% 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).
 
Status
Not open for further replies.
Back
Top