Skip to content

Conversation

@finger563
Copy link
Contributor

Description

  • Add shared_memory component for sharing memory between the emulators more easily
  • Refactored all emulators to use new shared memory for majority of state and data. This means each emulator component now has less than 5k of D/IRAM in use that is not from shared memory.
  • Update to have better clear screen function that wont have any race conditions and wont destroy buffers. Update video task to have simple clear screen function if passed nullptr.
  • Update cmake lists to use newer version of cmake
  • Update to hid roms from rom list if their emulator is not enabled for easier / faster iteration and testing when adding / modifying emulators
  • Update espp to latest

Motivation and Context

Shared memory means that it is not easier to add more emulators (such as msx and doom) since the emulators no longer fill up the internal ram. This does require in some cases modifying the emulators to leverage the shared memory, but this is a one-time operation and actually helps ensure the emulator maintains high performance by keeping its state within internal memory as much as possible, instead of in offboard PSRAM.

How has this been tested?

Build and run main on box-3 and ensure all emulators work for multiple sessions of different roms in sequence. Also ensure that saving state and loading state works for each emulator.

Screenshots (if appropriate, e.g. schematic, board, console logs, lab pictures):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Hardware (schematic, board, system design) change
  • Software change

Checklist:

  • My change requires a change to the documentation.
  • I have added / updated the documentation related to this change via either README or WIKI

Software

  • I have added tests to cover my changes.
  • I have updated the .github/workflows/build.yml file to add my new test to the automated cloud build github action.
  • All new and existing tests passed.
  • My code follows the code style of this project.
* Add `shared_memory` component for sharing memory between the emulators more easily * Refactored all emulators to use new shared memory for majority of state and data. This means each emulator component now has less than 5k of D/IRAM in use that is not from shared memory. * Update to have better clear screen function that wont have any race conditions and wont destroy buffers. Update video task to have simple clear screen function if passed nullptr. * Update cmake lists to use newer version of cmake * Update to hid roms from rom list if their emulator is not enabled for easier / faster iteration and testing when adding / modifying emulators * Update espp to latest Shared memory means that it is not easier to add more emulators (such as msx and doom) since the emulators no longer fill up the internal ram. This does require in some cases modifying the emulators to leverage the shared memory, but this is a one-time operation and actually helps ensure the emulator maintains high performance by keeping its state within internal memory as much as possible, instead of in offboard PSRAM. Build and run `main` on box-3 and ensure all emulators work for multiple sessions of different roms in sequence. Also ensure that saving state and loading state works for each emulator.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 80 out of 84 changed files in this pull request and generated 2 comments.

Files not reviewed (4)
  • CMakeLists.txt: Language not supported
  • components/espp: Language not supported
  • components/gbc/CMakeLists.txt: Language not supported
  • components/genesis/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (1)

components/box-emu/src/box-emu.cpp:736

  • [nitpick] The arithmetic used to select between vram0 and vram1 is unclear. Consider using a ternary operator to improve readability.
 Pixel* _buf = (Pixel*)((uint32_t)vram0 * (vram_index ^ 0x01) + (uint32_t)vram1 * vram_index); 
Comment on lines +64 to +65
scan = (struct gbc_scan*)shared_mem_allocate(&gbc_scan_request);

Copy link

Copilot AI Apr 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared memory allocation for 'scan' is not checked for a null return value. Consider adding a check to handle allocation failures.

Suggested change
scan = (struct gbc_scan*)shared_mem_allocate(&gbc_scan_request);
scan = (struct gbc_scan*)shared_mem_allocate(&gbc_scan_request);
if (!scan) {
ESP_LOGE(TAG, "Failed to allocate GBC scan object");
return;
}
Copilot uses AI. Check for mistakes.
Comment on lines +78 to +79
cpu = (struct cpu*)shared_mem_allocate(&cpu_request);

Copy link

Copilot AI Apr 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no null check after allocating shared memory for 'cpu'. Adding error handling here can prevent potential null pointer dereferences.

Suggested change
cpu = (struct cpu*)shared_mem_allocate(&cpu_request);
cpu = (struct cpu*)shared_mem_allocate(&cpu_request);
if (!cpu) {
ESP_LOGE(TAG, "Failed to allocate CPU structure");
return;
}
Copilot uses AI. Check for mistakes.
@github-actions
Copy link

github-actions bot commented Apr 6, 2025

⚡ Static analysis result ⚡

🔴 cppcheck found 1 issue! Click here to see details.

printf("Num bytes allocated: %d\n", current_offset_);
// Use SIMD-accelerated memset from ESP32
// heap_caps_memset(memory_pool_, 0, TOTAL_MEMORY_SIZE);
memset(memory_pool_, 0, TOTAL_MEMORY_SIZE);
current_offset_ = 0;
}

!Line: 64 - portability: %d in format string (no. 1) requires 'int' but the argument type is 'size_t {aka unsigned long}'. [invalidPrintfArgType_sint] 


@finger563 finger563 merged commit d67d1bd into main Apr 7, 2025
2 of 3 checks passed
@finger563 finger563 deleted the feat/shared-memory branch April 7, 2025 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment