ok so i coded this fucntion and was wondering if there is any way some one can clear this code up?
to me it feels like its taking up alot of space and that its repeating it self in a way over and over....
code was more then 9000 char so i had to place the ino
cleanup.ino (26.2 KB)
What does it do?
if (timerAmPm1On==1){
myGLCD.print("AM", 92, 32);
} else {
myGLCD.print("PM", 92, 32);}
if (timerAmPm1Off==1){
myGLCD.print("AM", 142, 43);
} else {
myGLCD.print("PM", 142, 43);}
}
WTF? Is there ANY possible situation where timerAmPm1On and timerAmPm1Off will both be true? If there is, you should be ashamed of yourself for choosing such crappy names for the variables.
PaulS:
if (timerAmPm1On==1){
myGLCD.print("AM", 92, 32);
} else {
myGLCD.print("PM", 92, 32);}
if (timerAmPm1Off==1){
myGLCD.print("AM", 142, 43);
} else {
myGLCD.print("PM", 142, 43);}
}
WTF? Is there ANY possible situation where timerAmPm1On and timerAmPm1Off will both be true? If there is, you should be ashamed of yourself for choosing such crappy names for the variables.
what your seeing here is 2 different variables...
what this does is a relay timer...
the ampmon is for getting the time to be turn on with a rtc on a 12hr mode....
the ampmoff is for the time to be turned off on a 12 hr mode....
the user has the ability to set when it turns on and when it turns off... so when u ask can both ampm on/off be true on both? yes it can but will do 2 different things...
But what is the whole function trying to acheive?
it checks and displays a int...? the user can set 4 timer on/off this displays what the user has set them at
but why does this need to be explain? doesn't the code speak for it self by looking at it?
bryanmc1988:
but why does this need to be explain? doesn't the code speak for it self by looking at it?
Sorry this just made me laugh so loud.
In short, NO it doesn't. What is "misting"?
If you present a clear concise objective that your function has to achieve, then you can create clear, concise code to achieve it.
it checks and displays a int...? the user can set 4 timer on/off this displays what the user has set them
So ALL that code is JUST to display the settings of 4 timers? That's ludicrous. If it were using an army of robots to place random objects onto a field to represent the values, then maybe.
Could I suggest you start from the beggining.
Step 1) Write a function that will just display a time. (Just pick a time at random and hard code it) You should be able to achieve this within about a dozen lines of code. (allowing for some nice formatting of text and drawing boxes etc)
Step 2) Modify the function so that it takes it's value from a variable. This step should barely make any difference to the size of the code block
Step 3) Change the variable to be an array of the same type. (you said 4 didn't you). You'll then need to change your function to display the value of just one member of this array.
Step 4) Alter your function so that it can accept an index parameter. Use this within your code block to point to the member of that array I mentioned earlier.
You will now have a nice concise function that will display ANY of the four timer values, simply by calling it with a reference to the one you want.
If you want all four displayed at the same time, create a for loop and call it all four times.
If the finished code to do this doesn't fit onto my screen (with it's modest 1024 x 768 resolution) then you're doing it wrong.
here is a thread about what i am doing i hope it clears some mind...
You obviously have great tenacity and attention to detail. It's a nice looking interface. BUT you could have made the coding much easier.
If you look at those screenshots, you'll see common elements appearing so for each of those elements you could write a function that is perfect for creating such an element. for example
void drawButton(x, y, bitmap)
{
//this is where the code goes to draw that bitmap at x, y
}
Is it a touch sensitive screen? If so you could create another function like so
void drawActiveButton (x, y, bitmap, command);
{
drawButton(x,y,bitmap);
addActiveRegion(x,y,command);
}
Any time you find yourself writing the same patterns of code, over and over again, you should think "there's a better way to do this" Work out what that piece does, and create a function that does it WELL. Then you just have to call it to do your bidding.
It also makes it far easier to maintain. If something is going wrong with the way a page is functioning, you shouldn't have to wade through an endless list of myGLCD.print statements. They should all be tucked away in the functions that draw the individual parts.
I'll wager if you take on board what I'm telling you and start from scratch, you'll be able to rebuild that sketch in a quarter of the size within a tenth of the time it took you the first time around but you have to start from the very basic building blocks. Get them right and everything else just falls into place.
good point but what you are telling me is what i have been doing... lol so its nothing new to me... the void above in post 1 is one of the many functions i am using but just through that it can be re-edit to make a long void shorter is all i was asking...
Any time you find yourself writing the same patterns of code, over and over again, you should think "there's a better way to do this
And to this you say
good point but what you are telling me is what i have been doing
Copy your code to a a text document and do a global replace of 1, 2, 3 and 4 with X Does this make the repeating patterns more obvious?
Once you've identified a huge slab of code that is the same except for a few numbers, Pull it out into a function, then see if you can't calculate what the value of those changing numbers are, given the value of X.
Now the variables (such as mistingTXOnH, mistingTXOnM, mistingTXOnS, mistingTXOffH, mistingTXOffM, mistingTXOffS, mistingSwitchXOnOff ) can all be arrays. Within your function you can then get the value of any of them just given the value of X
For example, in that function you're about to write
myGLCD.drawRect(0, (56 + X * 28) , 319, 58);//Horizontal Divider
would replace all this in your original sketch
myGLCD.drawRect(0, 56, 319, 58); //Horizontal Divider #1
myGLCD.drawRect(0, 84, 319, 86); //Horizontal Divider #2
myGLCD.drawRect(0, 112, 319, 114); //Horizontal Divider #3
myGLCD.drawRect(0, 140, 319, 142); //Horizontal Divider #4
Once you've been through this exercise, post your code again and I'm sure we can start knocking it into shape.
wouldnt using a byte array take up more space then if i was to just label 4 of them out?
bryanmc1988:
wouldnt using a byte array take up more space then if i was to just label 4 of them out?
The array itself takes up an extra 2 bytes compared to four separate variables of the same type, but you gain it in other places. Because your functions will reuse space for each item rather than having everything for every item in memory simultaneously.
Imagine a very simple scenario
Serial.print ("Timer set to ");
Serial.print (val1);
Serial.print ("Timer set to ");
Serial.print (val2);
Serial.print ("Timer set to ");
Serial.print (val3);
Serial.print ("Timer set to ");
Serial.print (val4);
While this is running, apart from the four values of val, you also have four string constants. Each holding "Timer set to " that's 52 bytes!
Compare this with
For (n=0;n<4;n++)
{
Serial.print ("Timer set to ");
Serial.print (val[n]);
}
It's costing that 2 bytes you mentioned but you've only got ONE copy of "Timer set to " . So already it saves 39 bytes of sRam. That two byte overhead never changes but the savings just grow and grow.
KenF:
Step 1) Write a function that will just display a time. (Just pick a time at random and hard code it) You should be able to achieve this within about a dozen lines of code. (allowing for some nice formatting of text and drawing boxes etc)Step 2) Modify the function so that it takes it's value from a variable. This step should barely make any difference to the size of the code block
Step 3) Change the variable to be an array of the same type. (you said 4 didn't you). You'll then need to change your function to display the value of just one member of this array.
Step 4) Alter your function so that it can accept an index parameter. Use this within your code block to point to the member of that array I mentioned earlier.
You will now have a nice concise function that will display ANY of the four timer values, simply by calling it with a reference to the one you want.
Splendid advice very clearly described. I will bookmark it.
...R
KenF:
The array itself takes up an extra 2 bytes compared to four separate variables of the same type, but you gain it in other places. Because your functions will reuse space for each item rather than having everything for every item in memory simultaneously.Imagine a very simple scenario
Serial.print ("Timer set to ");
Serial.print (val1);
Serial.print ("Timer set to ");
Serial.print (val2);
Serial.print ("Timer set to ");
Serial.print (val3);
Serial.print ("Timer set to ");
Serial.print (val4);
While this is running, apart from the four values of val, you also have four string constants. Each holding "Timer set to " that's 52 bytes! Compare this withFor (n=0;n<4;n++)
{
Serial.print ("Timer set to ");
Serial.print (val[n]);
}It's costing that 2 bytes you mentioned but you've only got ONE copy of "Timer set to " . So already it saves 39 bytes of sRam. That two byte overhead never changes but the savings just grow and grow.
if doing it this way wouldnt you still need to put in the postion on where it will be printed anyways?
take a look at this function here... how would u clean this up or can you do it and post a sketch of the clean up so that i can see it and figure it out? it would really help if i can see it clean on the sketch it self, any help would be great
cleanup.ino (18.9 KB)
Post a list of the variables with comments to say what they do and I may think about it.
here is the variables and its labeled to show u what they do....
all in the ino
here is some image of what it looks like on the display
the time you see on the image is what was set... and it being displayed by the ino posted
any help would be lovely... i know your a busy man but you have help a lot at learning the arduino coding tho... if u was here i would kiss u and thank you a million but sadly that is gay and no man wants a kiss from another so for that reason i have to just thank you in advance for any help and time u can give to me.
cleanup.ino (26.2 KB)
Why do you have variables called timerAmPm1On=1 and timerAmPm1Off=1 ?
Am I right in thinking that this is just to let you know whether the on time before or afternoon ?
from this calls here:
if (setTimeFormat==1) //If time set on 12hours 1= 12 hours 0 = 24 hours
{
if (timerAmPm1On==1){
myGLCD.print("AM", 92, 60);
} else {
myGLCD.print("PM", 92, 60);}
if (timerAmPm1Off==1){
myGLCD.print("AM", 142, 71);
} else {
myGLCD.print("PM", 142, 71);}
}
"setTimerFormat" 0 = 24 hour format or 1 = 12hours format which has an am and a pm...
the timerAmPm1On or timerAmPm1Off is called to see if RTC is on which format if its on 12hour formate(12 hr ) it will display morning (am) or noon (PM)
so to your question what your thinking is correct... i may have confused you from what i am saying above... but yes you are correct
so how do you make an array for :
Now the variables (such as mistingTXOnH, mistingTXOnM, mistingTXOnS, mistingTXOffH, mistingTXOffM, mistingTXOffS, mistingSwitchXOnOff ) can all be arrays. Within your function you can then get the value of any of them just given the value of X
an assign x to it? i cant seem to get it right...
i tryed
byte mistingT[]OnH, mistingT[]OnM, mistingT[]OnS,
mistingT[]OffH, mistingT[]OffM, mistingT[]OffS;
but its not working?
i wanted x to be in a "for" function:
for (x=1; x<5; x++)
{
myGLCD.drawRect(0, (28 + x * 28) , 319, (28 + x * 28)); //Horizontal Divider for 1-4
if ((mistingT[x]OnH==0) && (mistingT[x]OnM==0) && (mistingT[x]OnS==0)
&& (mistingT[x]OffH==0) && (mistingT[x]OffM==0) && (mistingT[x]OffS==0)){
myGLCD.setColor(0, 0, 0);
myGLCD.fillRect(1, (4 + x * 28), 318, (27 + x * 28)); //Clears on/off timer if Reset or timer is on 0.
setFont(SMALL, 255, 0, 0, 0, 0, 0);
myGLCD.print("Timer " x " has not been Scheduled.", CENTER, (10 + x * 28));
any help on doing it the right way? i'm not sure how to make an array for this...