Page 1 of 1

Proposed enhancement to Uzebox Kernel

Posted: Sun Jan 22, 2023 4:59 pm
by Artcfox
So I ran into a bug with my WIP that could easily be fixed with a tiny modification to the Mode 3 kernel.

The Background:

There is an entity spawning system, so enemies that haven't been seen yet (or have strayed too far off screen) don't take up any RAM or CPU. When they get spawned into existence, some of them use GetVsyncCounter() in order to know the global time so things like sprite animations can remain in sync with the sprite animation of an entity right next to it that may have been spawned into existence at a slightly different time. More importantly, the major rotation of the firebars is tied to this counter value, so if you walk offscreen and the firebar despawns, when you get near that section of the level again it spawns back in with the same "position on the clock" as if it had never been despawned at all.

The Problem:

Press START to bring up the pause menu while a firebar is onscreen, and then press START again to close the pause menu. It will immediately jump to a different clock position as if the game had never been paused. Because for it, and anything in the game that uses GetVsyncCounter() to keep track of world time, the world time didn't stop when the game was paused.

The Solution:

Code: Select all

      // Begin to display pause menu
      uint16_t backupVsyncCounter = GetVsyncCounter();
...
      // (pause menu stuff)
...
      // About to close pause menu

      // Restore the backed up VsyncCounter so time doesn't advance for the entities while the game is paused
      // (For this to compile, you need to add ".global vsync_counter" to the top of uzeboxVideoEngineCore.s)
      extern uint16_t vsync_counter;
      vsync_counter = backupVsyncCounter;
Currently there is a GetVsyncCounter(), and a ClearVsyncCounter(), but no SetVsyncCounter() in the kernel. I wouldn't want to add that and bloat the kernel for all every other video mode, because that might break the games out there that use up all of the available space, so my proposal is to just expose the vsync_counter variable the same way that some of the other variables are exposed in uzeboxVideoEngineCore.s like the following:

Code: Select all

;Public variables
.global sync_pulse
.global sync_phase
.global sync_flags
.global joypad1_status_lo
.global joypad2_status_lo
.global joypad1_status_hi
.global joypad2_status_hi
.global first_render_line
.global render_lines_count
.global vsync_counter
This variable is not used anywhere else by the kernel and the comments indicate that it can be cleared and read by user code at any time to use for timing purposes. Does anyone have any objections to me making that a public variable so it can also be set or restored to a specific counter value?

Re: Proposed enhancement to Uzebox Kernel

Posted: Sun Jan 22, 2023 9:30 pm
by Artcfox
After talking with CunningFellow, putting it in its own function section will not cause it to be included if it's not being used, so making it a proper function won't bloat the kernel like I was worried about.

Attached is my proposed patch. Unzip, and apply it with the command:

Code: Select all

git apply setvsynccounter.patch
in the uzebox/ directory.

For quick viewing/review, here are the contents of the zipped .patch file, but in order for the diff to apply cleanly, you will need to use the zipped patch file with the above git apply command, since those files still have DOS line endings, and any patches also need to have DOS line endings:

Code: Select all

diff --git a/kernel/uzebox.h b/kernel/uzebox.h
index 0458275..f06effa 100644
--- a/kernel/uzebox.h
+++ b/kernel/uzebox.h
@@ -82,6 +82,7 @@
 	extern   u8 GetVsyncFlag(void);
 	extern void ClearVsyncCounter();
 	extern u16	GetVsyncCounter();	
+        extern void SetVsyncCounter(u16 count);
 
 	extern void SetRenderingParameters(u8 firstScanlineToRender, u8 verticalTilesToRender);
 
diff --git a/kernel/uzeboxVideoEngineCore.s b/kernel/uzeboxVideoEngineCore.s
index 7b6e36f..f801aef 100644
--- a/kernel/uzeboxVideoEngineCore.s
+++ b/kernel/uzeboxVideoEngineCore.s
@@ -90,6 +90,7 @@
 .global IsRunningInEmulator
 .global GetVsyncCounter
 .global ClearVsyncCounter
+.global SetVsyncCounter
 
 ;Public variables
 .global sync_pulse
@@ -553,6 +554,18 @@ ClearVsyncCounter:
 	sts vsync_counter+1,r1
 	ret
 
+;************************************
+; Set the vsync counter.
+;
+; C-callable
+; r25:r24=(unsigned int)count
+;************************************
+.section .text.SetVsyncCounter
+SetVsyncCounter:
+        sts vsync_counter,r24
+        sts vsync_counter+1,r25
+        ret
+
 
 ;*****************************
 ; Return joypad 1 or 2 buttons status

Re: Proposed enhancement to Uzebox Kernel

Posted: Sat Jan 28, 2023 8:15 pm
by Artcfox
This is now in master along with a compiler warning fix.

I also added API documentation for SetVsyncCounter, and the missing API documentation for the pre-existing ClearVsyncCounter and GetVsyncCounter functions to the wiki.

If anyone knows when those API functions had been added, the version number might need to be corrected. I also wasn't sure what version number we are on now, so I just put today's date for when SetVsyncCounter was added.

Re: Proposed enhancement to Uzebox Kernel

Posted: Sun Jan 29, 2023 3:23 am
by D3thAdd3r
Thanks for adding that to the kernel.

I feel like Clear/Get were around from the start, but I have no evidence for it. Megatris being the first official game looks to use WaitVsync, at least in it's latest code.

Re: Proposed enhancement to Uzebox Kernel

Posted: Sun Jan 29, 2023 4:56 am
by Artcfox
No problem!

It looks like ClearVsyncCounter and GetVsyncCounter were added during the creation of Video Mode 13 in the following commit:

Code: Select all

commit 76790238c0aed587cd39f81bcd77e93ec89fce41
Author: uze@belogic.com <uze@belogic.com@c007265c-c25a-11dd-ae62-6dab0a2b1684>
Date:   Sat Feb 7 04:17:29 2015 +0000

    -Created video mode 13
    
diff --git a/kernel/uzeboxVideoEngineCore.s b/kernel/uzeboxVideoEngineCore.s
index b09f833..3af3214 100644
--- a/kernel/uzeboxVideoEngineCore.s
+++ b/kernel/uzeboxVideoEngineCore.s
@@ -75,6 +75,8 @@
 .global SetUserPostVsyncCallback
 .global UartInitRxBuffer
 .global IsRunningInEmulator
+.global GetVsyncCounter
+.global ClearVsyncCounter
 
 ;Public variables
 .global sync_pulse
@@ -121,6 +123,8 @@
 						.byte 1
 	joypad2_status_hi:	.byte 1
 						.byte 1
+
+	vsync_counter:		.word 1
 	
 #if TRUE_RANDOM_GEN == 1
 	random_value:			.word 1
@@ -420,6 +424,14 @@ no_render:
 	lds ZH,render_lines_count_tmp
 	sts render_lines_count,ZH
 
+	;increment the vsync counter
+	lds r24,vsync_counter
+	lds r25,vsync_counter+1
+	adiw r24,1
+	sts vsync_counter,r24
+	sts vsync_counter+1,r25
+
+
 	;process user pre callback
 	lds ZL,pre_vsync_user_callback+0
 	lds ZH,pre_vsync_user_callback+1
@@ -502,7 +514,7 @@ hsync_pulse:
 ; This flag is set on each VSYNC by
 ; the engine. This func is used to
 ; synchronize the programs on frame
-; rate (30hz).
+; rate (60hz).
 ;
 ; C-callable
 ;************************************
@@ -524,6 +536,33 @@ ClearVsyncFlag:
 	sts sync_flags,r18
 	ret
 
+
+;************************************
+; Read the current vsync counter.
+; This value is incremented by the kernel
+; on each vertical sync (60hz). Can be used
+; for timeout functions.
+;
+; C-callable
+;************************************
+.section .text.GetVsyncCounter
+GetVsyncCounter:
+	lds r24,vsync_counter
+	lds r25,vsync_counter+1
+	ret
+
+;************************************
+; Clear the vsync counter.
+;
+; C-callable
+;************************************
+.section .text.ClearVsyncCounter
+ClearVsyncCounter:
+	sts vsync_counter,r1
+	sts vsync_counter+1,r1
+	ret
+
+
 ;*****************************
 ; Return joypad 1 or 2 buttons status
 ; C-callable
I just don't know which V# that is a part of, or if that versioning system was abandoned, because the last reference to that type of version number I can find is in a commit made on Jan 17, 2012, removing those version numbers:

Code: Select all

commit 294299433f16af4f0ac119dfaa68396f7d46737a
Author: uze@belogic.com <uze@belogic.com@c007265c-c25a-11dd-ae62-6dab0a2b1684>
Date:   Tue Jan 17 03:54:54 2012 +0000

    -Added current field to sync flags for all video modes
    -Sprite struct now 4 bytes in size

diff --git a/kernel/uzeboxVideoEngineCore.s b/kernel/uzeboxVideoEngineCore.s
index 4b812ca..868559b 100644
--- a/kernel/uzeboxVideoEngineCore.s
+++ b/kernel/uzeboxVideoEngineCore.s
@@ -21,32 +21,7 @@
 /*
        Changes
        ---------------------------------------------
-       V2.0:
-       -Sprite engine
-       -Reset console with joypad
-       -ReadJoypad() now return int instead of char
-       -NTSC timing more accurate
-       -Use of conditionals (see defines.h)
-       -Many small improvements
-
-       V3.0
-       -Major Refactoring: All video modes in their own files
-       -New video modes: 3,4,6,7,8
-       -EEPROM functions
-       -Assembly functions in their own sections to save flash
-       -Added Vsync User callback
-       -UART Receive buffer & functions
-       -Color burst offset control
-
-       V3.2
-       -Rewrote sync code
-       -Use interrupt to pull back sync line for serration pulses
-       -Added inline mixer selectable with a compile switch
-       -Added channel 5 PCM (avail with inline mixer only)
-       -Fixed the "click" sound upon game resets
-       -Removed color burst offset code
-       -Removed RAM patch code in music engine
-
+       See http://code.google.com/p/uzebox/source/browse/trunk/README.txt
 */
but that URL is now broken, and I don't see it in the current README.md, so I'm still not sure, but GetVsyncCounter and ClearVsyncCounter happened at least 3 years after V3.2, so I guess I can just use the dates for now on the wiki.

Edit: I did find a slightly more "updated" version in GitHub. It is the tags, but it only goes to: Rev 3.3 Notes (Jan 03, 2012)

Re: Proposed enhancement to Uzebox Kernel

Posted: Sat Feb 04, 2023 4:38 am
by uze6666
Hi Artfox,

Couldn't this very game oriented pause behavior be implemented via the generic UserVsyncCallback mechanism instead? If I recall, the idea of this GetVsyncCounter() was really meant to act as a global system tick counter. Apps would have to handle the rollover logic if required and whatever they want to do with it.

Re: Proposed enhancement to Uzebox Kernel

Posted: Sat Feb 04, 2023 7:11 am
by Artcfox
uze6666 wrote: Sat Feb 04, 2023 4:38 am Hi Artfox,

Couldn't this very game oriented pause behavior be implemented via the generic UserVsyncCallback mechanism instead? If I recall, the idea of this GetVsyncCounter() was really meant to act as a global system tick counter. Apps would have to handle the rollover logic if required and whatever they want to do with it.
I did consider that, but the counter already takes up two bytes of RAM and the kernel spends cycles incrementing it every frame, even when games don't use it, so even if we go the UserVsyncCallback method, we still wouldn't be able to set the value of that counter, so we would have to spend 2 more bytes for a new variable + the cycles to increment it + the additional push/pop of registers for using a UserVsyncCallback. That seems like a waste when it already does 99% of the job, and the last 1% can be added in a way that is both backwards compatible and doesn't take up any additional space unless you call the new function. (CunningFellow gave me hints and reviewed the ASM to ensure it doesn't take up space unless called.)

I added API documentation to the wiki for ClearVsyncCounter/GetVsyncCounter/SetVsyncCounter, so people can discover those functions exist.

It's totally your call though, it just seemed weird to have a timer meant for user purposes with no way to set it.

Re: Proposed enhancement to Uzebox Kernel

Posted: Sat Feb 04, 2023 8:17 am
by CunningFellow
I can't see anything that would break by adding this. The only code so far that uses GetVsyncCounter() is the Uzenet demo for a "timeout". It also uses ClearVsyncCounter() to start the time count.

SetVSyncCounter() is really just ClearVSyncCounter() with a non-zero value.

It should not bloat flash usage at all as it has its own .section

It makes the VSyncCounter variable more "complete".

It is pretty common to have get/set/clear on things like this UNLESS the variable is meant to be read-only for some reason in which case CLEAR would normally not exist.

Re: Proposed enhancement to Uzebox Kernel

Posted: Tue Feb 07, 2023 5:17 am
by uze6666
Sorry guys, I have misread the whole thing!! Guess I'm, getting rusty. When viewing the diff on Github graphically, it make sense. I totally agree that adding a Set method just make it more useful and has, of course, no impact. :)

Re: Proposed enhancement to Uzebox Kernel

Posted: Tue Feb 07, 2023 5:34 am
by Artcfox
Awesome! Thank you. :D