Is there such a thing as too much code in a switch statement?

Projects Programming
1

Good evening everyone,
I am continuing my Arduino dishwasher project. Thing is, I noticed my code becomes unresponsive if I uncomment the "controlLED" function here:

void cycleRunning() {
  display.setBacklight(100);
  runCycle();
  countDownFunction();
  pauseCycleFunction();
  detectErrors();
  //controlLED();
  if (millis() - lastButtonReadingTime > 100) {
    buttonReading = readButtons();
    lastButtonReadingTime = millis();
  }
  setButtons();
}

whereas If I leave it like that the code runs perfectly. Thing is, I need to control those LEDs for the control panel in order to give the user an idea of what is going on.
A coder friend of mine says you should never put too much code in switch cases (my bad really) but could that lead to instability? In particular, what happens is that the program runs well through currentCycleState 1,2 but when it goes to currentCycleState 3 I get that millis() - stageStartTime never becomes greater than zero. If I leve it commented everything runs fine

Also, if I edit the fillFunction like this:

bool fillFunction() {
  switch (fillFunctionState) {
    case 0: //activate fill solenoid
      digitalWrite(fillSolenoid, HIGH);
      interrupts(); // enable interrupts
      fillFunctionState = 1;
      break;
    case 1: // static fill
      if (flowMeterPulses >= 2 * FILL_PULSES / 3) {
        digitalWrite(washingPump, HIGH);
      }
      if (flowMeterPulses >= FILL_PULSES) {
        digitalWrite(fillSolenoid, LOW);
        //fillSequenceStartTime = millis();
        noInterrupts();//disable interrupts
        //digitalWrite(washingPump, HIGH);
        //timeFlowSwitchWasHigh = millis();
        //fillFunctionState = 2;

      }
      Serial.println(F("OK"));
      return true; // fill routine successful
      break;
  }
  return false;
}

instead of this:

bool fillFunction() {
  switch (fillFunctionState) {
    case 0: //activate fill solenoid
      digitalWrite(fillSolenoid, HIGH);
      interrupts(); // enable interrupts
      fillFunctionState = 1;
      break;
    case 1: // static fill
      if (flowMeterPulses >= FILL_PULSES) {
        digitalWrite(fillSolenoid, LOW);
        noInterrupts();//disable interrupts
        fillFunctionState = 2;
      }
      Serial.println(F("OK"));
      return true; // fill routine successful
      break;

    case 2:
      Serial.println("OK");
      return true;
      break;
  }
  return false;
}

the same problem manifests. Why is that?
Attached is the whole code, thanks in advance to anyone who can help me!
Arduino_Dishwasher_7SEG 3-8-22.zip (14.9 KB)

2

Only because the code becomes difficult to verify due to complexity. Not operationally.

2 Likes
3

You call a function we can't see.

You are using interrupts it looks like, we see no details.

Post a complete sketch and describe the problem again. What happens that should not, or doesn't happen that should?

Why a new thread? You are gonna have to go over lotsa old ground or make us chase after your progress.

a7

4

Where is this function?

5

You should have a REALY GOOD REASON before you disable interrupts for more than a few lines of code.

6 
//FUNCTION TO CONTROL LEDs ON THE CONTROL PANEL
void controlLED() {
  byte bits = B11111111;
  bitWrite(bits, 0, !digitalRead(rinseAidSensor));
  bitWrite(bits, 1, !digitalRead(saltSensor));
  bitWrite(bits, progIndex + 2, 0);
  writePCF(bits, 255);
}

I took the time to pull the function from the ZIP, but in this forum, few will do that.

It I understand your issue; a call to the above function causes the sketch to become non-responsive?

Any program documentation?

7

Here is all the code in a Wokwi simulation: https://wokwi.com/projects/339029537180353106.
The function "controlLED()" is in the file "controlpanel.h", line 73.
The function "writePCF()" is just above that, and uses the I2C bus.

You have made a project that is too large and too complex. If you want to continue with it, then you have to improve the quality and the readability of the code.

I can give a few notes, but this is just an indication:

  1. Which Arduino board is used and which libraries ? A schematic would also be helpful.
  2. It does not compile.
  3. Never declare a variable (a normal integer or a object) in a *.h file.
  4. Never put a function in a *.h file.
  5. There is no need to #include <Arduino.h>
  6. Why does the file "controlpanel.h" have a gap of 26 empty lines ?
  7. This is typing mistake in EEPROM.h: "#ifndef eeproh.h".
  8. Why is the file EEPROM.h called EEPROM.h, there is no eeprom code in it.
  9. Using a 'enum' with states makes it better to read.

A problem with the I2C bus could halt the sketch. There is a timeout function (if the Arduino Uno is used), but first you have to improve the I2C bus.

1 Like
8

The millis() function will not be updated when interrupts are updated.
That is why the period of noInterrupts should be as short as possible.
Usually a few lines of code apart.
Here you have an unknown amount of code as interrupts are disabled in one case and enabled in another case. This makes verifying your code extremely difficult, as we now need to see if the code flows through both cases without ever using millis or other interrupt depending functions.
I strongly suggest that you keep the nointerrupt and interrupt in one code block.

9

First of all I want to thank everybody who took the time to analyze my code. I wasn't expecting so much attention and it's very appreciated.

I will try to answer more specifically about what the code does and how it should behave.
So, every state the cycle goes through (and for longer cycles there are 27 of them between prewash, wash, rinses and drying) is either time controlled or action controlled. What do I mean by this? Well, in order to give the user a visual indication of how much time is left till the end of the cycle, I have established that filling takes around a minute, draining around the same amount of time and heating about 1 minute for every 3°C increase in temperature required. However, these are all actions controlled by flowmeter or NTC, so even if the time required by these exceeds the time preset then they will still be run. As an example, let's say the cycle shows 2:00 hours left and heating takes 20 minutes. Well, if the preset time is let's say 15 minutes, then after 15 minutes heating will still be run and the display will show 1:45 minutes for the remaining 5 minutes until heating is over.
The same thing happens for filling and, most importantly, time controlled actions. For example, the rinse only cycle fills with water and then proceeds to rinse for around 12 minutes. Why am I mentioning this? Because right after the fill phase I have a problem where millis() does not increase, and I seem to understand this might be due to interrupts.

However, I need to use interrupts, since the fill phase needs to read a built-in flowmeter (which is really a reed switch with a small turbine and a magnet) until it reads a certain amount of pulses, which I estimated to be around 950 to 1000.
My objective would be to read these pulses activating the washing pump after around 700 pulses so that I have a static water fill (washing pump disabled) and a dynamic water fill (washing pump enabled)

Hoping to have given a clearer background of how the code works, now I will ask some questions.

Arduino Nano, Wire libraries and SevenSegmentTM1637 by Bram Harmsen

True, but downgrading the SevenSegment library to 1.0.0 seems to solve the issue

Didn't know this, thanks. Is a .cpp file more appropriate for this? I declared variables inside the .h files because if I declared them inside the main code the compiler would tell me the variables were "not defined in this scope" when compiling

I will look into it then. I seem to understand it does the same thing as writing #define like I used it, right?

I have not used pull up resistors, could it be due to that?

Didn't know this, thanks. But, is it a typo or am I missing something? Shouldn't it be that the period of interrupts should be as short as possible, rather than the period of noInterrupts?

Finally, how could I modify the fill function in order to work? Is something like this fine? I have interrupts() and noInterrupts() in a single block of code.

bool fillFunction() {
  switch (fillFunctionState) {
    case 0: //activate fill solenoid
      digitalWrite(fillSolenoid, HIGH);
      
      fillFunctionState = 1;
      break;
    case 1: // static fill
      interrupts(); // enable interrupts
      if (flowMeterPulses >= FILL_PULSES) {
        digitalWrite(fillSolenoid, LOW);
        noInterrupts();//disable interrupts
        fillFunctionState = 2;
      }
      noInterrupts();
      break;

    case 2:
      Serial.println("OK");
      return true;
      break;
  }
  return false;
}
10

To clarify:
An ISR should be as short as possible. So, an interrupt should basically only set a flag that will be handled and reset in the loop().
The period that you disable interrupts should also be as short as possible. You should take care that a noInterrupts() is always followed (asap) by a interrupts().
If you put a noInterrupts() in one case and a interrupts() in another case, it will be really hard to keep track...

11

They are in one block, that is good. But it should be in opposite order...
Disable interrupt;
Do something that cannot be done with interrupts happening;
Enable interrupts;
All in pseudo code....

12

The Arduino Nano runs at 4.5V when using only the USB power (a diode causes a 0.5V voltage drop). If you add more things, the voltage could drop more. How is everything powered ? You can draw a schematic/diagram on a piece of paper and make a photo of it.

There are examples for flow meter code that turns on and off interrupts for a long time. Sooner or later it will interfere with other code. Can you remove that code (or comment it out), then you can look for better code. You need a counter in the interrupt and using millis() with the number of pulses to calculate the flow per second (or per minute).

Thanks to johnwasser to spot the wrong use of 'noInterrupts()'. No sketch will work with that. Can you remove all the 'noInterrupts()' and all the 'interrupts()' calls ? Verify once more that they all have gone.

Maybe by downgrading the SevenSegment library, also fixed bugs come back again :grimacing:

The "setup()" and "loop()" are usually in the main *.ino file. You can put functions and other global variables into other *.ino files.
Global variables that are used in multiple files are usually in the main *.ino file.

In the past, a 'enum' was the same as a '#define'. Today with C++, the 'enum' is its own type.

You do not only need pullup resistors, you need the right value for the pullup resistors. You need short wires, SDA not next to SCL, no voltage mismatch, and so on.

13

Thing is, I need to do something WHILE interrupts are happening, not when they aren't happening. What should I do?

Also, do I really need to disable interrupts? When I disable the water inlet solenoid the turbine stops spinning shortly after and doesn't give any more pulses, so could I simply leave the code without disabling interrupts?

14

Tip: replying to someone's post does not work well on this forum. We use the main "REPLY" button at the bottom.
To answer a question, select someones question with the mouse, then click "Quote".
Like this:

No, only in some special cases and only for a short time.
Forget your pulse counting code and interrupts, it is no good.

15

Thanks Koepel for your invaluable contribution. I learned a lot by reading your answer
I am powering everything off a Xiaomi smartphone power adaptor rated at 67W maximum, 5 volts is around 6A. I get around 4.7V on the 5V line from the Nano pins, but my plan is to use a 12V switching adapter connected to the Vin pins as soon as it arrives from Aliexpress (which might take a while!). The TM1637 and PCF8575 are then connected to the 5V line and the current draw I get is around 100mA with the LEDs switched on.

I will do that ASAP

But I would get "variable X is not defined in this scope" warning when using that certain variable in a .h file

Well, short wires are a bit of a problem since the control panel and the main board are around 80cm apart. But I am using grounded wire with metallic insulation, that should help right? As for SDA next to SCL, how can I avoid that if pins A4 and A5 are right next to each other?

16

My bad @Koepel , I will use the "universal" Reply button.

How can I edit that then? I do not know what the turbine displacement is (and frankly I don't care) I only need to count those 950-1000 pulses

17

Yep, as suggested: do not make any calls to
noInterrupts() or interrupts(). By default interrupts are enabled...

18

The problems with the way you are using interrupts has been discussed in previous replies. You just need to go learn how to use them properly. It's not feasible to provide a full tutorial on interrupts in a discussion thread.

19

Alright, I removed interrupts() and noInterrupts() calls and now the code runs beautifully. I will look at methods to split a file into .ino files to make the code easier to write

20

There's got to be a reward for this heroic effort beyond the sincere thanks for using wokwi so we could all not have to do the same work k order to see the code…

But I will just thank you!

a7

1 Like