If you change the playback speed to 0.25x, you can see it happen very clearly in this video: https://youtu.be/Na5ePkX9mQY?t=72
This happens often and very reliably on certain levels of the game I'm working on, but it does not happen at all with the uzem from the master branch.
I ran git bisect telling it that the uzem140 branch was bad, and the master branch was good, and it automatically picked a commit in the middle and I tested that using a short input capture that I had saved. If the test passed, I would type git bisect good, if it failed, I would type git bisect bad. Each time it would automatically checkout a commit in the middle and tell me to test that one and report whether it was bad or good. Eventually it stopped and told me exactly which commit had introduced the error:
Code: Select all
9d65b0e0a98336dc718fae01703f77a25863699f is the first bad commit
commit 9d65b0e0a98336dc718fae01703f77a25863699f
Author: Uze <uze@belogic.com>
Date: Fri Aug 7 01:14:05 2015 -0400
work on timing
In the working code, TIFR1 was updated directly, but that commit introduced a tempTIFR1 variable that sometimes gets written into (and sometimes not), and then TIFR gets updated with tempTIFR at the end of that function. The problem lies further down in that function when trigger_interrupt() gets called. Right before the call to trigger_interrupt, TIFR1 is updated directly, however when it returns from trigger_interrupt() it will eventually get to the bottom of the update_hardware function, and overwrite TIFR1 with whatever was stored in tempTIFR1. Additionally, trigger_interrupt calls back into update_hardware, which means that when all is said and done, TIFR1 has who-knows-what stored inside it, and then we'll sometimes miss the first scanline, and input captures won't be recorded correctly.
Setting a flag when trigger_interrupt gets called and then only writing the tempTIFR1 back into TIFR1 at the end of the function if that flag is not set did not solve the problem, so I went ahead and removed that tempTIFR1 variable completely, so it just always writes directly into TIFR1, and I added back in the two conditionals that got dropped:
Code: Select all
if (TIMSK1 & OCIE1B)
Code: Select all
if (TIMSK1 & OCIE1A)
Lo and behold, that solved both the missing scanline problem (I verified this by playing through my entire WIP), and I was able to use a 25+ minute long input capture file recorded from one version with the other version, and they stayed perfectly in sync!
Here is exactly what I did to the code to make the fix:
Code: Select all
diff --git a/tools/uzem/avr8.cpp b/tools/uzem/avr8.cpp
index b1e5f62..4679826 100644
--- a/tools/uzem/avr8.cpp
+++ b/tools/uzem/avr8.cpp
@@ -632,24 +632,17 @@ void avr8::update_hardware(int cycles)
TCNT1 = TCNT1L | (TCNT1H<<8);
OCR1A = OCR1AL | (OCR1AH<<8);
OCR1B = OCR1BL | (OCR1BH<<8);
- tempTIFR1=TIFR1;
if(TCCR1B & WGM12){ //timer in CTC mode: count up to OCRnA then resets to zero
if (TCNT1 > (0xFFFF - cycles)){
- //TIFR1|=TOV1; //overflow interrupt
- tempTIFR1|=TOV1; //overflow interrupt
+ TIFR1|=TOV1; //overflow interrupt
}
if (TCNT1 <= OCR1B && (TCNT1 + cycles) > OCR1B){
- u16 tmp=TCNT1;
- tmp-=(OCR1B - cycles + 1);
-
- if(tmp==0){
- tempTIFR1|=OCF1B; //CTC match flag interrupt
- }else{
+ if (TIMSK1 & OCIE1B){
TIFR1|=OCF1B; //CTC match flag interrupt
}
@@ -658,11 +651,7 @@ void avr8::update_hardware(int cycles)
if (TCNT1 <= OCR1A && (TCNT1 + cycles) > OCR1A){
TCNT1 -= (OCR1A - cycles + 1);
- if(TCNT1==0){
- //if timer rolls over during the last cycle of an instruction, the int is acknowledged on the next machine cycle
- tempTIFR1|=OCF1A; //CTC match flag interrupt
- }else{
- //otherwise the int is acknowledged for multi-cycles instructions
+ if (TIMSK1 & OCIE1A){
TIFR1|=OCF1A; //CTC match flag interrupt
}
@@ -674,8 +663,7 @@ void avr8::update_hardware(int cycles)
if (TCNT1 > (0xFFFF - cycles)){
if (TIMSK1 & TOIE1){
- //TIFR1|=TOV1; //overflow interrupt
- tempTIFR1|=TOV1; //overflow interrupt
+ TIFR1|=TOV1; //overflow interrupt
}
}
TCNT1 += cycles;
@@ -820,7 +808,6 @@ void avr8::update_hardware(int cycles)
TCCR1B=newTCCR1B; //Timer increments starts after executing the instruction that sets TCCR1B
- TIFR1=tempTIFR1;
}
u8 avr8::exec()
Thoughts?