lronaldo / cpctelera

Astonishingly fast Amstrad CPC game engine for C developers
http://lronaldo.github.io/cpctelera/
GNU Lesser General Public License v3.0
229 stars 54 forks source link

Added configurable interrupt handler wrapper macros #143

Closed nestornillo closed 2 years ago

nestornillo commented 3 years ago

-Added macros for C and assembler that create a custom version of cpct_setInterruptHandler, saving selected registers (cpctm_createInterruptHandlerWrapper and cpctm_createInterruptHandlerWrapper_asm). -Added C macro for declaration of the created function (cpctm_declareInterruptHandlerWrapper).

lronaldo commented 3 years ago

This is a very interesting idea, quite similar to de one I did with cpctm_push and cpctm_pop. It is really interesting to simplify the creation of interrupt handlers with exactly the required push/pops. However, for a better interface I would use a different technique, like this one:

.mdelete save
.macro save
.endm

.mdelete savealt
.macro savealt
   ex af, af'
   exx
.endm

.mdelete saveaf
.macro saveaf
   push af
.endm

.mdelete savebc
.macro savebc
   push bc
.endm

.mdelete savede
.macro savede
   push de
.endm

.mdelete savehl
.macro savehl
   push hl
.endm

.mdelete saveix
.macro saveix
   push ix
.endm

.mdelete saveiy
.macro saveiy
   push iy
.endm

.mdelete restore
.macro restore
.endm

.mdelete restorealt
.macro restorealt
   ex af, af'
   exx
.endm

.mdelete restoreaf
.macro restoreaf
   pop af
.endm

.mdelete restorebc
.macro restorebc
   pop bc
.endm

.mdelete restorede
.macro restorede
   pop de
.endm

.mdelete restorehl
.macro restorehl
   pop hl
.endm

.mdelete restoreix
.macro restoreix
   pop ix
.endm

.mdelete restoreiy
.macro restoreiy
   pop iy
.endm

.macro createWrapper name, address, R1, R2, R3, R4, R5, R6, R7, R8, R9, R10, R11, R12
name::
   save'R1
   save'R2
   save'R3
   save'R4
   save'R5
   save'R6
   save'R7
   save'R8
   save'R9
   save'R10
   save'R11
   save'R12

   call #address

   restore'R12
   restore'R11
   restore'R10
   restore'R9
   restore'R8
   restore'R7
   restore'R6
   restore'R5
   restore'R4
   restore'R3
   restore'R2
   restore'R1
.endm

;; My interrupt handler
irh:
   call reproduceMusic
   ret

;;; Create a handler wrapper
createWrapper irh_wrapper, irh, af, bc, hl, ix, alt, af

main::
   ;;; We need a new cpct_setInterruptHandler_naked_asm that just patches interrupt vector
   cpct_setInterruptHandler_naked_asm irh_wrapper

   ;;; Other code

   jr .

This way, the user specifies the names of the registers he wants to save/restore, which is far easier to understand. The only caveat is using the keyword "alt" to switch register banks. but the user can save anything he/she wants.

Only important thing is to change names of these macros to something less common (save and restore are very common names). Something much more cpct_ish and internal, not to collide with user code.

Moreover, as the wrapper is going to be generated, there is no need to do the pathching of the general wrapper (which is a single code for every possible use). Here the user generates a personalized wrapper for his/her personalized handler.

However, I feel that the uses of this code are not many. C users need to save all the registers because they cannot know in advance which ones will the compiler use. Assembly users have complete control of their code and should know about saving/restoring registers and how interrupts work. Therefore, assembly users will probably prefer to produce their own Interrupt Handlers, in general. This tool seems to be useful only for a narrow range of novices that still don't understand interrupt handlers very well.

nestornillo commented 3 years ago

Yes, the interface of your method is much better (also code is more readable and more 'pro' like). After seeing your last videos it is pretty clear for me that the proper use of interrupts, in assembler, is to fully control each of them (different cpctm_push for each interrupt). This is why I agree with your last paragraph, there's not much use for this macros. However I think it is still needed an easy way, using only CPCtelera C functions, to save all registers (I'm thinking on a simple C game with music). So, Would it be more convenient to CPCtelera to just drop this PR and create a cpct_setInterruptHandlerAllRegisters?

lronaldo commented 3 years ago

I agree that at least an interrupt handler that saves all registers is required for C developers. Additionally, as this PR is almost done, we can include it. Maybe it could be of use for someone, even if it tangencial. It might be educative for novices or useful in some cases.

So, from my point of view, this general version could be finished and accompanied with the 2 general versions for C developers, which save main registers, and main + alternative registers. That would be nice :)

nestornillo commented 3 years ago

Ok, I will try to do that

lronaldo commented 3 years ago

Thank you very much, @nestornillo. You are doing and promoting a lot of features with your work ☺️

nestornillo commented 3 years ago

I've changed the macros to the style you proposed. In order use only exx/ex af,af' when needed, I've added some initial parameter checks (makes code a bit longer, though). The names I've chosen for the macros are a bit long, but I think they are quite descriptive. The documentation looks ok for me, but I'm pretty sure it can have some of you 'teacherness' applied (specially the parameter usage)

nestornillo commented 3 years ago

I didn't know that interrupts are automatically disabled on interrupt calls, there's always things to learn! I've also removed the 'di' from cpct_setInterruptHandler. (btw, I've touched something, and now it says "nestornillo requested a review from lronaldo", I don't know why)

nestornillo commented 2 years ago

Unfortunately, after the recent change in Z80CCINCLUDE, the "include" in the C macro fails again. It only seems to work with "-Wa -I$(CPCT_SRC)" parameter in Z80CCINCLUDE, but then we get the collisions between equal name local symbols in different files (btw, I don't understand why, I would uderstand it with SRCDIR, but not with CPCT_SRC).

I think I sholud put back the repeated code inside the macro.

Also, now the "-Wa -I$(CPCT_SRC),-I$(SRCDIR)" parameter is inside Z80CCLINKARGS varialble, I think it does nothing there, as Wa is for assembler and Z80CCLINKARGS is used for linking.

lronaldo commented 2 years ago

You are right with respect to the -Wa modifier. I missplaced it in the link parameters when it should be in the compiler parameters, but only for C-compiler, not for ASZ80-compiler. I have fixed it and uploaded to development. Could you please check it now works fine?

nestornillo commented 2 years ago

First tests with the modifier inside Z80CCFLAGS seems to work fine both with the 'include issue' and the 'local identifiers issue', but I'll test better tomorrow.

nestornillo commented 2 years ago

Ok, I've moved the '.mdelete' from the C macro to the '.asm' file. I've done more tests and all works fine (this macros and the 'local symbols issue').

lronaldo commented 2 years ago

Very nice. I think this feature is ready for production. I will merge them now.

Just one important thing. For people to know and use it, we need a simple example in the examples folder. A simple example doing simple things like changing the border each interrupt is enough, but it should show how to use create and declare macros in C for users to know how to use them. If you want, you may create one in the future :)

Nice work, @nestornillo :+1: