Structure of code. Need help.

WildBill,

Thanks much.

I am trying.

I even have an old box set up with C to try and get my head around things.

But how I do things, I go off on "tangents" and get stuck with things.
Alas by that stage, I am obsessed with it and can't always let it go.

Another problem is that I am trying to improve my alarm clock and add new functions/features.

Just recently I got stuck with XOR and MOD. Oops! :blush:

I have re-written the code again and removed all the return lines and replaced with with setting a variable and then at the end returning that variable.

That way I hope: Simplifying how it is laid out.
(This is about the third version of that code now.)

I haven't tried it as sometimes the problems "hide" and I was nearly caught a few times with either the alarms not going off, or another function not working.

Anyway, thanks again.

WildBill,

Your reply earlier on was most valuable.

Alas I need help again.

And I don't really like asking for help but when it gets to this, I will try anything.

The code was "working" - yeah, there was still a couple of bugs floating around, but they weren't THAT bad.
I was tracking them down, but was working on some other stuff.

I have/had TWO copies of the file. One for the laptop to flash into the clock and one on the main PC.

I did some code modifications, uploaded them and all seemed ok. Alas: SEEMED!

So with that, I copied over the PC version as well.

Now I have a problem and I think it is in this routine, because if I change it to just return 0; the sketch works.
(As seen at the top of the sketch.)

(and yeah: I can't spell defer/e.)

Talk though below.

//  My routine to defer alarms for people who beat the alarm.
/*
    Call it with 0 return the status of the defered alarms  DISPLAY ONLY.
    Call it with 1 to set the flag to skip the next alarm.
    Call it with 2 to set the MASTER defere flag.
    Call it with 3 to return the status and clear if needed.  This is called when the alarm is active.
*/
int alarm_defere(int fctn)
{
    //
    return 0;

    int rc;
    if (fctn==0)
        //return defered_flag;
        rc = defered_flag;
    if (fctn==1)
    {
        defered_flag = defered_flag +1;
        defered_flag = defered_flag %2;
        rc = defered_flag;
    }
    if (fctn==2)      //  This is for future use as a quick turn off for all alarm - toggle.
    {
        defered_flag = defered_flag +1;
        defered_flag = defered_flag %2;
        //
        if (defered_flag == 1)
        {
            defered_flag = 3;
        }
        rc = 1;
    }
    else
    if (fctn==3)
    {
        //
        Serial.println("IOIOIOIO");
         if (defered_flag == 3)
         {
              //return 1;
              rc = 1;
         }
         else
         if(defered_flag == 1)
         {
              defered_flag = 0;
              rc = 1;
         }
         else
         {
              rc = 0;
         }
    }
    EEPROM.write(EEPROM_user+3,defered_flag);
    delay(100);
    Serial.print("defered flag is ");
    Serial.println(rc);
    Serial.println("-=-=-=-=-=-=-=-=-=-=");
    return rc;
}

You can see my effort at tracking the code at the bottom - which is probably a dismal effort - but please indulge me.

When the clock is running I want a way to defer alarms, so if I am going away for a day, get up earlier than the alarm, feel sick and am not going to work the next day: I can defer the alarm.
Probably not the best word, but the one which came to mind when I needed the function.
Pressing one button makes it skip ONLY the next alarm. Pressing another button makes it skip ALL alarms until the button is pressed again.

Now, excuse how I { } things. I want to see the start and end of the { }. Some people put the { at the end of the previous line.
To each their own. This is how I do mine.
I also try to avoid the "trick" of things like:
if (blah == foo) do_something;
Because I am not that confident yet at parsing the code correctly, so it is all how it is above.

This is also a big cut down on what it was. There were EEPROM_write() all over the place in each sub-routine. I rationalised them and put ONE at the bottom.

I have checked the { } match and they are all there.

Now what I am worried about is I have become "obsessed" with all the ELSE statements.

Do I need to explain any more on what the routine is doing?

Any variables which aren't named here - defered_flag for instance - are GLOBAL.

P.S.
"Normally" defered_flag is 0.
The clock is "ticking" and then it just stops ticking.
I have code that sends the time out the serial port/debug thingy.
I don't get to see any of the stuff I put in, because it just stops ticking and hangs.
Yeah, so I should "track" what happens with defered_flag == 0.

So I shall guess the problem is in/around there.

Things done since my post:

I noticed the missing { } around the first condition:

if (fctn==0)
//return defered_flag;
rc = defered_flag;

Missing { }

Ok.

Still fails.

Included after each line setting rc =, I put a Serial.println(rc);
Well, suffice to say it crashed and all I saw was the rc value.

On another note:
This instance rc was ONE.

int alarm_defere(int fctn)
{
    //
    return 0;

...

}

I don't see the point of all the other code in the function.

Nic,

fair question in ways.

I thought I mentioned that I put that in to determin that it was this part of the code which was failing.

I think I can see one problem now - the last "else { return 0; } very near the end.

Anyway, it is "the rest of the code" which/where the problem is.

I really am trying to get my head around it and have removed all the "return....;" which were in my earlier posted example of problems with ELSE.

I'm sorry but neither I (nor other people) can debug code that is not what is actually exhibiting the problem. Post the code that is.

You have a lot of debugging (serial) prints. How about posting what output that is producing?

I shall later Nick.

What is known:

If the "return 0;" which is at the start is there, all works.

As soon as I delete that line, things fall appart.

What information I get from the serialprint() is basically: Nothing.

All I get is the number 1 - which is set (I believe) at the top just after the

    if (fctn==0)
        //return defered_flag;
        rc = defered_flag;

//  I put a Serial.println(rc); here.
//  and after all subsequent lines like this, so at the bottom I could see the vaule.

    if (fctn==1)

So when it runs - the last time - all I saw was a "1" printed and the whole thing died. As in stopped and nothing else happened.

Ok, still going over it in my mind.....

    else
    if (fctn==3)
    {
        //
        Serial.println("IOIOIOIO");
         if (defered_flag == 3)
         {
              //return 1;
              rc = 1;
         }
         else    //  ****  NOTE
         if(defered_flag == 1)
         {
              defered_flag = 0;
              rc = 1;
         }
         else
         {
              rc = 0;
         }
    }

Is the ELSE marked above with the NOTE line needed?

"defered_flag" can be 0, 1 or 3.
This part is called when an alarm goes off.
So, if it is 0, nothing should happen and the alarm should go off/sound/happen.
If it is 1, that alarm should be skipped. Therefore rc =1, and "defered_flag" is written to 0.
If it is 3, the alarm should also be skipped. rc = 1, but "defered_flag" stays set to 3.

Ok, it is written a bit back to front.
So it looks:
Is defered_flag == 3?
Set rc = 1 to skip this alarm
Is defered_flag == 1?
Clear defered_flag for next alarm
Set rc = 1 to skip this alarm
ELSE rc = 0 to make this alarm sound.

But now I am suspicious that the extra ELSE isn't helping.

I shall remove it tonight when I get home and see what happens.

To see if the "else" is required, ask yourself the question

"Can "deferred_flag" have the values three AND one simultaneously?"

The "else" is neither hindering nor helping in this case.

Look, you really, really need to start trying to write readable code. What does defered_flag of 3 mean? This is just mumbo-jumbo. What does fctn 3 mean?

Define some constants. Give things names. I don't understand this stuff, so I don't see how you can.

It should look more like this:

 else
    if (fctn == ALARM_ACTIVE)
    {
         if (defered_flag == SKIP_ALARM_ONCE)
           {
           rc = DONT_SOUND_ALARM;
           }
        else if (defered_flag == SKIP_ALARM)
           {
           defered_flag = NORMAL_ALARM;
           rc = DONT_SOUND_ALARM;
           }
         else
           {
           rc = SOUND_ALARM;
           }
    }

You see? With names rather than numbers to code starts to make sense. All this 0, 1, 2, 3 isn't clever. It's just rubbish. It's meaningless gibberish.

I might have got the names wrong because frankly I don't understand what you are doing. But you should replace all those "magic numbers" with meaningful constants.

Nick,

I fully agree.

But - and there's always a but - I DON'T KNOW HOW TO DO THAT.

Considering I bought my FIRST Arduino on 12 April 2012 at 14:19 (Yeah, I still have the receipt).

Before that I knew NOTHING about this language. N O T H I N G !

All I knew was Level2 Basic and a bit of Z80 machine code.
Oh, well and a bit of AREXX.

The person who "introduced" me to them still insists I lean C rather than C++ before I try too much.
Ofcourse it is problematic when the code I am using uses C++ stuff. Anyway.....

Me, being who I am, couldn't wait and also needed an Alarm clock - mine died.

So it was 300% priority!

I got it working as supplied and have since learned how to code BY MY SELF.

Yes, people here have helped me, and I am very appreciative of that. VERY.

I can't afford to sit down and "learn" the language just yet. Probably won't every. Dunno. That's the future.
I get curious and try things. Sure maybe I should study things before I do it, but again: That isn't me.

So, what's gone on.

Well, I got home and sat down and read, read and read the code. Re-wrote whole routines from a working version.

Guess what!

I got it working. It now does what I want.

Now, to what you were suggesting:
How do I put "names" on return values?

If I am shown what to do/how to do it, I learn.

Friday I buy a solenoid and get back to my plant watering system. That's going to be fun.

After that, I have other projects lined up which are backing up on me. (Have I created a monster?) (smile)

For what ever reason: This is what the code looks like now.
** NOTE **
Yes, it is full of old code (// lines) because I have only just got it working and I keep "throwing code at the thing until it works" kind of thing.

Or, should I say:
When I get it working, I change things and leave the old there so I can see where I have been.

/*
    Call it with 0 return the status of the defered alarms  DISPLAY ONLY.
    Call it with 1 to set the flag to skip the next alarm.
    Call it with 2 to set the MASTER defere flag.
    Call it with 3 to return the status and clear if needed.  This is called when the alarm is active.
*/
int alarm_defere(int fctn)
{
    Serial.print("alarm defer called with ");
    Serial.println(fctn);
    int rc;
    if (fctn==0)
        return defered_flag;
    if (fctn==1)
    {
        defered_flag = defered_flag +1;
        defered_flag = defered_flag %2;
//        EEPROM.write(EEPROM_user+3,defered_flag);
//        delay(100);
//        return 0;
        rc = 0;
    }
    if (fctn==2)      //  This is for future use as a quick turn off for all alarm - toggle.
    {
        defered_flag = defered_flag +1;
        defered_flag = defered_flag %2;
        if (defered_flag == 1)
        {
            defered_flag = 3;
        }
        rc = 1;
    }
    else
    if (fctn==3)
    {
        if (defered_flag == 3)
        {
            rc = 1;
        }
        if(defered_flag == 1)
        {
            defered_flag = 0;
            //EEPROM.write(EEPROM_user+3,defered_flag);
            //delay(100);
            rc = 1;
        }
        else
        rc = 0;
        // this is because defered_flag is not set and so I need to return a 0 to make the alarm happen.
    }
    //  more EEPROM writing to go here - right?
    //EEPROM.write(EEPROM_user+3,defered_flag);
    //delay(100);
    Serial.print("Alarm defer returning value ");
    Serial.println(rc);
    return rc;
}

Oh, and the rhetorical question at the end is for me.

As you can see, I have the lines there now, just didn't get to actually make them ...... (what's the term?) "work"?

    if (fctn==2)      //  This is for future use as a quick turn off for all alarm - toggle.
    {
        defered_flag = defered_flag +1;
        defered_flag = defered_flag %2;
        if (defered_flag == 1)
        {
            defered_flag = 3;
        }

I have no idea what this all means - will you, in a month?

AWOL:

    if (fctn==2)      //  This is for future use as a quick turn off for all alarm - toggle.

{
       defered_flag = defered_flag +1;
       defered_flag = defered_flag %2;
       if (defered_flag == 1)
       {
           defered_flag = 3;
       }



I have no idea what this all means - will you, in a month?

Honestly?

Probably.
Increment "defered_flag"
Mod(2)
If it is 1, make it 3.

That is kind of how I think.

Though honestly I don't know if that is 100% true. But who really knows how they REALLY think.

But if it was written like:

    if (fctn==2)      //  This is for future use as a quick turn off for all alarm - toggle.
    {
        defered_flag = ++;
        defered_flag = %2;
        if (defered_flag == 1)
        {
            defered_flag = 3;
        }

Or some other "short hand" way, it probably would have me confused.
I don't know all the "short hand" tricks. Reading lines written like that throws my parsing system crashing.

Maybe in a few years I may be able to read it. But now, not for me.

Yes, I understand the operations being carried out, but what is the meaning of the values that result from those operations?
That's what Nick meant when he was talking about meaningful names.

It doesn't matter if you're programming in C, C++, z80 assembler or (I can hardly bring myself to type this) BASIC, meaningful names will always aid comprehension - if you don't know what they mean or how they are derived, what chance the rest of us?

Yes AWOL,

As I said: I don't know how to do that.

So instead of telling me what I do and/or don't know, explain to me how to do it so I can learn.

That would be better for me.

400 posts in and you don't know about how to make a constant?

Go read some C or C++ tutorials. Enough of this passive-aggressive crap.

lost_and_confused:
byte run(); // Returns which alarm is triggered, 0-Max_alarms, 255 means no alarm is triggered.

Excuse the "language" but: What the....?

Google for "C function prototype"

Fungus,

The code has become a mixture of the orignal code and what I did to it.

Way back when.... I was wanting to get the number of the active alarm in the system.

Someone showed me where to get the value and return it.

"alarm()" has the active alarm in it. It is returned form there to me to use.

When this was told to me I applied that and added the information in the remark.
Though reading some of the original remarks, I can not be certain.

Anyway, since then it has become accademic.

I'm now going to go away and .... what ever.

I need to do things which make me happy, not upset.

Anyway, since then it has become academic.

I'm now going to go away and .... what ever.

I need to do things which make me happy, not upset.

Best thought I've heard all day...

Bob