This code works but there must be a better/more elegant way

Hi,

I have written a sketch which allows the Arduino to output to 3 digital pins, delivering binary code 0 -7, which in turn changes the channel (Ch 1 - 8) on a small video receiver. The plan is to have a button increment the channels and display the current channel and its frequency on an LCD. Why replace a simple binary switch with electronics? - because the hardware project (FPV ground station) will have an Arduino and LCD built in. Also I intend to use the RSSI output of the module to set up a channel scanning feature.

The sketch compiles (Arduino 1.5.2 on Mac) but as you can see there is much repetition, its more of a hack than a programme. How do I use an array or some other device to pass parameters to the channel setting sub-routine in order to remove the duplication?

Any help gratefully received. (I have already used some code from this forum (counting button presses) and went through about another 400 pages before my head exploded.)

Regards

Alan

/* Sketch to change channel on Foxtech 5.8 Ghz Video receiver module.
And anything else I can think of!

The module requires 3 digital output pins connected to the module to select
one of 8 channels:

Channel 1 5705 Mhz = 000
Channel 2 5685 Mhz = 001
Channel 3 5665 Mhz = 010
Channel 4 5645 Mhz = 011
Channel 5 5885 Mhz = 100
Channel 6 5905 Mhz = 101
Channel 7 5925 Mhz = 110
Channel 8 5945 Mhz = 111

This is the simplest if not the most elegant approach
*/
#include <LiquidCrystal.h>;
LiquidCrystal lcd(12, 11, 10, 9, 8, 7); //Pins used for LCD,

boolean ChBut = 3; //Declare channel change button pin - we will count button presses to step through the channels

//Declare digital outputs to RX Module
boolean ChSet1 = 4; //Connect to Ch1 - module pin 9
boolean ChSet2 = 5; //Connect to Ch2 - module pin 8
boolean ChSet3 = 6; //Connect to Ch3 - module pin 7

int SigPin = 2; //Connect to RSSI - module pin 4

int RSSIVal; //Declare integer to hold RSSI value.

byte ChCount = 1; //Declare an integer to hold number of button counts and initialise at 1 press (Ch 1)
byte LastChCount = 1; //This is to compare channel counts to see if a change has occured
void setup() {
   
  pinMode(ChBut, INPUT); //Set channel change button pin to an input
  digitalWrite(ChBut, HIGH); //Set internal pull-up resistor
  
  pinMode(ChSet1, OUTPUT); //Set channel change pins to outputs
  pinMode(ChSet2, OUTPUT);
  pinMode(ChSet3, OUTPUT); 
  lcd.setCursor(0, 0); //Home the cursor
  lcd.print("Channel 1"); //Display Channel number
  lcd.setCursor(0, 1); //Set cursor to line 2
  lcd.print("5.705 Ghz"); //Display Channel frequency
  digitalWrite(ChSet1, LOW); //Set channel change pins to low (0v).
  digitalWrite(ChSet2, LOW); //This will set the default channel to 1,
  digitalWrite(ChSet3, LOW);  //which might prove unecessary when tested with hardware
  RSSIVal = analogRead(SigPin); //Set the RSSI value to the analogue voltage on SigPin (pin 2) - use later for channel sweep
  lcd.begin(16, 2); //Initialise LCD with 16 columns and 2 rows
  lcd.clear(); //Clear the screen
}

void loop() {
 
  ReadChBut(); //Read the condition of the channel change button and take action accordingly
}
// All other routines here

void ReadChBut() {
  if (digitalRead(ChBut)  == LOW)  //See if channel change button has been pressed
  {
    ChCount++;                           //if the button is pressed increment channel count
    delay(250);                           //debounce switch
  }
  if (ChCount == 9) ChCount = 1; //Only count up to 8 channels then reset counter to channel 1
  if (LastChCount != ChCount);     //Only do output if channel count has changed
  {
    if (ChCount == 1) SetCh1();    //Interogate the channel counter and set the channel
    if (ChCount == 2) SetCh2();
    if (ChCount == 3) SetCh3();
    if (ChCount == 4) SetCh4();
    if (ChCount == 5) SetCh5();
    if (ChCount == 6) SetCh6();
    if (ChCount == 7) SetCh7();
    if (ChCount == 8) SetCh8();
  }
  LastChCount = ChCount;
}

void SetCh1()
{
  lcd.clear();                                        //Clear the screen
  lcd.setCursor(0, 0);                                //Home the cursor
  lcd.print("Channel 1");                             //Display Channel number
  lcd.setCursor(0, 1);                                //Set cursor to line 2
  lcd.print("5.705 Ghz");                             //Display Channel frequency
  digitalWrite(ChSet1, LOW);                          //Outputs set to Ch 1 = 000
  digitalWrite(ChSet2, LOW);
  digitalWrite(ChSet3, LOW);
}
void SetCh2() 
{
  lcd.clear();                                        //Clear the screen
  lcd.setCursor(0, 0);                                //Home the cursor
  lcd.print("Channel 2");                             //Display Channel number
  lcd.setCursor(0, 1);                                //Set cursor to line 2
  lcd.print("5.685 Ghz");                             //Display Channel frequency
  digitalWrite(ChSet1, HIGH);                         //Outputs set to Ch 2 = 001
  digitalWrite(ChSet2, LOW);                          
  digitalWrite(ChSet3, LOW);
}

void SetCh3()
{
  lcd.clear();                                        //Clear the screen
  lcd.setCursor(0, 0);                                //Home the cursor
  lcd.print("Channel 3");                             //Display Channel number
  lcd.setCursor(0, 1);                                //Set cursor to line 2
  lcd.print("5.665 Ghz");                             //Display Channel frequency
  digitalWrite(ChSet1, LOW);                          //Outputs set to Ch 3 = 010
  digitalWrite(ChSet2, HIGH);                          
  digitalWrite(ChSet3, LOW);
}
void SetCh4()
{
  lcd.clear();                                        //Clear the screen
  lcd.setCursor(0, 0);                                //Home the cursor
  lcd.print("Channel 4");                             //Display Channel number
  lcd.setCursor(0, 1);                                //Set cursor to line 2
  lcd.print("5.645 Ghz");                             //Display Channel frequency
  digitalWrite(ChSet1, HIGH);                         //Outputs set to Ch 4 = 011
  digitalWrite(ChSet2, HIGH);                          
  digitalWrite(ChSet3, LOW);
}
void SetCh5()
{
  lcd.clear();                                        //Clear the screen
  lcd.setCursor(0, 0);                                //Home the cursor
  lcd.print("Channel 5");                             //Display Channel number
  lcd.setCursor(0, 1);                                //Set cursor to line 2
  lcd.print("5.885 Ghz");                             //Display Channel frequency
  digitalWrite(ChSet1, LOW);                         //Outputs set to Ch 5 = 100
  digitalWrite(ChSet2, LOW);                          
  digitalWrite(ChSet3, HIGH);
}
void SetCh6()
{
  lcd.clear();                                        //Clear the screen
  lcd.setCursor(0, 0);                                //Home the cursor
  lcd.print("Channel 6");                             //Display Channel number
  lcd.setCursor(0, 1);                                //Set cursor to line 2
  lcd.print("5.905 Ghz");                             //Display Channel frequency
  digitalWrite(ChSet1, HIGH);                         //Outputs set to Ch 6 = 101
  digitalWrite(ChSet2, LOW);                          
  digitalWrite(ChSet3, HIGH);
}
void SetCh7()
{
  lcd.clear();                                        //Clear the screen
  lcd.setCursor(0, 0);                                //Home the cursor
  lcd.print("Channel 7");                             //Display Channel number
  lcd.setCursor(0, 1);                                //Set cursor to line 2
  lcd.print("5.925 Ghz");                             //Display Channel frequency
  digitalWrite(ChSet1, LOW);                         //Outputs set to Ch 7 = 110
  digitalWrite(ChSet2, HIGH);                          
  digitalWrite(ChSet3, HIGH);
}
void SetCh8()
{
  lcd.clear();                                        //Clear the screen
  lcd.setCursor(0, 0);                                //Home the cursor
  lcd.print("Channel 8");                             //Display Channel number
  lcd.setCursor(0, 1);                                //Set cursor to line 2
  lcd.print("5.945 Ghz");                             //Display Channel frequency
  digitalWrite(ChSet1, HIGH);                         //Outputs set to Ch 8 = 111
  digitalWrite(ChSet2, HIGH);                          
  digitalWrite(ChSet3, HIGH);
}

Also I intend to use the RSSI output of the module to set up a channel scanning feature.

Of what module? Most of the time, RSSI seems to be guess at best. RSSI is not a measure of distance.

How do I use an array or some other device to pass parameters to the channel setting sub-routine in order to remove the duplication?

Why do you think an array is appropriate? What input does the channel-setting function need? What are you currently supplying? When you refer to your code, you should be using terms that are defined in the code or in the comments.

If you are referring to the SetCHN() functions, then, yes, you have a ridiculous amount of duplication. Print two of the functions out, on separate pieces of paper. Take a highlighter and highlight the differences. Those differences should be arguments to the single function.

This would be a good place to use an "array of structs".

A struct is a collection of variables that you can treat as a unit. You could use a struct to represent a channel, with a byte for the channel id, a char* for the name, a float for the value, and so on.

You could then set up a (constant) array of these structs to represent your list of channels. A particular channel would be specified by the index into the array, a small integer to select the channel.

Then you can simplify your code by replacing all the lengthy per-channel setups with a call to a single function, passing the struct index for the desired channel, that would set up the desired properties like the per-channel setups do now.

Does that make any sense?

-br

 if (ChCount == 9, ChCount = 1);

Well, that's fairly elegant.
Do you know how it works?

Thanks for the replies guys:

@AWOL - I know what it does - I saw it in this forum (many thanks) and I have successfully used the syntax in other projects.

@billroy - I will look up "array of structs" but can you please point me at an illustrative example.

@PaulS - RSSI is a relative signal strength indicator normally delivered as a PWM or straight analog voltage output from a receiver. I will use it to check if a channel is in use, which is likely to be my transmitter thus automating channel selection. I have no idea if an array is appropriate, which is why I asked. It seems to me that the data for each channel could be an array in the form;

Channel number, frequency, binary output

How I set this up and pass the data to the various element of the program - 3 digital pins and the LCD - is where I am unskilled. Any examples/pointers to tutorials would be gratefully accepted.

regards

Alan

I know what it does - I saw it in this forum (many thanks) and I have successfully used the syntax in other projects.

Can you explain to me what it does and why, please ?
Presumably you want to set the channel number back to 1 if it has reached 9

@AWOL - I know what it does -

I'm not sure you do - it would be more succinct, and much clearer written simply ChCount = 1;

@UKHeliBob

My counter ChCount will increment by 1 every time the button is pressed using:

ChCount++ (++ is used to add 1 to the variable)

I only require to count from 1 to 8 but the count will go on for ever (more or less depending on the variable type) unless I stop it.

I only have one button so, on the 9th button press: (ChCount == 9, ChCount = 1) which checks that the chCount is equal to 9 and resets the counter to 1, the comparison is made using "==" but could also be "= 9" or "> 8". By using "==" or "=" I could easily miss the condition "ChCount equals 9" if I get a random switch bounce, so I will possibly change that to "> 8" in the hardware testing phase.

Normally, counters are reset to 0 but the hardware has channel numbering 1-8 so I chose that range instead of 0-7.

regards

Alan

I only have one button so, on the 9th button press: (ChCount == 9, ChCount = 1) which checks that the chCount is equal to 9 and resets the counter to 1,

My reading of if (ChCount == 9, ChCount = 1); goes something like this:
Compare the variable "ChCount" to the decimal constant 9.
Throw away the result of the comparison, i.e, ignore it.
Set the variable "ChCount" to the decimal constant one.

Edit: I forgot: and do nothing with the "true" value which results from the assignment.

I saw it in this forum (many thanks) and I have successfully used the syntax in other projects

Can you show where, please?

Wow.
There is so much wrong with that explanation that I hardly know where to begin.
(ChCount == 9, ChCount = 1)always sets ChCount to 1 whatever it was previously.

the comparison is made using "==" but could also be "= 9"

No, == is a comparison, = sets the variable on the left to the value on the right.
If you may get bounce on the switch then debounce it.

Perhaps it will help to make the issue explicit. Please compare this creative use of the ',' syntax, which happens to compile but does not do what you think it does:

if (ChCount == 9, ChCount = 1);

and this, which would do what you want:

if (ChCount == 9) ChCount = 1;

See the difference?

Add "C comma operator" to your list of stuff to learn about. In this case, to avoid, mostly.

-br

@billroy - thank you, very useful - will do.

@UkHeliBob - Wow! - thank you but unhelpful. If you are finding this topic too difficult then please feel free put your feet up and watch the tele instead.

@AWOL - thank you but, realistically, you are confusing me. I find your responses a little too cryptic, all I want to know is what I can do to improve the program in the post - if stuff is wrong then just say so - but please tell me why. I am sure my ego can stand it.

Regards

Alan

but please tell me why.

See reply #8

Your issue, as billroy notes is a misunderstanding of the comma operator in C. Wikipedia has a nice explanation:Comma operator - Wikipedia

Are you by chance an Excel user? The syntax of your if would work there.

@UkHeliBob - Wow! - thank you but unhelpful. If you are finding this topic too difficult then please feel free put your feet up and watch the tele instead

It's not me that's finding it difficult.

In my experience with this and other forums there are 2 ways to answer queries.

  1. Give the answer. The OP often then copies it and learns nothing
  2. Indicate problem areas so that the OP knows where to look. The OP then learns to solve their own problems.

Getting the balance right can be difficult because responders usually have no knowledge of the OP and how they will react. In your case your assertion that

I saw it in this forum (many thanks) and I have successfully used the syntax in other projects.

cannot be true, hence some of the sharp comments here.

@UKHeliBob

OK, since you are now referring to me in the third person (OP) clearly, we are still not on the same page so I apologise and I will start again.

My original question was how to remove the repetition, not about a simple syntax error that would have been fixed when the sketch failed to deliver.

I do not expect you to write my code for me but what might be obvious to you is not necessarily the case for us less gifted individuals. I need something more than a Times crossword clue - more Daily Star or Beano end of the market.

I trawled over 400 of the 863 pages of this forum before I posted and found some of what I need, so an example or a pointer to a similar post/tutorial would be helpful.

I would like to know how I can pass different parameters, selected by a button push count, to a single iteration of the channel setting routine, either from an array or some other variable system. Please remember - Beano.

And by the way,

AWOL:

 if (ChCount == 9, ChCount = 1);

Well, that's fairly elegant.
Do you know how it works?

Since the code is wrong, you were obviously taking the Michael. Not nice to abuse the newbie.

Regards

Alan

@AWOL - reply number 8 tells me nothing - we are now in a loop. Anyway that issue is resolved.

Thanks

Alan

Since the code is wrong, you were obviously taking the Michael. Not nice to abuse the newbie.

How do I know you're a newbie?
I once had a low post count too.
I also don't know what level to pitch the crossword clues - too low and it's patronising, too high and it's dispiriting.
Besides, the code works, you told us so - it's in the thread title.

@AWOL

Yes it appears to work - having made the claim I thought I'd better check - I have uploaded it to an UNO, error fixed, The LEDs on the outputs cycle and the LCD reads. However, see title, not soak tested and not yet connected to the RX module.

If the moderators didn't realise I am a newbie, then I can only assume it must be common practice to take the mickey out of programmers who post errors in their code - bloody confused me.... no mind, as long as I know for the future.

Hang on - it says Newbie in my profile!!