#1988 Handling of AM/PM wrong if time of day is already at 12 o'clock

[André Steinert]

Handling of AM/PM flag when setting CIA time of day for hour 12 in VICE 3.8 is wrong if internal stored hour is already 12 o'clock (was correct in a previous version, but don't know the exact one).

In core/ciacore.c it is checked if the value to be stored differs from the internally stored value. The flipping of AM/PM for hour 12 is done after this check. This results in not being able to change from AM to PM and vice-versa in case the internal stored hour already is at 12, as the new value without the flipping equals the value already stored.

I checked with a real C64: Storing 12:30:00 AM first time results in 12:30:00 PM. Storing 12:30:00 PM directly after that results in 12:30:00 AM. In VICE, it keeps being 12:30:00 PM as the value to be stored (without flipping) equals the one already stored.

[gpz]

could you provide a specific test case? We have quite a bunch of elaborated tests for the TOD (which all pass), so i wonder how this can be wrong :)

see https://sourceforge.net/p/vice-emu/code/HEAD/tree/testprogs/CIA/tod/

[André Steinert]

Thanks for mentioning the test suite. Unfortunately, I can't tell you at first glance why the hour-test.bas doesn't recognize the error even if I insert an inner loop so that every hour gets tested twice. I would have to reverse-engineer the test code, especially the embedded machine code, to know what is happening.

It's just that I can see the difference and that I can see the change in ciacore.c where the code reflects the observed behavior. In you look into ciacore.c in line 891 following:

                    changed = cia_context->c_cia[addr] != byte;
                    if (changed) {
                        /* Flip AM/PM on hour 12 on the rising edge of the comparator */
                        if ((addr == CIA_TOD_HR) && ((byte & 0x1f) == 0x12))
                            byte ^= 0x80;
                        cia_context->c_cia[addr] = byte;
                    }

In a previous version it was first flipped and then compared:

            if (addr == CIA_TOD_HR) {
                /* force bits 6-5 = 0 */
                byte &= 0x9f;
                /* Flip AM/PM on hour 12  */
                /* Flip AM/PM only when writing time, not when writing alarm */
                if ((byte & 0x1f) == 0x12 && !(cia_context->c_cia[CIA_CRB] & 0x80)) {
                    byte ^= 0x80;
                }

[gpz]

That change was probably made to fix some bug :) There IS quite some odd (and unexpected) behaviour with that bit. That's why we need a specific test case (that we can put into the test bench as well), else it's very hard to tell what the problem is.

[André Steinert]

I just have a program I'm currently creating (based on a program of a book) to test a CIA with a faulty time of day. That was the reason I recognized this other VICE bug :)

I attached the current version as a D64. Steps to reproduce:
- load "tod",8
- run
- enter "12" as hour
- enter "A" for AM
- enter "30" as minute
- enter "0" as seconds
- You will see the running clock (the program compensates for AM/PM flipping whilst setting). "S:" mentiones the set value, "1:" CIA1 and "2:" CIA2 ... They are correct
- RUN/STOP to exit the program
- directly "RUN" it again
- enter "12" as hour
- enter "P" for PM
- enter "30" as minute
- enter "0" as seconds
- You will see that AM/PM hasn't changed. On a real C64 you will see "P" at "1:" and "2:"
- If you RUN/STOP and "RUN" again with 11:30:00 AM and RUN/STOP and "RUN" again with 12:30:00 PM, you will see the correct value in VICE too

[gpz]

I can confirm the different behaviour at least - i find that basic program extremely hard to read though :)

[gpz]

https://sourceforge.net/p/vice-emu/code/43714/ contains the change in question

apparently it was ment to be a refactoring that does not change behaviour - but it did :)

[André Steinert]

I just typed in the time of day program of book "Mapping the Commodore 64" (page 181/182) and am added missing things (like testing CIA2 at the same time, AM/PM handling, ...) - because I wanted to have a fast result for the faulty CIA. Whenever the functionality is complete, I might rewrite it completely new in a cleaned-up way ;) My goal would be to have an automatic test of AM to PM and PM to AM flipping from 11 to 12 o'clock and the flipping of all TOD registers from 12:59:59 to 01:00:00.

[Olaf Seibert]

So looking at both variants of the code, it seems that the only difference is that in the new state, there is an extra condition, if (changed). That does match the remark /* Flip AM/PM on hour 12 on the rising edge of the comparator */.

So there are 2 things to check:
- if the schematics from http://forum.6502.org/viewtopic.php?f=4&t=7418&sid=38219c61eebb1ccd1190624aaf233042&start=30#p96981 really say that the AM/PM bit is flipped on exactly that condition
- change the code to remove that condition and see if that matches the observed behaviour better.

Index: ciacore.c
===================================================================
--- ciacore.c   (revision 44985)
+++ ciacore.c   (working copy)
                         cia_context->todstopped = 1;
                     }
                     changed = cia_context->c_cia[addr] != byte;
-                    if (changed) {
+                    if (1 || changed) {
                         /* Flip AM/PM on hour 12 on the rising edge of the comparator */
                         if ((addr == CIA_TOD_HR) && ((byte & 0x1f) == 0x12))
                             byte ^= 0x80;

[André Steinert]

I think there is a difference in letting the flipping be included in the change detection (byte is only written in the case of a real change and there is code following in a separate "changed" condition) or in just ignoring changed state and letting it flip.

In my opinion, the flip has to be done before identifying if the byte has changed, so that - in case of a change - the following code gets executed as well (and ONLY if it has changed):

 if (changed) {
                    check_ciatodalarm(cia_context, rclk);
                    cia_ifr_catchup(cia_context, rclk);
                    cia_ifr_current(cia_context, rclk, CIA_IFR_CUR_NXT);
                }

[Olaf Seibert]

Yes you have a point there.

I was musing... suppose we must take "on the rising edge" as absolute truth (but I haven't re-looked at the schematic yet), could this be caused a write causing some glitch on the comparator output? And if so, could it be model-dependent? (old vs new CIA) Just wondering so far...

[gpz]

A clean test for this would be nice (one that - like the ones in the test bench - is selfcontained, needs no interaction, etc). I can run on a couple different C64/CIA then

[André Steinert]

Something like this? This is just a dirty modification of the existing test but it can identify this bug (failing in VICE 3.8, ok on real C64). @gpz: Thanks again for pointing to the test suite! I'm really thinking of doing my planned tests based on these tests instead of enhancing the other program.

[Olaf Seibert]

I just added it to the testbench.

[Olaf Seibert]

So now I have this patch so I can switch quicly between OLD and NEW versions of the code while testing. For now this assumes it isn't model-dependent, so it doesn't need to be a run time choice.

Index: ciacore.c
===================================================================
--- ciacore.c   (revision 44986)
+++ ciacore.c   (working copy)
@@ -859,6 +859,16 @@
             if (addr == CIA_TOD_HR) {
                 /* force bits 6-5 = 0 */
                 byte &= 0x9f;
+#define OLD     1
+#define NEW     (!OLD)
+#if OLD
+                /* Flip AM/PM on hour 12  */
+                /* Flip AM/PM only when writing time, not when writing alarm */
+                if ((byte & 0x1f) == 0x12 &&
+                        (cia_context->c_cia[CIA_CRB] & CIA_CRB_ALARM) == CIA_CRB_ALARM_TOD) {
+                    byte ^= 0x80;
+                }
+#endif
             } else if (addr == CIA_TOD_MIN) {
                 byte &= 0x7f;
             } else if (addr == CIA_TOD_SEC) {
@@ -868,7 +878,7 @@
             }

             {
-                char changed;
+                bool changed;
                 if (cia_context->c_cia[CIA_CRB] & CIA_CRB_ALARM_ALARM) {
                     /* set alarm */
                     changed = cia_context->todalarm[addr - CIA_TOD_TEN] != byte;
@@ -889,9 +899,11 @@
                     }
                     changed = cia_context->c_cia[addr] != byte;
                     if (changed) {
+#if NEW
                         /* Flip AM/PM on hour 12 on the rising edge of the comparator */
                         if ((addr == CIA_TOD_HR) && ((byte & 0x1f) == 0x12))
                             byte ^= 0x80;
+#endif
                         cia_context->c_cia[addr] = byte;
                     }
                 }

I tried ./testbench.sh x64sc tod with both variants and the results were the same (all pass except stability-ntsc.prg which timeouts).

[Olaf Seibert]

I just thought of another question.

What happens if you write $12 to the hours register when it already contails 12 o'clock , and the AM/PM bit is the same as what was in it before. Does PM toggle?

And what happens if you do this but the only changed bit is the PM bit?

[Olaf Seibert]

I'm committing change back to the old way of toggling the AM/PM bit. We're assuming for now that this does not depend on the CIA model (or other hardware variation) nor on the old state of the AM/PM bit. If tests turn up that contradict this we'll fix it.

[gpz]

so we can close this?

[Olaf Seibert]

Yes let's close it.