next topic »

[Reply #120 on:]

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.

Code:

//  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.

[Reply #121 on:]

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.

[Reply #122 on:]

Code:

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

...

}

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

[Reply #123 on:]

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.

[Reply #124 on:]

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?

[Reply #125 on:]

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

Code:

    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.

[Reply #126 on:]

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

Code:

    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.

[Reply #127 on:]

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

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

[Reply #128 on:]

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:

Code:

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.

[Reply #129 on:]

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.

Code:

/*
    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"?

[Reply #130 on:]

Code:

    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?

[Reply #131 on:]

Code:

   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:

Code:

    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.

[Reply #132 on:]

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?

[Reply #133 on:]

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.

[Reply #134 on:]

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.