semiversus / jeg Goto Github PK

In order to support dirty-matrix technique, I suggest to use callback to replace the direct frame-buffering access:

Original:
ppu->video_frame_data[ppu->scanline*256+ppu->cycle-1]=ppu->palette[color];

Callback:

typedef void ppu_draw_pixel_t (uint_fast8_t y, uint_fast8_t x, uint_fast8_t color);

ppu->draw_pixel(ppu->scanline, ppu->cycle-1, ppu->palette[color]);

I understand that using callback will reduce the memory access performance due to the unnecessary parameter passing, but comparing the performance gain of eliminating refreshing unchanged tiles, it is worth it.

For PC or other architecture which have rich computing power, the overhead of this callback negligible , but it is vital to enable the dirty-matrix technique which saves a lot of computing power on embedded processors.

I propose to use an unified configuration style:

typedef struct {

} xxxxx_cfg_t;

bool xxxx_init( xxxx_t *ptObj, xxxx_cfg_t *ptCFG)
{

}

The benefits are:

• You don't have to change the prototype to add new parameters
• You can omit some structure members (let it to be zero by default) during optimisation.
• Some popular architecture (such as Arm) using R0~R3 for the first four parameters and it's inefficient to pass more than 4 parameters.
• configurations could be saved in ROM rather than in RAM. If service will be initialised frequently (it's rare and not a good practise), then configuration stored in ROM could save the time used to allocate a memory in stack, copy content to that memory and pass them to the xxxx_init.

I have noticed that there are plenty of dummy readings in 6502 emulation? Are they really necessary? ( I guess they might be important if the target address belongs to PPU, as the dummy read might affect the latch).

Currently, the example code calls update_frame on each round:

    nes_set_controller(&this.tNESConsole, this.chController[0], this.chController[0]);
    nes_iterate_frame(&this.tNESConsole);
    update_frame(&this.tFrame);

Actually, we concern that this could lead to some unnecessary frame updates before a complete frame is actually ready. Could we add a method to sync up between ppu_update and update_frame?

My proposal is either adding a call-back or a dedicated flag which can be checked by update_frame.

Could we apply dirty-matrix technique by monitoring nametable, if there is no any x/y scrolling ?
So we can only draw the changed tiles.

I also think maybe we can maintain two nametable images, and using memory copy methods to cope with scrolling.

What do you think?

Hi,

I have encountered an issue which I cannot understand. If the sprite buffer is enabled, the Road Fighter will render incorrect sprites. The sprite buffer works well with Super Mario Bro and City Tank

Please help.

Cheers,
Gabriel

It looks like we don’t support Four-screen mirroring uses an additional 2 KB of RAM in the cartridge itself to allow logical name tables to each map to separate physical name tables. Do we have plan?

Hi

I haven't heard from you for quite a long time.
Have you tried the NES simulator on STM32F746G-Discovery Board?

Cheers,
Gabriel

I found some do{ ... }while(false) constructs in code. Are these left overs from former macro definitions or may I'm missing something?

• jeg/cpu6502.c

  Line 66 in ce4c94a

  do {

• jeg/nes.c

  Line 277 in ce4c94a

  do {

• jeg/ppu.c

  Line 41 in ce4c94a

  do {

• jeg/ppu.c

  Line 359 in ce4c94a

  do {

In cpu6502_opcodes.h, the structure opcode_tbl_entry is defined improperly:

struct opcode_tbl_entry {
char *mnemonic;
operation_enum_t operation;
address_mode_enum_t address_mode;
int bytes;
int cycles;
int page_cross_cycles;
}opcode_tbl_entry ;

As this header file is included by multiple source files, linker ends up throw following errors:

.\Out\msp2_file_io_template.axf: Error: L6200E: Symbol opcode_tbl_entry multiply defined (by cpu6502_debug.o and cpu6502.o).
.\Out\msp2_file_io_template.axf: Error: L6200E: Symbol opcode_tbl_entry multiply defined (by cpu6502_opcodes.o and cpu6502.o).

It's safe to change it to:

struct opcode_tbl_entry {
char *mnemonic;
operation_enum_t operation;
address_mode_enum_t address_mode;
int bytes;
int cycles;
int page_cross_cycles;
};

Hi,
About the sprite test failure, is there any update?
Since my next ppu improvement is mainly based on adding screen buffer for sprite rendering, I hesitate to move forward...

Cheers,
Gabriel

Hi

I have done some improvements to jeg, but I need permission to create a new branch.
Please help.

Cheers,
Gabriel

I have seen an unnecessary pointer validation in the ppu_update code:

      if (ppu->video_frame_data) {
        ppu->video_frame_data[ppu->scanline*256+ppu->cycle-1]=ppu->palette[color];
      }

This kind of validation should be done in the initialisation phase rather than in such a HOTSPOT area. Especially the ppu->video_frame_data is less likely to be changed dynamically (on-fly).

Please consider to remove it.

Here is an example of the definition and using of a function pointer in this project:

typedef uint_fast8_t  (*cpu6502_read_func_t) (void *, uint_fast16_t hwAddress);

typedef struct cpu6502_t {
    ...
    cpu6502_read_func_t read;
} cpu6502_t

Add "" to the cpu6502_read_func_t definition makes the definition of read not looking like a pointer. My suggestion is, remove the "" from the definition, so we can use the unified (and simple) syntax for all kinds of pointers:

//! we define a function prototype instead of defining a function pointer type
typedef uint_fast8_t  cpu6502_read_func_t(void *, uint_fast16_t hwAddress);

typedef struct cpu6502_t {
    ...
    cpu6502_read_func_t *read;    //!< we define a pointer pointing to a prototype 
} cpu6502_t

Hence, we also know:

• When getting the address of a function, we should use "&" rather than ignore it.
• When using a function pointer to issue a function call, we should add "*" to de-referencing the function pointer.

This makes the C language more easy to use and understand.

With commit 4372a45 I started to rework the interface between emulator and ppu. At the moment the ppu is defined by a big structure containing all the internals states, references to the emulator and references to the actual video output.
When playing around with different ideas for ppu optimizations things started to get complicated so here is a proposal to solve this issue:

• The only interface the emulator is getting to the ppu is via function pointers for read and write ppu registers and reset the ppu.
• An additional void * reference is here to point to the internal ppu data structure. The emulator don't need to have any knowledge about it.
• The ppu will be initialized outside of the emulator and is a module of it's own. No matter what the output looks like or which internal optimizations are used (fpga implementation, opengl buffering, ...) it's all up to the implementation.
• This separation helps to clean up nes.c which is holding a lot of ppu centric functionality.
• Different ppu implementations can share same functionality

The separation is work in progress. Missing is clean up of nes.c and adapting ppu_pixel_based.c (renamed to ppu_caching.c). The same separation makes sense for APU and controller input as well.

What do you think? @GorgonMeducer

• enable continuous integration
  • run unittests
  • build a windows .exe
• benchmark script to compare various versions
• rework folder layout
  • src for the main source
    • cpu
    • ppu
    • apu (not part of version 0.1)
  • platforms for different platforms
    • windows
    • linux
    • ...

Hi Günther

I have cloned your re-write branch and ported the EmbeddedNES to this branch using ppu_caching.c and ppu_caching.h. I manage to load a ROM and run it on FVP but there is nothing rendered.

After several hours struggling, I found that ppu->ppumask never got updated. Since there are a lot of structural update, I wonder, did you validated that the ppu_caching was still working?

Cheers,
Gabriel

In function fetch_sprite_pattern(ppu_t *ppu, int i, int row),

for (int j=0; j<8; j++) {
int p1, p2;
if ((attributes&0x40)==0x40) {
p1=(low_tile_byte&0x01);
p2=(high_tile_byte&0x01)<<1;
low_tile_byte>>=1;
high_tile_byte>>=1;
}
else {
p1=(low_tile_byte&0x80)>>7;
p2=(high_tile_byte&0x80)>>6;
low_tile_byte<<=1;
high_tile_byte<<=1;
}
data<<=4;
data|=((attributes&3)<<2)|p1|p2;
}

It's obvious that the result of the if-then clause is unchanged during the looping time, so do it 8 times would be a waster.

This is I suggested:

int p1, p2;
if (attributes&0x40) {
    for (int j=0; j<8; j++) {
        p1=(low_tile_byte&0x01);
        p2=(high_tile_byte&0x01)<<1;
        low_tile_byte>>=1;
        high_tile_byte>>=1;
        
        data<<=4;
        data|=((attributes&3)<<2)|p1|p2;
    }
} else {
    for (int j=0; j<8; j++) {
        p1=(low_tile_byte&0x80)>>7;
        p2=(high_tile_byte&0x80)>>6;
        low_tile_byte<<=1;
        high_tile_byte<<=1;

        data<<=4;
        data|=((attributes&3)<<2)|p1|p2;
    }
}