How to make this run faster/more responsive?

Chugging away at this-so far now all the libraries are working, and nothing complains about hardware. I haven't finished writing in all the control commands yet but it's already sluggish, misses inputs, and is generally slow to respond. Debounce doesn't seem to be a problem, but that's likely because the code is too busy to care about bounces so far.

Here's the complete current code: #include <si5351.h>#include <Encoder.h>#include <MCUFRIEND_kbv.h>#include - Pastebin.com

I assume this is the time to start breaking things out of void (loop) and into separate subroutines to allow void (loop) to execute faster?

If that is the case, then the biggest slowdown is drawing the display-and it's going to have a moving bargraph on there eventually for received signal strength indication, and when the encoder knob is spun quickly, the main tuning and receive tuning indicators will be scrolling numbers pretty fast. I think the solution is to not draw to the display every loop and only update it if I need to.

The buttons are currently on normal digital IO pins, not interrupt pins-so I think this is a case of attachInterrupt() being needed?

Also, if you see any glaring errors that could be corrected, feel free to mention them-it will be appreciated.

You will get help faster if you post the code here, using code tags when you do.

Breaking the program into functions is probably a good idea but it won't make loop() run any faster. It is unlikely that you will need interrupts to read pushbutton inputs.

My crystall ball tells me that you are using blocking code such as delay() or while that cause the program to be slow at reacting to inputs. Post the code then we can see.

In the meantime look at Using millis() for timing. A beginners guide and Several things at the same time and look at the BlinkWithoutDelay example in the IDE to see how to use millis() for timing, which I expect you will need to do.

Please post your code here. If it is too long to include in your next Reply (hopefully not) you can add it as an attachment.

...R

I went through your code but did not read the whole thing carefully.

I think you are using a Graphical LCD and it is color graphical LCD. The problem is that so much data has to be sent to graphical LCD to get things working and on top of that you have to set the colors too. So every line of code you write for writing things on your LCD, the Arduino has to do so much work. It has to tell the color for each pixel and it also has to map the characters. So the LCD part of your code is very time consuming.

Considering that if you do not want to miss the button press and stuff like that you can scan them more often in your code. Rather than doing it once at the beginning, do it multiple time. In order to keep your code neat you can make a function and call it multiple times in the loop. However you should make sure after every scan you apply the correct condition so that you code does not get screwed. for example you do not want the screen to show half of the setting from previous condition and another half from the new selection. (although if your loop run fast enough you may get lucky and not experience any problem).

I am not sure about the graphical library you are using but I doubt it can become faster by few easy modification in the code. However there may be settings that can make it faster. Sometimes you can adjust the clock (i2c and SPI), baud rate (UART) to higher values to increase the speed. Overall that is the place where you should improve the speed. The code you have written yourself (non the libraries) should run very fast judging by what I understood in my glance through your code.

The code is too big to post here, so I made the link clickable in the first post.

Lots and LOTS of other folks doing the same thing with this display, SPI-communication displays, and even bigger displays-none of them seem to have the same or as serious of a problem.

Not the same display, but lots more info:

16 bit parallel version of a similar, but larger display:

Display is the same as mine, startup time very similar, but even this one is faster than mine:

By far the most responsive example-on a bigger screen and including all of the features I plan to include:

So, it's got to be either a library I've chose (likely the adafruit gfx library) or the method of drawing the screen that slows things down-also I need to figure out how to make the buttons more responsive...they often miss buttonpresses.

Xnke:
The code is too big to post here, so I made the link clickable in the first post.

Then post the complete code as an attachment (as instructed in Reply #2). Very few people will want to click on your random link.

Do post clickable links to the libraries you’re using People will generally click on a recognizable URL like Adafruit.

No experience with tft but you do a lot of updates that don't seem necessary. E.g only update the relevant part of the screen if filtermode changes, not every time that you ho through loop().

Out of curiosity, what are those last two elses suposed to do?

if (Encoderbutton == LOW) {
    // encoder button was pressed, increment menu position
    MenuSelect++;
    if (MenuSelect >= 8) {
        MenuSelect = 0;
    }
    else (MenuSelect);
}
else (MenuSelect);{
}

You have a few more like that.

The last two lines do nothing, but complete the “else” part of the statement-they probably are useless.

GFvalvo, if Pastebin isn’t a well known site here…well…

You do realize it’s one of the first, longest lasting, and most used code/text sharing websites out there? And that when you paste to it, for the express purpose of sharing code, it allows you to set the syntax highlighting for most languages?

Why WOULDN’T you use it? It’s literally why it was created.

Just special for you, here’s the same thing, as an attachment.

radio-main.ino (7.71 KB)

Here’s OP’s code. Turns out it will fit in line with the post. But, I’ve lost interest.

#include <si5351.h>
#include <Encoder.h>
#include <MCUFRIEND_kbv.h>
#include <Adafruit_GFX.h>
#include <Wire.h>

//define constants for the digital inputs here

Encoder myEnc(20, 21); //setup the pins for the rotary encoder-must be interrupts!

const int encoderbutton = 22; //encoder button
const int button2 = 23; //top button
const int button3 = 24; //middle button
const int button4 = 25; //bottom button
const int MicKey = 17; //mic key switch

//define constants for the digital outputs here

const int SelectBand1 = 12; //output to band 1 relay stack
const int SelectBand2 = 13; //output to band 2 relay stack
const int SelectBand3 = 14; //output to band 3 relay stack
const int SelectBand4 = 15; //output to band 4 relay stack
const int TXenable = 16; //output to control RX/TX relay stack

//define constants for analog inputs here
int PAtemp = A0; //PF0
int TXPower = A1; //PF1
int AGClevel = A2; //PF2

//define variables for digital inputs here
int Encoderbutton = 0;
int Button2 = 0;
int Button3 = 0;
int Button4 = 0;
int MicKeybutton = 0;

//define working variables here
int MenuSelect = 0;
int BandSelect = 0;
int ModeSelect = 0;
int FilterMode = 0;

//define variables for digital outputs here
int band1 = 0;
int band2 = 0;
int band3 = 0;
int band4 = 0;
int RXTX = 0;

//define variables for analog inputs here
int PAtempvalue = 0;
int TXPowervalue = 0;
int AGClevelvalue = 0;

//invoke the display
MCUFRIEND_kbv tft;

//invoke the clock generator
Si5351 si5351;

//Define some colors:
#define BLACK   0x0000
#define BLUE    0x001F
#define RED     0xF800
#define GREEN   0x07E0
#define CYAN    0x07FF
#define MAGENTA 0xF81F
#define YELLOW  0xFFE0
#define WHITE   0xFFFF

//VFO setup variables and stuff
volatile uint32_t vfo = 7000000L; //start freq - change to suit
//volatile uint32_t LSB = 4963600L;  //change these to suit your needs
//volatile uint32_t USB = 5000000L;
volatile uint32_t bfo = 9000000L;   //start bfo freq
volatile uint32_t radix = 1000; //start step size
boolean changed_f = 0;
String tbfo = "LSB";

long oldPosition = -999;

void setup() {
  Serial.begin(9600);
  Serial.println("serial console works");
  
  
  // setup the digital inputs here
  pinMode(Encoderbutton, INPUT);
  pinMode(button2, INPUT_PULLUP);
  pinMode(button3, INPUT_PULLUP);
  pinMode(button4, INPUT_PULLUP);
  pinMode(MicKey, INPUT);
  Serial.println("digital inputs setup");
  
  //setup the digital outputs here
  pinMode(SelectBand1, OUTPUT);
  pinMode(SelectBand2, OUTPUT);
  pinMode(SelectBand3, OUTPUT);
  pinMode(SelectBand4, OUTPUT);
  pinMode(TXenable, OUTPUT);
  Serial.println("digital outputs setup");
  
  //setup the ADC here

  //setup the analog inputs here

  //setup the display
  tft.reset(); //reset the display
  tft.begin(0x9341); //start up the display
  tft.fillScreen(BLACK); //black out the screen so everything is on a black background
  Serial.println("started the display, boss");

  //setup the Wire library
  Wire.begin();

  //setup the SI5351
  // Start serial and initialize the Si5351
  si5351.init(SI5351_CRYSTAL_LOAD_10PF, 0, 0);
  //si5351.set_correction(-388600); //correction factor defined here-test using calibration sketch first!
  si5351.set_pll(SI5351_PLL_FIXED, SI5351_PLLA); // Set CLK0 to output 7 MHz with a fixed PLL frequency
  si5351.set_freq(vfo, SI5351_CLK0); // Set CLK0 to output the VFO with a fixed PLL frequency
  //si5351.set_freq(frequency1, 0, SI5351_CLK1); // Set CLK1 to output 20 MHz
  si5351.set_freq( bfo, SI5351_CLK2); // Set CLK2 to output BFO variable-normally 9Mhz
  si5351.drive_strength(SI5351_CLK0, SI5351_DRIVE_8MA);
  //si5351.drive_strength(SI5351_CLK1,SI5351_DRIVE_2MA);
  si5351.drive_strength(SI5351_CLK2,SI5351_DRIVE_8MA);
}


void loop() {

Encoderbutton = digitalRead(encoderbutton);
Button2 = digitalRead(button2);
Button3 = digitalRead(button3);
Button4 = digitalRead(button4);

//read the encoder
long newPosition = myEnc.read()>>2;
  if (newPosition != oldPosition) {
    oldPosition = newPosition;
  }

//do some buttonpresses
if (Encoderbutton == LOW) {
    // encoder button was pressed, increment menu position
    MenuSelect++;
    if (MenuSelect >= 8) { 
        MenuSelect = 0; 
    } 
    else (MenuSelect);
}
else (MenuSelect);{
}

Serial.print("Menuselect is ");
Serial.println(MenuSelect);

if (Button2 == LOW) {
    // turn LED on:
    Serial.println("Button 2 pushed");
    BandSelect++;
    if (BandSelect >= 4) {
        BandSelect = 0;
    }
        else (BandSelect);
}
else (BandSelect);{}

if (Button3 == LOW) {
    // turn LED on:
    Serial.println("Button 3 pushed");
    ModeSelect++;
    if (ModeSelect > 2) {
        ModeSelect = 0;
    }
        else (ModeSelect);
}
else (ModeSelect);{}

  if (Button4 == LOW) {
    // turn LED on:
    Serial.println("Button 4 pushed");
  } else {
    // turn LED off:
   // Serial.println("");
  }


  tft.setRotation(1); //landscape mode with data pins on the top and address lines on the bottom
  
  //Filter Indicator
  tft.setCursor(6,6);
  tft.setTextColor(CYAN, BLACK);
  tft.setTextSize(2);
  if(FilterMode == 0){  
     tft.println("3Khz Filter");
  }
  else if(FilterMode == 1){
       tft.println("6Khz Filter");
  }
  else{}

  //Band Indicator, display indicated band
  tft.setCursor(6,25);
  tft.setTextColor(CYAN, BLACK);
  tft.setTextSize(2);
  if(BandSelect == 0){  
       tft.println("40M Band");
  }
  else if(BandSelect == 1){
       tft.println("20M Band");
  }
  else if(BandSelect == 2){
       tft.println("10M Band");
  }
  else if(BandSelect == 3){
       tft.println("6M Band ");
  }
  else{}
 //Setup to display Mode Indication, select only one at a time
  tft.setCursor(40, 180);
  tft.setTextColor(CYAN,BLACK);
  tft.setTextSize(3);
  if(ModeSelect == 0){
       tft.println("LSB");
       FilterMode = 0;
  }
  else if(ModeSelect == 1){
       tft.println("AM ");
       FilterMode = 1;
  }
  else if(ModeSelect == 2){
       tft.println("USB");
       FilterMode = 0;
  }
  else{}

  
  //Memory Indicator
  //tft.setCursor(220,6);
  //tft.setTextColor(CYAN, BLACK);
  //tft.setTextSize(2);
  //tft.println("Memory A"); //replace A with A/B/C for whichever stored frequency is in position A/B/C

  //Setup Main TX Tuning Display
  tft.setCursor(6, 80); //Don't get closer than "6" from the edge, it's too close.
  tft.setTextColor(RED, BLACK);
  tft.setTextSize(2); //text size 2 is about 3/16" tall
  tft.println("TX");
  tft.setCursor(60, 80);
  tft.setTextColor(GREEN, BLACK);
  tft.setTextSize(3); //text size 4 is about 1/4" tall
  tft.println("Mhz"); //Make the "123" the Transmit Mhz tuning, "456" the Transmit khz tuning, and "789" the Transmit hz tuning later
  tft.setCursor(110, 80);
  tft.println("."); //seperator
  tft.setCursor(128, 80);
  tft.println("Khz"); //Khz tuning
  tft.setCursor(180, 80);
  tft.println("."); //seperator
  tft.setCursor(198, 80);
  tft.println("hz_"); //hz tuning
  tft.setCursor(280, 80);
  tft.setTextColor(YELLOW, BLACK);
  tft.setTextSize(2);
  tft.println("MHz");
  
   //Setup Main RX Tuning Display
  tft.setCursor(6, 136); //Don't get closer than "6" from the edge, it's too close.
  tft.setTextColor(RED, BLACK);
  tft.setTextSize(2); //text size 2 is about 3/16" tall
  tft.println("RX");
  tft.setCursor(60, 130);
  tft.setTextColor(GREEN, BLACK);
  tft.setTextSize(3); //text size 4 is about 1/4" tall
  tft.println("123"); //Make the "123" the Recive Mhz tuning, "456" the Recieve khz tuning, and "789" the Recieve hz tuning later
  tft.setCursor(110, 130);
  tft.println("."); //seperator
  tft.setCursor(128, 130);
  tft.println("456"); //Khz tuning
  tft.setCursor(180, 130);
  tft.println("."); //seperator
  tft.setCursor(198, 130);
  tft.println("789"); //hz tuning
  tft.setCursor(280, 136);
  tft.setTextColor(YELLOW, BLACK);
  tft.setTextSize(2);
  tft.println("MHz");
  //delay(50); //slow the redraw flicker down for testing

}

I assume this is the time to start breaking things out of void (loop) and into separate subroutines to allow void (loop) to execute faster?

Yes. Only call the subroutines(functions) as needed.

I think the solution is to not draw to the display every loop and only update it if I need to.

Definitely, same goes for all the functions

The buttons are currently on normal digital IO pins, not interrupt pins-so I think this is a case of attachInterrupt() being needed?

No, Once the loop() function is fixed polling the buttons with a debounce if necessary is preferred.

I like to think of the loop() as a dispatcher. Its only job is to know when to call in the other functions that do the real work.

Let's assume I am wrong (I am sure, I am not but just to keep things interesting).

Use millis and measure the time you spend on each stage of your code. Then you can see which part is taking most of the time.

I also checked the youtube links. Firstly they do not look that responsive at least to me. But also the interface is important too. for example on some LCD controllers you can have 16 i/o connection for data bus and controlling signals and that would be way faster than using SPI (SPI has only 2 or 3 wires depending on how it is used and chip select which is not even counted). Moreover the SPI frequency is one thing that can affect this as well. Arduino (the non arm ones) do not offer a very high speed SPI compare to what is available in SPI world. but then again the LCD controller should support that higher frequency setting.

Rethink your basic framework.

Your loop() function's primary job is to just watch the user (buttons & data reading). When there is a change, jump to your handler function for that change, deal with it then exit.

Once exited, you automatically show up in loop() again watching Mr user.

You will have different handler functions for each thing the user could do. Change buttonA? handle buttonA, Change buttonB? handle buttonB. Analog valueA changed? handle AnalogA (maybe update that value on screen?).

What your stuff looks like its doing is handling everything all the time change or not. Hard to code and wastes massive amounts of time doing pointless activity.

Good luck.

-jim lee

Maybe have a look at Planning and Implementing a Program

...R

Alright-so I need to break out each individual part of the program and write subroutines (I normally work in GM's obfuscated 68HC11 assembler...and I'm terrible at writing that too. I can read it pretty well though!) for each individual part, and then only call them when needed-this will speed up the loop, and should make it more responsive. In the GM assembler, stuff is placed in 1 of 5 timer loops, when the timer is up, do everything in that loop, next timer, next loop, etc. So some functions get called every 10ms, some every 40ms, some every 160ms, etc.

So, I've got five buttons that will get pressed total-four which will actually have to do something, and one that just toggles an output for it's duration. In my case here, I want to check all the front panel buttons at a rate fast enough that a buttonpress isn't missed. Right now, I have to press and hold the button for a "long click"...don't have to hold it down til it changes on the display but it's longer than just a quick push. Do I need to scan the five buttons every 10-100ms and return the results?

For example, Button1 is the "memory" function-pushing it 1 time should recall Memory A, pushing it again should increment to Memory B, pushing it a 3rd time should increment to Memory C, and the display should update to show which memory position we are in. To set a memory position, I want to set the frequency via the encoder (haven't written that yet) and do a long press-this should set the current memory position to the current frequency. If this is the case, I'll start a new thread on how I should define and setup the functions needed.

To do this, I think I need to define several functions, because the "memory" function of this, minus being able to set the memory, is almost the same as the "band select" function and the "mode select" function.

I think:

Define Buttonpress(whichbutton) function to handle debounce and long/short press, returning (Thisbutton, longpress) or (Thatbutton, shortpress) This function will get used for all buttons, but I might not care if it was a short press or a long press.

Define SetMemory(memoryx, currentVFO), return (memoryx) where memoryx is an integer decimal value up to 54 million. I'd like to store (memoryx) in flash or EEPROM, so that they are retained when the power is shut off.

Define Cycle(whichparameter) where if we call Cycle(memory) we advance one position in the memory, or if we call Cycle(band) we advance one position in the band, this would be simpler if I had 4 memory positions for each band and four bands, so that seems like I should make it that way. This might also work for Cycle(Something?) to select the magnitude of each step of the encoder, when the encoder button is pressed.

Then, lastly, I should define ScanButtons() to check the state of the buttons every so often, and act on the results if anything has changed.

I've shot and cooked an elephant, time to get out the fork and knife.

This isn't helping:

Serial.print("Menuselect is ");
Serial.println(MenuSelect);

It looks like that is being done every time through the loop.
At 9600 baud that can start to create at least a 17ms delay once the tx buffer fills.
The actual delay will be dependent on the number of characters needed to represent the number MenuSelect and what else is happening inside the loop code.

--- bill

Xnke:
Alright-so I need to break out each individual part of the program and write subroutines [...] for each individual part, and then only call them when needed-this will speed up the loop, and should make it more responsive.

It's a little more complicated than that. If the code in all the functions needs to be called there won't be any time saving.

But by breaking the code into short single-purpose functions it will be much easier to see if time is being wasted somewhere and to identify the parts that may be causing the poor response.

...R

Alright. I think I may have made some progress here, this is a new “subroutine” I worked out to replace some of the button code for button2 (the memory button)

pinMode(button2, INPUT_PULLUP);

int currentMemory = 0;
volatile uint32_t currentFrequency;

void Memorybutton(){
     int held = 0;
     delay(200); //debounce the button
     while ((button2 == LOW) && held <=10) {
      delay(100);
      held++;
      }
      if (held <10){
      Setmemory(currentMemory, currentFrequency);
      }
      else {
        (currentMemory++);
      }
}

void Setmemory(int currentMemory, volatile uint32_t currentFrequency){
  
}

Keep in mind, Setmemory hasn’t been defined yet, because I’m not sure how. MemoryButton() seems to be OK, I think it does what I need.

First, defining currentMemory, and currentFrequency. Both of these are “global” because other functions will use them too. Current frequency changes by the encoder so I think it should be a volatile?

I made MemoryButton() type void, because it doesn’t return anything, I just call it when I check the buttons.

held is locally defined variable, it’s only used inside the function. The delay(200) is supposed to be a debounce-but I think it may be in the wrong spot?

The while loop is supposed to differentiate between a short press and a long press. If the button is held down longer than delay(100), increment the held counter, and if the button is held down for longer than 10 increments of held, it’s a long button push-call Setmemory. If less than 10 increments of held, increment the currentMemory variable by 1.

The desired behavior is that a short press will increment from Memory Position 1, to Memory Position 2, and so on. A long press will NOT increment Memory Position, but will instead save the currentFrequency value to non-volatile memory.

Setmemory will have to wait a bit until I get the other button functions worked out. It will involve an EEPROM write, so it’s a bit more advanced than I am yet. Also, I need to learn how to configure the parameters so that when you call Setmemory(arg1, arg2) that it knows that arg1 is the current memory position, and arg2 is the current frequency setting.

Your loop() rewrites the entire display every time it runs. Alter your logic so that you only write to the display when something changes. This is the main thing affecting the speed of your sketch.

This:

if (Encoderbutton == LOW) {
    // encoder button was pressed, increment menu position
    MenuSelect++;
    if (MenuSelect >= 8) {
        MenuSelect = 0;
    }
    else (MenuSelect);
}

Will cycle through the menu select as fast as possible for as long as the button is held down. Probably not what you want. What you need is “if the button is held down, and either it wasn’t held down last time I looped or it has been held down for more than 200ms since last time I incremented the variable, increment the variable”.

Oh, and can you please stop doing this:

else (ModeSelect);{}

It compiles, but it doesn’t mean anything.

Varioius other things.

const int button2 = 23; //top button
const int button3 = 24; //middle button
const int button4 = 25; //bottom button

Why not name the variables “topButton”, “middleButton”, and “bottomButton”? “button3” doesn’t tell you anything.

I always like to suffix variables containing numbers with “Pin”, eg, “middleButtonPin”. That way, you can see in code if you are supposed to be using the value or if you are supposed to be digital reading it.

Note this:

if (Button3 == LOW) {
    // turn LED on:
    //Serial.println("Button 3 pushed");
    ModeSelect++;
    if (ModeSelect > 2) {
        ModeSelect = 0;
    }
        else (ModeSelect);
}

The buttton is named “button 3”, but it’s entirely to do with the mode selection. And using upper and lower case is a really, really bad idea. Why not

modeSelectButton = digitalRead(modeSelectButtonPin);
if (modeSelectButton == LOW) {
    // turn LED on:
    //Serial.println("Mode select pushed");
    ModeSelect++;
    if (ModeSelect > 2) {
        ModeSelect = 0;
    }
}

Rather than

void loop() {
 if(BandSelect == 0){  
       tft.println("40M Band");
  }
  else if(BandSelect == 1){
       tft.println("20M Band");
  }
  else if(BandSelect == 2){
       tft.println("10M Band");
  }
  else if(BandSelect == 3){
       tft.println("6M Band ");
  }
}

I would tend to use

char *bandName[] = {
       "40M Band",
       "20M Band",
       "10M Band",
       "6M Band "
};

void loop() {
  tft.println(bandName[BandSelect]);
}

I would also be inclined to use a class for these selection things of yours.

class ButtonLooper {
    const int N; // the looper will loop between 0 and N-1
    const byte pin;
    const uint32_t repeatRate;

    int n = 0; // current number
    uint32_t mostRecentRepeat;
    byte state = HIGH;

  public:
    ButtonLooper(int setN, byte attachPin, uint32_t setRepeatRate) :
      N(setN), pin(attachPin), repeatRate(setRepeatRate) {
    }

    void setup() {
      pinMode(pin, INPUT_PULLUP);
    }

    // rewturns true if n has *changed* as a result of this read.

    boolean read() {
      byte prevState = state;
      state = digitalRead(pin);
      
      if (state == HIGH) return false;
      if (prevState == LOW && millis() - mostRecentRepeat < repeatRate) return false;

      mostRecentRepeat = millis();
      if (++n >= N) n = 0; // note the use of pre-increment!
      return true;
    }

    int getN() {
      return n;
    }
};

ButtonLooper bandSelect(4, 23, 200); // four states 0-3, pin 23, 200ms delay
ButtonLooper modeSelect(3, 24, 200); // three states 0-2, pin 24, 200ms delay

void setup() {
  bandSelect.setup();
  modeSelect.setup();
}

void loop() {
  boolean needToRedraw = false;

  if (bandSelect.read()) needToRedraw = true;
  if (modeSelect.read()) needToRedraw = true;

  if (needToRedraw) {
    // redraw the LCD here.
    // use 'bandSelect.getN()' to get the value.
  }
}