Code reduction help

Captain Haddock

Senior Member
I've been playing with using infra red remote to change the settings in a RTC and got this code working quite nicely but could do with some pointers to reduce it as it's quite big, appologies for lack of comments and poor layout, I'm sure it could be done much smaller but not by me :eek:
It currently running on a 40x2 as it happens to be on my breadboard, IR is on D.1, LCD is on A.0, power button on remote opens settings menu, arrows choose day and number buttons set date & time.
Any pointers greatly appreciated.
This is modified from code I was using for a keypad so variable choices will probably make no sense at all.

Code:
#Picaxe 40X2
#No_Data
Symbol Baud = N2400_16
Symbol Lcd = A.0
Symbol keyPress     = b24	'raw keypress code
Symbol lastKeyPress = b25	'resolved key number
hi2csetup i2cmaster, %11010000,i2cslow,i2cbyte
; ===================================================
SetFreq M16
serout Lcd,Baud,(254,1)
main:
	
	
	do
	
	hi2cin 0, (b0,b1,b2,b3,b4,b5,b6)
	pause 100

	bcdtoascii b0,b19,b20		'Secs Convert to ASCII
	bcdtoascii b1,b8,b9		'Mins 
	bcdtoascii b2,b10,b11		'Hours
	bcdtoascii b3,b21,b22         'DayOfWeek;	
	bcdtoascii b4,b12,b13		'Date
	bcdtoascii b5,b14,b15		'Month
	bcdtoascii b6,b16,b17		'Year
	
	serout Lcd,Baud,(254,132)
	serout Lcd,Baud,(b10,b11, ":",b8,b9,":",b19,b20)
	serout Lcd,Baud,(254,200)
	serout Lcd,Baud,(b12,b13,"/",b14,b15,"/",b16,b17)
	If b22="1" then serout Lcd,Baud,(254,192,"Sun ")
	elseif b22="2" then serout Lcd,Baud,(254,192,"Mon ")
	elseif b22="3" then serout Lcd,Baud,(254,192,"Tue ")
	elseif b22="4" then serout Lcd,Baud,(254,192,"Wed ")
	elseif b22="5" then serout Lcd,Baud,(254,192,"Thur")
	elseif b22="6" then serout Lcd,Baud,(254,192,"Fri ")
	elseif b22="7" then serout Lcd,Baud,(254,192,"Sat ")
	endIf

	gosub readkeypress
	if lastkeypress = 21 then gosub menu
	
	pause 500
	
	loop
	
	
	goto main
	

	
menu: let lastkeypress = 128
	pause 1000
	serout Lcd,Baud,(254,1,254,128,"Time  Date  Day",254,192," <     >     ^")
	do
	Gosub ReadKeyPress 
	if lastkeypress = 17 then gosub settime
	if lastkeypress = 16 then gosub setdate
	if lastkeypress = 13 then gosub day
	if lastkeypress = 21 then serout Lcd,Baud,(254,1):let lastkeypress = 128:goto main:endif
	pause 870
	loop while keyPress = 128


settime:let lastkeypress = 0
	pause 1000
hour:	serout Lcd,Baud,(254,1,254,128,"Set time (hh:mm)",254,200,"00:00",254,200,254,13)
	
	do
	Gosub ReadKeyPress : let b4 = lastkeypress
	loop while keyPress = 128
	pause 1000
	if b4 = 21 then serout Lcd,Baud,(254,1):let lastkeypress = 128:goto main:endif
	serout Lcd,Baud,(254,200,#b4,"0:00",254,201)
	if b4 > 2 then goto hour
	do
	Gosub ReadKeyPress : let b5 = lastkeypress
	loop while keyPress = 128
	pause 1000
	serout Lcd,Baud,(254,200,#b4,#b5,":00",254,203)
	let b6 = b4*10 + b5
	if b6 > 23 then goto hour
	
minute:do
	Gosub ReadKeyPress : let b7 = lastkeypress
	loop while keyPress = 128
	pause 1000
	serout Lcd,Baud,(254,200,#b4,#b5,":",#b7,"0",254,204)
	if b7 > 5 then goto minute
	
	do
	Gosub ReadKeyPress : let b8 = lastkeypress
	loop while keyPress = 128
	pause 1000
	
	let b9 = b7*10+b8
	if b9 > 59 then goto minute

	let b9 = b7*16+b8
	let b6 = b4*16+b5
	hi2cout 0, ($00, b9, b6)
	pause 1000
	serout Lcd,Baud,(254,1,254,12):goto main
	

setdate:let lastkeypress = 0
	pause 1000
date:	serout Lcd,Baud,(254,1,254,128,"Set date (d/m/y)",254,200,"00/00/00",254,13,254,200)
	do
	Gosub ReadKeyPress : let b4 = lastkeypress
	loop while keyPress = 128
	pause 1000
	if b4 = 21 then serout Lcd,Baud,(254,1):let lastkeypress = 128:goto main:endif
	serout Lcd,Baud,(254,200,#b4,"0","/","00","/","00",254,201)
	if b4 > 3 then goto date
	do
	Gosub ReadKeyPress : let b5 = lastkeypress
	loop while keyPress = 128
	pause 1000
	serout Lcd,Baud,(254,200,#b4,#b5,"/","00","/","00",254,203)
	let b6 = b4*10 + b5
	if b6 > 31 then goto date
	
month:do
	Gosub ReadKeyPress : let b7 = lastkeypress
	loop while keyPress = 128
	pause 1000
	serout Lcd,Baud,(254,200,#b4,#b5,"/",#b7,"0","/","00",254,204)
	if b7 > 1 then goto month
	
	do
	Gosub ReadKeyPress : let b8 = lastkeypress
	loop while keyPress = 128
	pause 1000
	serout Lcd,Baud,(254,200,#b4,#b5,"/",#b7,#b8,"/","00",254,206)
	let b9 = b7*10+b8
	if b9 > 12 then goto month
	
year:	do
	Gosub ReadKeyPress : let b10 = lastkeypress
	loop while keyPress = 128
	pause 1000
	serout Lcd,Baud,(254,200,#b4,#b5,"/",#b7,#b8,"/",#b10,"0",254,207)
	if b10 > 2 then goto year
	do
	Gosub ReadKeyPress : let b11 = lastkeypress
	loop while keyPress = 128
	pause 1000
	
	let b12 = b10*16+b11
	let b9 = b7*16+b8
	let b6 = b4*16+b5
	hi2cout 04, (b6, b9,b12)
	serout Lcd,Baud,(254,1,254,12):goto main
	
day:
	let lastkeypress = 0
	serout Lcd,Baud,(254,1,254,128,"Set Day",254,192,"<dec  Sun   inc>")
	let b7 = 1
	do
	Gosub ReadKeyPress 
	if lastkeypress = 17 then dec b7:let lastkeypress = 0:endif
	if lastkeypress = 16 then inc b7:let lastkeypress = 0:endif
	if b7 > 7 then let b7 = 1:endif
	if b7 < 1 then let b7 = 7:endif
	If b7= 1 then serout Lcd,Baud,(254,198,"Sun "):let b8 = b7:endif
	if b7= 2 then serout Lcd,Baud,(254,198,"Mon "):let b8 = b7:endif
	if b7= 3 then serout Lcd,Baud,(254,198,"Tue "):let b8 = b7:endif
	if b7= 4 then serout Lcd,Baud,(254,198,"Wed "):let b8 = b7:endif
	if b7= 5 then serout Lcd,Baud,(254,198,"Thur"):let b8 = b7:endif
	if b7= 6 then serout Lcd,Baud,(254,198,"Fri "):let b8 = b7:endif
	if b7= 7 then serout Lcd,Baud,(254,198,"Sat "):let b8 = b7:endif
	if lastkeypress = 73 then exit
	pause 200
	loop
	hi2cout 03, (b8)
	serout Lcd,Baud,(254,1):goto main


ReadKeyPress:
	let keypress = 128
	irin [500],D.1,keypress

	if keypress = 0 then let lastkeypress = 1  : endif;1
	if keypress = 1 then let lastkeypress = 2  : endif;2
	if keypress = 2 then let lastkeypress = 3  : endif;3
	if keypress = 3  then let lastkeypress = 4  : endif;4
	if keypress = 4  then let lastkeypress = 5  : endif;5
	if keypress = 5  then let lastkeypress = 6  : endif;6
	if keypress = 6  then let lastkeypress = 7  : endif;7
	if keypress = 7  then let lastkeypress = 8  : endif;8
	if keypress = 8  then let lastkeypress = 9  : endif;9
	if keypress = 9  then let lastkeypress = 0  : endif;0
	if keypress = 13 then let lastkeypress = 13 : endif;A
	if keypress = 16  then let lastkeypress = 16 : endif;B
	if keypress = 72  then let lastkeypress = 72 : endif;C
	if keypress = 17  then let lastkeypress = 17 : endif;D
	if keypress = 73  then let lastkeypress = 73 : endif;*
	if keypress = 21  then let lastkeypress = 21 : endif;#
	
  Return
 

geoff07

Senior Member
A few initial comments:

- you have used Do/loop for the main loop, so you don't need the main:/goto main

- you can replace if/elseif/endif with select/case/endselect. It won't shorten the code but it will be easier to read. Similarly for the if lastkeypress section.

- despite using do/loop for the main loop you have used goto main at the end of the menu gosub. You should end a subroutine with a return as otherwise you will likely blow the stack (which is limited in size)

- strict indentation would help clarify the code, as would removing any remaining gotos (which confuse the logic for the reader as they don't define the flow of control). You should be able to do all you want with only a few clear structures: do/loop, select/endselect, for/next, if/endif, using exit where needed.

- some comments to explain wjhat each gosub is for and what each test is looking for might help too.

If you do these things I think you will find it will be much clearer for you to refine further.
 

nick12ab

Senior Member
Code:
ReadKeyPress:
	let keypress = 128
	irin [500],D.1,keypress

	if keypress = 0 then let lastkeypress = 1  : endif;1
	if keypress = 1 then let lastkeypress = 2  : endif;2
	if keypress = 2 then let lastkeypress = 3  : endif;3
	if keypress = 3  then let lastkeypress = 4  : endif;4
	if keypress = 4  then let lastkeypress = 5  : endif;5
	if keypress = 5  then let lastkeypress = 6  : endif;6
	if keypress = 6  then let lastkeypress = 7  : endif;7
	if keypress = 7  then let lastkeypress = 8  : endif;8
	if keypress = 8  then let lastkeypress = 9  : endif;9
	if keypress = 9  then let lastkeypress = 0  : endif;0
	if keypress = 13 then let lastkeypress = 13 : endif;A
	if keypress = 16  then let lastkeypress = 16 : endif;B
	if keypress = 72  then let lastkeypress = 72 : endif;C
	if keypress = 17  then let lastkeypress = 17 : endif;D
	if keypress = 73  then let lastkeypress = 73 : endif;*
	if keypress = 21  then let lastkeypress = 21 : endif;#
	
  Return
Isn't lastkeypress supposed to be set to the same as keypress for all of these IF statements above, not just for the last few?

If yes then there's no need for the IF statements at all - just use:
Code:
lastkeypress = keypress
If you don't want to change the lastkeypress variable if keypress is a certain value, just use an IF statement such as 'if keypress <> undesired_value then'.


Code:
serout Lcd,Baud,(b12,b13,"/",b14,b15,"/",b16,b17)
	If b22="1" then serout Lcd,Baud,(254,192,"Sun ")
	elseif b22="2" then serout Lcd,Baud,(254,192,"Mon ")
	elseif b22="3" then serout Lcd,Baud,(254,192,"Tue ")
	elseif b22="4" then serout Lcd,Baud,(254,192,"Wed ")
	elseif b22="5" then serout Lcd,Baud,(254,192,"Thur")
	elseif b22="6" then serout Lcd,Baud,(254,192,"Fri ")
	elseif b22="7" then serout Lcd,Baud,(254,192,"Sat ")
	endIf
There's no need to send 254,192 in each of those serout commands! Here's a much better version:
Code:
serout Lcd,Baud,(b12,b13,"/",b14,b15,"/",b16,b17,254,192)
	If b22="1" then serout Lcd,Baud,("Sun ")
	elseif b22="2" then serout Lcd,Baud,("Mon ")
	elseif b22="3" then serout Lcd,Baud,("Tue ")
	elseif b22="4" then serout Lcd,Baud,("Wed ")
	elseif b22="5" then serout Lcd,Baud,("Thur")
	elseif b22="6" then serout Lcd,Baud,("Fri ")
	elseif b22="7" then serout Lcd,Baud,("Sat ")
	endIf

Code:
hour:	serout Lcd,Baud,(254,1,254,128,"Set time (hh:mm)",254,200,"00:00",254,200,254,13)
	
	do
	Gosub ReadKeyPress : let b4 = lastkeypress
	loop while keyPress = 128
	pause 1000
	if b4 = 21 then serout Lcd,Baud,(254,1):let lastkeypress = 128:goto main:endif
	serout Lcd,Baud,(254,200,#b4,"0:00",254,201)
	if b4 > 2 then goto hour
	do
	Gosub ReadKeyPress : let b5 = lastkeypress
	loop while keyPress = 128
	pause 1000
	serout Lcd,Baud,(254,200,#b4,#b5,":00",254,203)
	let b6 = b4*10 + b5
	if b6 > 23 then goto hour
	
minute:do
	Gosub ReadKeyPress : let b7 = lastkeypress
	loop while keyPress = 128
	pause 1000
	serout Lcd,Baud,(254,200,#b4,#b5,":",#b7,"0",254,204)
	if b7 > 5 then goto minute
	
	do
	Gosub ReadKeyPress : let b8 = lastkeypress
	loop while keyPress = 128
	pause 1000
	
	let b9 = b7*10+b8
	if b9 > 59 then goto minute
I'm going to let you think about this one yourself - there are similar loops for year, month, date, day, hour and minute so could these be compacted into a FOR : NEXT loop and just use IF statements utilizing the loop counter variable for the parts that are different for each part.
 

Captain Haddock

Senior Member
Many thanks for the input Guys, a few things to think about to save some space, the point about strict indentation is very valid as it's one of the things I'm realy bad for.
I'm hoping to squeeze this down to leave enough space for another prog on an 18m2, should be doable with a bit of effort, give me something to do over xmas other than drinking....

I have replaced the 'readkeypress' bit with:
Code:
ReadKeyPress:
	let keypress = 128
	irin [500],D.1,keypress
	if keypress >= 0 and keypress <= 9 then let lastkeypress = keypress + 1:endif 
	if keypress = 9  then let lastkeypress = 0  : endif;0
	if keypress > 10 then let lastkeypress = keypress:endif

  Return
 

westaust55

Moderator
In the menu: subroutine you have the line:
if lastkeypress = 21 then serout Lcd,Baud,(254,1):let lastkeypress = 128:goto main:endif

You cannot just break out of a subroutine and return to the start of the main loop with GOTO main

Change the GOTO Main to RETURN
otherwise after a time there will be stack overflow and your program will not work as expected.
 
Last edited:

westaust55

Moderator
For 85 bytes redfuction in code size:
Code:
ReadKeyPress:
	let keypress = 128
	irin [500],D.1,keypress
	if keypress >= 0 AND keypress<= 9  then : let lastkeypress = keypress + 1 //10  : endif
	if keypress = 13 OR keypress = 16 OR keypress = 17 OR keypress = 72 OR keypress = 73 OR keypress = 21 then let lastkeypress =keypress : endif
	  Return
But first, you should make sure that your code is correct, as per my post above.
 

Captain Haddock

Senior Member
In the menu: subroutine you ahve the line:
if lastkeypress = 21 then serout Lcd,Baud,(254,1):let lastkeypress = 128:goto main:endif

You cannot just break out of a subroutine and return to the start of the main loop with GOTO main

Change the GOTO Main to RETURN
otherwise after a time there will be stack overflow and your program will not work as expected.
Yeah I have to admit I was wondering about the workings of that, I'm never sure wether to use goto's or gosub's, obviously goto is ok if it always goes back to the same point and gosub is the way to return to multiple points, is there a proper way to break out of a subroutine to a different place from where you started?
Edit: I have to admit I've not been picked up on as many points as I was expecting to be....
 

nick12ab

Senior Member
is there a proper way to break out of a subroutine to a different place from where you started?
Use a spare variable to hold a value set by using the let command in the subroutine and use an if statement after the gosub that called the subroutine.

However most times it is better just to use goto instead.
 

geoff07

Senior Member
Gosub/return

All subroutines end with a return. That is so the system can keep track of control, done by recording the return address on what is called the 'stack'. If you don't use a return then the system has lost one of the (limited number of) stack entries and your program will probably fail sooner or later when it runs out of stack.

Goto

This is an unconditional jump, commonly used if writing in hex or assembler or Fortran, where there is no alternative, and which can be very confusing. That is because you know where the jump is going, but when looking at the code with the label, you don't know where it has come from (people that use gotos usually have several to the same place). Gotos have been deprecated for some 40 years by software engineers. Fortunately goto is not necessary in high level languages like Picaxe Basic (except in a couple of very specific cases) so you can use the control structures mentioned in my last post to fully define the flow of control in your code. This starts to matter if your code is of any size or expected to be understandable in future.

The point about a subroutine is that it is a section of code that can replace a single statement. If a decision is required on what to do next, make it in the main code based on information returned by the subroutine in a suitable variable. That way, your subroutine can be used many times in many places and does its job independent of the main logic.

If you can write your code using the main control structures then you probably understand your logic. If you can't then the design needs more work before you start coding.

I didn't mention variable names last time but it helps greatly if, instead of e.g. b22 you write e.g. 'B_day_number' - you still know it is a byte, but now you also know what it means when reading the code.
 

inglewoodpete

Senior Member
I notice that you're avoiding using the internal EEPROM. You can use a small area of EEPROM to store a lookup table of day names.

Taking your "Day" routine (code box 1, below) and replace it with the second code area, below, saves about 111 bytes of code space. Of course, you can move the "Symbol" and "EEPROM" commands to the top of the program. Also, remove your #No data directive.

The SerOut command is codespace hungry, even more so when used as you have it configured. Note that I have not debugged the code. I think the formula is correct. I have used registers b51 - b55 for my convenience.

Code:
day:
	let lastkeypress = 0
	serout Lcd,Baud,(254,1,254,128,"Set Day",254,192,"<dec  Sun   inc>")
	let b7 = 1
	do
		Gosub ReadKeyPress 
		if lastkeypress = 17 then dec b7:let lastkeypress = 0:endif
		if lastkeypress = 16 then inc b7:let lastkeypress = 0:endif
		if b7 > 7 then let b7 = 1:endif
		if b7 < 1 then let b7 = 7:endif
		If b7= 1 then serout Lcd,Baud,(254,198,"Sun "):let b8 = b7:endif
		if b7= 2 then serout Lcd,Baud,(254,198,"Mon "):let b8 = b7:endif
		if b7= 3 then serout Lcd,Baud,(254,198,"Tue "):let b8 = b7:endif
		if b7= 4 then serout Lcd,Baud,(254,198,"Wed "):let b8 = b7:endif
		if b7= 5 then serout Lcd,Baud,(254,198,"Thur"):let b8 = b7:endif
		if b7= 6 then serout Lcd,Baud,(254,198,"Fri "):let b8 = b7:endif
		if b7= 7 then serout Lcd,Baud,(254,198,"Sat "):let b8 = b7:endif
		if lastkeypress = 73 then exit
		pause 200
	loop
	hi2cout 03, (b8)
	serout Lcd,Baud,(254,1):goto main
Replace the above with:
Code:
day:
	Symbol EEPROMPtr = b54
	EEPROM 0, ("SunMonTueWedThuFriSat")
	let lastkeypress = 0
	serout Lcd,Baud,(254,1,254,128,"Set Day",254,192,"<dec  Sun   inc>")
	let b7 = 1
	do
		Gosub ReadKeyPress 
		if lastkeypress = 73 then exit
		if lastkeypress = 17 then dec b7:let lastkeypress = 0:endif
		if lastkeypress = 16 then inc b7:let lastkeypress = 0:endif
		if b7 > 7 then let b7 = 1:endif
		if b7 < 1 then let b7 = 7:endif
		bEEPROMPtr = b7 - 1 * 3	'[I]+ offset[/I] if EEPROM is not loaded at location 0
		Read bEEPROMPtr, b51, b52, b53
		serout Lcd,Baud,(254, 198, b51, b52, b53, 254, 1)
		pause 200
	loop
	hi2cout 03, (b8)
	goto main
 
Last edited:

vttom

Senior Member
I, too, was thinking that the section where the days of the week are transmitted could be reduced. How about something like this?

Code:
lookup b7, ("SMTWTFS"), b0
lookup b7, ("uouehra"), b1
lookup b7, ("nneduit"), b2
lookup b7, ("    r  "), b3
serout Lcd,Baud,(254,198,b0,b1,b2,b3)
 

Captain Haddock

Senior Member
More good ideas to look at, thanks guys.
I have used lookups before in clock showing text days/months but didn't get around to it this time.
 
Top