[main] Next release changes #162
Conversation
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Adding sound for make the navigation not so borring. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> This [issue](#89) ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): --> https://github.com/Polprzewodnikowy/N64FlashcartMenu/assets/111246491/0f8086f6-16b3-4adb-a925-afbfc9fa6ba9 ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL> --------- Co-authored-by: Robin Jones <[email protected]>
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Warns users that the display is meant to be empty. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> It is not yet implemented. ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
Adds extra emulator ROM extensions. Adds Controller Pak extension used by ares.
<!--- Provide a general summary of your changes in the Title above --> ## Description Make the root of the SD card a little cleaner by hiding the crap that macOS puts there. ## Motivation and Context Whenever an SD card touches macOS, a handful of hidden files get created that cannot be touched. N64FlashcartMenu has no need for these files, so they should be hidden. <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> ## How Has This Been Tested? Built and run on SummerCart64 with an NTSC N64 <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [X] 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. - [X] All new and existing tests passed. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: meeq <[email protected]>
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> * Update libdragon submodule. * Add optimization. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> ellipsis was broken. ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Improvement (non-breaking change that adds a new feature) - [x] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Adds ability to set the config.ini settings from the menu. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> Ability to set the settings was missing from the menu. ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> Tested on an SC64. ## Screenshots <!-- (if appropriate): -->  ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Allow L or Z to be used in the menu. For instance, could be used to show extra info in the `ROM Info` display. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> These buttons were not available. ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Merge the L and Z action to be the "same" button press, depending on how the joypad is gripped. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> Simplifies the menu controls. #119 (comment) ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Move certain parameters to a seperate screen (messagebox). ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> The ROM info was convoluted Builds on #120 ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): -->  ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> improves upon #121 ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): -->   ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Show whether a ROM supports (or requires) a controller pak Builds on #122 ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): -->    ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Allow realtime setting changes for SFX in menu. Add note that the PAL60 setting still requires a reboot. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> The sfx setting previously required a flashcart reboot. ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Remove a newline. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Improvement (non-breaking change that adds a new feature) - [x] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> This adds in the 64DD disk load menu to visualize the game box. I also created a European and 64DD boxart packs ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> Improves boxart image support. ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> Tested on a local SC64 ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced instructions in the README for boxart dimensions and resources. - Improved handling of box art rendering for various formats in the menu. - New functionality for displaying box art during disk loading in the menu interface, including resource management for cleanup. - **Bug Fixes** - Resolved issues with rendering dimensions for specific types of box art, improving visual consistency. - **Documentation** - Updated README to clarify the boxart requirements and provide multiple download links for different regions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Robin Jones <[email protected]>
It needs more thought.
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Update libdragon submodule. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> Keeps it up-to-date. ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> Tested locally on SC64, no regressions witnessed. ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] 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: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Updated the underlying subproject version for potential enhancements and bug fixes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Updates the miniz git submodule ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> keep it up-to-date ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> Assuming it is used for PNG conversion. Tested changing backgrounds locally on an SC64, which still works. ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] 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: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Updated the reference to a newer version of a subproject, potentially incorporating various enhancements and bug fixes. No changes were made to the existing functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> Some flashcarts may be old (which would include old 64drive's), and not use the Ultra CIC, or old one that does not handle auto switching of regions, we should have them as a feature switch. ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] 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: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
<!--- Provide a general summary of your changes in the Title above --> Title sums it up ## Description <!--- Describe your changes in detail --> Adds some extra logic in the controller polling to read from ports 2-4, letting those controllers input on the menu. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> Port 1 no longer feels alone in the dark, cold world of menu selection. ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> Same method I used for libdragon's error screen which had the same issue. Tested on a system with four controllers inserted. ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] 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: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: FazanaJ <GITHUB_USER_EMAIL> --------- Co-authored-by: Robin Jones <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/menu/views/settings_editor.c (2)
109-111: Consider adding error handling for context menu processingThe function returns early if context menu processing succeeds, but doesn't handle potential errors.
Consider adding error handling:
- if (ui_components_context_menu_process(menu, &options_context_menu)) { + int result = ui_components_context_menu_process(menu, &options_context_menu); + if (result < 0) { + // Handle error + return; + } + if (result > 0) { return; }
180-181: Remove unnecessary whitespaceThere's an extra blank line that affects readability.
Apply this fix:
void view_settings_init (menu_t *menu) { - ui_components_context_menu_init(&options_context_menu);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/menu/views/settings_editor.c(2 hunks)
🔇 Additional comments (3)
src/menu/views/settings_editor.c (3)
14-46: Avoid casting between void* and integer types for portability
The pattern of casting between void* and integer types via uintptr_t appears in multiple functions.
76-76:
Fix inconsistent casting pattern
The casting pattern is inconsistent with other similar cases in the file.
Apply this fix:
- {.text = "Off", .action = set_pal60_type, .arg = (void *) (false) },
+ {.text = "Off", .action = set_pal60_type, .arg = (void *)(uintptr_t)(false) },Likely invalid or redundant comment.
48-51: Address or remove the commented-out function
The commented-out set_use_default_settings function with a FIXME comment should either be implemented or removed to maintain code cleanliness.
Would you like me to help implement this function or create a GitHub issue to track this task?
✅ Verification successful
Let me search for any references or usages of this function to determine if it's needed or can be safely removed.
Let me check the actual content and context of the function in the file to understand its usage better.
Remove the commented-out function and menu entry
The commented-out set_use_default_settings function and its corresponding menu entry are both under #ifdef BETA_SETTINGS and are currently inactive. The function is not referenced anywhere else in the codebase, and its menu entry is also commented out in the options_context_menu structure. Since this beta feature was never implemented and there are no other dependencies on it, it should be safely removed to maintain code cleanliness.
- Remove lines 48-51: The commented-out
set_use_default_settingsfunction - Remove line 103: The commented-out menu entry
{ .text = "Restore Defaults", .action = set_use_default_settings }
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for any references to set_use_default_settings in the codebase
rg "set_use_default_settings" -l
# Search for any menu-related default settings functionality
rg "default.*settings.*menu" -l
# Look for function declarations in settings_editor.c
ast-grep --pattern 'void $FUNC($_) { $$$ }' src/menu/views/settings_editor.c
Length of output: 211
Script:
#!/bin/bash
# Get the content of settings_editor.c with context
cat src/menu/views/settings_editor.c
# Look for any menu structure definitions that might use this function
rg -A 5 "struct.*menu"
Length of output: 7747
networkfusion
added
bug
People were confused.
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Add detection for the flashcart firmware version and show it within the Flashcart Info view. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> The feature was incomplete. ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] 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: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/menu/views/flashcart_info.c (1)
Beta features need to be addressed before release
The verification has revealed that PAL60 mode functionality is actively used in the codebase and is not properly guarded for release:
- The feature is referenced in multiple locations:
src/menu/menu.c: Used in menu configurationsrc/menu/views/settings_editor.c: Used in settings UI and functionalitysrc/menu/settings.handsettings.c: Core settings implementationWhile some parts are guarded with
BETA_SETTINGS, the core functionality remains active. This needs attention before release as beta features should either be:
- Fully enabled and documented for release, or
- Properly disabled/removed for the release version
🔗 Analysis chain
Line range hint
1-95: Verify release readinessThe changes look appropriate for release, but let's verify there are no references to beta or alpha features mentioned in the PR objectives.
Beta features found in the codebase - needs attention
The verification revealed that there are still beta features present in the codebase, specifically:
BETA_SETTINGSmacro and PAL60 mode functionality insrc/menu/views/settings_editor.c- PAL60 settings in multiple files:
settings.h,settings.c, andmenu.cLet's verify if these beta features are actually enabled in the build:
Let's try a different approach to check the build configuration and header files:
Let's check one more thing to see if there are any feature flags defined in the compilation process:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to beta/alpha features that shouldn't be in the release rg -i "BETA_SETTING|PAL60|ALPHA_FEATURE|ED64V3" --type cLength of output: 1508
Script:
#!/bin/bash # Check if BETA_SETTINGS is defined in build configuration rg -l "define.*BETA_SETTINGS" --type c --type h # Check the build system files for beta feature flags fd -e mk -e makefile -e cmake . -X rg -i "BETA_SETTINGS|PAL60"Length of output: 114
Script:
#!/bin/bash # Check header files for any configuration or feature flags fd -e h "config|feature" -X cat {} # Look for build configuration in common locations cat Makefile 2>/dev/null || true cat CMakeLists.txt 2>/dev/null || true # Check if the code guarded by BETA_SETTINGS is actually compiled ast-grep --pattern 'ifdef BETA_SETTINGS $$$ endif'Length of output: 5118
Script:
#!/bin/bash # Check for any compiler definitions in source files rg -A 2 "#define.*BETA|#define.*FEATURE|#define.*PAL60" --type c --type h # Check if these settings are actually used in the code ast-grep --pattern 'menu->settings.pal60_enabled'Length of output: 380
♻️ Duplicate comments (2)
src/flashcart/ed64/ed64_vseries.c (1)
129-137:⚠️ Potential issueAddress FIXME comments in save management functions.
Both save management functions have FIXME comments indicating they need proper implementation. This could lead to data loss if users expect save functionality to work.
Would you like me to help implement the save management functionality or create a GitHub issue to track this task?
src/flashcart/64drive/64drive.c (1)
78-80:⚠️ Potential issueRemove or implement AUTO_CIC and AUTO_REGION features
Based on past review comments, while these features are marked as supported, they lack actual implementation code. This could lead to runtime issues if code relies on these features.
Consider either:
- Implementing the features properly, or
- Setting them to
falseuntil implemented:- case FLASHCART_FEATURE_AUTO_CIC: return true; - case FLASHCART_FEATURE_AUTO_REGION: return true; + case FLASHCART_FEATURE_AUTO_CIC: return false; + case FLASHCART_FEATURE_AUTO_REGION: return false; case FLASHCART_FEATURE_SAVE_WRITEBACK: return true;
🧹 Nitpick comments (9)
src/flashcart/ed64/ed64_vseries.c (4)
14-24: Add documentation for memory constraints.While the base addresses are defined, it would be helpful to document:
- The maximum addressable size for SRAM and ROM
- Any alignment requirements
- Any hardware-specific limitations
26-32: Document the empty initialization functions.Please add comments explaining why these functions are empty. If no initialization is needed, document this explicitly to avoid confusion.
49-89: LGTM! Robust ROM loading implementation.The implementation includes:
- Proper error handling
- Size limit checks
- Chunked loading with progress updates
- Resource cleanup on errors
Consider extracting the magic numbers (64MB, 128KB) into named constants for better maintainability.
91-127: Consider adding progress callback for consistency.The implementation is solid, but unlike
ed64_vseries_load_rom, it lacks progress updates. Consider adding the progress callback parameter for consistency, especially for large files.src/flashcart/flashcart.h (1)
77-78: Add documentation for the new firmware version functionConsider adding documentation for the
flashcart_get_firmware_versionfunction declaration to maintain consistency with other public functions.+/** @brief Get the firmware version of the current flashcart */ flashcart_firmware_version_t flashcart_get_firmware_version (void);Also applies to: 100-100
src/menu/views/flashcart_info.c (2)
29-34: Add bounds checking for version number formattingThe static buffer of 16 bytes might overflow if version numbers are unexpectedly large. Consider adding bounds checking or using snprintf for safer string formatting.
static const char *format_cart_version () { flashcart_firmware_version_t version = flashcart_get_firmware_version(); static char buffer[16]; - sprintf(buffer, "%u.%u.%lu", version.major, version.minor, version.revision); + snprintf(buffer, sizeof(buffer), "%u.%u.%lu", version.major, version.minor, version.revision); return buffer; }
84-86: Consider tracking the TODO as a GitHub issueThe TODO comment about displaying battery and temperature information could be better tracked as a GitHub issue, especially since this PR is preparing for a release.
Would you like me to create a GitHub issue to track this enhancement for battery and temperature information display?
src/flashcart/sc64/sc64.c (2)
190-196: Consider handling errors fromsc64_ll_get_version.
Currently, the function ignores the return code fromsc64_ll_get_version, which might cause unexpected behavior if the call fails. Checking and handling the error code would make it more robust.
583-583: Good addition of theget_firmware_versionfunction pointer.
Registering this function in theflashcart_sc64structure nicely exposes firmware version retrieval. Consider adding some inline documentation for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/flashcart/64drive/64drive.c(2 hunks)src/flashcart/ed64/ed64_vseries.c(1 hunks)src/flashcart/flashcart.c(3 hunks)src/flashcart/flashcart.h(4 hunks)src/flashcart/sc64/sc64.c(3 hunks)src/menu/views/flashcart_info.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/flashcart/flashcart.c
🔇 Additional comments (12)
src/flashcart/ed64/ed64_vseries.c (5)
1-13: LGTM! Headers are well-organized.
The includes are logically grouped and all appear necessary for the implementation.
39-47: Feature detection depends on accurate cart model detection.
The implementation is logically sound, but its effectiveness is compromised by the current cart model detection issue. Once the cart model detection is fixed, this implementation will work correctly.
154-156: LGTM! Simple and correct implementation.
34-37:
Critical: Implement proper cart model detection before release.
The current implementation always returns ED64_V1_0, which could lead to incorrect feature detection. This is particularly important as:
- It affects
has_feature()responses - Could prevent ED64V3 features from being available
- May impact the release quality mentioned in PR objectives
Would you like me to help implement the cart model detection or create a GitHub issue to track this task?
139-151:
Implement firmware version retrieval before release.
The get_firmware_version function pointer is NULL with a FIXME comment. Since this is for a release merge, consider implementing this to provide users with proper version information.
Would you like me to help implement the firmware version retrieval or create a GitHub issue to track this task?
src/flashcart/flashcart.h (1)
60-65: LGTM! Well-structured version information.
The firmware version structure is well-designed with appropriate field types and clear documentation.
src/flashcart/64drive/64drive.c (2)
85-108: LGTM! Well-implemented firmware version retrieval.
The implementation is thorough with:
- Clear documentation
- Proper error handling
- Logical version mapping based on device variant
305-305: LGTM! Proper integration of firmware version function.
The function pointer is correctly integrated into the flashcart structure.
src/menu/views/flashcart_info.c (3)
6-27: LGTM! Well-structured formatting functions
The boolean and cart type formatting functions are well-implemented with clean, safe code and good handling of all cases.
37-39: LGTM! Good user feedback addition
The sound effect addition provides good user feedback and maintains consistency with other views.
50-82: LGTM! Well-structured information display
The flashcart information display is well-organized with:
- Clear section headings
- Comprehensive feature support checks
- Consistent formatting
src/flashcart/sc64/sc64.c (1)
265-268: Verify that these features are actually implemented.
All of these newly added features are hard-coded to return true, which might be misleading if some features are still partially implemented or untested. Please confirm that each feature is fully supported and functional.
| FLASHCART_FEATURE_AUTO_CIC, | ||
| FLASHCART_FEATURE_AUTO_REGION, | ||
| FLASHCART_FEATURE_DIAGNOSTIC_DATA, | ||
| FLASHCART_FEATURE_BIOS_UPDATE_FROM_MENU, | ||
| FLASHCART_FEATURE_SAVE_WRITEBACK |
There was a problem hiding this comment.
💡 Codebase verification
Implementation inconsistency found across flashcart types
The verification reveals inconsistent feature support across flashcart types:
- SC64 supports all new features: AUTO_CIC, AUTO_REGION, DIAGNOSTIC_DATA, BIOS_UPDATE_FROM_MENU, and SAVE_WRITEBACK
- 64drive supports AUTO_CIC, AUTO_REGION, and SAVE_WRITEBACK, but lacks DIAGNOSTIC_DATA and BIOS_UPDATE_FROM_MENU
- ED64 V-series only supports AUTO_CIC (and only for V3 model)
Additionally, while SAVE_WRITEBACK is marked as supported in both SC64 and 64drive, the implementation is present and complete in both drivers:
- SC64: Has
sc64_set_save_writebackimplementation - 64drive: Has
d64_set_save_writebackimplementation with proper enable/disable controls
🔗 Analysis chain
Verify implementation consistency across flashcart types
Based on past review comments and the PR objectives, please ensure these features are consistently implemented across all supported flashcart types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check implementation consistency across flashcart types
echo "Checking feature implementations in all flashcart drivers..."
# Search for has_feature implementations
rg "FLASHCART_FEATURE_(AUTO_CIC|AUTO_REGION|DIAGNOSTIC_DATA|BIOS_UPDATE_FROM_MENU|SAVE_WRITEBACK)" src/flashcart/ -C 3
# Check for actual implementation code
echo -e "\nChecking for actual feature implementations..."
rg "(auto.*cic|auto.*region|diagnostic.*data|bios.*update|save.*write)" src/flashcart/ -C 3
Length of output: 10417
introduces ` --outline 1` as a mkfont flag. makes other flags more verbose.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
105-116: Add error handling for asset conversion tools.While the build rules are correct, they should handle potential failures of the conversion tools.
Consider updating the rules to check for conversion tool errors:
$(FILESYSTEM_DIR)/%.font64: $(ASSETS_DIR)/fonts/%.ttf @echo " [FONT] $@" - @$(N64_MKFONT) $(MKFONT_FLAGS) -o $(FILESYSTEM_DIR) "$<" + @$(N64_MKFONT) $(MKFONT_FLAGS) -o $(FILESYSTEM_DIR) "$<" || (echo "Error: Font conversion failed for $<" && exit 1) $(FILESYSTEM_DIR)/%.wav64: $(ASSETS_DIR)/sounds/%.wav @echo " [AUDIO] $@" - @$(N64_AUDIOCONV) $(AUDIOCONV_FLAGS) -o $(FILESYSTEM_DIR) "$<" + @$(N64_AUDIOCONV) $(AUDIOCONV_FLAGS) -o $(FILESYSTEM_DIR) "$<" || (echo "Error: Audio conversion failed for $<" && exit 1) $(FILESYSTEM_DIR)/%.sprite: $(ASSETS_DIR)/images/%.png @echo " [SPRITE] $@" - @$(N64_MKSPRITE) $(MKSPRITE_FLAGS) -o $(dir $@) "$<" + @$(N64_MKSPRITE) $(MKSPRITE_FLAGS) -o $(dir $@) "$<" || (echo "Error: Sprite conversion failed for $<" && exit 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.devcontainer/Dockerfile(1 hunks)Makefile(3 hunks)
🔇 Additional comments (7)
.devcontainer/Dockerfile (2)
Line range hint 3-15: LGTM! Version pinning is maintained
The Dockerfile maintains good practices by:
- Pinning to a specific version for reproducibility
- Cleaning up temporary files after installation
- Using a multi-stage command to minimize layers
3-3: Verify the SC64 deployer version upgrade
The version bump from v2.20.0 to v2.20.2 appears to be a minor update. Let's verify the version exists and is accessible.
✅ Verification successful
SC64 deployer version v2.20.2 is valid and accessible
The version v2.20.2 exists as a published release and contains the required Linux deployer tarball. The release notes indicate it's a bug fix release that addresses a regression in the SD card module from the previous version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SC64 deployer version exists and is accessible
# Expected results: Version exists and tarball is accessible
# Check if the release exists
gh api \
-H "Accept: application/vnd.github+json" \
repos/Polprzewodnikowy/SummerCart64/releases/tags/v2.20.2
# Verify the tarball URL is accessible
curl -I https://github.com/Polprzewodnikowy/SummerCart64/releases/download/v2.20.2/sc64-deployer-linux-v2.20.2.tar.gz
Length of output: 16779
Makefile (5)
94-96: LGTM: Filesystem entries properly configured.
The FILESYSTEM variable correctly integrates all asset types using appropriate make functions for path manipulation.
54-58: Verify complete migration of UI components.
The restructuring from menu/components to menu/ui_components improves code organization.
Let's verify no UI components were left behind:
✅ Verification successful
UI components migration appears complete and consistent
The verification shows that:
- The old directory
menu/componentsno longer exists, indicating a complete migration - All UI components are properly located in
menu/ui_components/, including:- Core UI files (background.c, boxart.c, common.c)
- Interface elements (context_menu.c, file_list.c)
- Supporting header (constants.h)
The restructuring has been carried out consistently with no remnants in the old location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining files in the old components directory
echo "Checking for remnants in old directory:"
fd --type f ".*\.[ch]$" src/menu/components/
# List all files in new directory to ensure completeness
echo "Verifying files in new directory:"
fd --type f ".*\.[ch]$" src/menu/ui_components/
Length of output: 578
81-86: Verify sound assets existence and format.
The addition of UI sound effects enhances user experience with appropriate feedback for different actions.
Let's verify the sound assets:
✅ Verification successful
Let me try a different approach to verify the sound files.
Sound assets are present in the expected location
All five WAV files listed in the SOUNDS variable are present in the assets/sounds directory:
- assets/sounds/cursorsound.wav
- assets/sounds/back.wav
- assets/sounds/enter.wav
- assets/sounds/error.wav
- assets/sounds/settings.wav
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all sound files exist and are valid WAV files
for sound in cursorsound back enter error settings; do
echo "Checking $sound.wav:"
file "assets/sounds/$sound.wav"
done
Length of output: 1163
Script:
#!/bin/bash
# Check if sound files exist using fd
fd -t f -e wav . assets/sounds
# Try alternate locations
fd -t f -e wav . sounds
fd -t f -e wav . src/assets/sounds
Length of output: 422
32-32: LGTM: New ED64 V-series source file addition.
The addition of ed64_vseries.c aligns with the PR objectives regarding ED64V3 support.
Let's verify the new source file exists and follows project structure:
✅ Verification successful
Let me verify the structure and content of these files to ensure they follow the project's patterns.
Let me check the actual implementation details of the V-series file to ensure it follows the project's patterns.
✓ ED64 V-series implementation follows project patterns
The implementation is well-structured and follows the project's patterns:
- Proper header guards and documentation
- Implements the required flashcart interface with appropriate function signatures
- Includes necessary feature detection for V-series variants
- Contains proper memory mappings and file handling
A few TODOs are marked with FIXME comments for future improvements:
- Cart model detection from low-level interface
- Save type handling implementation
- Firmware version reporting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the new source file exists and check its structure
fd --type f "ed64_vseries.[ch]" src/flashcart/ed64/
Length of output: 121
Script:
#!/bin/bash
# Check the structure of the V-series implementation
ast-grep --pattern 'struct flashcart_device $_vseries = {
$$$
};' src/flashcart/ed64/ed64_vseries.c
# Check header file structure
cat src/flashcart/ed64/ed64_vseries.h
# Check for similar ED64 implementations for pattern comparison
fd --type f "ed64.*\.c" src/flashcart/ed64/ --exec ast-grep --pattern 'struct flashcart_device $_ = {
$$$
};'
Length of output: 608
Script:
#!/bin/bash
# Check the content of the implementation file
cat src/flashcart/ed64/ed64_vseries.c
# Check other ED64 implementations for pattern comparison
fd --type f "ed64.*\.c" --exclude "ed64_vseries.c" src/flashcart/ed64/ --exec head -n 50 {}
Length of output: 4409
100-100: Consider performance impact of font compression and outline.
While compression saves space, and outline improves readability, both features can impact rendering performance. Ensure this has been tested on actual hardware.
Let's check if font processing flags are documented:
✅ Verification successful
Let me try searching for font-related documentation in other locations and with different patterns.
Let me check the menu customization documentation and search for any performance-related information about fonts.
Let me check if there are any other font files or similar configurations to understand if this is a common pattern.
Let me check the font-related source files to understand the rendering implementation.
Font compression and outline settings are appropriate for this use case
The font configuration is well-aligned with the implementation:
- The font is used with libdragon's
rdpq_fontsystem which is optimized for N64 rendering - Only one font file (FiraMonoBold.font64) is loaded and shared across different styles
- The character ranges are specifically chosen (basic ASCII, extended chars, and ellipsis)
- Compression level 1 is a reasonable balance between size and performance
The compression and outline settings won't significantly impact performance since:
- Font loading happens once during initialization
- The rendering is handled by the optimized
rdpqsystem - The font is used for simple UI elements like menus and file lists
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for font-related documentation
rg -i "font.*compression|font.*outline" "docs/"
Length of output: 47
Script:
#!/bin/bash
# Search for font documentation in README files and other common doc locations
rg -i "font" -g "README*" -g "*.md" -g "*.txt"
# Search for mkfont documentation or usage
fd -e md -e txt mkfont
# Look for font-related build or configuration files
fd -e mk -e conf -e ini font
Length of output: 252
Script:
#!/bin/bash
# Check content of the menu customization doc
cat docs/07_menu_customization.md
# Search for performance-related discussions
rg -i "performance|render" -g "*.md" -g "*.txt"
# Look for other font flag usages in Makefile for comparison
rg "MKFONT_FLAGS" Makefile
Length of output: 646
Script:
#!/bin/bash
# Find all font64 files and their references
fd -e font64
# Look for other font-related files
fd -g "*font*"
# Check for any build or performance documentation
rg -i "build.*font|font.*generation" -g "*.md"
# Look for libdragon tool documentation or configuration
rg -i "libdragon.*tool|n64_mkfont" -g "*.md" -g "Makefile"
Length of output: 521
Script:
#!/bin/bash
# Check font implementation files
cat src/menu/fonts.h
cat src/menu/fonts.c
# Look for any performance-related comments or implementation details
rg -i "compress|outline|render" src/menu/
Length of output: 2776
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Changes the button context from `R` to `L|Z` and moves the `move load_disk_with_rom` to `menu_state` so that it can be used for things like autoload. ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> Aligns the button context with the ROM info menu. Makes it easier to set and re-use expansion ROM's. ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [x] Breaking change (breaking change) - [ ] Documentation Improvement - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] 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. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/menu/views/load_disk.c (1)
Line range hint
119-150: Consider adding memory allocation error handling.While the loading logic is sound, consider adding checks for potential memory allocation failures when setting up boot parameters and cheat lists.
if (menu->load.combined_disk_rom) { + if (menu->boot_params == NULL) { + menu_show_error(menu, "Failed to allocate boot parameters"); + return; + } menu->boot_params->device_type = BOOT_DEVICE_TYPE_ROM;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/menu/menu_state.h(2 hunks)src/menu/views/load_disk.c(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/menu/menu_state.h
🔇 Additional comments (5)
src/menu/views/load_disk.c (5)
4-4: LGTM! Sound effects and boxart integration looks good.
The addition of sound effects and boxart component is well-structured with proper static scoping.
Also applies to: 7-7
33-36: Verify action mapping for "Load with ROM" functionality.
There might be a mismatch between the action check and the UI text:
- Code checks for
menu->actions.lz_context - UI shows "L|Z: Load with ROM"
Please confirm that lz_context correctly represents both L and Z button presses.
Also applies to: 90-90
Line range hint 46-96: LGTM! UI components are well-structured.
The UI drawing code is well-organized with:
- Consistent use of ui_components_ prefix
- Proper conditional rendering of boxart
- Clear layout structure
153-173: LGTM! Proper resource management.
Good improvements:
- Clean boxart resource cleanup in deinit
- Early return on disk info load error
- Proper initialization of boxart component
181-188: LGTM! Clean state management.
The display function properly manages state transitions:
- Clears boot pending flag after loading
- Calls deinit when changing modes
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/menu/views/settings_editor.c (1)
14-46:⚠️ Potential issueAvoid void to boolean casting for improved portability*
The pattern of casting between
void*and boolean types viauintptr_tappears across all settings handlers. This matches a previously identified issue.Consider refactoring all handler functions to use boolean parameters directly:
-static void set_protected_entries_type(menu_t *menu, void *arg) { +static void set_protected_entries_type(menu_t *menu, bool enabled) { - menu->settings.show_protected_entries = (bool)(uintptr_t)(arg); + menu->settings.show_protected_entries = enabled; settings_save(&menu->settings); menu->browser.reload = true; }Apply similar changes to all other handler functions.
🧹 Nitpick comments (5)
README.md (3)
54-57: Add cautionary note about save statesThe autoload documentation is clear, but consider adding a warning about ensuring games are properly saved before using the power switch, as the autoload feature bypasses the menu's normal shutdown sequence.
To use the autoload function, while on the `N64 ROM information` display, press the `R` button on your joypad and select the `Set ROM to autoload` option. When you restart the console, it will now only load the selected ROM rather than the menu. The autoload setting is stored in `config.ini` and persists until changed. This feature may slightly increase boot time as the menu needs to check for the Start button state. + +> **Important**: Always ensure your game progress is properly saved before powering off the console when using autoload, as this feature bypasses the menu's normal shutdown sequence. NOTE: To return to the menu, hold the joypad `Start` button while powering on the console.
69-71: Fix list style inconsistencyFor consistency with the rest of the document, use asterisks instead of dashes for unordered lists.
-Supported PNG formats: -- RGB/RGBA color formats -- 8-bit color depth +Supported PNG formats: +* RGB/RGBA color formats +* 8-bit color depth🧰 Tools
🪛 Markdownlint (0.37.0)
70-70: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
71-71: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
82-86: Fix list style inconsistency and add storage noteFix the list style and add a note about storage requirements for future sprite support.
-**Note**: For future support, boxart sprites should also include: -- `boxart_back.png` -- `boxart_top.png` -- `boxart_bottom.png` -- `boxart_left.png` -- `boxart_right.png` +**Note**: For future support, boxart sprites should also include: +* `boxart_back.png` +* `boxart_top.png` +* `boxart_bottom.png` +* `boxart_left.png` +* `boxart_right.png` + +> **Storage Note**: Consider the SD card space requirements when including multiple sprite views per game.🧰 Tools
🪛 Markdownlint (0.37.0)
82-82: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
83-83: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
84-84: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
85-85: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
86-86: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
src/menu/views/settings_editor.c (2)
48-51: Track the FIXME comment properlyThe commented-out function with FIXME should be tracked in the issue system rather than left as dead code.
Would you like me to create a GitHub issue to track the implementation of the default settings reset functionality?
129-163: Consider extracting UI strings as constantsThe UI text is well-organized but could benefit from being extracted into constants for easier maintenance and localization.
Consider creating a header file with string constants:
// settings_strings.h #define SETTINGS_TITLE "MENU SETTINGS EDITOR" #define SETTINGS_DEFAULT_DIR_LABEL "Default Directory" // ... more strings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(3 hunks)docs/00_getting_started_sd.md(3 hunks)src/menu/views/settings_editor.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/00_getting_started_sd.md
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~26-~26: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...omprehensive ROM information display. * Real Time Clock support. * Music playback (MP3). ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~144-~144: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...//github.com/networkfusion/altra64) # Open source software and licenses used * [libdrago...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 Markdownlint (0.37.0)
README.md
70-70: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
71-71: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
82-82: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
83-83: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
84-84: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
85-85: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
86-86: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
🔇 Additional comments (9)
README.md (4)
21-21: LGTM: Feature list updates are clear and accurate
The additions to the feature list properly document the new capabilities including expanded emulator support, sound effects, and ROM autoload functionality.
Also applies to: 28-29
35-35: LGTM: Documentation structure update
The addition of the menu customizations guide enhances the documentation structure.
128-141: LGTM: ED64 documentation improvements
The ED64 section has been improved with clearer warnings and better organization of information for different variants.
152-156: LGTM: Sound assets properly documented
The new sounds section clearly documents the sources and licenses for all audio assets.
src/menu/views/settings_editor.c (5)
1-3: LGTM: Clean includes and utility setup
The includes are well-organized and the sound module integration is properly set up.
109-116: Clean event handling with good user feedback
The process logic is well-structured with appropriate sound feedback for user actions.
180-181: LGTM: Clean initialization
The view initialization is properly handled.
74-77:
Fix inconsistent casting in PAL60 context menu
The false value casting is inconsistent with other similar definitions:
- {.text = "Off", .action = set_pal60_type, .arg = (void *) (false) },
+ {.text = "Off", .action = set_pal60_type, .arg = (void *)(uintptr_t)(false) },Likely invalid or redundant comment.
93-105: Well-structured menu organization
The options menu is well-organized with clear grouping of standard and beta features.
Let's verify the beta settings are properly gated:
✅ Verification successful
Let me verify the usage of these beta features further to ensure they are properly gated.
Beta features are properly gated and well-organized
The verification shows that the beta features (PAL60 Mode, Background Music, and Rumble Feedback) are consistently wrapped in #ifdef BETA_SETTINGS blocks in settings_editor.c. This includes:
- Menu entries in the options context menu
- Their respective setting functions and context menus
- Status display in the settings view
The other occurrences of "Rumble" in the codebase are related to hardware support (Rumble Pak accessory) and game compatibility, which are separate from the beta feedback feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify BETA_SETTINGS macro usage consistency
# Expected: All beta features should be consistently wrapped in BETA_SETTINGS
# Check for any beta features outside BETA_SETTINGS blocks
rg -p "PAL60|BGM|[^A-Za-z]Rumble" --type c -C 3 | rg -v "#ifdef BETA_SETTINGS" -C 3
Length of output: 3535
Script:
#!/bin/bash
# Search for definitions and implementations of beta features
rg "(set_pal60|set_bgm_enabled|set_rumble_enabled|PAL60|BGM|[^A-Za-z]Rumble)" --type c -C 3 src/menu/
Length of output: 7228
networkfusion
force-pushed
the
develop
branch
from
bb55445 to
bf1e71d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/menu/views/settings_editor.c (1)
14-46: 🛠️ Refactor suggestionAvoid casting between
void*and integer types for portabilityThe pattern of casting between
void*and integer types viauintptr_tis repeated across all toggle functions.Add error handling for settings persistence
The
settings_savefunction is called without error handling. Consider checking the return value to handle potential storage failures.static void set_protected_entries_type (menu_t *menu, void *arg) { menu->settings.show_protected_entries = (bool)(uintptr_t)(arg); - settings_save(&menu->settings); + if (settings_save(&menu->settings) != 0) { + // Handle error - perhaps show a message to user + sound_play_effect(SFX_ERROR); + } menu->browser.reload = true; }
🧹 Nitpick comments (6)
README.md (2)
60-61: Remove duplicate instruction lineThe instruction for GamePak sprites placement is duplicated.
-To use N64 GamePak sprites, place PNG files within the `sd:/menu/boxart/` folder. -To use N64 `GamePak` sprites, place `PNG` files within the `sd:/menu/boxart/` folder. +To use N64 GamePak sprites, place PNG files within the `sd:/menu/boxart/` folder.
26-26: Consider hyphenating compound adjectivesFor technical accuracy, consider hyphenating these compound adjectives:
- "Real-Time Clock support"
- "Open-source software"
Also applies to: 144-144
🧰 Tools
🪛 LanguageTool
[uncategorized] ~26-~26: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...omprehensive ROM information display. * Real Time Clock support. * Music playback (MP3). ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
src/menu/views/settings_editor.c (4)
93-105: Consider grouping related settings in the options menuThe options menu mixes different types of settings (UI, audio, input). Consider organizing them into logical groups for better user experience.
static component_context_menu_t options_context_menu = { .list = { + // UI Settings { .text = "Show Hidden Files", .submenu = &set_protected_entries_type_context_menu }, + { .text = "Use Saves Folder", .submenu = &set_use_saves_folder_type_context_menu }, + COMPONENT_CONTEXT_MENU_LIST_SEPARATOR, + // Audio Settings { .text = "Sound Effects", .submenu = &set_sound_enabled_type_context_menu }, - { .text = "Use Saves Folder", .submenu = &set_use_saves_folder_type_context_menu }, #ifdef BETA_SETTINGS - { .text = "PAL60 Mode", .submenu = &set_pal60_type_context_menu }, { .text = "Background Music", .submenu = &set_bgm_enabled_type_context_menu }, + COMPONENT_CONTEXT_MENU_LIST_SEPARATOR, + // System Settings + { .text = "PAL60 Mode", .submenu = &set_pal60_type_context_menu }, { .text = "Rumble Feedback", .submenu = &set_rumble_enabled_type_context_menu }, // { .text = "Restore Defaults", .action = set_use_default_settings }, #endif
109-116: Add visual feedback for menu interactionsConsider adding visual feedback when menu items are selected or when settings are changed.
if (ui_components_context_menu_process(menu, &options_context_menu)) { + // Flash or highlight the changed setting + ui_components_highlight_last_change(); return; } if (menu->actions.enter) { + // Highlight the selected menu item + ui_components_highlight_selection(); ui_components_context_menu_show(&options_context_menu); sound_play_effect(SFX_SETTING);
135-163: Improve settings documentation and accessibilityThe settings display could be enhanced with more detailed descriptions and better visual organization.
ui_components_main_text_draw( ALIGN_LEFT, VALIGN_TOP, "\n\n" - " Default Directory : %s\n\n" - " Autoload ROM : %s\n\n" + "System Information:\n" + " • Default Directory : %s\n" + " • Autoload ROM : %s\n" + "\n" "To change the following menu settings, press 'A':\n" - " Show Hidden Files : %s\n" - " Use Saves folder : %s\n" - " Sound Effects : %s\n" + " UI Settings:\n" + " • Show Hidden Files : %s (Shows system and backup files)\n" + " • Use Saves folder : %s (Organizes save files separately)\n" + "\n" + " Audio Settings:\n" + " • Sound Effects : %s (Menu navigation sounds)\n"
179-181: Consider adding cleanup function for resource managementAdd a cleanup function to properly release any resources allocated during initialization.
void view_settings_init (menu_t *menu) { ui_components_context_menu_init(&options_context_menu); } +void view_settings_cleanup (menu_t *menu) { + ui_components_context_menu_cleanup(&options_context_menu); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(3 hunks)docs/00_getting_started_sd.md(3 hunks)src/menu/views/settings_editor.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/00_getting_started_sd.md
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~26-~26: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...omprehensive ROM information display. * Real Time Clock support. * Music playback (MP3). ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~144-~144: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...//github.com/networkfusion/altra64) # Open source software and licenses used * [libdrago...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🔇 Additional comments (7)
README.md (5)
21-21: LGTM: Feature list updates are clear and accurate
The additions to the emulator support list and new features are well-documented and align with the PR objectives.
Also applies to: 28-29
54-57: LGTM: N64 ROM autoload instructions are clear
The autoload feature instructions are well-documented with clear steps for enabling and disabling the feature.
63-87: LGTM: Comprehensive sprite documentation
The sprite requirements and file structure are well-documented with clear specifications for dimensions and formats.
128-141: LGTM: ED64 support documentation improvements
The ED64 support section has been improved with:
- Clearer warning message about save functionality
- Specific instructions for different ED64 variants
- Clear references to feature parity goals
152-156: LGTM: Sound effects properly attributed
Sound effects are well-documented with proper attribution and license information.
✅ Verification successful
Sound effects properly attributed with corresponding files present
The verification confirms that all referenced sound files exist in the project under assets/sounds/:
cursorsound.wavmatching the "Cursor sound" attributionenter.wavandback.wavmatching the "Actions (Enter, back) sound" attributionerror.wavmatching the "Error sound" attribution
The documentation in README.md correctly attributes these sound files to their respective authors with appropriate licensing information from Pixabay.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the referenced sound files exist in the project
fd -e wav -e mp3 "cursorsound|back|enter|error|settings"
Length of output: 186
src/menu/views/settings_editor.c (2)
1-2: LGTM: Appropriate includes added
The new includes for stdbool.h and sound.h are correctly added to support the boolean operations and sound functionality.
74-78:
Fix inconsistent casting in PAL60 context menu
The false value casting is inconsistent with other context menus.
static component_context_menu_t set_pal60_type_context_menu = { .list = {
{.text = "On", .action = set_pal60_type, .arg = (void *)(uintptr_t)(true) },
- {.text = "Off", .action = set_pal60_type, .arg = (void *)(false) },
+ {.text = "Off", .action = set_pal60_type, .arg = (void *)(uintptr_t)(false) },
COMPONENT_CONTEXT_MENU_LIST_END,
}};Likely invalid or redundant comment.
README.md
Outdated
|
|
||
|
|
||
| ## Documentation | ||
| * [Getting started guide](./docs/00_getting_started_sd.md) | ||
| * [Menu controls](./docs/01_menu_controls.md) | ||
| * [Menu customizations](./docs/07_menu_customizations.md) |
There was a problem hiding this comment.
💡 Codebase verification
Broken documentation link detected
The link to the menu customizations documentation (docs/07_menu_customizations.md) is invalid as the file does not exist in the repository.
./docs/07_menu_customizations.md- File is missing
🔗 Analysis chain
Verify the menu customizations documentation link
The link format is consistent with other documentation links.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the linked documentation file exists
test -f docs/07_menu_customizations.md && echo "Documentation file exists" || echo "Documentation file missing"
Length of output: 103
networkfusion
force-pushed
the
develop
branch
2 times, most recently
from
97b7692 to
979e9ab
Compare
networkfusion
force-pushed
the
develop
branch
from
979e9ab to
e14ea9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/menu/views/settings_editor.c (1)
14-46:⚠️ Potential issueFix type casting between void and boolean*
The type casting pattern used across all toggle functions has potential portability issues.
This is a duplicate of a previous review comment. The same issue applies to all toggle functions in this range. Consider changing all function signatures to use
booldirectly instead ofvoid*.Additionally, verify that the
sound_use_sfxcall inset_sound_enabled_typeis thread-safe.#!/bin/bash # Search for concurrent access to sound functionality rg -p "sound_use_sfx|sound_play" -A 3 -B 3
🧹 Nitpick comments (6)
src/menu/views/settings_editor.c (4)
48-51: Address TODO: Implement restore defaults functionalityThe commented-out
set_use_default_settingsfunction needs implementation.Would you like me to help implement the restore defaults functionality?
113-116: Consider error handling for sound effectsThe sound effect calls should handle potential failures gracefully.
- sound_play_effect(SFX_SETTING); + if (menu->settings.sound_enabled) { + if (sound_play_effect(SFX_SETTING) != SUCCESS) { + // Log error or handle gracefully + } + }
144-151: Improve accessibility for beta settings indicationThe asterisk (*) used to mark beta settings might not be sufficiently visible or meaningful to all users.
Consider:
- Using a more visible indicator
- Adding a screen reader friendly label
- Using color coding (ensuring sufficient contrast)
180-181: Add error handling for context menu initializationThe context menu initialization should handle potential failures.
- ui_components_context_menu_init(&options_context_menu); + if (ui_components_context_menu_init(&options_context_menu) != SUCCESS) { + // Log error or handle gracefully + }README.md (2)
54-57: Enhance readability with formatting improvementsConsider using bullet points or a more structured format for better readability:
### N64 ROM autoload -To use the autoload function, while on the `N64 ROM information` display, press the `R` button on your joypad and select the `Set ROM to autoload` option. When you restart the console, it will now only load the selected ROM rather than the menu. -The autoload setting is stored in `config.ini` and persists until changed. This feature may slightly increase boot time as the menu needs to check for the Start button state. -NOTE: To return to the menu, hold the joypad `Start` button while powering on the console. +#### Setup +1. Navigate to the `N64 ROM information` display +2. Press the `R` button on your joypad +3. Select the `Set ROM to autoload` option + +#### Important Notes +- The setting is stored in `config.ini` and persists until changed +- Boot time may slightly increase due to Start button state checking +- To return to the menu: Hold the `Start` button while powering on the console
59-106: Consider adding a visual directory tree exampleThe sprite documentation is comprehensive, but a visual directory tree example would make the structure even clearer.
Add a directory tree example like this:
sd:/menu/boxart/ ├── N/ │ └── G/ │ └── E/ │ ├── E/ │ │ └── boxart_front.png # NTSC USA │ ├── P/ │ │ └── boxart_front.png # PAL │ └── boxart_front.png # Fallback
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(3 hunks)docs/00_getting_started_sd.md(3 hunks)src/menu/views/settings_editor.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/00_getting_started_sd.md
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~26-~26: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...omprehensive ROM information display. * Real Time Clock support. * Music playback (MP3). ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~143-~143: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...//github.com/networkfusion/altra64) # Open source software and licenses used * [libdrago...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🔇 Additional comments (5)
src/menu/views/settings_editor.c (1)
1-2: LGTM: Appropriate includes added
The new includes for stdbool.h and sound.h are correctly added to support the boolean operations and sound functionality.
README.md (4)
21-21: LGTM: Feature list updates are clear and accurate
The new features are well-documented and align with the PR objectives.
Also applies to: 28-29
127-140: LGTM: ED64 section improvements
The warning message and feature parity goals are clearly documented, incorporating previous review suggestions.
151-155: LGTM: Sound effects attribution is complete
The sound effects are properly attributed with clear licensing information.
35-35: Verify menu customization documentation exists
Please ensure the referenced documentation file exists at the specified path.
✅ Verification successful
Let me verify with a slightly different path since there might be a typo in the path (customizations vs customization):
Documentation link is valid and file exists
The documentation file docs/07_menu_customization.md exists in the repository, making the link in the README.md valid and accessible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the menu customization documentation exists
test -f docs/07_menu_customizations.md && echo "Documentation exists" || echo "Documentation missing"
Length of output: 93
Script:
#!/bin/bash
# Check if the menu customization documentation exists with correct path
test -f docs/07_menu_customization.md && echo "Documentation exists" || echo "Documentation missing"
# List all files in docs directory to see available documentation
ls -la docs/
Length of output: 510
src/menu/views/settings_editor.c
Outdated
| static component_context_menu_t set_protected_entries_type_context_menu = { .list = { | ||
| {.text = "On", .action = set_protected_entries_type, .arg = (void *)(uintptr_t)(true) }, | ||
| {.text = "Off", .action = set_protected_entries_type, .arg = (void *)(uintptr_t)(false) }, | ||
| COMPONENT_CONTEXT_MENU_LIST_END, | ||
| }}; | ||
|
|
||
| static component_context_menu_t set_sound_enabled_type_context_menu = { .list = { | ||
| {.text = "On", .action = set_sound_enabled_type, .arg = (void *)(uintptr_t)(true) }, | ||
| {.text = "Off", .action = set_sound_enabled_type, .arg = (void *)(uintptr_t)(false) }, | ||
| COMPONENT_CONTEXT_MENU_LIST_END, | ||
| }}; | ||
|
|
||
| static component_context_menu_t set_use_saves_folder_type_context_menu = { .list = { | ||
| {.text = "On", .action = set_use_saves_folder_type, .arg = (void *)(uintptr_t)(true) }, | ||
| {.text = "Off", .action = set_use_saves_folder_type, .arg = (void *)(uintptr_t)(false) }, | ||
| COMPONENT_CONTEXT_MENU_LIST_END, | ||
| }}; | ||
|
|
||
| #ifdef BETA_SETTINGS | ||
| static component_context_menu_t set_pal60_type_context_menu = { .list = { | ||
| {.text = "On", .action = set_pal60_type, .arg = (void *)(uintptr_t)(true) }, | ||
| {.text = "Off", .action = set_pal60_type, .arg = (void *) (false) }, | ||
| COMPONENT_CONTEXT_MENU_LIST_END, | ||
| }}; | ||
|
|
||
| static component_context_menu_t set_bgm_enabled_type_context_menu = { .list = { | ||
| {.text = "On", .action = set_bgm_enabled_type, .arg = (void *)(uintptr_t)(true) }, | ||
| {.text = "Off", .action = set_bgm_enabled_type, .arg = (void *)(uintptr_t)(false) }, | ||
| COMPONENT_CONTEXT_MENU_LIST_END, | ||
| }}; | ||
|
|
||
| static component_context_menu_t set_rumble_enabled_type_context_menu = { .list = { | ||
| {.text = "On", .action = set_rumble_enabled_type, .arg = (void *)(uintptr_t)(true) }, | ||
| {.text = "Off", .action = set_rumble_enabled_type, .arg = (void *)(uintptr_t)(false) }, | ||
| COMPONENT_CONTEXT_MENU_LIST_END, | ||
| }}; | ||
| #endif | ||
|
|
||
| static component_context_menu_t options_context_menu = { .list = { | ||
| { .text = "Show Hidden Files", .submenu = &set_protected_entries_type_context_menu }, | ||
| { .text = "Sound Effects", .submenu = &set_sound_enabled_type_context_menu }, | ||
| { .text = "Use Saves Folder", .submenu = &set_use_saves_folder_type_context_menu }, | ||
| #ifdef BETA_SETTINGS | ||
| { .text = "PAL60 Mode", .submenu = &set_pal60_type_context_menu }, | ||
| { .text = "Background Music", .submenu = &set_bgm_enabled_type_context_menu }, | ||
| { .text = "Rumble Feedback", .submenu = &set_rumble_enabled_type_context_menu }, | ||
| // { .text = "Restore Defaults", .action = set_use_default_settings }, | ||
| #endif | ||
|
|
||
| COMPONENT_CONTEXT_MENU_LIST_END, | ||
| }}; |
There was a problem hiding this comment.
Inconsistent boolean value casting in context menus
The context menu definitions use inconsistent casting patterns. Note line 76 where (void *) (false) differs from the pattern (void *)(uintptr_t)(false) used elsewhere.
This inconsistency could lead to undefined behavior on some platforms. Apply consistent casting across all menu definitions or preferably refactor to use bool directly as suggested earlier.
Rename functions to better describe them.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/menu/menu.c (1)
38-39:⚠️ Potential issueReplace assert with proper error handling
Using assertions for memory allocation checks is not suitable for production code as they can be disabled in release builds.
src/menu/settings.c (2)
45-49:⚠️ Potential issueFix potential memory leak and improve section consistency
- The
strdupcalls for ROM paths could leak memory if these settings are reloaded without freeing previous values.- The autoload settings are split across different sections, which could be confusing.
Consider this safer implementation:
settings->soundfx_enabled = mini_get_bool(ini, "menu", "soundfx_enabled", init.soundfx_enabled); -settings->rom_autoload_enabled = mini_get_bool(ini, "menu", "autoload_rom_enabled", init.rom_autoload_enabled); -settings->rom_autoload_path = strdup(mini_get_string(ini, "autoload", "rom_path", init.rom_autoload_path)); -settings->rom_autoload_filename = strdup(mini_get_string(ini, "autoload", "rom_filename", init.rom_autoload_filename)); +// Move all autoload settings to the autoload section +settings->rom_autoload_enabled = mini_get_bool(ini, "autoload", "enabled", init.rom_autoload_enabled); + +// Free existing strings before assignment +if (settings->rom_autoload_path) free(settings->rom_autoload_path); +if (settings->rom_autoload_filename) free(settings->rom_autoload_filename); + +settings->rom_autoload_path = strdup(mini_get_string(ini, "autoload", "rom_path", init.rom_autoload_path)); +settings->rom_autoload_filename = strdup(mini_get_string(ini, "autoload", "rom_filename", init.rom_autoload_filename));
65-68: 🛠️ Refactor suggestionUpdate section consistency to match loading changes
For consistency with the previous suggestion, move all autoload settings to the "autoload" section.
mini_set_bool(ini, "menu", "soundfx_enabled", settings->soundfx_enabled); -mini_set_bool(ini, "menu", "autoload_rom_enabled", settings->rom_autoload_enabled); +mini_set_bool(ini, "autoload", "enabled", settings->rom_autoload_enabled); mini_set_string(ini, "autoload", "rom_path", settings->rom_autoload_path); mini_set_string(ini, "autoload", "rom_filename", settings->rom_autoload_filename);src/menu/views/settings_editor.c (2)
14-46:⚠️ Potential issueAvoid casting between
void*and integer types for portabilityCasting between
void*and integer types likeboolviauintptr_tmay lead to undefined behavior and portability issues, especially on platforms where the sizes of these types differ.Consider changing the
argparameter to be of typebooldirectly, eliminating the need for casting. Modify the function signatures and context menu initializations accordingly.
74-78:⚠️ Potential issueInconsistent boolean value casting in context menus
The context menu definitions use inconsistent casting patterns. Note line 76 where
(void *) (false)differs from the pattern(void *)(uintptr_t)(false)used elsewhere.This inconsistency could lead to undefined behavior on some platforms. Apply consistent casting across all menu definitions or preferably refactor to use
booldirectly as suggested earlier.
🧹 Nitpick comments (2)
src/menu/menu.c (1)
30-31: Consider making display settings configurableThese hardcoded values could be moved to configuration settings to provide more flexibility:
INTERLACEDcould be user-configurable for different display preferencesFPS_LIMITcould be adjustable based on different game or region requirements-#define INTERLACED (true) -#define FPS_LIMIT (30.0f) +// Add to settings structure +typedef struct { + // ... existing fields ... + bool interlaced; + float fps_limit; +} settings_t; + +// Use settings instead of defines +#define DEFAULT_INTERLACED (true) +#define DEFAULT_FPS_LIMIT (30.0f)src/menu/views/settings_editor.c (1)
48-51: Implement the default settings reset functionalityThe commented-out function with FIXME indicates that the "Restore Defaults" feature is not implemented yet. This could be useful for users who want to reset their settings.
Would you like me to help implement this feature or create a GitHub issue to track this task? I can provide an implementation that:
- Resets all settings to their default values from the
initstructure- Saves the default settings
- Reloads the browser as indicated by the existing code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/menu/menu.c(6 hunks)src/menu/settings.c(3 hunks)src/menu/settings.h(1 hunks)src/menu/views/settings_editor.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/menu/settings.h
🔇 Additional comments (8)
src/menu/menu.c (5)
46-48: LGTM: Good error handling for flashcart initialization
The error handling for flashcart initialization is well-implemented, properly setting the next mode based on the initialization result.
73-81: LGTM: Clean display initialization with configurable settings
The display initialization is well-structured with:
- Clear resolution configuration
- Proper use of interlacing settings
- Appropriate frame rate limiting
Line range hint 106-127: LGTM: Improved deinitialization sequence
The deinitialization sequence is well-ordered:
- UI components are freed before display closure
- Display is closed before flashcart deinitialization
- All resources are properly cleaned up
169-169: LGTM: Simplified frame handling
The removal of frame counter logic in favor of display_try_get() makes the code cleaner and more maintainable.
94-94: Verify sound effects with disk drive attachment
Given the PR objectives mention issues with background sound effects when a 64 Disk Drive is attached, we should verify this implementation.
src/menu/settings.c (1)
16-19: LGTM: Initialization structure updated with appropriate defaults
The new fields are properly initialized with sensible default values.
src/menu/views/settings_editor.c (2)
129-163: LGTM: Well-structured and informative UI layout
The UI text is well-organized with:
- Clear labeling and grouping of settings
- Visual indicators (*) for beta features
- Helpful explanatory notes for settings with special requirements
113-117: LGTM: Appropriate sound effect integration
Sound effects are well-integrated into the UI interactions:
- Different sounds for different actions (SFX_SETTING vs SFX_EXIT)
- Consistent with the PR's goal of enhancing sound feedback
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/menu/views/rtc.c (3)
30-34: Consider using consistent parameter types.The
wrapfunction mixesintanduint16_ttypes, which could lead to unexpected behavior with negative values or values > 65535.-int wrap( int val, uint16_t min, uint16_t max ) { +int wrap( int val, int min, int max ) {
76-76: Address the FIXME comment about UI improvements.The comment indicates that this UI component should be moved to ui_components.c once improved.
Would you like me to help create a GitHub issue to track this UI improvement task?
144-146: Track the settimeofday implementation.The FIXME comment indicates that settimeofday is not available in libdragon yet.
Would you like me to help create a GitHub issue to track the implementation of settimeofday support in libdragon?
src/menu/views/browser.c (1)
291-291: Update struct types to match new naming convention.While function calls have been updated to use the
ui_components_prefix, the struct types still use the oldcomponent_prefix:-static component_context_menu_t entry_context_menu = { +static ui_components_context_menu_t entry_context_menu = { -static component_context_menu_t settings_context_menu = { +static ui_components_context_menu_t settings_context_menu = { - COMPONENT_CONTEXT_MENU_LIST_END, + UI_COMPONENTS_CONTEXT_MENU_LIST_END,Also applies to: 295-295
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/menu/views/browser.c(5 hunks)src/menu/views/credits.c(4 hunks)src/menu/views/rtc.c(1 hunks)src/menu/views/system_info.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/menu/views/credits.c
- src/menu/views/system_info.c
🔇 Additional comments (5)
src/menu/views/rtc.c (3)
8-13: LGTM! Well-defined macros and constants.
The macros are properly parenthesized to prevent operator precedence issues, and the year range is appropriate for the N64 era.
15-28: LGTM! Well-structured enum and state management.
The enum clearly defines all editable RTC fields, and the static variables provide appropriate state management for the RTC editing functionality.
70-72:
Add error handling for mktime and gmtime.
Both mktime and gmtime can return error values that should be handled to prevent potential crashes.
- time_t timestamp = mktime( t );
- *t = *gmtime( ×tamp );
+ time_t timestamp = mktime(t);
+ if (timestamp == -1) {
+ // Handle error
+ return;
+ }
+ struct tm* result = gmtime(×tamp);
+ if (result == NULL) {
+ // Handle error
+ return;
+ }
+ *t = *result;Likely invalid or redundant comment.
src/menu/views/browser.c (2)
281-285: LGTM! Well-organized menu structure.
The menu items are logically ordered, with system-related settings grouped together.
307-307: LGTM! Consistent sound feedback implementation.
Sound effects are appropriately integrated for all user actions, providing good audio feedback:
- Cursor movement: SFX_CURSOR
- Selection: SFX_ENTER
- Back action: SFX_EXIT
- Settings/options: SFX_SETTING
Also applies to: 313-313, 319-319, 354-354, 357-357, 360-360
| char full_dt[30]; | ||
| char current_selection_chars[30]; | ||
|
|
||
| snprintf( full_dt, sizeof(full_dt), ">%04d|%02d|%02d|%02d|%02d|%02d< %s", | ||
| t.tm_year + 1900, | ||
| t.tm_mon + 1, | ||
| t.tm_mday, | ||
| t.tm_hour, | ||
| t.tm_min, | ||
| t.tm_sec, | ||
| DAYS_OF_WEEK[t.tm_wday] | ||
| ); |
There was a problem hiding this comment.
Add buffer overflow protection for snprintf.
The function should check the return value of snprintf to ensure the formatted string fits within the buffer.
- snprintf( full_dt, sizeof(full_dt), ">%04d|%02d|%02d|%02d|%02d|%02d< %s",
+ int written = snprintf(full_dt, sizeof(full_dt), ">%04d|%02d|%02d|%02d|%02d|%02d< %s",
t.tm_year + 1900,
t.tm_mon + 1,
t.tm_mday,
t.tm_hour,
t.tm_min,
t.tm_sec,
DAYS_OF_WEEK[t.tm_wday]
);
+ if (written < 0 || written >= sizeof(full_dt)) {
+ // Handle truncation
+ 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.
| char full_dt[30]; | |
| char current_selection_chars[30]; | |
| snprintf( full_dt, sizeof(full_dt), ">%04d|%02d|%02d|%02d|%02d|%02d< %s", | |
| t.tm_year + 1900, | |
| t.tm_mon + 1, | |
| t.tm_mday, | |
| t.tm_hour, | |
| t.tm_min, | |
| t.tm_sec, | |
| DAYS_OF_WEEK[t.tm_wday] | |
| ); | |
| char full_dt[30]; | |
| char current_selection_chars[30]; | |
| int written = snprintf(full_dt, sizeof(full_dt), ">%04d|%02d|%02d|%02d|%02d|%02d< %s", | |
| t.tm_year + 1900, | |
| t.tm_mon + 1, | |
| t.tm_mday, | |
| t.tm_hour, | |
| t.tm_min, | |
| t.tm_sec, | |
| DAYS_OF_WEEK[t.tm_wday] | |
| ); | |
| if (written < 0 || written >= sizeof(full_dt)) { | |
| // Handle truncation | |
| return; | |
| } |
Description
This PR merges
developandmainfor a tagged release so it can be used by most users (utilising debug flags where required).Release Notes
New Features
startto return to menu when autoload is enabled).config.ini.SMS,GG, andCHFROMs.Bug Fixes
Documentation
Refactor
Current known Issues
Breaking changes
Z|Linstead ofRto align with ROM info context menu (and future functionality).Motivation and Context
Merge the changes:
#110
#106
#101
#89
#74
and other improvements.
How Has This Been Tested?
Locally on an SC64.
Screenshots
Types of changes
Checklist:
Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores