0

Good night! Based on stuff I get a bit here and there, came up with a program for an Arduino Audio Selector with shift registers and some features. Working as expected, only missing connecting the relay boads, but that's for future. Since I have a few devices, instead of having to write all, is there anyway of I change for instance a line and then on display show that line?

Thanks. Here's my code, realy big, try to short the part of the shift registers but seems to me it was unstable.

Arduino selector 4x20

  • Please explain what you mean when you say "is there anyway of I change for instance a line and then on display show that line?". Which line are you talking about? – VE7JRO May 23 at 22:19
  • Sorry, forgot to mention... i was thinking in something like: line1 = mp3 player, cd 1 = sony, cd2 =pioneer, and when writing the sources, only need tho change at the begginning instead of having to scroll trough entire code. 73’s CT1JQJ – Nuno Brandão May 24 at 9:22
  • In traditional C they use #define / #ifdef . In C++ there are more possibilities but interpreting/guessing your question's background that might help already. – DataFiddler May 28 at 10:37
2

I see a lot of 'almost' repetitions like :

  if (RelaySourceDigi == 7) {
    shifter.setPin(16, LOW);
    shifter.setPin(17, LOW);
    shifter.setPin(18, LOW);
    shifter.setPin(19, LOW);
    shifter.setPin(20, LOW);
    shifter.setPin(21, LOW);
    shifter.setPin(22, HIGH);
    shifter.setPin(23, LOW);
    shifter.write();
    lcd.setCursor(0, 2);
    lcd.write(8);
    lcd.setCursor(2, 2);
    lcd.print("Coaxial 3");
  }

Move this to a separate function, and make an argument (or more) of items that vary. This will greatly reduce your code, making it more managable/maintainable.

Below I changed the entire program. Note I didn't check for runtime errors, and it is highly possible I made some copy/paste errors, but it will give you a clue what you can change. As you can see, the program is radically shortened (and more maintainable).

What you need to do is:

  • Check each change
  • Check for copy/paste errors
  • Read a little bit about pointers (e.g. the &RelaySource and *t constructs)
  • Also I highly suggest not to use so many constants, but use enums, to denote the value for each different mode you have. So instead of case 1, you can write case TAPE_1.
  • Split loop in smaller functions (e.g. one function per mode).
  • I didn't install your libraries but used e.g. #include "LiquidCrystal_I2C.h" and stubbed all functions (meaning I can call them but they don't do anything, just to check for compiler errors in the main program).
  • Also instead of Aux, use a boolean (true, false).
  • You see I use a lot of (state - 1) constructs. It's common practice to start counting at 0, not at 1, it will make your life easier. Only when displaying a value for a user, add +1. Below the program.

    //Libraries
    #include "LCD.h"
    #include "LiquidCrystal_I2C.h"
    #include "Shifter.h"
    
    #define SER_Pin 8 //SER_IN 14 on 74HC595
    #define RCLK_Pin 9 //L_CLOCK 12 on 74HC595
    #define SRCLK_Pin 10 //CLOCK 11 on 74HC595
    #define NUM_REGISTERS 3 //how many registers are in the chain
    //initaize shifter using the Shifter library
    Shifter shifter(SER_Pin, RCLK_Pin, SRCLK_Pin, NUM_REGISTERS);
    
    LiquidCrystal_I2C lcd(0x27, 2, 1, 0, 4, 5, 6, 7, 3, POSITIVE); // Set the LCD I2C address
    
    const int ButtonPinUp = 2;
    int ButtonPinUpState = 0;
    const int ButtonPinDn = 3;
    int ButtonPinDnState = 0;
    const int MuteButtonPin = 5;
    int MuteButtonPinState = 0;
    const int AuxButtonPin = 4;
    int AuxButtonPinState = 0;
    const int fbutton = 6;
    int RelaySource = 1;
    int RelaySourceDigi = 1;
    int RelayRec = 1;
    int RecDigi = 1;
    int Mute = 0;
    int Mode = 0;
    int Aux = 1;
    int state = 0;
    int old = 0;
    int ButtonPoll = 0;
    
    
    byte opt[8] = {
      0b00000,
      0b01110,
      0b11011,
      0b10001,
      0b10001,
      0b10001,
      0b11111,
      0b00000
    };
    
    byte coax[8] = {
      0b00100,
      0b01110,
      0b11111,
      0b10001,
      0b10001,
      0b01110,
      0b00100,
      0b00100
    };
    
    byte mut[8] = {
      0b00001,
      0b00011,
      0b00111,
      0b11111,
      0b11111,
      0b00111,
      0b00011,
      0b00001
    };
    
    
    byte outp[8] = {
      B00000,
      B00100,
      B00110,
      B11111,
      B11111,
      B00110,
      B00100,
      B00000
    };
    
    byte upp[8] = {
      0b11111,
      0b11111,
      0b11111,
      0b11111,
      0b11111,
      0b11111,
      0b11111,
      0b11111
    };
    
    byte inpp[8] {
      0b10000,
      0b10000,
      0b10100,
      0b10110,
      0b11111,
      0b00110,
      0b00100,
      0b00000
    };
    
    
    byte ann[8] {
    
      0b11111,
      0b10001,
      0b10101,
      0b10101,
      0b10001,
      0b10101,
      0b10101,
      0b11111
    };
    
    
    byte dnn[8] = {
      0b11111,
      0b10011,
      0b10101,
      0b10101,
      0b10101,
      0b10101,
      0b10011,
      0b11111
    };
    
    byte too[8] = {
      0b00000,
      0b00100,
      0b00110,
      0b11111,
      0b00110,
      0b00100,
      0b00000,
      0b00000
    };
    
    void printAtCursor(int a, int b, const char* c)
    {
      lcd.setCursor(a, b);
      lcd.print(c);
    }
    
    void setup() {
      lcd.begin(20, 4);
      lcd.backlight();
    
      byte* list[] = {outp, coax, mut, opt, upp, inpp, ann, dnn, too };
      for (int n = 0; n < 9; n++)
      {
        lcd.createChar(n + 1, list[n]);
      }
    
      pinMode(fbutton, INPUT);
      pinMode(ButtonPinUp, INPUT);
      pinMode(MuteButtonPin, INPUT);
      pinMode(ButtonPinDn, INPUT);
      pinMode(AuxButtonPin, INPUT);
    
      for (int n = 11; n < 14; n++)
      {
        pinMode(n, OUTPUT);
      }
    
      printAtCursor(3, 0, "Digital/Analog");
      printAtCursor(3, 1, "Audio Selector");
      printAtCursor(4, 2, "Built-In DAC");
      printAtCursor(2, 3, "V2.0f (23052020)");
      delay(4000);
      lcd.clear();
    }
    
    
    void setPin(int first, int last, int pin)
    {
      for (int n = first; n < last + 1; n++)
      {
        shifter.setPin(n, pin == n ? HIGH : LOW);
      }
    
      shifter.write();
    }
    
    
    void setLines(int a, int b, const char* t, int x = -1, const char* u = "")
    {
        lcd.setCursor(0, a);
        lcd.write(b);
        lcd.setCursor(2, a);
        lcd.print(t);
        if (x != -1)
        {
          lcd.write(x);
          lcd.print(u);
        }
    }
    
    void loop() 
    {
      // shifter.clear(); //set all pins on the shift register chain to LOW
      shifter.write(); //send changes to the chain and display them
      //check button states
      ButtonPinUpState = digitalRead(ButtonPinUp);
      ButtonPinDnState = digitalRead(ButtonPinDn);
      MuteButtonPinState = digitalRead(MuteButtonPin);
      AuxButtonPinState = digitalRead(AuxButtonPin);
      ButtonPoll = digitalRead(fbutton);
    
      if (AuxButtonPinState == HIGH && RelaySource == 1 && state == 1) {
        lcd.clear();
        Aux           = ~Aux;
        digitalWrite(13, Aux == 0 ? LOW : HIGH);
      }
    
      if (ButtonPoll == HIGH) {
        state = old + 1;
      }
    
      lcd.clear(); // Happens in all cases
      switch (state) {
        case 1: // Fall through
        case 2: // Fall through
        case 3: // Fall through
        case 4:
          lcd.setCursor(1, state - 1);
          lcd.write(5);
          old = state;
          break;
    
        default:
          old = 0;
          break;
      }
    
      // Assuming state = 1..4
      if (ButtonPinUpState == HIGH)
      {
        lcd.clear();
        int* t = NULL;
        int maxValue = -1;
    
        switch (state)
        {
          case 1: t = &RelaySource;     maxValue = 8; break;
          case 2: t = &RelayRec;        maxValue = 5; break;
          case 3: t = &RelaySourceDigi; maxValue = 8; break;
          case 4: t = &RecDigi;         maxValue = 4; break;
          default: break;
        }
    
        *t = min(1, max(maxValue, *t + (ButtonPinUpState == HIGH ? 1 : 0) + (ButtonPinDnState == HIGH ? -1 : 0)));
      }
    
      if (MuteButtonPinState == HIGH) {
        lcd.clear();
        Mute = (Mute == 0 ? 1 : 0);
        digitalWrite(12, Mute == 0 ? HIGH : LOW);
      }
    
      // RelaySource
      setPin(0, 7, RelaySource - 1);
      if (RelaySource == 1 && Aux == 0)
      {
        setLines(0, 7, "D", 9, "A:");
      }
      else
      {
        const char* rs1[] = { "Line 1", "Line 2", "CD 1", "CD 2", "Tape 1", "Tape 2/DCC", "Tape 3/DAT", "MD", "MD" };
        setLines(0, 7, rs1[RelaySource - 1]);
      }
    
      if (RelaySource == 8)
      {
        shifter.setPin(0, LOW); //set pin 1 in the chain(second pin) HIGH
      }
    
      // RelayRec
      int         rr1[] = { 8      , -1   , 9       , 10      , 11       };
      const char* rr2[] = {"Source", "OFF", "Tape 1", "Tape 2", "Tape 3" };
      const int   rr3[] = { -1     , -1   , 9       , 9       , 9        };
      const char* rr4[] = { ""     , ""   , "2,3"   , "1,3"   , "1,2"    };  
    
      setPin(8, 11, rr1[RelayRec - 1]);
      setLines(1, 6, rr2[RelayRec - 1], rr3[RelayRec - 1], rr4[RelayRec - 1]);
    
      // RelaySourceDigi
      setPin(16, 23, 17 - RelaySourceDigi);
      if (RelaySource == 1 && Aux == 0) 
      {
        lcd.setCursor(7, 0);
        const char* t[] = { "Optical 1", "Optical 2", "Optical 3", "Optical 4", "Coaxial 1", "Coaxial 2", "Coaxial 3", "Coaxial 4" };
        lcd.print(t[RelaySource - 1]);
      }
    
      const char* t1[] = { "Sony 5-CD", "Sony MD[", "Optical 3", "Optical 4", "Philips CD-R [", "Coaxial 2", "Coaxial 3", "Coaxial 4" };
      const int   t2[] = { 4          , 4         , -1         , -1         , 2               , -1        , -1         , -1          };
      const char* t3[] = { "]"        , "]"       , ""         , ""         , "]"             , ""        , ""         , ""          };
      setLines(2, 8, t1[RelaySourceDigi - 1], t2[RelaySourceDigi - 1], t3[RelaySourceDigi - 2]);
    
      // RecDigi
      const char* t[] = { "Optical Out1", "Optical Out 2", "Coaxial Out 1", "Coaxial Out2" };
      setPin(12, 15, 11 + RecDigi);
      setLines(3, 6, t[RecDigi - 1]);
    
      delay(150);
    }
    

Update

I wanted for myself to see how to clean it fully, so I tried below.

Still check for copy/paste problems, as I didn't execute the sketch (but it compiles without warnings).

Original sketch: 2746 bytes (8%) program storage space, 142 bytes (6%) of dynamic memory Sketch below : 1546 bytes (4%) program storage space, 40 bytes (2%) of dynamic memory.

Conclusion:

  • 25% less program storage space used
  • 67% less dynamic memory used
  • Less lines of code
  • Better readability
  • Better maintainability
  • No (or less) code duplication
  • Smaller function complexity
  • Less statements (added more to initialization)

Code:

//Libraries
#include "LCD.h"
#include "LiquidCrystal_I2C.h"
#include "Shifter.h"

const uint8_t SER_Pin = 8; //SER_IN 14 on 74HC595
const uint8_t RCLK_Pin = 9; //L_CLOCK 12 on 74HC595
const uint8_t SRCLK_Pin = 10; //CLOCK 11 on 74HC595
const uint8_t NUM_REGISTERS = 3; //how many registers are in the chain

Shifter _shifter(SER_Pin, RCLK_Pin, SRCLK_Pin, NUM_REGISTERS);
LiquidCrystal_I2C _lcd(0x27, 2, 1, 0, 4, 5, 6, 7, 3, POSITIVE); // Set the LCD I2C address

enum EMode
{
  RELAY_SOURCE,
  RELAY_REC,
  RELAY_SOURCE_DIGI,
  REC_DIGI,
  LAST_MODE
};

static const uint8_t _buttonPinUp = 2;
bool _buttonPinUpState = false;
static const uint8_t _buttonPinDown = 3;
bool _buttonPinDownState = false;
static const uint8_t _muteButtonPin = 5;
bool _muteButtonPinState = false;
static const uint8_t _auxButtonPin = 4;
bool _auxButtonPinState = false;
static const uint8_t _forwardButton = 6;
uint8_t _relaySource = 0;
uint8_t _relaySourceDigi = 0;
uint8_t _relayRec = 0;
uint8_t _recDigi = 0;
bool _mute = false;
bool _aux = true;
EMode _mode = RELAY_SOURCE;
bool _buttonPoll = false;

static const byte OPT[8]  PROGMEM = {  0b00000,  0b01110,  0b11011,  0b10001,  0b10001,  0b10001,  0b11111,  0b00000 };
static const byte COAX[8] PROGMEM = {  0b00100,  0b01110,  0b11111,  0b10001,  0b10001,  0b01110,  0b00100,  0b00100 };
static const byte MUT[8]  PROGMEM = {  0b00001,  0b00011,  0b00111,  0b11111,  0b11111,  0b00111,  0b00011,  0b00001 };
static const byte OUTP[8] PROGMEM = {  B00000 ,  B00100 ,  B00110 ,  B11111 ,  B11111 ,  B00110 ,  B00100 ,  B00000  };
static const byte UPP[8]  PROGMEM = {  0b11111,  0b11111,  0b11111,  0b11111,  0b11111,  0b11111,  0b11111,  0b11111 };
static const byte INPP[8] PROGMEM = {  0b10000,  0b10000,  0b10100,  0b10110,  0b11111,  0b00110,  0b00100,  0b00000 };
static const byte ANN[8]  PROGMEM = {  0b11111,  0b10001,  0b10101,  0b10101,  0b10001,  0b10101,  0b10101,  0b11111 };
static const byte DNN[8]  PROGMEM = {  0b11111,  0b10011,  0b10101,  0b10101,  0b10101,  0b10101,  0b10011,  0b11111 };
static const byte TOO[8]  PROGMEM = {  0b00000,  0b00100,  0b00110,  0b11111,  0b00110,  0b00100,  0b00000,  0b00000 };

static const byte* const CHAR_LIST[] PROGMEM = {OUTP, COAX, MUT, OPT, UPP, INPP, ANN, DNN, TOO };
static const uint8_t PINS[] PROGMEM = { _forwardButton, _buttonPinUp, _muteButtonPin, _buttonPinDown, _auxButtonPin };

static uint8_t* const    MODE_VALUES[] PROGMEM = { &_relaySource, &_relayRec, &_relaySourceDigi, &_recDigi };
static const uint8_t     MAX_VALUES[] PROGMEM = { 7             , 4         , 7                , 3         };

static const char* const RELAY_SOURCES[] PROGMEM = { "Line 1", "Line 2", "CD 1", "CD 2", "Tape 1", "Tape 2/DCC", "Tape 3/DAT", "MD", "MD" };

static const char* const RELAY_SOURCE_DIGI_1[] PROGMEM = { "Sony 5-CD", "Sony MD[", "Optical 3", "Optical 4", "Philips CD-R [", "Coaxial 2", "Coaxial 3", "Coaxial 4" };
static const int         RELAY_SOURCE_DIGI_2[]         = { 4          , 4         , -1         , -1         , 2               , -1        , -1         , -1          };
static const char* const RELAY_SOURCE_DIGI_3[] PROGMEM = { "]"        , "]"       , ""         , ""         , "]"             , ""        , ""         , ""          };

static const int         RELAY_REC_1[] PROGMEM = { 8      , -1   , 9       , 10      , 11       };
static const char* const RELAY_REC_2[] PROGMEM = {"Source", "OFF", "Tape 1", "Tape 2", "Tape 3" };
static const int         RELAY_REC_3[] PROGMEM = { -1     , -1   , 9       , 9       , 9        };
static const char* const RELAY_REC_4[] PROGMEM = { ""     , ""   , "2,3"   , "1,3"   , "1,2"    };  

static const char* const DIGI_RELAY_SOURCES[] PROGMEM = { "Optical 1", "Optical 2", "Optical 3", "Optical 4", "Coaxial 1", "Coaxial 2", "Coaxial 3", "Coaxial 4" };


void printAtCursor(int a, int b, const char* c)
{
  _lcd.setCursor(a, b);
  _lcd.print(c);
}


void setup() 
{
  _lcd.begin(20, 4);
  _lcd.backlight();

  for (int n = 0; n < 9; n++)
  {
    _lcd.createChar(n + 1, (byte*) CHAR_LIST[n]);
  }

  for (int n = 0; n < sizeof(PINS) / sizeof(uint8_t); n++)
  {
    pinMode(PINS[n], INPUT);
  }

  for (int n = 11; n < 14; n++)
  {
    pinMode(n, OUTPUT);
  }

  printAtCursor(3, 0, "Digital/Analog");
  printAtCursor(3, 1, "Audio Selector");
  printAtCursor(4, 2, "Built-In DAC");
  printAtCursor(2, 3, "V2.0f (23052020)");
  delay(4000);
}


void setPin(int first, int last, int pin)
{
  for (int n = first; n < last + 1; n++)
  {
    _shifter.setPin(n, pin == n ? HIGH : LOW);
  }

  _shifter.write();
}


void setLines(int a, int b, const char* t, int x = -1, const char* u = "")
{
    _lcd.setCursor(0, a);
    _lcd.write(b);
    _lcd.setCursor(2, a);
    _lcd.print(t);

    if (x != -1)
    {
      _lcd.write(x);
      _lcd.print(u);
    }
}


void ResetShifter()
{
  // _shifter.clear(); //set all pins on the shift register chain to LOW
  _shifter.write(); //send changes to the chain and display them
}


void ReadButtons()
{
  _buttonPinUpState = digitalRead(_buttonPinUp) == HIGH;
  _buttonPinDownState = digitalRead(_buttonPinDown) == HIGH;
  _muteButtonPinState = digitalRead(_muteButtonPin) == HIGH;
  _auxButtonPinState = digitalRead(_auxButtonPin) == HIGH;
  _buttonPoll = digitalRead(_forwardButton) == HIGH;
}


void HandleAuxButton()
{
  if (_auxButtonPinState && (_relaySource == 0) && (_mode == EMode::RELAY_SOURCE)) 
  {
    _aux = !_aux;
    digitalWrite(13, _aux ? HIGH : LOW);
  }
}


void HandlePollButton()
{
  if (_buttonPoll) 
  {
    _mode = (EMode) ((_mode + 1) % EMode::LAST_MODE);
  }
}


void StartLcd()
{
  _lcd.clear(); // Happens in all cases
  _lcd.setCursor(1, (int) _mode);
  _lcd.write(5);
}


void HandleButtonPinUp()
{
  if (_buttonPinUpState)
  {
    static uint8_t* const modeValue = MODE_VALUES[_mode];
    static uint8_t maxValue = MAX_VALUES[_mode];
    *modeValue = min(0, max(maxValue, *modeValue + (_buttonPinUpState ? 1 : 0) + (_buttonPinDownState ? -1 : 0)));
  }
}


void HandleMuteButton()
{
  if (_muteButtonPinState) 
  {
    _mute = !_mute;
    digitalWrite(12, _mute ? HIGH : LOW);
  }
}


void ProcessRelaySource()
{
  // RelaySource
  setPin(0, 7, _relaySource);
  if (_relaySource == 0 && !_aux)
  {
    setLines(0, 7, "D", 9, "A:");
  }
  else
  {
    setLines(0, 7, RELAY_SOURCES[_relaySource]);
  }

  if (_relaySource == 7)
  {
    _shifter.setPin(0, LOW); //set pin 1 in the chain(second pin) HIGH
  }
}


void ProcessRelayRec()
{
  setPin(8, 11, RELAY_REC_1[_relayRec]);
  setLines(1, 6, RELAY_REC_2[_relayRec], RELAY_REC_3[_relayRec], RELAY_REC_4[_relayRec]);
}


void ProcessRelaySourceDigi()
{
  setPin(16, 23, 17 - _relaySourceDigi);
  if (_relaySource == 0 && !_aux) 
  {
    _lcd.setCursor(7, 0);
    _lcd.print(DIGI_RELAY_SOURCES[_relaySource]);
  }
  setLines(2, 8, RELAY_SOURCE_DIGI_1[_relaySourceDigi - 1], RELAY_SOURCE_DIGI_2[_relaySourceDigi - 1], RELAY_SOURCE_DIGI_3[_relaySourceDigi - 2]);
}


void ProcessRecDigi()
{
  static const char* const t[] PROGMEM = { "Optical Out1", "Optical Out 2", "Coaxial Out 1", "Coaxial Out2" };
  setPin(12, 15, 12 + _recDigi);
  setLines(3, 6, t[_recDigi]);
  delay(150);
}


void loop() 
{
  ResetShifter();
  ReadButtons();

  HandleAuxButton();
  HandlePollButton();
  StartLcd();
  HandleButtonPinUp();
  HandleMuteButton();

  ProcessRelaySource();
  ProcessRelayRec();
  ProcessRelaySourceDigi();
  ProcessRecDigi();
}
| improve this answer | |
  • 1
    Hello. Still trying to figure that, newbbie to arduino, thanks for the info – Nuno Brandão May 24 at 9:25
  • 1
    upvote for a very nice answer ... you can further simplify your code ... lcd.setCursor(0, 2); lcd.write(8); lcd.setCursor(2, 2); lcd.print("Sony MD ["); lcd.write(4); lcd.print("]"); .... is same as this .... lcd.setCursor(0, 2); lcd.print("\x08 Sony MD [\x04]"); – jsotola May 26 at 17:32
  • @jsotola thanks ... however, in my optimized code I'm not sure if I can use it as positions for the cursor are variable. – Michel Keijzers May 26 at 20:51
  • 1
    Thank you very much, after seeing this I realized I’m in fact a newbbie! I’ll try it tomorroy, meanwhile tried with a rotary encoder and menu but dont like that much. – Nuno Brandão May 26 at 23:15
  • I'm a professional software engineer, so this is kind of 'normal stuff' fo rme, although I still don't know much about many Arduino specific details. (btw the sketch can be improved more, but these are the 'low hanging fruit' items. If you next time try to make your loop as simple as I did know, your overall design will be much easier in general. – Michel Keijzers May 27 at 8:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.