buttons don't work about stc_diyclock

[zerog2k]

Regarding buttons, this should be same as all of them. (Note that you may have to disconnect serial connection to PC, i.e. txd and rxd wires, in order for buttons to work.)

Does the clock seem to function otherwise? Does it display some time, and does colon flash with seconds?

How did you build the firmware?

Regarding usage, yes I probably need to create a diagram or something of the current flow.
It should be similar to original clock operation, which is diagrammed here:
https://github.com/zerog2k/stc_diyclock/blob/master/docs/DIY_LED_Clock_operation_original.png

[adcurtin]

I tried disconnecting both serial lines, that didn't seem to make a difference. Also, when I started a flash with stcgal once, the clock's hours ( I think ) started flashing like a button was pressed.

The clock does seem to function otherwise. It displays the time that was set in the ds1302, and the colon flashes.

for building the firmware, I cloned the repo, installed sdcc from homebrew (sdcc -v output: SDCC : mcs51/z80/z180/r2k/r3ka/gbz80/tlcs90/ds390/pic16/pic14/TININative/ds400/hc08/s08/stm8 3.7.0 #10231 (Mac OS X x86_64)). I tried building the code unmodified first, then I #defined stc15w404as at the top of main.c, and neither made the buttons responsive.

[adcurtin]

it is a problem with how I'm building it, the precompiled hex release works.

[zerog2k]

Thanks it's helpful to know that an older precompiled hex works.

I just built one and tested it on a very similar stc15w408as (only difference is 4k vs 8k code size, but this code does not exceed 4k)

stc_diyclock_fw.zip

Let me know if this is working, then we can troubleshoot into your build process.

[adcurtin]

yes, that one is working as well.

$ git diff
diff --git a/src/main.c b/src/main.c
index e2446d5..65c4ac4 100644
--- a/src/main.c
+++ b/src/main.c
@@ -14,6 +14,7 @@
 #include "led.h"
 
 #define FOSC    11059200
+#define stc15w404as
 
 // clear wdt
 #define WDT_CLEAR()    (WDT_CONTR |= 1 << 4)
diff --git a/stcgal b/stcgal
index eb8eecb..b470920 160000
--- a/stcgal
+++ b/stcgal
@@ -1 +1 @@
-Subproject commit eb8eecbc9b8ce664984a706955265392a4521c76
+Subproject commit b47092093ee1b390de60e5cae8021286874c6d48

the code compiled from the above, the buttons did not work.

[zerog2k]

ok, one alternative to try:
use platformio to compile instead of make:

1. get vscode or something
2. install platformio
3. pull the latest version of stc_diyclock (made some platformio fixes last night)
4. open stc_diyclock folder in vscode. Open platformio.ini and in [platformio] section, uncomment only env_default = stc15w404as
5. Do platformio build (little check box icon in lower left). It should download all the needed dependencies, etc, and drop the fw into .pioenvs/stc15w404as/firmware.ihx (you can also try using upload from here as well).

[zerog2k]

for make try:

make clean
SDCCREV="-Dstc15w404as" make

[flohwie]

I had exactly the same problem (no response to S1 and S2) and I think I could solve it by downgrading to SDCC version 3.5.0. The code you released (0.60, unchanged for this board), compiled with 3.5.0, results in a perfectly working "main.hex" file. Compiled with the newer SDCC 3.8.0, however, it yields a "main.hex" reproducing exactly what was described earlier by adcurtin (see also my response to issue #10 of aFewBits' library).

[zerog2k]

ok interesting.. yeah i use sdcc 3.5.0 (comes with ubuntu 16.04 by default).

I just tested compiling same code wtih sdcc 3.7.2-something and while not sure about functionality, it definately produces about 15% larger code - which on this platform is very significant.
I then tried 3.8.1 snapshot, and it improved, but still about 14% larger than 3.5.0

sdcc_ver	code size
3.5.0	2711
3.7.2	3105
3.8.1	3084

just taking a short look through some of the asm code produced by the newer compilers there seems to be some quite strange "optimizations" which are far from that... example, sectional diff from 3.5.0 -> 3.8.1:

 ;	src/main.c:378: if (count % 4 == 0) {
-	mov	a,_count
-	anl	a,#0x03
+	mov	r5,_count
+	mov	r6,#0x00
+	mov	__modsint_PARM_2,#0x04
+;	1-genFromRTrack replaced	mov	(__modsint_PARM_2 + 1),#0x00
+	mov	(__modsint_PARM_2 + 1),r6
+	mov	dpl,r5
+	mov	dph,r6
+	push	ar7
+	lcall	__modsint
+	mov	a,dpl
+	mov	b,dph
+	pop	ar7
+	orl	a,b
 	jnz	00107$
 ;	src/main.c:379: temp = gettemp(getADCResult(ADC_TEMP));

There seems to be some sort of totally unnecessary modulo optimization (div replacement attempt?). Perplexing. fyi @spth

For now, please use sdcc 3.5.0

[zerog2k]

ok i can confirm that at least sdcc which ships with the platformio package toolchain-sdcc (3.6.3) produces correct size, and functional code.

$ ~/.platformio/packages/toolchain-sdcc/bin/sdcc -v
SDCC : mcs51/z80/z180/r2k/r3ka/gbz80/tlcs90/ds390/pic16/pic14/TININative/ds400/hc08/s08/stm8 3.6.3 #9771 (Linux)
published under GNU General Public License (GPL)

so somewhere between sdcc 3.6.3 and 3.7.2 looks suspicious for now

[flohwie]

I also noticed increased code size after compiling with 3.8.0 (and the difference to the .hex file you uploaded!).
Maybe it is worth noting that 3.5.0. compiled on my system (Debian 9), but 3.6.0 did not, and 3.8.0 issued gputils warnings during compiling.
I did not try 3.7.0 because it was released in 2018 (after you released 0.6.0 of your firmware).

[spth]

If you have code that doesn't work (as opposed to just the code size being too big), please file a bug report: https://sourceforge.net/p/sdcc/bugs/
If you have a concrete code sample with a substantial code size regression, maybe a feature request could make sense:
https://sourceforge.net/p/sdcc/feature-requests/

For those interested in code sizes on mcs51, there also are some statistics:

https://sourceforge.net/p/sdcc/code/HEAD/tree/trunk/sdcc-extra/historygraphs/dhrystone-mcs51-size.svg
https://sourceforge.net/p/sdcc/code/HEAD/tree/trunk/sdcc-extra/historygraphs/whetstone-mcs51-size.svg

To explain a bit: The mcs51 backend in SDCC has been mostly seen as mature, and didn't get as much attention lately as newer backends like stm8 or the z80 backend. This means that not that much effort went into optimizations for mcs51 (though there were some optimizations directed at mcs51 in between 3.7.0 and 3.8.0). On the other hand, some bug-fixes in SDCC resulted in code size regressions (mcs51 was particularly hit by https://sourceforge.net/p/sdcc/feature-requests/574/).

Philipp

[spth]

The gputils warnings do not affect the mcs51 backend (gputils is used for pic14 / pic16 only). I don't know which gputils warnings you saw, but the most common one is that there is only an older gputils installed (so support of PIC devices is limited to those supported by gputils, while SDCC could support more when built with a newer gputils).

Philipp

[zerog2k]

thanks @spth
besides the significant (>10% increase) difference in code size (which I and many others using "small" mcs51) would consider a regression ;),
there is certainly some kind of functionality bug. Our clock works, but buttons are not working, and strange stuff is happening (glitches in display) - I had actually seen something like this before when we were having strange timer/bank switching/critical section issues - but not immediately seeing where the issue is.

However, after reviewing, diff'ing, and stepping through the asm output, I have found at least one bug (IMHO) and reported it (issue with incorrectly pre-incrementing a loop var before passing to subroutine, when should be post-incremented):
https://sourceforge.net/p/sdcc/bugs/2826/
(edit: perhaps this was a bug before and has been fixed? not sure - I guess we'll sort it out on the ticket)

will keep digging for more ;)

[zerog2k]

ok, think i found the source of the issue --- whew!!
(actually there are two problems, one minor, one major)

first problem is the one I previously mentioned. We have a logic error at https://github.com/zerog2k/stc_diyclock/blob/master/src/ds1302.c#L35
We should increment loop var j only after making the call to ds_readbyte. I think that before, sdcc incorrectly evaluated the post-increment and incremented j after calling ds_readbyte, but newer versions seem to correctly? evaluate this and do basically a pre-increment (so ds1302 read index can be off). I think this causes some strange flickering and other behavior, etc. I'll rewrite this to be more clear (j increment after func call).

second problem:
I noticed with newer sdcc compiles, when I press and release the buttons (P3_0 or P3_1) these seem to stay latched low and do not go back high. P3 is also used for the led segment shared lines (low to turn on segment).
In timer0 isr, we have a macro expansion (from hwconfig.h) to select one of the bits in P3 representing this line, invert to a mask and AND it with P3 port (essentially bit clearing the one pin).
https://github.com/zerog2k/stc_diyclock/blob/master/src/main.c#L172-L173
this was:

LED_DIGITS_PORT &= ~((1<<LED_DIGITS_PORT_BASE) << digit);

and in previous versions of sdcc, this was compiled to asm which produced a single, atomic P3 register AND operation. In newer versions, this seemed to get turned into several additional steps (copies P3 to register, ANDs there, then copies back to P3), which is "non-atomic"

by rewriting the c to:

tmp = ~((1<<LED_DIGITS_PORT_BASE) << digit);
LED_DIGITS_PORT &= tmp;

sdcc now compiles again like earlier versions with atomic register AND.

old sdcc with old, single-line form of c statement:

;	src/main.c:175: LED_DIGITS_PORT &= ~((1<<LED_DIGITS_PORT_BASE) << digit);
	mov	b,r6
	inc	b
	mov	a,#0x04
	sjmp	00223$
00221$:
	add	a,acc
00223$:
	djnz	b,00221$
	cpl	a
	mov	r6,a
	anl	_P3,a

newer sdcc with old, single-line form of c statement:

;	src/main.c:175: LED_DIGITS_PORT &= ~((1<<LED_DIGITS_PORT_BASE) << digit);
	mov	b,r6
	inc	b
	mov	a,#0x04
	sjmp	00246$
00244$:
	add	a,acc
00246$:
	djnz	b,00244$
	cpl	a
	mov	r6,a
	mov	r5,_P3
	anl	a,r5
	mov	_P3,a

newer sdcc with rewritten c statements:

;	src/main.c:176: tmp = ~((1<<LED_DIGITS_PORT_BASE) << digit);
	mov	b,r6
	inc	b
	mov	a,#0x04
	sjmp	00246$
00244$:
	add	a,acc
00246$:
	djnz	b,00244$
;	src/main.c:177: LED_DIGITS_PORT &= tmp;
	cpl	a
	mov	r6,a
	anl	_P3,a

the functional difference here seems to be:

	mov	r5,_P3
	anl	a,r5
	mov	_P3,a

vs just

	anl	_P3,a

I'm not completely understanding why (other than suspecting non-atomic port register access?) this causes some latching behavior of P3_0, P3_1

anyone have thoughts here? (@spth ?)

(I'll submit a "fix" to use the temp var workaroundfor short term)

[zerog2k]

ok reading keil and silicon labs notes, the anl operation should be directly on the P3 port latch SFR:
"Use the ANL (&=), ORL (|=), or XRL (^=) instructions to safely clear, set, or toggle port pins without altering other input pin latch values. Also, most port SFRs are bit addressable, so use SBITs to modify port pin values directly."
opened https://sourceforge.net/p/sdcc/bugs/2827/

@spth fyi (and thanks for taking time to look into it, and also for contributing to sdcc - otherwise this project would not be possible!)

[zerog2k]

just for reference, the port access issue (sdcc 2827) fixes the buttons, but also found another issue with recent sdcc, which is that the temperature display was super erratic (giving all kinds of garbage values).
It seems to be related to the way the modulo stuff changed as well. Not sure why, but the workaround for the modulo issue (casting constant) fixes the temperature display issue. Further documented in https://sourceforge.net/p/sdcc/bugs/2828/