Add live cheat control via MiSTer_cmd#7
Open
wizzomafizzo wants to merge 5 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds public cheat accessor functions (name, enabled, get/set selected), implements a name-based cheat command parser (clear/on/off/toggle) with helpers to resolve and toggle cheats, and wires the parser into MiSTer_cmd to handle "cheat ..." FIFO commands. ChangesCheat Command Control System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cheats.cpp`:
- Around line 463-579: Extract the entire new backend command-control logic
(functions cheats_find_by_name, cheats_external_load_entry,
cheats_external_set_enabled, cheats_set_enabled_by_name, cheats_toggle_by_name,
cheats_clear_enabled and any static helpers/locals they use) into a new module
(e.g., cheats_backend.cpp + cheats_backend.h), keep their signatures public in
the header, and move any static file-scope state they require into that new
module; then reduce cheats.cpp to a surgical hook: add a single `#include` of the
new header and replace the moved implementations with minimal forwarding calls
(or a single call/site that delegates to the new API) so cheats.cpp only
contains the hook/include while the full logic lives in the new files. Ensure
you preserve symbol names and behavior, and update compilation units to compile
the new files.
In `@cheats.h`:
- Line 21: cheats.h exposes APIs using uint32_t and bool (e.g., functions like
cheats_set_enabled_by_name and other prototypes in the header) but doesn't
include the headers that define those types; make the header self-contained by
adding the appropriate standard includes (stdint.h or inttypes.h for uint32_t
and stdbool.h for bool) at the top of cheats.h so any translation unit or C
consumer can include cheats.h without relying on include order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfdbad29-730d-427c-8a0c-3d3824292013
📒 Files selected for processing (5)
cheat_cmd.cppcheat_cmd.hcheats.cppcheats.hinput.cpp
Comment on lines
+463
to
+579
| static int cheats_find_by_name(const char *name) | ||
| { | ||
| if (!name || !*name) return -1; | ||
|
|
||
| for (int i = 0; i < cheats_available(); i++) | ||
| { | ||
| if (!strcmp(cheats[i].name, name)) return i; | ||
| } | ||
|
|
||
| return -1; | ||
| } | ||
|
|
||
| static int cheats_external_load_entry(int idx) | ||
| { | ||
| if (idx < 0 || idx >= cheats_available()) return CHEATS_CMD_NOT_FOUND; | ||
| if (cheats[idx].cheatData) return CHEATS_CMD_OK; | ||
| if (!cheat_zip[0]) return CHEATS_CMD_LOAD_FAILED; | ||
|
|
||
| static char filename[1024]; | ||
| fileTYPE f = {}; | ||
| snprintf(filename, sizeof(filename), "%s/%s", cheat_zip, cheats[idx].name); | ||
| if (!FileOpen(&f, filename)) | ||
| { | ||
| printf("Cannot open cheat file %s.\n", filename); | ||
| return CHEATS_CMD_LOAD_FAILED; | ||
| } | ||
|
|
||
| int res = CHEATS_CMD_LOAD_FAILED; | ||
| int len = f.size; | ||
| if (!len || (len % cheat_unit_size)) | ||
| { | ||
| printf("Cheat file %s has incorrect length %d -> skipping.\n", filename, len); | ||
| } | ||
| else if (((len / cheat_unit_size) + cheats_loaded()) <= cheat_max_active) | ||
| { | ||
| cheats[idx].cheatData = new char[len]; | ||
| if (cheats[idx].cheatData) | ||
| { | ||
| if (FileReadAdv(&f, cheats[idx].cheatData, len) == len) | ||
| { | ||
| cheats[idx].cheatSize = len; | ||
| res = CHEATS_CMD_OK; | ||
| } | ||
| else | ||
| { | ||
| printf("Cannot read cheat file %s.\n", filename); | ||
| delete[] cheats[idx].cheatData; | ||
| cheats[idx].cheatData = NULL; | ||
| cheats[idx].cheatSize = 0; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| printf("Could not allocate required memory (%d) for cheat file %s.\n", len, filename); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| printf("No more room in current selection for cheat file %s.\n", filename); | ||
| res = CHEATS_CMD_NO_ROOM; | ||
| } | ||
|
|
||
| FileClose(&f); | ||
| return res; | ||
| } | ||
|
|
||
| static int cheats_external_set_enabled(int idx, bool enabled) | ||
| { | ||
| if (!cheats_available()) return CHEATS_CMD_NO_CHEATS; | ||
| if (idx < 0 || idx >= cheats_available()) return CHEATS_CMD_NOT_FOUND; | ||
| if (cheats[idx].enabled == enabled) return CHEATS_CMD_OK; | ||
|
|
||
| if (enabled) | ||
| { | ||
| int res = cheats_external_load_entry(idx); | ||
| if (res != CHEATS_CMD_OK) return res; | ||
| if (((cheats[idx].cheatSize / cheat_unit_size) + cheats_loaded()) > cheat_max_active) return CHEATS_CMD_NO_ROOM; | ||
| } | ||
|
|
||
| cheats[idx].enabled = enabled; | ||
| cheats_send(); | ||
| return CHEATS_CMD_OK; | ||
| } | ||
|
|
||
| int cheats_set_enabled_by_name(const char *name, bool enabled) | ||
| { | ||
| if (!cheats_available()) return CHEATS_CMD_NO_CHEATS; | ||
| return cheats_external_set_enabled(cheats_find_by_name(name), enabled); | ||
| } | ||
|
|
||
| int cheats_toggle_by_name(const char *name) | ||
| { | ||
| if (!cheats_available()) return CHEATS_CMD_NO_CHEATS; | ||
|
|
||
| int idx = cheats_find_by_name(name); | ||
| if (idx < 0) return CHEATS_CMD_NOT_FOUND; | ||
|
|
||
| return cheats_external_set_enabled(idx, !cheats[idx].enabled); | ||
| } | ||
|
|
||
| int cheats_clear_enabled() | ||
| { | ||
| if (!cheats_available()) return CHEATS_CMD_NO_CHEATS; | ||
|
|
||
| bool changedCheats = false; | ||
| for (int i = 0; i < cheats_available(); i++) | ||
| { | ||
| if (cheats[i].enabled) | ||
| { | ||
| cheats[i].enabled = false; | ||
| changedCheats = true; | ||
| } | ||
| } | ||
|
|
||
| if (changedCheats) cheats_send(); | ||
| return CHEATS_CMD_OK; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move this new backend command-control block into a new file and keep this upstream file to a hook-level change.
Lines 463-579 introduce substantial new functionality directly in cheats.cpp. Please extract this logic into a new .cpp/.h pair and keep upstream cheats.cpp edits minimal (bridge/hook only).
As per coding guidelines, “Put new functionality in new .cpp/.h files wherever possible rather than modifying upstream source files…” and “When touching upstream source files, make only surgical edits: add a single #include, a single function call, or a single hook point.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cheats.cpp` around lines 463 - 579, Extract the entire new backend
command-control logic (functions cheats_find_by_name,
cheats_external_load_entry, cheats_external_set_enabled,
cheats_set_enabled_by_name, cheats_toggle_by_name, cheats_clear_enabled and any
static helpers/locals they use) into a new module (e.g., cheats_backend.cpp +
cheats_backend.h), keep their signatures public in the header, and move any
static file-scope state they require into that new module; then reduce
cheats.cpp to a surgical hook: add a single `#include` of the new header and
replace the moved implementations with minimal forwarding calls (or a single
call/site that delegates to the new API) so cheats.cpp only contains the
hook/include while the full logic lives in the new files. Ensure you preserve
symbol names and behavior, and update compilation units to compile the new
files.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
support/zaparoo/cheat_cmd.cpp (1)
54-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject extra arguments for
clearto avoid accidental mass-disable.
clearcurrently ignores trailing tokens, which can unintentionally clear all cheats on malformed input.Proposed fix
if (!strcmp(action, "clear")) { + const char *extra = skip_space(cmd); + if (*extra) + { + printf("MiSTer_cmd cheat clear: unexpected argument '%s'\n", extra); + return; + } int status = cheats_clear_enabled(); printf("MiSTer_cmd cheat clear: %s\n", cheat_cmd_status(status)); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@support/zaparoo/cheat_cmd.cpp` around lines 54 - 59, The "clear" branch currently ignores trailing tokens and can run on malformed input; inside the existing if (!strcmp(action, "clear")) block, validate that there are no extra arguments/tokens after action (e.g., check the next token/remaining input is empty or NULL) and if any extra token exists, print an error/usage message and return without calling cheats_clear_enabled(); keep using cheats_clear_enabled() and cheat_cmd_status(status) when the argument list is exactly "clear".
♻️ Duplicate comments (1)
cheats.cpp (1)
462-517: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove new cheat command backend logic out of upstream
cheats.cpp.Please extract this new command-control block into new files and keep
cheats.cppto a surgical hook/include only.As per coding guidelines, “Put new functionality in new
.cpp/.hfiles wherever possible rather than modifying upstream source files…” and “When touching upstream source files, make only surgical edits: add a single#include, a single function call, or a single hook point.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cheats.cpp` around lines 462 - 517, The block implementing cheats_find_by_name, cheats_toggle_by_name, cheats_set_enabled_by_name and cheats_clear_enabled should be extracted into a new backend pair (e.g., cheats_commands.h / cheats_commands.cpp): declare the four functions in the new header, move their implementations into the new .cpp (keeping references to existing symbols like cheats, cheats_available(), iSelectedEntry, cheats_toggle(), cheats_send(), and the CHEATS_CMD_* constants), and in cheats.cpp replace the moved implementations with a single `#include` of the new header (or thin forwarding stubs that call the new implementations) so cheats.cpp only contains a surgical include/hook and no new logic. Ensure build files are updated to compile the new .cpp and that function signatures remain identical so call sites are unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@support/zaparoo/cheat_cmd.cpp`:
- Around line 54-59: The "clear" branch currently ignores trailing tokens and
can run on malformed input; inside the existing if (!strcmp(action, "clear"))
block, validate that there are no extra arguments/tokens after action (e.g.,
check the next token/remaining input is empty or NULL) and if any extra token
exists, print an error/usage message and return without calling
cheats_clear_enabled(); keep using cheats_clear_enabled() and
cheat_cmd_status(status) when the argument list is exactly "clear".
---
Duplicate comments:
In `@cheats.cpp`:
- Around line 462-517: The block implementing cheats_find_by_name,
cheats_toggle_by_name, cheats_set_enabled_by_name and cheats_clear_enabled
should be extracted into a new backend pair (e.g., cheats_commands.h /
cheats_commands.cpp): declare the four functions in the new header, move their
implementations into the new .cpp (keeping references to existing symbols like
cheats, cheats_available(), iSelectedEntry, cheats_toggle(), cheats_send(), and
the CHEATS_CMD_* constants), and in cheats.cpp replace the moved implementations
with a single `#include` of the new header (or thin forwarding stubs that call the
new implementations) so cheats.cpp only contains a surgical include/hook and no
new logic. Ensure build files are updated to compile the new .cpp and that
function signatures remain identical so call sites are unaffected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 620db5bc-0aa7-4849-b3e1-3f4de42321b4
📒 Files selected for processing (4)
cheats.cppinput.cppsupport/zaparoo/cheat_cmd.cppsupport/zaparoo/cheat_cmd.h
🚧 Files skipped from review as they are similar to previous changes (1)
- input.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cheats.h (1)
1-4:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMake
cheats.hself-contained for exposed scalar types.Line 4 and Line 13 expose
uint32_t/boolbut the header does not include their defining headers. This is already surfacing as a Clang error (unknown type name 'bool').Proposed minimal fix
`#ifndef` CHEATS_H `#define` CHEATS_H + +#include <stdbool.h> +#include <stdint.h> void cheats_init(const char *rom_path, uint32_t romcrc);#!/bin/bash set -euo pipefail echo "== cheats.h top section ==" sed -n '1,40p' cheats.h | cat -n echo echo "== type usage and includes in cheats.h ==" rg -n '^\s*#\s*include\s*<stdbool\.h>|^\s*#\s*include\s*<stdint\.h>|\buint32_t\b|\bbool\b' cheats.h echo echo "== C translation units including cheats.h (if any) ==" rg -n --iglob '*.c' '#\s*include\s*"cheats\.h"' .Also applies to: 12-15
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cheats.h` around lines 1 - 4, The header cheats.h exposes scalar types uint32_t and bool (e.g., in the cheats_init prototype and other declarations) but does not include their defining headers; update cheats.h to be self-contained by adding the appropriate standard includes (stdint.h for uint32_t and stdbool.h for bool) near the top (inside the header guard) so that declarations like cheats_init(const char *rom_path, uint32_t romcrc) and any bool-typed symbols compile in translation units that include only cheats.h.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@cheats.h`:
- Around line 1-4: The header cheats.h exposes scalar types uint32_t and bool
(e.g., in the cheats_init prototype and other declarations) but does not include
their defining headers; update cheats.h to be self-contained by adding the
appropriate standard includes (stdint.h for uint32_t and stdbool.h for bool)
near the top (inside the header guard) so that declarations like
cheats_init(const char *rom_path, uint32_t romcrc) and any bool-typed symbols
compile in translation units that include only cheats.h.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b034275-715a-4ea4-9fb9-e21c49870d8c
📒 Files selected for processing (3)
cheats.cppcheats.hsupport/zaparoo/cheat_cmd.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- support/zaparoo/cheat_cmd.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/zaparoo/main_capabilities.cpp (1)
1-5: ⚡ Quick winDocument
VDATE’s source (it’s injected via MakefileDFLAGS)
VDATEis defined by the build system inMakefile(DFLAGS ... -DVDATE="\date +"%y%m%d"`"), so no#includeis required insupport/zaparoo/main_capabilities.cpp. Static analysis likely flags it because it runs without the same-D` flags—adding a short comment near the symbol would clarify this for future maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@support/zaparoo/main_capabilities.cpp` around lines 1 - 5, Add a short comment above the declaration of zaparoo_main_capabilities noting that VDATE is injected by the build system via Makefile DFLAGS (e.g. -DVDATE="`date +\"%y%m%d\"`") so static analysis or readers know no `#include/definition` is missing; reference the VDATE symbol in the comment and keep it concise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@support/zaparoo/main_capabilities.cpp`:
- Around line 1-5: Add a short comment above the declaration of
zaparoo_main_capabilities noting that VDATE is injected by the build system via
Makefile DFLAGS (e.g. -DVDATE="`date +\"%y%m%d\"`") so static analysis or
readers know no `#include/definition` is missing; reference the VDATE symbol in
the comment and keep it concise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9296e31-9c0a-4400-90e5-9f7d3b8f9914
📒 Files selected for processing (1)
support/zaparoo/main_capabilities.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cheatcommand handler for/dev/MiSTer_cmdCommands
Validation
source setup_default_toolchain.sh && make -j16Summary by CodeRabbit