Changing LED pulse frequency with buttons

Hello,
I am working on circuit with two buttons, an LCD, and an LED, and trying to get it so that you can increase and decrease the pulse frequency of the LED by clicking one of two buttons, and the pulse frequency will be displayed on the LCD. When I run my code as it is now, the LCD correctly displays the frequency that changes as I push the buttons, but when I push the buttons, the LED just flashes once instead of pulsing. I think it's a problem with where that part of my code is located, but I can't figure out how else to do it. I've tried various approaches of putting that part of the code in a for loop, in a while loop, and in a nested if statement but nothing has worked. I would greatly appreciate any suggestions for how to get the LED to pulse.

#include <LiquidCrystal.h>
const int rs = 12, en = 11, d4 = 5, d5 = 4, d6 = 3, d7 = 2;
LiquidCrystal lcd(rs, en, d4, d5, d6, d7);
const int button1Pin = 8;
const int button2Pin = 7;
int frequency = 0;
const int ledPin = 13;
void setup() {
  lcd.begin(16, 2);
  pinMode(button1Pin, INPUT);
  pinMode(button2Pin, INPUT);
  pinMode(frequency, OUTPUT);
  pinMode(ledPin, OUTPUT);
}

void loop() {
  int button1State, button2State;
  button1State = digitalRead(button1Pin);
  button2State = digitalRead(button2Pin);
  if ((button1State == LOW) && (button2State != LOW))
  { frequency = frequency + 1;
    lcd.clear();
    lcd.setCursor(0, 0);
    lcd.print("pulse frequency:");
    lcd.setCursor(0, 1);
    lcd.print(frequency);
    delay(1000);                  
    digitalWrite(ledPin, HIGH);     //This and the follwing three lines are supposed to make the LED blink on and off
    delay((frequency - 1) / 10);
    digitalWrite(ledPin, LOW);
    delay((frequency - 1) / 10);
  }

  if ((button2State == LOW) && (button1State != LOW))
  { frequency = frequency - 1;
    lcd.clear();
    lcd.setCursor(0, 0);
    lcd.print("pulse frequency:");
    lcd.setCursor(0, 1);
    lcd.print(frequency);
    delay(1000);
    digitalWrite(ledPin, HIGH);   //This and the following three lines are supposed to make the LED blink on and off
    delay((frequency - 1) / 10);
    digitalWrite(ledPin, LOW);
    delay((frequency - 1) / 10);
  }
}

Oh, that's easy; you're only flashing the LED whenever one of the buttons is pressed, which should only show up once outside all your conditionals, and you're LCD drawing should only occur once too right before the LED flashing

However, there's a lot of things I'm seeing that are wrong or could be better, here's a few:

const int rs = 12, en = 11, d4 = 5, d5 = 4, d6 = 3, d7 = 2;
LiquidCrystal lcd(rs, en, d4, d5, d6, d7);

Why do you use const ints here? You don't use them anywhere else, so just put the values in the lcd() initializer, i.e. lcd(12, 11, 5, 4, 3, 2);. It's cleaner that way and will save you some eyestrain, always be trying to use as little variables as possible.

void loop() {
  int button1State, button2State;

You shouldn't re-initialize these each time in the loop, just initialize them with the rest of your variables; they also should be ints, they should be bool types since the states are only True or False.

frequency = frequency + 1;

and

frequency = frequency - 1;

This is fine, but you can use ++frequency; to add 1 to frequency, and --frequency; to subtract 1. If you ever want to add more than 1, just use:

frequency += 2;

and/or

frequency -= 2;

That's easier to read, it just automatically sets frequency to itself plus/minus whatever value you add. Remember this for the future!

int frequency = 0;

delay((frequency - 1) / 10);

The frequency should never go below 1, since that would give you a negative number and therefore a negative delay. Initialize it at 10 or something higher, and maybe add some code that keeps the frequency within a certain range, since you can also keep pressing buttons to go way below zero.

   delay(1000);                 
    digitalWrite(ledPin, HIGH);     //This and the follwing three lines are supposed to make the LED blink on and off
    delay((frequency - 1) / 10);
    digitalWrite(ledPin, LOW);
    delay((frequency - 1) / 10);

That delay(1000) means that the LED goes high and then low, but then stays low for (at least) a second.
Also, If you want a specific frequency in hertz then just use tone(), it generates a frequency that can be used for driving small speakers or other components that need a certain speed output. You could use digitalWrite() if you only needed to turn on the LED every second or so, but digitalWrite() is painfully slow, so tone is better, or just finding the raw bit that controls whether the pin is on or off and toggling it; note that that means your code will only work on the arduino version you code it for, but that's some advanced stuff you don't need to worry about. You're also writing data to the LCD during this time, which adds some very small delay that would get your LED out of sync with the frequency you want as the loop runs, tone() uses interrupts so that won't be an issue with it.

You're also trying to toggle the builtin LED on the arduino, which you don't need to declare a variable for, you can use LED_BUILTIN. Don't use that LED anyway though, since you can't toggle it very quickly for PWM (pulse-width-modulation) signals, which is what tone uses. You'll need to see which pins your arduino can use for PWM.

Finally, PLEASE ORGANIZE THIS UNHOLY MESS. For an embarrassingly long amount of time, I wrote code basically the same way you have here: very few new lines and enter spaces. You will hate yourself later for how ugly your code looks, and debugging will be a pain; I speak from experience, save yourself while you can. Just throw in some lines between variable initializers that aren't closely related, and keep chunks of code that rely on each other grouped, but separate the rest, like this:

#include <LiquidCrystal.h>

LiquidCrystal lcd(12, 11, 5, 4, 3, 2);

const int button1Pin = 8;
const int button2Pin = 7;

bool button1State, button2State;

int frequency = 0;

const int ledPin = 13;

void setup()
{
  lcd.begin(16, 2);

  pinMode(button1Pin, INPUT);
  pinMode(button2Pin, INPUT);

  pinMode(frequency, OUTPUT);
  pinMode(ledPin, OUTPUT);
}

void loop() {
  button1State = digitalRead(button1Pin);
  button2State = digitalRead(button2Pin);

  if ((button1State == false) && (button2State == true))
  {
    ++frequency1;

  } else if ((button2State == LOW) && (button1State != LOW) && (frequency > 1)) {

    --frequency;
  }

  lcd.clear();

  lcd.setCursor(0, 0);
  lcd.print("pulse frequency:");

  lcd.setCursor(0, 1);
  lcd.print(frequency);

  tone(ledPin, frequency, 10); 
}

I haven't run any of the above code or even compiled it, so there may be a few grammar/syntax errors, but it's your job to make it work, I just teach you to do better. There's hopefully enough information here to keep you busy for a while, so go try to fix all those little things and learn some new stuff, the arduino reference is your friend!

Also, kudos to you for making a intelligently designed first post, that's about as rare as efficient python code around here, keep up the streak.

Why do you use const ints here? You don't use them anywhere else, so just put the values in the lcd() initializer, i.e. lcd(12, 11, 5, 4, 3, 2);. It's cleaner that way and will save you some eyestrain, always be trying to use as little variables as possible.

I don't agree. The const variables help to identify the usage of the variables in the function parameter list or declaration. This is a common problem, you get anonymous values like

someFunc(33, 5, 24, 0, 0, 1);

and you have to go look up the position and order of the numbers in order to understand it. As it was written, it is clear that '11' is the 'en' pin, and also that the enable is specified by the second parameter. Often these identifiers are also the PCB markings on a module, and are immensely helpful in hardware troubleshooting as well.

There is no performance penalty for using const values as they are easily optimized by the compiler, and it makes a world of difference in the readability, considering that readability is heavily influenced by understandability.

aarg:
The const variables help to identify the usage of the variables in the function parameter list or declaration.

That just sounds like comments but harder to understand.

aarg:
and you have to go look up the position and order of the numbers in order to understand it. As it was written, it is clear that '11' is the 'en' pin, and also that the enable is specified by the second parameter

Again, it's clear that it's the "en" pin, but I didn't know it was "enable" until you said so since I've only used SPI/I2C LCDs before. Only assume that whoever reads your code knows the language, but not the functions; that way you can leave good notes for newbies.

aarg:
There is no performance penalty for using const values as they are easily optimized by the compiler, and it makes a world of difference in the readability, considering that readability is heavily influenced by understandability.

Everything here is absolutely correct, although I never said that const types are bad for performance. It's just that using them for a number that is only used once isn't necessary. While shorthand with variables is useful, it's common courtesy to leave a brief comment before uncommon functions, like this:

//resize for sprite dimensions 30x40
sprite = gfx_MallocSprite(30,40);

//decompress to sprite var, DO NOT USE RETURNED POINTER
zx7_Decompress(sprite, sprite_compressed);

I will concede that variable names are good too, but as someone who had to read through some Dutch C with abbreviated variable names and almost no comments, I say comments are better. "en" for "enable" makes sense in English (with context too), but "reset pin: 12, enable pin: 11, data1 pin: 5..." is even more clear, and can be run through google translate.