Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Smsplus64 Emulator #159

Merged
merged 6 commits into from
Nov 24, 2024
Merged

Conversation

networkfusion
Copy link
Collaborator

@networkfusion networkfusion commented Nov 11, 2024

Description

Switch to smsPlus64 (https://github.com/fhoedemakers/smsplus64) from TotalSMS.

Motivation and Context

This emulator, although still WIP (like most others) works with the plugin system.

How Has This Been Tested?

Screenshots

Types of changes

  • Improvement (non-breaking change that adds a new feature)
  • Bug fix (fixes an issue)
  • Breaking change (breaking change)
  • Documentation Improvement
  • Config and build (change in the configuration and build system, has no impact on code or features)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>

Copy link

coderabbitai bot commented Nov 11, 2024

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The pull request includes significant updates across multiple files, focusing on enhancing build workflows, documentation, and functionality in the project. Key changes involve modifications to the GitHub Actions workflow for releases, updates to the .gitignore file, and enhancements to the Makefile for asset management. Additionally, new features are documented in the README.md, and various source files have been updated to improve functionality, including support for new flashcart types and enhanced menu interactions with sound effects.

Changes

File Path Change Summary
.github/workflows/build.yml Modified release parameters, added make_latest: true, updated job names and descriptions, and changed conditions.
.gitignore Added /filesystem/*.sprite to ignore files with .sprite extension.
Makefile Added new source files and asset categories, updated file processing rules, and introduced new audio and sprite rules.
README.md Documented new features like menu sound effects, N64 ROM autoload, and GamePak sprites.
docs/00_getting_started_sd.md Added sections for SC64 and other supported flashcarts, updated emulator support.
docs/99_developer_guide.md Restructured submodule update instructions and added prerequisites for local documentation testing.
libdragon Updated subproject commit reference.
src/boot/boot_io.h Added typedefs and macros for IO types, enhanced documentation for structures.
src/boot/vr4300_asm.h Enhanced documentation for instruction types and fields.
src/flashcart/64drive/64drive.c Expanded d64_has_feature function to include new flashcart features.
src/flashcart/ed64/ed64_vseries.c Introduced functionality for ED64 flashcart series, including ROM loading and feature checks.
src/menu/actions.c Enhanced joypad input handling and added new actions for menu interactions.
src/menu/components.h Added new UI component functions and enumerations for file image types.
src/menu/menu.c Refactored menu initialization and rendering logic, removed frame counter.
src/menu/views/browser.c Integrated sound effects for menu navigation actions.
src/menu/views/error.c Added sound effect on error display.
src/menu/views/file_info.c Updated file type handling and integrated sound effects for back action.
src/menu/views/rtc.c Introduced new functions and structures for RTC editing features.
src/menu/views/settings_editor.c Added new functions and context menus for toggling settings.
src/menu/views/startup.c Updated to handle ROM autoload settings based on user input.
src/menu/views/system_info.c Added sound effects for back action and refined date/time display.
src/menu/views/text_viewer.c Integrated sound effects for back action in text viewer.

Possibly related PRs

Poem

In the meadow where rabbits play,
New features hop in, brightening the day.
With sounds that chirp and menus that gleam,
Our flashcart dreams are more than a dream!
So let’s celebrate with a joyful cheer,
For every change brings us closer, my dear! 🐰✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@networkfusion networkfusion changed the base branch from main to develop November 11, 2024 14:44
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

🛑 Comments failed to post (43)
src/flashcart/ed64/ed64_xseries.h (1)

1-24: 💡 Codebase verification

Integration is incomplete - ED64 X-series implementation is missing

The header file ed64_xseries.h is properly structured, but the implementation file ed64_xseries.c is missing. Additionally, the flashcart registration in flashcart.c is commented out with a FIXME note awaiting a fix from libcart:

// FIXME: this is commented out awaiting a fix from libcart.
// case CART_EDX:  // Series X EverDrive-64
//     flashcart = ed64_xseries_get_flashcart();
  • Missing implementation file: ed64_xseries.c
  • Commented out registration in src/flashcart/flashcart.c
🔗 Analysis chain

Verify flashcart subsystem integration.

Let's verify how this new flashcart type integrates with the existing subsystem.


Let me gather more information about the flashcart integration.


Let me check the implementation of the ED64 X-series flashcart to verify its integration.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check how the new flashcart type is registered and used
echo "Checking flashcart registration..."
rg -p "ed64xseries_get_flashcart" --type c
echo -e "\nChecking flashcart feature usage..."
ast-grep --pattern 'FLASHCART_FEATURE_$_'

Length of output: 337


Script:

#!/bin/bash
# Check flashcart registration and implementation
echo "Checking flashcart implementation..."
rg -p "ed64xseries_get_flashcart" --type c -A 10

echo -e "\nChecking flashcart.h interface..."
cat src/flashcart/flashcart.h

echo -e "\nChecking where flashcarts are registered..."
rg "register_flashcart|get_flashcart" --type c

Length of output: 5048


Script:

#!/bin/bash
# Check ED64 X-series implementation
echo "Checking ED64 X-series implementation..."
find . -type f -name "ed64_xseries.c" -exec cat {} \;

echo -e "\nChecking flashcart initialization..."
rg -p "flashcart =" --type c -A 5 -B 5

Length of output: 1558

src/flashcart/ed64/ed64_vseries.h (1)

11-11: 💡 Codebase verification

Feature implementation inconsistency detected

The ED64 V-series implementation doesn't handle all defined features from flashcart.h. Specifically:

  • Missing cases for: FLASHCART_FEATURE_64DD, FLASHCART_FEATURE_AUTO_REGION, FLASHCART_FEATURE_DIAGNOSTIC_DATA, FLASHCART_FEATURE_BIOS_UPDATE_FROM_MENU, FLASHCART_FEATURE_SAVE_WRITEBACK
  • The implementation returns false for these features without explicit handling
  • This differs from other flashcart implementations which typically handle all defined features explicitly
🔗 Analysis chain

Verify flashcart interface implementation

Let's ensure this new flashcart type properly implements all required features consistently with other flashcart implementations.


Let me check the implementation of the feature support in ed64_vseries.c to verify it properly handles all defined features.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent feature implementation across flashcart types

# Find all flashcart implementation files
echo "=== Flashcart Implementations ==="
fd -e c -e h "ed64|sc64" src/flashcart

# Check for feature consistency
echo -e "\n=== Feature Definitions ==="
rg "FLASHCART_FEATURE_" src/flashcart/flashcart.h

echo -e "\n=== Feature Usage ==="
rg "has_feature|get_features" src/flashcart

Length of output: 1815


Script:

#!/bin/bash
# Get the implementation of ed64_vseries_has_feature
ast-grep --pattern 'bool ed64_vseries_has_feature(flashcart_features_t feature) {
  $$$
}'

# Check if all features are handled
echo -e "\n=== Feature Handling ==="
rg -A 10 "ed64_vseries_has_feature" src/flashcart/ed64/ed64_vseries.c

Length of output: 1071

src/menu/views/error.c (1)

53-53: 🛠️ Refactor suggestion

Add error handling for sound effect in critical error path

While the error sound effect addition is appropriate, this function handles critical error states and should be more robust.

Implement error handling to ensure sound playback failures don't interfere with error display:

-    sound_play_effect(SFX_ERROR);
+    if (sound_play_effect(SFX_ERROR) != SOUND_OK) {
+        debugf("Warning: Failed to play error sound effect\n");
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    if (sound_play_effect(SFX_ERROR) != SOUND_OK) {
        debugf("Warning: Failed to play error sound effect\n");
    }
src/menu/views/startup.c (2)

17-22: 🛠️ Refactor suggestion

Add error handling for settings save operation

The settings save operation could fail due to storage issues. Consider adding error handling:

 if (menu->settings.rom_autoload_enabled && b_held.start) {
     menu->settings.rom_autoload_enabled = false;
     menu->settings.rom_autoload_path = "";
     menu->settings.rom_autoload_filename = "";
-    settings_save(&menu->settings);
+    if (!settings_save(&menu->settings)) {
+        // Handle save failure - perhaps set an error state or retry
+        menu->error_state = MENU_ERROR_SETTINGS_SAVE;
+    }
 }

Committable suggestion skipped: line range outside the PR's diff.


24-31: ⚠️ Potential issue

Add validation and error handling for path operations

The path handling code needs additional safety checks:

  1. Validate paths before use
  2. Handle path initialization failures
  3. Ensure proper memory management for cloned paths
 if (menu->settings.rom_autoload_enabled) {
+    if (!menu->settings.rom_autoload_path || !menu->settings.rom_autoload_filename) {
+        menu->next_mode = MENU_MODE_BROWSER;
+        return;
+    }
     menu->browser.directory = path_init(menu->storage_prefix, menu->settings.rom_autoload_path);
+    if (!menu->browser.directory) {
+        menu->error_state = MENU_ERROR_PATH_INIT;
+        menu->next_mode = MENU_MODE_BROWSER;
+        return;
+    }
     menu->load.rom_path = path_clone_push(menu->browser.directory, menu->settings.rom_autoload_filename);
+    if (!menu->load.rom_path) {
+        path_free(menu->browser.directory);
+        menu->error_state = MENU_ERROR_PATH_CLONE;
+        menu->next_mode = MENU_MODE_BROWSER;
+        return;
+    }
     menu->boot_pending.rom_file = true;
     menu->next_mode = MENU_MODE_LOAD_ROM;
     return;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    if (menu->settings.rom_autoload_enabled) {
        if (!menu->settings.rom_autoload_path || !menu->settings.rom_autoload_filename) {
            menu->next_mode = MENU_MODE_BROWSER;
            return;
        }
        menu->browser.directory = path_init(menu->storage_prefix, menu->settings.rom_autoload_path);
        if (!menu->browser.directory) {
            menu->error_state = MENU_ERROR_PATH_INIT;
            menu->next_mode = MENU_MODE_BROWSER;
            return;
        }
        menu->load.rom_path = path_clone_push(menu->browser.directory, menu->settings.rom_autoload_filename);
        if (!menu->load.rom_path) {
            path_free(menu->browser.directory);
            menu->error_state = MENU_ERROR_PATH_CLONE;
            menu->next_mode = MENU_MODE_BROWSER;
            return;
        }
        menu->boot_pending.rom_file = true;
        menu->next_mode = MENU_MODE_LOAD_ROM;
        return;
    }
src/menu/png_decoder.h (1)

37-72: 🛠️ Refactor suggestion

Enhance function declarations and documentation

The function declarations need several improvements:

  1. The path parameter in png_decoder_start should be const as it's not modified
  2. Thread safety/reentrancy information should be documented

Apply these changes:

- png_err_t png_decoder_start (char *path, int max_width, int max_height, png_callback_t *callback, void *callback_data);
+ png_err_t png_decoder_start (const char *path, int max_width, int max_height, png_callback_t *callback, void *callback_data);

Add thread safety documentation at the top of the file:

 /**
  * @file png_decoder.h
  * @brief PNG decoder
  * @ingroup menu 
+ * 
+ * @note This decoder is not thread-safe. All functions must be called from the same thread.
  */

Committable suggestion skipped: line range outside the PR's diff.

src/menu/sound.c (2)

43-51: 🛠️ Refactor suggestion

Consider improving robustness of SFX initialization.

A few suggestions to enhance this function:

  1. Add error handling for WAV file loading
  2. Consider making the volume configurable
  3. Consider respecting a user preference for SFX state instead of forcing it enabled

Here's a suggested improvement:

 void sound_init_sfx (void) {
-    mixer_ch_set_vol(SOUND_SFX_CHANNEL, 0.5f, 0.5f);
-    wav64_open(&sfx_cursor, "rom:/cursorsound.wav64");
-    wav64_open(&sfx_exit, "rom:/back.wav64");
-    wav64_open(&sfx_setting, "rom:/settings.wav64");
-    wav64_open(&sfx_enter, "rom:/enter.wav64");
-    wav64_open(&sfx_error, "rom:/error.wav64");
-    sfx_enabled = true;
+    const float default_volume = 0.5f;
+    mixer_ch_set_vol(SOUND_SFX_CHANNEL, default_volume, default_volume);
+    
+    // Load sound effects
+    if (!wav64_open(&sfx_cursor, "rom:/cursorsound.wav64") ||
+        !wav64_open(&sfx_exit, "rom:/back.wav64") ||
+        !wav64_open(&sfx_setting, "rom:/settings.wav64") ||
+        !wav64_open(&sfx_enter, "rom:/enter.wav64") ||
+        !wav64_open(&sfx_error, "rom:/error.wav64")) {
+        // Handle error - maybe disable SFX if files can't be loaded
+        return;
+    }
+    
+    // Initialize enabled state from user preferences if available
+    sfx_enabled = get_user_sfx_preference();
 }

Committable suggestion skipped: line range outside the PR's diff.


62-84: 🛠️ Refactor suggestion

Consider enhancing sound effect playback robustness.

The current implementation could benefit from:

  1. Channel availability validation
  2. Per-effect volume control
  3. Error handling in the default case
 void sound_play_effect(sound_effect_t sfx) {
-    if(sfx_enabled) {
+    if (!sfx_enabled || !sound_initialized) {
+        return;
+    }
+
+    // Ensure channel is available
+    if (mixer_ch_is_active(SOUND_SFX_CHANNEL)) {
+        mixer_ch_stop(SOUND_SFX_CHANNEL);
+    }
+
     switch (sfx) {
         case SFX_CURSOR:
             wav64_play(&sfx_cursor, SOUND_SFX_CHANNEL);
             break;
         // ... other cases ...
         default:
-            break;
+            debugf("Unknown sound effect: %d\n", sfx);
+            return;
     } 
-    }
 }

Committable suggestion skipped: line range outside the PR's diff.

src/menu/settings.c (1)

45-49: 💡 Codebase verification

Memory leak detected in settings management

The code review has revealed a legitimate concern. While the rom_autoload_path and rom_autoload_filename fields are properly freed when updating them in load_rom.c, there is no cleanup of these fields when the menu is destroyed. The menu_deinit function doesn't free the settings structure members before freeing the menu itself.

  • Memory leak in src/menu/menu.c:menu_deinit() - The settings struct members (rom_autoload_path, rom_autoload_filename, and potentially default_directory) are not freed before the menu struct is freed.
🔗 Analysis chain

Verify memory management for settings struct

Please ensure that the memory allocated for rom_autoload_path and rom_autoload_filename is properly freed when the settings are cleaned up.

Also applies to: 65-68

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for settings cleanup/free function
ast-grep --pattern 'void settings_free($_) {
  $$$
}'

# Search for direct free calls on these fields
rg -l "free\(.*?->rom_autoload_"

Length of output: 90


Script:

#!/bin/bash
# Let's try different patterns to find memory cleanup functions
# Search for any function that might free settings
rg -A 5 "void.*settings.*free"

# Search for any function containing settings cleanup
rg -A 5 "settings.*cleanup"

# Search for any function that frees these specific fields
rg -A 5 "free.*rom_autoload"

# Look for the settings struct definition to understand the full context
rg -A 10 "typedef struct.*settings"

Length of output: 800


Script:

#!/bin/bash
# Let's try to find the settings struct and related functions with different patterns
# Look for settings struct definition
rg -A 10 "struct\s+settings"

# Search for settings initialization
rg -A 5 "settings_init|init_settings"

# Search for settings cleanup in menu code
rg -A 5 "menu.*free|menu.*cleanup" src/menu/

# Look for settings.h to understand the structure
fd -t f "settings.h$" --exec cat {}

Length of output: 3490


Script:

#!/bin/bash
# Let's look for menu cleanup/destruction code since settings is part of menu
rg -A 10 "menu.*destroy|menu.*free|menu.*cleanup" src/menu/menu.[ch]

# Look for any function that might handle menu struct cleanup
rg -A 10 "void.*menu.*free" src/menu/

# Check if there's a dedicated menu destructor
fd -t f "menu.[ch]$" --exec cat {}

Length of output: 10263

docs/99_developer_guide.md (2)

48-53: 🛠️ Refactor suggestion

Enhance submodule update documentation for clarity and safety

The current documentation could benefit from additional context and safety warnings:

  1. The submodule update command should include:

    • A warning about potential breaking changes
    • Instructions to test after updating
    • How to revert if needed
  2. The libdragon section should:

    • Explain why the "preview" branch is used
    • Document what each make command does
    • Include troubleshooting steps

Consider this enhanced version:

 ## Update submodules
-To update to the latest version, use `git submodule update --remote` from the terminal.
+### Updating All Submodules
+⚠️ WARNING: Updating submodules may introduce breaking changes.
+
+1. Update submodules to their latest versions:
+   ```bash
+   git submodule update --remote
+   ```
+2. Test thoroughly after updating
+3. To revert if needed:
+   ```bash
+   git submodule update --init --recursive
+   ```
 
 ### libdragon
-This repo currently uses the `preview` branch as a submodule at a specific commit.
-* To ensure your local instance is building against it, use `cd ./libdragon && make clobber -j && make libdragon tools -j && make install tools-install -j && cd ..`
+This repo uses the `preview` branch (instead of stable) to access the latest features needed for flashcart support.
+
+To build libdragon:
+1. Clean previous build artifacts:
+   ```bash
+   cd ./libdragon
+   make clobber -j
+   ```
+2. Build the library and tools:
+   ```bash
+   make libdragon tools -j
+   ```
+3. Install built components:
+   ```bash
+   make install tools-install -j
+   cd ..
+   ```
+
+If you encounter build errors, try:
+- Ensuring all prerequisites are installed
+- Running `make clean` before rebuilding
+- Checking the libdragon documentation for your specific platform

63-64: 🛠️ Refactor suggestion

Enhance documentation testing instructions

The current instructions could be more comprehensive by including:

  • Recommended versions for Ruby and Jekyll
  • How to access the served documentation
  • Common troubleshooting steps

Consider this enhanced version:

 Testing the documentation locally allows you to preview changes and ensure everything renders correctly before submitting your changes.
 
-Install Prerequisites:
+### Prerequisites
+Recommended versions:
+- Ruby >= 2.7.0
+- Jekyll >= 4.2.0
+
+Install required packages:
 ```bash
 apt-get install ruby-full build-essential zlib1g-dev
 gem install jekyll bundler

-You can then serve the webpage:
+### Serve and View Documentation
+1. Start the Jekyll server:

-cd output/docs && jekyll serve
+cd output/docs
+jekyll serve --host=0.0.0.0 --port=4000

+2. Access the documentation:

+### Troubleshooting
+If you encounter issues:
+1. Ensure all gems are installed:

  • bundle install

+2. Clear Jekyll cache:

  • jekyll clean

+3. Check Jekyll build output:

  • jekyll build --verbose

</blockquote></details>
<details>
<summary>docs/00_getting_started_sd.md (2)</summary><blockquote>

12-15: _:warning: Potential issue_

**Fix heading hierarchy for consistency**

Similar to the SC64 section, the heading level should be h3 for consistent document structure.

Apply this diff to fix the heading hierarchy:

```diff
- #### Other supported flashcarts
+ ### Other supported flashcarts
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

### Other supported flashcarts
- FAT32 recommended.
- An SD formatted with default cluster size is recommended.

3-6: ⚠️ Potential issue

Fix heading hierarchy for better document structure

The heading structure jumps from h2 (##) to h4 (####) which violates markdown best practices and may affect document readability and navigation.

Apply this diff to fix the heading hierarchy:

- #### SC64
+ ### SC64
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Using your PC, insert the SD card and ensure it is formatted for compatibility with your flashcart
### SC64
- FAT32 and EXFAT are fully supported.
- An SD formatted with 128 kiB cluster size is recommended.
🧰 Tools
🪛 Markdownlint

4-4: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time

(MD001, heading-increment)

src/menu/actions.c (1)

116-120: 🛠️ Refactor suggestion

Enhance rumble initialization robustness

The rumble deactivation is straightforward but could benefit from additional safety measures:

  1. Error handling for failed rumble deactivation
  2. Protection against multiple initialization calls
  3. State tracking for rumble status

Consider this more robust implementation:

+static bool initialized = false;
+
 void actions_init(void) {
+    if (initialized) return;
+
     JOYPAD_PORT_FOREACH (port) {
-        joypad_set_rumble_active(port, false);
+        if (!joypad_set_rumble_active(port, false)) {
+            // Handle error or log warning
+        }
     }
+    initialized = true;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

static bool initialized = false;

void actions_init (void) {
    if (initialized) return;

    JOYPAD_PORT_FOREACH (port) {
        if (!joypad_set_rumble_active(port, false)) {
            // Handle error or log warning
        }
    }
    initialized = true;
}
src/menu/views/file_info.c (1)

16-17: 💡 Codebase verification

Extension arrays need to be synchronized with other definitions

The search results revealed inconsistencies between extension definitions across different files:

  • browser.c defines emulator_extensions as "nes", "sfc", "smc", "gb", "gbc", "sms", "gg", "sg", NULL
  • file_info.c defines it as "nes", "smc", "gb", "gbc", "sms", "gg", NULL
  • load_emulator.c has separate arrays that show SNES extensions should include both "sfc" and "smc"

The extension arrays in file_info.c need to be updated to:

  • Add "sfc" and "sg" to emulator_extensions to match the supported formats in other files
🔗 Analysis chain

Verify completeness of supported extensions.

The extension arrays look well-structured. Let's verify we haven't missed any common extensions:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any references to these file extensions in the codebase
# to ensure we haven't missed any supported formats

# Search for controller pak extensions
echo "Searching for controller pak extensions..."
rg -i "\.mpk|\.pak" --type c

# Search for emulator ROM extensions
echo "Searching for emulator ROM extensions..."
rg -i "\.nes|\.smc|\.gb|\.gbc|\.sms|\.gg" --type c

Length of output: 269


Script:

#!/bin/bash
# Let's try a different approach to find file extension handling in the codebase

# Search for file extension checks or assignments
echo "Searching for extension-related code..."
rg -i 'extension.*=|\.extension|strcmp.*\.' --type c -A 2 -B 2

# Search for file handling patterns
echo "Searching for file handling patterns..."
ast-grep --pattern 'if ($_ != NULL) {
  $$$
  strstr($_, ".")
  $$$
}'

Length of output: 4126

src/menu/views/music_player.c (1)

47-51: ⚠️ Potential issue

Move sound effect after error check

The sound effect currently plays even if mp3player_toggle() fails, which could mislead users. Consider moving it inside the success branch.

Apply this change:

    } else if (menu->actions.enter) {
        err = mp3player_toggle();
        if (err != MP3PLAYER_OK) {
            menu_show_error(menu, convert_error_message(err));
+        } else {
+            sound_play_effect(SFX_ENTER);
        }
-        sound_play_effect(SFX_ENTER);
    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        err = mp3player_toggle();
        if (err != MP3PLAYER_OK) {
            menu_show_error(menu, convert_error_message(err));
        } else {
            sound_play_effect(SFX_ENTER);
        }
src/menu/mp3_player.h (3)

25-46: 🛠️ Refactor suggestion

Clarify initialization sequence requirements.

The initialization functions could benefit from more detailed documentation about their relationship and usage:

 /**
  * @brief Initialize the MP3 player mixer.
  * 
  * This function initializes the mixer for the MP3 player.
+ * 
+ * @note This function must be called before mp3player_init().
+ * @note On failure, the application should call mp3player_deinit() to clean up resources.
  */
 void mp3player_mixer_init(void);

 /**
  * @brief Initialize the MP3 player.
  * 
  * This function initializes the MP3 player and prepares it for playback.
+ * 
+ * @pre mp3player_mixer_init() must be called first
+ * @note On error, the application must still call mp3player_deinit() to clean up any
+ *       partially initialized resources.
  * 
  * @return mp3player_err_t Error code indicating the result of the initialization.
  */
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

/**
 * @brief Initialize the MP3 player mixer.
 * 
 * This function initializes the mixer for the MP3 player.
 * 
 * @note This function must be called before mp3player_init().
 * @note On failure, the application should call mp3player_deinit() to clean up resources.
 */
void mp3player_mixer_init(void);

/**
 * @brief Initialize the MP3 player.
 * 
 * This function initializes the MP3 player and prepares it for playback.
 * 
 * @pre mp3player_mixer_init() must be called first
 * @note On error, the application must still call mp3player_deinit() to clean up any
 *       partially initialized resources.
 * 
 * @return mp3player_err_t Error code indicating the result of the initialization.
 */
mp3player_err_t mp3player_init(void);

/**
 * @brief Deinitialize the MP3 player.
 * 
 * This function deinitializes the MP3 player and releases any resources.
 */
void mp3player_deinit(void);

92-124: 🛠️ Refactor suggestion

Document error handling and thread safety requirements.

The playback control functions have inconsistent error handling and lack important usage details:

  1. mp3player_play() returns errors but mp3player_stop() doesn't. Consider making error handling consistent.
  2. Thread safety information is missing.
  3. Valid state transitions should be documented.

Example improvement:

 /**
  * @brief Start playback of the MP3 file.
  * 
  * This function starts playback of the currently loaded MP3 file.
+ * 
+ * @note This function is not thread-safe and should only be called from the main thread.
+ * @pre A file must be successfully loaded via mp3player_load()
+ * @pre The player must not be currently playing
  * 
  * @return mp3player_err_t Error code indicating the result of the play operation.
  */

Committable suggestion skipped: line range outside the PR's diff.


126-170: 🛠️ Refactor suggestion

Document parameter validation and metadata availability.

The seeking and metadata functions need more detailed documentation:

  1. Seeking bounds and validation:
 /**
  * @brief Seek to a specific position in the MP3 file.
  * 
  * This function seeks to a specific position in the currently loaded MP3 file.
  * 
  * @param seconds Number of seconds to seek.
+ * @pre seconds must be >= 0 and <= total duration
+ * @note Seeking beyond file duration will seek to the end
  * @return mp3player_err_t Error code indicating the result of the seek operation.
  */
  1. Metadata availability:
 /**
  * @brief Get the bitrate of the MP3 file.
  * 
  * This function gets the bitrate of the currently loaded MP3 file.
+ * 
+ * @note This information is only available after a successful mp3player_load()
+ * @note Returns 0 if no file is loaded or metadata is unavailable
  * 
  * @return float Bitrate of the MP3 file in kbps.
  */
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

/**
 * @brief Seek to a specific position in the MP3 file.
 * 
 * This function seeks to a specific position in the currently loaded MP3 file.
 * 
 * @param seconds Number of seconds to seek.
 * @pre seconds must be >= 0 and <= total duration
 * @note Seeking beyond file duration will seek to the end
 * @return mp3player_err_t Error code indicating the result of the seek operation.
 */
mp3player_err_t mp3player_seek(int seconds);

/**
 * @brief Get the duration of the MP3 file.
 * 
 * This function gets the duration of the currently loaded MP3 file.
 * 
 * @return float Duration of the MP3 file in seconds.
 */
float mp3player_get_duration(void);

/**
 * @brief Get the bitrate of the MP3 file.
 * 
 * This function gets the bitrate of the currently loaded MP3 file.
 * 
 * @note This information is only available after a successful mp3player_load()
 * @note Returns 0 if no file is loaded or metadata is unavailable
 * 
 * @return float Bitrate of the MP3 file in kbps.
 */
float mp3player_get_bitrate(void);

/**
 * @brief Get the sample rate of the MP3 file.
 * 
 * This function gets the sample rate of the currently loaded MP3 file.
 * 
 * @return int Sample rate of the MP3 file in Hz.
 */
int mp3player_get_samplerate(void);

/**
 * @brief Get the current playback progress.
 * 
 * This function gets the current playback progress of the MP3 file.
 * 
 * @return float Current playback progress as a percentage (0.0 to 100.0).
 */
float mp3player_get_progress(void);
Makefile (1)

81-86: 💡 Codebase verification

Sound files have notable size variations that may affect user experience

Based on the file analysis:

  • error.wav (194K) and cursorsound.wav (176K) are significantly larger than other sounds
  • enter.wav and settings.wav (both 19K) and back.wav (34K) are more reasonably sized
  • All sounds are loaded with the same volume setting (0.5f) in the code

Consider normalizing the sound files to have more consistent file sizes to ensure:

  • More uniform loading times
  • Consistent memory usage
  • Similar audio quality across UI interactions
🔗 Analysis chain

Verify sound file properties for consistent user experience

While the sound files are well-organized and clearly named, we should verify their properties to ensure a consistent user experience.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check WAV file properties for consistency
for sound in cursorsound.wav back.wav enter.wav error.wav settings.wav; do
  echo "Checking $sound properties:"
  # Use fd to find the file and pipe to ffprobe for analysis
  fd -t f "$sound" -x ffprobe -show_streams {} 2>&1 | grep -E "duration|bit_rate|sample_rate"
done

Length of output: 650


Script:

#!/bin/bash
# Let's first check if these sound files exist and their locations
fd -t f -e wav

Length of output: 144


Script:

#!/bin/bash
# Let's check the file sizes and basic properties
for sound in assets/sounds/*.wav; do
  echo "File: $sound"
  ls -lh "$sound"
done

# Let's also check if there are any references to these sounds in the code
rg -l "\.wav" --type cpp --type c

Length of output: 1131


Script:

#!/bin/bash
# Let's check how these sounds are used in the code
rg -A 5 -B 5 "\.wav" src/menu/sound.c

Length of output: 466

src/flashcart/flashcart.c (1)

112-115: 💡 Codebase verification

Consider removing the commented code and tracking the EDX support in the issue tracker

The codebase search reveals:

  • The EDX (Series X EverDrive-64) support is referenced in multiple places, including flashcart_info.c where it's listed as a valid cart type
  • The commented code is only blocked in flashcart.c with a vague FIXME comment
  • No existing tracking of this issue in the code comments or related libcart fixes

Recommendations:

  1. Remove the commented code block as it adds maintenance overhead
  2. Create a GitHub issue to track the EDX support implementation
  3. Add a single-line comment with the issue reference: // EDX support tracked in issue #XXX
🔗 Analysis chain

Consider tracking the libcart fix in an issue tracker

Instead of keeping commented-out code with a FIXME comment, consider:

  1. Removing the commented code
  2. Creating a GitHub issue to track the pending libcart fix
  3. Adding a comment with a reference to the issue number

This approach would make it easier to track the status of the fix and maintain the codebase.

Let's check if there's already an issue tracking this:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing issues about the libcart EDX fix
gh issue list --search "libcart EDX" --state all

# Search the codebase for related TODOs/FIXMEs
rg -i "libcart.*fix|EDX.*fix" --type c --type h

Length of output: 445


Script:

#!/bin/bash
# Search for EDX or EverDrive related code and comments
rg -i "EDX|everdrive.*series" --type c --type h -A 2 -B 2

# Search for FIXME/TODO comments related to cart support
rg "FIXME|TODO" --type c --type h -A 2 -B 2

# Look for libcart related files and mentions
fd -e c -e h libcart
rg "libcart" --type c --type h

Length of output: 8672

src/menu/components/constants.h (1)

91-99: 💡 Codebase verification

Box art positions need adjustment to stay within visible area

The calculations show that Japanese box art position (BOXART_X_JP) extends beyond the visible area:

  • VISIBLE_AREA_X1 = 640 - 32 = 608
  • BOXART_X_JP = 608 - 158 + 21 = 471
  • With BOXART_WIDTH_MAX = 158, the right edge would be at 629, which exceeds VISIBLE_AREA_X1 (608)

Suggested fixes:

  • Adjust BOXART_X_JP to: (VISIBLE_AREA_X1 - BOXART_WIDTH_MAX - 24) to maintain consistent margin
  • Consider adding static assertions using _Static_assert to validate positions at compile time
🔗 Analysis chain

Verify box art positions stay within visible area

Please verify that all box art positions (Japanese and 64DD) stay within the visible area bounds. Consider adding static assertions to validate this at compile time.


Let me gather more information about the box art dimensions and layout constants to verify the positions.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for visible area bounds usage and validation

# Check for any existing boundary checks
rg -A 2 "VISIBLE_AREA" src/menu/components/

# Look for box art rendering code to verify bounds checking
ast-grep --pattern 'void $_(boxart|draw|render)$_($$$) {
  $$$
}'

Length of output: 7868


Script:

#!/bin/bash
# Search for box art dimensions and layout constants
rg "(BOXART_WIDTH|BOXART_HEIGHT|BOXART_WIDTH_MAX|BOXART_HEIGHT_MAX|BOXART_WIDTH_DD|BOXART_HEIGHT_DD|LAYOUT_ACTIONS_SEPARATOR_Y|OVERSCAN|DISPLAY_WIDTH|DISPLAY_HEIGHT)" src/menu/components/constants.h

Length of output: 1716

README.md (1)

117-130: ⚠️ Potential issue

Enhance warning visibility and fix documentation issues

  1. The warning about potential data loss should be more prominent
  2. The "action run - assets" links are incomplete
  3. Fix the grammatical error (double dots)
-### ED64 - WIP - UNTESTED AND UNSUPPORTED - USE AT OWN RISK
-Currently not supported, but work is in progress (See [PR's](https://github.com/Polprzewodnikowy/N64FlashcartMenu/pulls)).
-NOTE: The menu may be able to load ROM's but not perform saves and may break existing ones..
+### ⚠️ ED64 - EXPERIMENTAL SUPPORT
+> **WARNING: RISK OF DATA LOSS**
+> - Currently not supported - work in progress
+> - May corrupt existing save data
+> - Use at your own risk
+>
+> For development status, see [Pull Requests](https://github.com/Polprzewodnikowy/N64FlashcartMenu/pulls).
+
+The menu may be able to load ROMs but cannot guarantee save functionality.

 #### ED64 (Vseries)
 The aim is to reach feature parity with [ED64-UnofficialOS](https://github.com/n64-tools/ED64-UnofficialOS-binaries) / [ED64-OfficialOS](https://krikzz.com/pub/support/everdrive-64/v2x-v3x/os-bin/).
-Download the `OS64.v64` ROM from the latest [action run - assets] and place it in the `/ED64` folder.
+Download the `OS64.v64` ROM from the [latest release](https://github.com/Polprzewodnikowy/N64FlashcartMenu/releases/latest) and place it in the `/ED64` folder.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

### ⚠️ ED64 - EXPERIMENTAL SUPPORT
> **WARNING: RISK OF DATA LOSS**
> - Currently not supported - work in progress
> - May corrupt existing save data
> - Use at your own risk
>
> For development status, see [Pull Requests](https://github.com/Polprzewodnikowy/N64FlashcartMenu/pulls).

The menu may be able to load ROMs but cannot guarantee save functionality.

#### ED64 (Vseries)
The aim is to reach feature parity with [ED64-UnofficialOS](https://github.com/n64-tools/ED64-UnofficialOS-binaries) / [ED64-OfficialOS](https://krikzz.com/pub/support/everdrive-64/v2x-v3x/os-bin/).
Download the `OS64.v64` ROM from the [latest release](https://github.com/Polprzewodnikowy/N64FlashcartMenu/releases/latest) and place it in the `/ED64` folder.

#### ED64 (X series)
X Series support is currently awaiting fixes, in the meantime use the official [OS](https://krikzz.com/pub/support/everdrive-64/x-series/OS/) instead.

#### ED64 (P clone)
Download the `OS64P.v64` ROM from the latest [action run - assets] and place it in the `/ED64P` folder.
The aim is to reach feature parity with [Altra64](https://github.com/networkfusion/altra64)
🧰 Tools
🪛 LanguageTool

[typographical] ~119-~119: Two consecutive dots
Context: ...erform saves and may break existing ones.. #### ED64 (Vseries) The aim is to reac...

(DOUBLE_PUNCTUATION)


[style] ~126-~126: ‘in the meantime’ might be wordy. Consider a shorter alternative.
Context: ...es support is currently awaiting fixes, in the meantime use the official [OS](https://krikzz.co...

(EN_WORDINESS_PREMIUM_IN_THE_MEANTIME)

src/menu/rom_info.c (1)

838-838: ⚠️ Potential issue

Good catch! Fixed memory leak in error path.

The added path_free(overrides_path) call prevents a memory leak when the INI file fails to load. Memory cleanup in error paths is crucial for preventing resource leaks.

src/flashcart/ed64/ed64_vseries.c (2)

34-37: 🛠️ Refactor suggestion

Implement cart model detection in get_cart_model()

The get_cart_model() function currently returns a hardcoded value ED64_V1_0. Implementing proper cart model detection is essential for accurate feature handling across different ED64 versions.


69-78: ⚠️ Potential issue

Fix the ROM loading loop to read only up to the ROM size

The current loop in ed64_vseries_load_rom reads up to sdram_size (64 MiB), regardless of the actual ROM size. This can lead to reading beyond the ROM file, unnecessary data transfers, and potential undefined behavior.

Apply this diff to fix the issue:

-size_t sdram_size = MiB(64);

 size_t chunk_size = KiB(128);
-for (int offset = 0; offset < sdram_size; offset += chunk_size) {
-    size_t block_size = MIN(sdram_size - offset, chunk_size);
+for (size_t offset = 0; offset < rom_size; offset += chunk_size) {
+    size_t block_size = MIN(rom_size - offset, chunk_size);
     if (f_read(&fil, (void *) (ROM_ADDRESS + offset), block_size, &br) != FR_OK) {
         f_close(&fil);
         return FLASHCART_ERR_LOAD;
     }
     if (progress) {
-        progress(f_tell(&fil) / (float)(f_size(&fil)));
+        progress(f_tell(&fil) / (float)(rom_size));
     }
 }

Committable suggestion skipped: line range outside the PR's diff.

src/menu/components/boxart.c (5)

67-72: 🛠️ Refactor suggestion

Refactor duplicate code into a helper function for loading images

The code block that checks for file existence and initiates the PNG decoder is repeated multiple times. Refactoring this logic into a helper function will reduce code duplication and improve maintainability.

Create a helper function load_png_if_exists:

+bool load_png_if_exists(const char* file_path, component_boxart_t* b) {
+    if (file_exists(file_path)) {
+        if (png_decoder_start(file_path, BOXART_WIDTH_MAX, BOXART_HEIGHT_MAX, png_decoder_callback, b) == PNG_OK) {
+            return true;
+        }
+    }
+    return false;
+}

Then replace the repeated code with calls to this helper function:

-    if (file_exists(path_get(path))) { 
-        if (png_decoder_start(path_get(path), BOXART_WIDTH_MAX, BOXART_HEIGHT_MAX, png_decoder_callback, b) == PNG_OK) {
-            path_free(path);
-            return b;
-        }
-    }
+    if (load_png_if_exists(path_get(path), b)) {
+        path_free(path);
+        return b;
+    }

Also applies to: 85-90, 96-101, 107-112


82-83: ⚠️ Potential issue

Ensure 'game_code' has sufficient length before formatting 'file_name'

The code assumes that game_code has at least 4 characters when creating file_name. If game_code is shorter, it could result in out-of-bounds access.

Add a length check before formatting file_name:

+    if (strlen(game_code) < 4) {
+        // Handle error or try alternative filename formats
+    } else {
         snprintf(file_name, sizeof(file_name), "%c%c%c%c.png", game_code[0], game_code[1], game_code[2], game_code[3]);
         path_push(path, file_name);
+        // Existing code...
+    }

Committable suggestion skipped: line range outside the PR's diff.


93-94: ⚠️ Potential issue

Check 'game_code' length before accessing for three-character filenames

When creating filenames with three characters, ensure game_code has at least 3 characters to prevent out-of-bounds access.

Implement a length check:

+    if (strlen(game_code) < 3) {
+        // Handle error or try alternative filename formats
+    } else {
         snprintf(file_name, sizeof(file_name), "%c%c%c.png", game_code[0], game_code[1], game_code[2]);
         path_push(path, file_name);
+        // Existing code...
+    }

Committable suggestion skipped: line range outside the PR's diff.


32-33: ⚠️ Potential issue

Validate 'game_code' length before accessing its elements

Accessing game_code[0] to game_code[3] without verifying the length of game_code may lead to out-of-bounds access if game_code is shorter than expected. This could cause undefined behavior or crashes.

Apply this diff to add a length check for game_code:

+    if (strlen(game_code) < 4) {
+        // Handle error: game_code is too short
+        path_free(path);
+        free(b);
+        return NULL;
+    }
     sprintf(boxart_id_path, "%c/%c/%c/%c", game_code[0], game_code[1], game_code[2], game_code[3]);

Committable suggestion skipped: line range outside the PR's diff.


105-106: ⚠️ Potential issue

Validate 'game_code' length before accessing 'game_code[1]' and 'game_code[2]'

Accessing game_code[1] and game_code[2] without checking the length of game_code can lead to out-of-bounds memory access if game_code is shorter than expected.

Add a length check:

+    if (strlen(game_code) < 3) {
+        // Handle error or fallback
+    } else {
         snprintf(file_name, sizeof(file_name), "%c%c.png", game_code[1], game_code[2]);
         path_push(path, file_name);
+        // Existing code...
+    }

Committable suggestion skipped: line range outside the PR's diff.

src/menu/menu.c (1)

38-39: ⚠️ Potential issue

Handle memory allocation failure without using assert

Using assert(menu != NULL); may not be suitable for production code because assert is disabled when NDEBUG is defined, potentially leading to undefined behavior if menu is NULL. Instead, explicitly check the result of calloc and handle the error gracefully to ensure robustness.

Apply this diff to handle allocation failure:

menu = calloc(1, sizeof(menu_t));
-if (menu == NULL) {
-    assert(menu != NULL);
+if (menu == NULL) {
+    // Handle allocation failure appropriately
+    // For example, log the error and transition to fault mode
+    // Note: Ensure that error handling does not dereference NULL pointers
+    // Log error message if logging system is available
+    menu_mode_t previous_mode = menu->next_mode;
+    menu = &default_menu_instance; // Use a default instance if possible
+    menu->next_mode = MENU_MODE_FAULT;
+    // Optionally, restore previous mode or take other corrective actions
+    return;
}

Committable suggestion skipped: line range outside the PR's diff.

src/menu/views/settings_editor.c (5)

14-17: 🛠️ Refactor suggestion

Ensure immediate effect when changing PAL60 Mode

In the function set_pal60_type, consider performing any necessary actions to apply the PAL60 mode change immediately, such as updating the display mode or alerting the user to restart the system if required. This ensures the setting takes effect without confusion.


19-24: 🛠️ Refactor suggestion

Consistent browser reload after settings change

In set_protected_entries_type, you correctly set menu->browser.reload = true; to update the browser view. However, similar settings like set_use_saves_folder_type do not trigger a browser reload. For consistency and to ensure changes are reflected immediately, consider adding menu->browser.reload = true; in functions that modify file system-related settings.


26-29: 🛠️ Refactor suggestion

Add browser reload for 'Use Saves Folder' setting

To ensure that changes to the 'Use Saves Folder' setting are immediately reflected in the file browser, add menu->browser.reload = true; after updating the setting in set_use_saves_folder_type.

Apply this diff to implement the change:

 static void set_use_saves_folder_type (menu_t *menu, void *arg) {
     menu->settings.use_saves_folder = (bool)(uintptr_t)(arg);
     settings_save(&menu->settings);
+    menu->browser.reload = true;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

static void set_use_saves_folder_type (menu_t *menu, void *arg) {
    menu->settings.use_saves_folder = (bool)(uintptr_t)(arg);
    settings_save(&menu->settings);
    menu->browser.reload = true;
}

38-41: 🛠️ Refactor suggestion

Implement immediate effect for Background Music setting

In set_bgm_enabled_type, consider adding a function call to start or stop the background music based on the new setting. This ensures that the user's preference is applied immediately without needing to restart the application.

For example:

 static void set_bgm_enabled_type (menu_t *menu, void *arg) {
     menu->settings.bgm_enabled = (bool)(uintptr_t)(arg);
+    sound_use_bgm(menu->settings.bgm_enabled);
     settings_save(&menu->settings);
 }

Ensure that sound_use_bgm is implemented to handle enabling or disabling background music.

Committable suggestion skipped: line range outside the PR's diff.


43-46: 🛠️ Refactor suggestion

Implement immediate effect for Rumble Feedback setting

Similarly, in set_rumble_enabled_type, add functionality to apply the rumble feedback setting immediately. This might involve initiating or stopping rumble effects based on the user's choice.

For example:

 static void set_rumble_enabled_type (menu_t *menu, void *arg) {
     menu->settings.rumble_enabled = (bool)(uintptr_t)(arg);
+    rumble_set_enabled(menu->settings.rumble_enabled);
     settings_save(&menu->settings);
 }

Ensure that rumble_set_enabled is a function that manages the rumble hardware accordingly.

src/menu/views/rtc.c (3)

71-72: 🛠️ Refactor suggestion

Simplify time recalculations and avoid unnecessary gmtime call

The mktime function adjusts the struct tm in place, updating fields like tm_wday and tm_yday. Calling gmtime and reassigning t is unnecessary and may introduce issues with time zones or thread safety. Consider removing the gmtime call.

Apply this diff to simplify the code:

     // Recalculate day-of-week and day-of-year
     time_t timestamp = mktime( t );
-    *t = *gmtime( &timestamp );
+    // mktime updates 't' with correct 'tm_wday' and 'tm_yday' in place
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    time_t timestamp = mktime( t );
    // mktime updates 't' with correct 'tm_wday' and 'tm_yday' in place

8-10: 🛠️ Refactor suggestion

Avoid using GCC-specific extensions in macros for portability

The MAX and MIN macros use the GCC-specific typeof extension, which can reduce portability to other compilers. To enhance compatibility, consider redefining these macros using standard C expressions.

Apply this diff to redefine the macros without typeof:

-#define MAX(a,b)  ({ typeof(a) _a = a; typeof(b) _b = b; _a > _b ? _a : _b; })
-#define MIN(a,b)  ({ typeof(a) _a = a; typeof(b) _b = b; _a < _b ? _a : _b; })
+#define MAX(a,b)  ((a) > (b) ? (a) : (b))
+#define MIN(a,b)  ((a) < (b) ? (a) : (b))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

#define MAX(a,b)  ((a) > (b) ? (a) : (b))
#define MIN(a,b)  ((a) < (b) ? (a) : (b))
#define CLAMP(x, min, max) (MIN(MAX((x), (min)), (max)))

38-38: ⚠️ Potential issue

Resolve potential syntax error in rtc_time_from_tm

The use of the CLAMP macro within designated initializers may cause syntax errors with some compilers or static analysis tools like cppcheck. This could be due to the macro expansion within the initializer list.

Consider computing the clamped values before the return statement:

 rtc_time_t rtc_time_from_tm( struct tm *time ) {
+    int year = CLAMP(time->tm_year + 1900, YEAR_MIN, YEAR_MAX);
+    int month = CLAMP(time->tm_mon, 1, 12);
+    int day = CLAMP(time->tm_mday, 1, 31);
+    int hour = CLAMP(time->tm_hour, 0, 23);
+    int min = CLAMP(time->tm_min, 0, 59);
+    int sec = CLAMP(time->tm_sec, 0, 59);
+    int week_day = CLAMP(time->tm_wday, 0, 6);
+
     return(rtc_time_t){
-        .year = CLAMP(time->tm_year + 1900, YEAR_MIN, YEAR_MAX),
-        .month = CLAMP(time->tm_mon, 1, 12),
-        .day = CLAMP(time->tm_mday, 1, 31),
-        .hour = CLAMP(time->tm_hour, 0, 23),
-        .min = CLAMP(time->tm_min, 0, 59),
-        .sec = CLAMP(time->tm_sec, 0, 59),
-        .week_day = CLAMP(time->tm_wday, 0, 6),
+        .year = year,
+        .month = month,
+        .day = day,
+        .hour = hour,
+        .min = min,
+        .sec = sec,
+        .week_day = week_day,
     };
 }

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 cppcheck

[error] 38-38: Syntax Error

(internalAstError)

src/menu/views/load_rom.c (3)

277-279: ⚠️ Potential issue

Handle boxart initialization when autoload is enabled

When menu->settings.rom_autoload_enabled is true, boxart may remain uninitialized, but the draw function still attempts to draw it:

if (boxart != NULL) {
    component_boxart_draw(boxart);
}

This could lead to unexpected behavior if boxart was not properly initialized.

Ensure that boxart is initialized regardless of the autoload setting or adjust the draw function to account for when boxart is uninitialized.


373-376: 🛠️ Refactor suggestion

Initialize components regardless of autoload setting

Components like boxart and context menus are not initialized when autoload is enabled. This might lead to incomplete UI initialization.

Consider initializing these components outside the conditional block or ensuring that the rest of the code accounts for their uninitialized state.

-    if (!menu->settings.rom_autoload_enabled) {
         boxart = component_boxart_init(menu->storage_prefix, menu->load.rom_info.game_code, IMAGE_BOXART_FRONT);
         component_context_menu_init(&options_context_menu);
-    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    boxart = component_boxart_init(menu->storage_prefix, menu->load.rom_info.game_code, IMAGE_BOXART_FRONT);
    component_context_menu_init(&options_context_menu);

148-157: 🛠️ Refactor suggestion

Add confirmation dialog before setting ROM to autoload

A FIXME comment indicates the need for a confirmation dialog when setting a ROM to autoload. This is important to prevent users from unintentionally changing their autoload settings.

Implement a confirmation dialog to ensure the user intentionally sets the ROM to autoload. Here's a suggested implementation:

 static void set_autoload_type (menu_t *menu, void *arg) {
+    if (!menu_confirm_action(menu, "Set this ROM to autoload on startup?")) {
+        return;
+    }
     free(menu->settings.rom_autoload_path);
     menu->settings.rom_autoload_path = strdup(strip_fs_prefix(path_get(menu->browser.directory)));
     free(menu->settings.rom_autoload_filename);
     menu->settings.rom_autoload_filename = strdup(menu->browser.entry->name);
-    // FIXME: add a confirmation box here! (press start on reboot)
     menu->settings.rom_autoload_enabled = true;
     settings_save(&menu->settings);
     menu->browser.reload = true;
 }

This uses a hypothetical menu_confirm_action function to display a confirmation dialog. Would you like assistance in implementing this function?

Committable suggestion skipped: line range outside the PR's diff.

@networkfusion networkfusion marked this pull request as ready for review November 24, 2024 15:57
@networkfusion networkfusion merged commit b779af4 into Polprzewodnikowy:develop Nov 24, 2024
3 checks passed
@networkfusion networkfusion deleted the smsplus64 branch November 24, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant