Security spotlights controller, buttons vert slugish

Projects Programming
1

Hi,
Not used to communicating on a forum, but here goes. Please advise if this ought to be posted elsewhere.

I have created a a controller, that, when the home alarm is triggered, will illuminate four spotlights around the house.

When the alarm is not triggered, the four lights can all be simultaneously set on or off, or independently be
set on for a selectable period.

The code spans more than 1250 lines, of which 1000 is probably in-line comments and white space.

My concern is that the three primary buttons used for setting the lights individually, are "sluggish" (An understatement).

The code is riddled with Switch Case statements, which is probably the cause.

I would like to know if anybody out there may be interested at looking at the code, with suggestions of making it more effective.

I have posted the code as an attachment, should some kind soul read this post and may offer advice or assistance.

Hoping someone finds my message in a bottle!
:slight_smile:

FloodLightController.ino (46 KB)

2

Hi,
Welcome to the forum.

Please read the first post in any forum entitled how to use this forum.
http://forum.arduino.cc/index.php/topic,148850.0.html .
Then look down to item #7 about how to post your code.
It will be formatted in a scrolling window that makes it easier to read.

What model Arduino are you using?

Thanks.. Tom... :slight_smile:

3

Your code it littered with delay() calls which block anything else from happening (like detecting a button press). The same is true for your tone() calls although I don't know if you call those while checking the buttons.

Also, why all the port manipulation? It doesn't seem like critical timing is necessary and doing several digitalWrite()s are a lot more human readable.

4

You should really start using arrays for your variables. Have not read the code very closely, but if you replace this:

//Timer variables
int lamp1TimeSelect = 1;   // 0 = Off,     1 = 2min,  2 = 5min, 3 = 10min
int lamp2TimeSelect = 1;   // RGB Led off, RGB Green, RGB Blue, RGB Red
int lamp3TimeSelect = 1;
int lamp4TimeSelect = 1;

with this

int lampTimeSelect[] = {1,1,1,1};

Then you could eliminate nested switch statements where the blocks of code do the same thing. For example, instead of

           switch (lampSelect)    // Indicate which lamp we're setting
           {
                case 1:     // lamp 1
                      // Display current time selection by RGB led
                         switch (lamp1TimeSelect)
                         {
                             case 0:  //Lamp switched off
                                SwitchRGBLedOff();
                             break;
                             
                             case 1:  // Set for short period
                                SwitchRGBGreenOn();
                             break;
                             
                             case 2:  // Set for medium period
                                SwitchRGBBlueOn();
                             break;
                             
                             case 3:  // Set for long period
                                SwitchRGBRedOn();
                             break;
                         }
                break;

                case 2:    // lamp 2
                      // Display current time selection by RGB led
                         switch (lamp2TimeSelect)
                         {
                             case 0:  //Lamp switched off
                                SwitchRGBLedOff();
                             break;

You could use lampSelect to index into the array and only have one switch statement.

            // Display current time selection by RGB led
            switch (lampTimeSelect[lampSelect - 1])  
            {
              case 0:  //Lamp switched off
                SwitchRGBLedOff();
                break;

              case 1:  // Set for short period
                SwitchRGBGreenOn();
                break;

              case 2:  // Set for medium period
                SwitchRGBBlueOn();
                break;

              case 3:  // Set for long period
                SwitchRGBRedOn();
                break;

              default:
                break;
            }

It would take some work to change all the references but once you did you would have code that is much more compact, I bet you could get it down to forum posting size.

5

TomGeorge:
Hi,
Welcome to the forum.

Please read the first post in any forum entitled how to use this forum.
http://forum.arduino.cc/index.php/topic,148850.0.html .
Then look down to item #7 about how to post your code.
It will be formatted in a scrolling window that makes it easier to read.

What model Arduino are you using?

Thanks.. Tom... :slight_smile:

Did you notice how big the code is?

6

Please read the first post in any forum entitled how to use this forum.

Then look down to item #7 about how to post your code.
It will be formatted in a scrolling window that makes it easier to read.

What model Arduino are you using?

Thanks.. Tom... :slight_smile:
[/quote]

Thanx, Tom. I did indeed read the first post. I also stated quite categorically that the code is immensely long; I didn't want to flood the forum with such excess code.

I built an ATMEGA 328p-PU standalone board, specifically for this. So in essence, an Arduino UNO.

The entire system is a working unit.

But I would like to tidy the code. Make it more effective.

I did start using arrays, but started getting tripped up with arrays within arrays, pointers, etc., so continued to do it the "long way round", knowing I am duplicating code unneccessarily.

The excess use of ports was to be able to effectively switch the led's initially (All on, running or flashing).

Eventually it appeared to come so easy, I used it as the default means of switching!

I'm actually looking at transferring this to an OO project, as there are so many elements involved.

But the first stop would be to reign in the excess code duplication.

The suggestion using arrays, is probably where I should and will start.

Thanx to all interested parties who have commented and posted suggestions. I am overwhelmed. I did not expect such a prompt, meaningful response.

7

If you truly want to get rid of the sluggishness, you will have to get rid of those delay() calls you are using. Since they are buried throughout the code, it will not be a trivial task. Have a look at the BlinkWithoutDelay example (File->Example->02 Digital) for how to do this.

Attached is a cut down version leveraging the power of arrays...

FloodLightController.ino (30.7 KB)

8

blh64:
Attached is a cut down version leveraging the power of arrays...

I am immensely grateful for the effort you have made to guide me in the effective use of arrays!

It was a massive task! Thank you so much.

I will first have to work through all the alterations you have made, to digest it all.

I noticed you (wisely) discarded the PORT routines. It does make it endlessly easier to follow, understand and update the code.

I will be busy with this for a while, so won't be posting too soon.

Hopefully, shortly I will be able to post the updated version, with much less delay() routines in it!

Thanx to everybody who have so kindly posted on this post.
:slight_smile:

9

@ blh64

I compiled and uploaded the code you kindly revised. Unfortunately, everything did not work out of the box as I had hoped....

I am in the process of revising the code, but it is taking quite a bit of time.

Of the items that failed, were:

The FlashLed sequence
The RGB Led does not light at all
The lampTimeSelect does not allocate the time correctly (At all, actually!)

But you have provided me the framework in which to code. It will take me a while, but I'll get it right, and then post the result.

I have learned an enormous amount working with the code you provided.

I have never used "enum" before, but going through the code and som Googled examples, I got the general gist of things. But the "enum" for the selected times still eludes me.

There appears to be a set of integers, that have variable names, that are assigned to an array, that has an enum to select the index ???

But I'll get it sorted!

I realise you don't have the board laid out, so would not be able to test. In my case, I have now mounted the project, so have to crawl up the ladder, switch the circuit off and replace the MCU and then test again.

This weekend I'll rebuild the circuit on breadboard again.

Thanx againfor your hard work!

10

djbosman:
I have never used "enum" before, but going through the code and som Googled examples, I got the general gist of things. But the "enum" for the selected times still eludes me.

There appears to be a set of integers, that have variable names, that are assigned to an array, that has an enum to select the index ???

Yes, enumerations (enum) are merely a collection of names associated with sequential integer values. They don't have to be sequential since you can actually assign them a value, but most of the time people don't do that.

They are a handy mechanism for turning bare numbers into more understandable code. Rather than having a switch statement with case 1, case 2, case 3, you might define your enum and then have a switch statement with case STOP, case START, case PAUSE,... which adds a lot to the readability/understanding of the code.

If you accomplish the same thing with a series of #defines, then you don't get any type checking. You could also accomplish the same thing with a series of const int STOP = 0; const int START = 1, etc. but way more typing.

11

OK! I have setup the circuit on breadboard again.

I have decided to break the circuit into separate individual sketches first.

Basically, the circuit comprises of:

Buttons
Led's
Lamps
RGB Led

Once I have all those individual sketches sorted, I will unite them into one big sketch again.

This is good practice for effectively using arrays, so I am looking forward to it.

12

What a weekend! Sat through the night(s), revising the code.

I am quite pleased with the results. Much more effective now. And lots of tweaks and debugging done.

The program has shrunk from a massive 1250 lines, to a mere 770 lines (Of which the first 150 is just the program info header!).

So there is probably only about 350 lines of actual code, but it is now WORKING code.

There still is some delay calls, but used where there is not potential to block required running code.

I could still improve, but for now will first settle to complete the hardware side of things.

@blh64: I re-introduced the PORT instruction for the RunLed function. I can't think of a more efficient means to get the same result. Also, I re-introduced using the interrupts for switching all the lights on and off. It just worked so much more effectively.

I have again attached the full code, should someone wish to see what it looks like now.

Thanx, everybody, for all your valuable input

FloodLightController.ino (28.1 KB)