From c02657c80428d31d05a3758e9c42f5647e7561ef Mon Sep 17 00:00:00 2001 From: David Hauf Date: Tue, 28 Jul 2026 12:57:02 -0400 Subject: [PATCH 1/7] cue: refuse split-track images instead of mounting a wrong disc A cue sheet that names one .bin per track cannot work here. The loader picks its data file by rewriting the cue's own extension, so it only ever opens the first one, and the parser cannot place a later file's tracks without knowing how long the earlier files are - every firmware caller uses the no-argument next_track(), which passes a previous size of zero. The arithmetic was bounded earlier so those tracks no longer land at LBAs in the billions. That stopped the nonsense but not the wrongness: the disc still mounted, with a TOC that put the wrong tracks in the wrong places. Refusing the image says what is actually true. Detection lives in cueutil.cpp and runs the real parser rather than scanning for the word FILE, so quoting, case and REM lines are read exactly as they will be when the sheet is used - a disc called "MY FILE (1996).bin" is not a split rip, and a rem'd-out FILE line is not a second file. It also lives there rather than in util.cpp so it can be tested: util.cpp is deliberately outside the host suite. Refusing is only an improvement if the user finds out, and until now they could not. Mounting is asynchronous: SetNextCDByName() queues an index and returns, so the web UI's "ok" only means the name was in the catalog. When the load failed afterwards, the old disc stayed mounted, current_cd went on pointing at it, and the sole record was one line in a log file the user has no reason to read - an unmountable image looked exactly like a mountable one. The service now keeps the reason for the last failed mount and the image-name endpoint the page already polls reports it. --- addon/cueparser/cueutil.cpp | 19 +++++ addon/cueparser/cueutil.h | 14 ++++ addon/discimage/util.cpp | 18 +++++ addon/scsitbservice/scsitbservice.cpp | 10 +++ addon/scsitbservice/scsitbservice.h | 12 +++ addon/webserver/handlers/imagenameapi.cpp | 10 +++ .../test-suite/test_cuequirks.cpp | 75 ++++++++++++++++++- 7 files changed, 156 insertions(+), 2 deletions(-) diff --git a/addon/cueparser/cueutil.cpp b/addon/cueparser/cueutil.cpp index 8f042645..d0284511 100644 --- a/addon/cueparser/cueutil.cpp +++ b/addon/cueparser/cueutil.cpp @@ -28,6 +28,25 @@ bool CueFindTrackForLBA(const char *cue_sheet, uint32_t lba, CUETrackInfo *out) return found; } +bool CueHasMultipleFiles(const char *cue_sheet) { + if (cue_sheet == nullptr) { + return false; + } + + CUEParser parser(cue_sheet); + const CUETrackInfo *trackInfo; + + while ((trackInfo = parser.next_track()) != nullptr) { + // file_index counts FILE lines as the parser walks them, so any track + // reporting the second or later file means the sheet is split. + if (trackInfo->file_index > 1) { + return true; + } + } + + return false; +} + uint64_t CueLBAToByteOffset(const char *cue_sheet, uint32_t lba) { CUETrackInfo track; if (!CueFindTrackForLBA(cue_sheet, lba, &track)) { diff --git a/addon/cueparser/cueutil.h b/addon/cueparser/cueutil.h index 8a7a185f..b60361cf 100644 --- a/addon/cueparser/cueutil.h +++ b/addon/cueparser/cueutil.h @@ -25,3 +25,17 @@ bool CueFindTrackForLBA(const char *cue_sheet, uint32_t lba, CUETrackInfo *out); // unstored PREGAP have no bytes in the file and clamp to the track's data // start. Falls back to lba * 2352 if the cue sheet is null or has no tracks. uint64_t CueLBAToByteOffset(const char *cue_sheet, uint32_t lba); + +// True if the cue sheet references more than one data file - a "split track" +// rip, where each track is its own .bin. +// +// USBODE cannot mount one. The loader opens the single file named after the +// cue and never looks at the FILE lines, and the parser cannot place tracks +// from a later file without knowing how long the earlier ones are. The result +// is a disc whose TOC is wrong rather than a disc that fails to load, so this +// exists to let the loader refuse the image outright. +// +// Uses the real parser rather than scanning for the word FILE, so quoting, +// comments and REM lines are treated exactly as they will be when the sheet is +// actually read. +bool CueHasMultipleFiles(const char *cue_sheet); diff --git a/addon/discimage/util.cpp b/addon/discimage/util.cpp index 52ddb9f0..5940815d 100644 --- a/addon/discimage/util.cpp +++ b/addon/discimage/util.cpp @@ -25,6 +25,8 @@ #include "mdsfile.h" #include "chdfile.h" +#include + LOGMODULE("discimage-util"); char tolower(char c) { @@ -251,6 +253,22 @@ IImageDevice* loadCueBinIsoFileDevice(const char* imagePath) { } LOGNOTE("Loaded CUE sheet"); + // A split-track rip names one .bin per track. Nothing below can honour + // that: the data file is chosen by rewriting the cue's own extension, + // so only the first file would ever be opened, and the parser cannot + // place the later tracks without knowing how long the earlier files + // are. The disc would mount with a TOC that is simply wrong, which is + // worse than not mounting - so refuse it and say why. + if (CueHasMultipleFiles(cue_str)) { + LOGERR("Cannot mount %s: the cue sheet references more than one data " + "file (a split-track rip). USBODE needs a single .bin; " + "re-rip the disc as one image or merge the tracks.", + fullPath); + delete imageFile; + delete[] cue_str; + return nullptr; + } + // Switch to BIN file for data change_extension_to_bin(fullPath); } diff --git a/addon/scsitbservice/scsitbservice.cpp b/addon/scsitbservice/scsitbservice.cpp index 09acde5a..dfdb64ab 100644 --- a/addon/scsitbservice/scsitbservice.cpp +++ b/addon/scsitbservice/scsitbservice.cpp @@ -469,11 +469,17 @@ void SCSITBService::ProcessPendingMount() { return; } + // Any failure below leaves the previous disc mounted, so the reason has to + // outlive this function or the user is told nothing at all. + m_LastMountError[0] = '\0'; + // Build full path using relativePath from cache const char* relativePath = m_FileEntries[next_cd].relativePath; // Ensure we have room for "1:/" prefix (3 chars) + relativePath + null terminator if (strlen(relativePath) > MAX_PATH_LEN - 4) { LOGERR("Path too long: %s", relativePath); + snprintf(m_LastMountError, sizeof(m_LastMountError), + "Path is too long to mount: %s", relativePath); next_cd = -1; return; } @@ -483,6 +489,10 @@ void SCSITBService::ProcessPendingMount() { if (imageDevice == nullptr) { LOGERR("Failed to load image: %s", m_CurrentImagePath); + snprintf(m_LastMountError, sizeof(m_LastMountError), + "Could not load %s. It may be an unsupported or damaged image; " + "the log has the details. The previous disc is still mounted.", + relativePath); next_cd = -1; return; } diff --git a/addon/scsitbservice/scsitbservice.h b/addon/scsitbservice/scsitbservice.h index f0830290..5eaf52a2 100644 --- a/addon/scsitbservice/scsitbservice.h +++ b/addon/scsitbservice/scsitbservice.h @@ -56,6 +56,15 @@ class SCSITBService : public CTask void SetPendingInsert(); bool IsEjected() const; // delegates to cdromservice + /// Why the last mount attempt failed, or "" if the last one worked. + /// + /// Mounting is asynchronous: SetNextCDByName() only queues an index, so + /// the web UI's "ok" says the name was found in the catalog, not that the + /// image loaded. When the load then fails the old disc stays mounted and + /// the only record is a line in the log, which is how an image that cannot + /// be mounted looks to the user exactly like one that can. + const char* GetLastMountError() const { return m_LastMountError; } + // Task entry point void Run(void); @@ -81,6 +90,9 @@ class SCSITBService : public CTask bool m_bBootEjectPending = false; bool m_bPersistedEjected = false; + // Set by ProcessPendingMount() on every failure path, cleared on success. + char m_LastMountError[160] = {0}; + // Full path of currently mounted image (e.g., "1:/Games/game.iso") char m_CurrentImagePath[MAX_PATH_LEN]; diff --git a/addon/webserver/handlers/imagenameapi.cpp b/addon/webserver/handlers/imagenameapi.cpp index 072b3b84..965f36e8 100644 --- a/addon/webserver/handlers/imagenameapi.cpp +++ b/addon/webserver/handlers/imagenameapi.cpp @@ -26,6 +26,16 @@ THTTPStatus ImageNameAPIHandler::GetJson(nlohmann::json& j, j = { {"name", svc->GetCurrentCDName()} }; + + // Mounting happens on the service task well after the mount request was + // answered "ok", so a failure has no other way back to the user. The page + // already polls this endpoint for the current image name; a disc that + // refused to mount shows up here as the reason the name did not change. + const char* mountError = svc->GetLastMountError(); + if (mountError != nullptr && mountError[0] != '\0') { + j["mount_error"] = mountError; + } + return HTTPOK; } diff --git a/integration-tests/test-suite/test_cuequirks.cpp b/integration-tests/test-suite/test_cuequirks.cpp index c40102ea..f205efa1 100644 --- a/integration-tests/test-suite/test_cuequirks.cpp +++ b/integration-tests/test-suite/test_cuequirks.cpp @@ -19,6 +19,7 @@ #include "framework.h" #include +#include #include #include @@ -224,8 +225,12 @@ TEST(cue_unstored_pregap_shifts_following_tracks) // // The parser cannot place these tracks correctly without the real file sizes, // and the loader does not have them (it ignores the FILE names and always -// opens the bin named after the cue). So the guarantee here is the one that -// can be met today: every track lands somewhere sane on the disc. +// opens the bin named after the cue). Bounding the arithmetic keeps the +// addresses sane, but sane is not correct - so the loader refuses these images +// outright rather than mounting a disc whose TOC is wrong. See +// multifile_cue_is_detected_so_the_loader_can_refuse_it below; this test still +// covers the parser, which has to survive the sheet either way because the +// same parser reads it to make that decision. TEST(multifile_cue_does_not_underflow_into_a_nonsense_lba) { const char *cue = @@ -262,3 +267,69 @@ TEST(multifile_cue_does_not_underflow_into_a_nonsense_lba) CHECK_EQ(third->track_number, 3); CHECK_EQ(third->track_start, 150u); } + +// The check the loader makes before it will open anything. A split-track rip +// has to be recognised from the cue sheet alone, because by the time the disc +// is mounted the damage is a wrong TOC rather than a failure. +// +// The negative cases matter as much as the positive one: refusing an ordinary +// single-file cue would make perfectly good images unmountable, and these are +// the shapes that could fool a scan for the word FILE - a filename containing +// it, a REM line mentioning it, and a commented-out second FILE. +TEST(multifile_cue_is_detected_so_the_loader_can_refuse_it) +{ + const char *split = + "FILE \"Game (Track 1).bin\" BINARY\n" + " TRACK 01 MODE1/2352\n" + " INDEX 01 00:00:00\n" + "FILE \"Game (Track 2).bin\" BINARY\n" + " TRACK 02 AUDIO\n" + " INDEX 01 00:00:00\n"; + CHECK(CueHasMultipleFiles(split)); + + const char *single = + "FILE \"Game.bin\" BINARY\n" + " TRACK 01 MODE1/2352\n" + " INDEX 01 00:00:00\n" + " TRACK 02 AUDIO\n" + " INDEX 01 04:12:33\n" + " TRACK 03 AUDIO\n" + " INDEX 01 08:24:66\n"; + CHECK(!CueHasMultipleFiles(single)); + + // "FILE" inside the data file's own name. + const char *namedFile = + "FILE \"MY FILE (1996).bin\" BINARY\n" + " TRACK 01 MODE1/2352\n" + " INDEX 01 00:00:00\n"; + CHECK(!CueHasMultipleFiles(namedFile)); + + // Metadata that mentions a file, and a rem'd-out second FILE line - the + // kind of leftover a ripper or a hand edit produces. + const char *remmed = + "REM ORIGINAL FILE Game.iso\n" + "FILE \"Game.bin\" BINARY\n" + " TRACK 01 MODE1/2352\n" + " INDEX 01 00:00:00\n" + "REM FILE \"Game (Track 2).bin\" BINARY\n"; + CHECK(!CueHasMultipleFiles(remmed)); + + // A sheet with no tracks at all must not be reported as split; it fails + // later, for its own reason. + CHECK(!CueHasMultipleFiles("")); + CHECK(!CueHasMultipleFiles(nullptr)); + + // Three files, and lowercase - keywords are case-insensitive everywhere + // else in the parser and have to be here too. + const char *lower = + "file \"a.bin\" binary\n" + " track 01 audio\n" + " index 01 00:00:00\n" + "file \"b.bin\" binary\n" + " track 02 audio\n" + " index 01 00:00:00\n" + "file \"c.bin\" binary\n" + " track 03 audio\n" + " index 01 00:00:00\n"; + CHECK(CueHasMultipleFiles(lower)); +} From 82f14ad90cccad9dcc0be58bd6553f18e66077c0 Mon Sep 17 00:00:00 2001 From: David Hauf Date: Tue, 28 Jul 2026 14:42:30 -0400 Subject: [PATCH 2/7] mount: show the user why an image would not mount, and stop hiding split cues David put a split-track rip on the card and the browser offered him the individual track .bin files while the cue sheet itself was nowhere to be seen. Three separate things were wrong. The scanner listed a .cue only when a same-stem .bin sat next to it. That rule was written to keep a cue whose data file is missing out of the way, but a split-track rip is exactly the case where the stems differ - "Game.cue" against "Game (Track 1).bin" - so the disc vanished from the browser and its raw tracks were offered instead. Every cue is now listed; one that cannot be mounted says why when you try, which is more use than not being there. Nothing displayed the mount error. The previous commit gave SCSITBService a reason and put it in the /api/imagename JSON, which no page consumes - so the information existed and no user could reach it. Same mistake as reporting a bad log path only to the serial console. It is now a banner on every page, from the shared page handler. And the reason was generic. The loaders all report failure the same way, by returning nullptr, so "unsupported or damaged image" was the best the UI could say about a split-track cue, a missing .bin, a bad .mds and an unknown extension alike. They now record what actually went wrong and the mount error repeats it: a split rip says it is a split rip and what to do about it. --- addon/discimage/util.cpp | 32 ++++++++++++++++++++ addon/discimage/util.h | 10 ++++++ addon/scsitbservice/scsitbservice.cpp | 30 +++++++++++++----- addon/webserver/handlers/pagehandlerbase.cpp | 11 +++++++ addon/webserver/pages/template.html | 6 ++++ 5 files changed, 82 insertions(+), 7 deletions(-) diff --git a/addon/discimage/util.cpp b/addon/discimage/util.cpp index 5940815d..555e2b4a 100644 --- a/addon/discimage/util.cpp +++ b/addon/discimage/util.cpp @@ -26,9 +26,30 @@ #include "chdfile.h" #include +#include LOGMODULE("discimage-util"); +// Reason the last load failed. Every loader here reports failure the same way +// - by returning nullptr - so without somewhere to put the reason it only ever +// reached the log. +static char s_LastImageLoadError[192] = {0}; + +const char* GetLastImageLoadError() { + return s_LastImageLoadError; +} + +static void ClearImageLoadError() { + s_LastImageLoadError[0] = '\0'; +} + +static void SetImageLoadError(const char* format, ...) { + va_list args; + va_start(args, format); + vsnprintf(s_LastImageLoadError, sizeof(s_LastImageLoadError), format, args); + va_end(args); +} + char tolower(char c) { if (c >= 'A' && c <= 'Z') return c + ('a' - 'A'); @@ -202,6 +223,7 @@ IImageDevice* loadMDSFileDevice(const char* imagePath) { size_t mds_size = 0; if (!ReadFileToString(fullPath, &mds_str, &mds_size)) { LOGERR("Failed to read MDS file: %s", fullPath); + SetImageLoadError("Could not read the .mds file. It may be unreadable or too large."); return nullptr; } @@ -209,6 +231,7 @@ IImageDevice* loadMDSFileDevice(const char* imagePath) { CMDSFileDevice* mdsDevice = new CMDSFileDevice(fullPath, mds_str, mds_size, mediaType); if (!mdsDevice->Init()) { LOGERR("Failed to initialize MDS device: %s", imagePath); + SetImageLoadError("Not a valid Alcohol 120%% image, or its .mdf data file is missing."); delete mdsDevice; return nullptr; } @@ -248,6 +271,7 @@ IImageDevice* loadCueBinIsoFileDevice(const char* imagePath) { LOGNOTE("Loading CUE sheet from: %s", fullPath); if (!ReadFileToString(fullPath, &cue_str)) { LOGERR("Failed to read CUE file: %s", fullPath); + SetImageLoadError("Could not read the cue sheet for this image."); delete imageFile; return nullptr; } @@ -264,6 +288,9 @@ IImageDevice* loadCueBinIsoFileDevice(const char* imagePath) { "file (a split-track rip). USBODE needs a single .bin; " "re-rip the disc as one image or merge the tracks.", fullPath); + SetImageLoadError("This is a split-track rip: the cue sheet points at one .bin " + "per track. USBODE needs a single .bin, so re-rip the disc as " + "one image or merge the tracks."); delete imageFile; delete[] cue_str; return nullptr; @@ -278,6 +305,7 @@ IImageDevice* loadCueBinIsoFileDevice(const char* imagePath) { FRESULT result = f_open(imageFile, fullPath, FA_READ); if (result != FR_OK) { LOGERR("Cannot open data file for reading: %s (error %d)", fullPath, result); + SetImageLoadError("The data file this image needs is missing: %s", fullPath); delete imageFile; if (cue_str) delete[] cue_str; return nullptr; @@ -311,6 +339,7 @@ IImageDevice* loadCHDFileDevice(const char* imagePath) { CCHDFileDevice* chdDevice = new CCHDFileDevice(fullPath, mediaType); if (!chdDevice->Init()) { LOGERR("Failed to initialize CHD device: %s", imagePath); + SetImageLoadError("Not a valid CHD image, or it uses an unsupported compression."); delete chdDevice; return nullptr; } @@ -396,6 +425,8 @@ IImageDevice* loadImageDevice(const char* imagePath) { // imagePath is a full path like "1:/Games/game.iso" LOGNOTE("loadImageDevice called for: %s", imagePath); + ClearImageLoadError(); + if (hasMdsExtension(imagePath)) { LOGNOTE("Detected MDS format - using MDS plugin"); return loadMDSFileDevice(imagePath); @@ -410,6 +441,7 @@ IImageDevice* loadImageDevice(const char* imagePath) { } else { LOGERR("Unknown file format: %s", imagePath); + SetImageLoadError("Unsupported file type. USBODE mounts .iso, .cue/.bin, .chd, .mds and .toast images."); return nullptr; } } \ No newline at end of file diff --git a/addon/discimage/util.h b/addon/discimage/util.h index 41a60378..07279cba 100644 --- a/addon/discimage/util.h +++ b/addon/discimage/util.h @@ -24,6 +24,16 @@ bool hasDvdHint(const char* imageName); // Image loading - returns base IImageDevice interface IImageDevice* loadImageDevice(const char* imageName); +/// Why the last loadImageDevice() call returned nullptr, in words a user can +/// act on, or "" if the last one succeeded. +/// +/// The loaders only ever returned nullptr, so the reason existed solely in the +/// log - which meant a mount that failed for a specific, fixable reason (a +/// split-track cue, a missing .bin) was reported to the user, if at all, as a +/// generic failure. Written by the loaders, read by SCSITBService when it +/// records why a mount did not happen. +const char* GetLastImageLoadError(); + // Format-specific loaders IImageDevice* loadMDSFileDevice(const char* imageName); IImageDevice* loadCueBinIsoFileDevice(const char* imageName); diff --git a/addon/scsitbservice/scsitbservice.cpp b/addon/scsitbservice/scsitbservice.cpp index dfdb64ab..16af91d7 100644 --- a/addon/scsitbservice/scsitbservice.cpp +++ b/addon/scsitbservice/scsitbservice.cpp @@ -339,9 +339,16 @@ void SCSITBService::ScanDirectoryRecursive(const char* fullPath, const char* rel bool listIt = iequals(ext, ".iso") || iequals(ext, ".mds") || iequals(ext, ".chd") || iequals(ext, ".toast"); if (!listIt && iequals(ext, ".cue")) { - // Mounting a .cue opens the same-stem .bin, so only list - // cue sheets whose data file is actually there - listIt = siblingWithExtExists(fullPath, fno.fname, ".bin"); + // List every cue sheet, including one with no same-stem + // .bin next to it. Hiding those was meant to keep a cue + // whose data file is missing out of the way, but it also + // hid split-track rips - where the cue is "Game.cue" and + // the data is "Game (Track 1).bin" - so the browser + // offered the raw track files and the disc itself simply + // vanished with no explanation. A cue that cannot be + // mounted now says why when you try, which is more use + // than not being there. + listIt = true; } else if (!listIt && iequals(ext, ".bin")) { // Hide the raw .bin of a cue/bin pair; its .cue is listed listIt = !siblingWithExtExists(fullPath, fno.fname, ".cue"); @@ -489,10 +496,19 @@ void SCSITBService::ProcessPendingMount() { if (imageDevice == nullptr) { LOGERR("Failed to load image: %s", m_CurrentImagePath); - snprintf(m_LastMountError, sizeof(m_LastMountError), - "Could not load %s. It may be an unsupported or damaged image; " - "the log has the details. The previous disc is still mounted.", - relativePath); + // Prefer the loader's own reason. "Unsupported or damaged" is true of + // every failure and useful for none of them, so it is only the + // fallback for a path that has not been given words yet. + const char* why = GetLastImageLoadError(); + if (why != nullptr && why[0] != '\0') { + snprintf(m_LastMountError, sizeof(m_LastMountError), + "%s (%s) The previous disc is still mounted.", why, relativePath); + } else { + snprintf(m_LastMountError, sizeof(m_LastMountError), + "Could not load %s. It may be an unsupported or damaged image; " + "the log has the details. The previous disc is still mounted.", + relativePath); + } next_cd = -1; return; } diff --git a/addon/webserver/handlers/pagehandlerbase.cpp b/addon/webserver/handlers/pagehandlerbase.cpp index ca046ebe..31e397ee 100644 --- a/addon/webserver/handlers/pagehandlerbase.cpp +++ b/addon/webserver/handlers/pagehandlerbase.cpp @@ -64,6 +64,17 @@ THTTPStatus PageHandlerBase::GetContent(const char *pPath, // Get current loaded image std::string current_image = svc->GetCurrentCDName(); + // If the last mount attempt failed, say so on every page. Mounting is + // asynchronous - the request is answered "ok" as soon as the name is + // found in the catalog - so without this the UI reports success for an + // image that never loaded and goes on showing the previous disc as + // though nothing happened. Set only when non-empty, so the template + // section stays falsy the rest of the time. + const char* mountError = svc->GetLastMountError(); + if (mountError != nullptr && mountError[0] != '\0') { + context.set("mount_error", std::string(mountError)); + } + // Get our config service ConfigService* config = static_cast(CScheduler::Get()->GetTask("configservice")); diff --git a/addon/webserver/pages/template.html b/addon/webserver/pages/template.html index bac2dd14..d16af1f7 100644 --- a/addon/webserver/pages/template.html +++ b/addon/webserver/pages/template.html @@ -19,6 +19,12 @@

USB Optical Drive Emulator

+ {{#mount_error}} +
+ Last mount failed: {{mount_error}} +
+ {{/mount_error}} + {{#cdrom}} {{>content}} {{/cdrom}} From 66cda16bf294e92f33be5c4e16a835346a497887 Mon Sep 17 00:00:00 2001 From: David Hauf Date: Tue, 28 Jul 2026 14:52:58 -0400 Subject: [PATCH 3/7] browser: stop listing .bin files, which were never mountable The rule was "list a .bin unless a same-stem .cue exists", which is exactly backwards. Mounting a .bin rewrites its extension and reads the .cue first, so a .bin whose cue is missing cannot be mounted at all - and that is precisely the set the browser was offering. The ones that could be mounted, through their cue, were the ones it hid. Nobody noticed while stems matched, because the cue was listed alongside and the hidden .bin was the right thing to hide. A split-track rip broke the symmetry: "Game.cue" against "Game (Track 1).bin" share no stem, so the browser filled up with track files, every one of them a dead end. A .bin is now never listed. Either its cue is there, and that cue represents the disc, or it is not, and there is nothing to mount. Drops siblingWithExtExists(), which has no callers left. --- addon/scsitbservice/scsitbservice.cpp | 31 +++++++++------------------ 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/addon/scsitbservice/scsitbservice.cpp b/addon/scsitbservice/scsitbservice.cpp index 16af91d7..1352b322 100644 --- a/addon/scsitbservice/scsitbservice.cpp +++ b/addon/scsitbservice/scsitbservice.cpp @@ -51,24 +51,6 @@ static bool iequals(const char* a, const char* b) { return *a == *b; } -// True if a file with the same stem but a different extension exists next to -// fileName in dirFullPath (e.g. "game.bin" + ".cue" -> "1:/Games/game.cue"). -// FAT name matching is case-insensitive, so GAME.CUE is found too. -static bool siblingWithExtExists(const char* dirFullPath, const char* fileName, const char* newExt) { - const char* dot = strrchr(fileName, '.'); - if (dot == nullptr) - return false; - - char sibling[MAX_PATH_LEN]; - int n = snprintf(sibling, sizeof(sibling), "%s/%.*s%s", - dirFullPath, (int)(dot - fileName), fileName, newExt); - if (n < 0 || (size_t)n >= sizeof(sibling)) - return false; - - FILINFO fi; - return f_stat(sibling, &fi) == FR_OK && !(fi.fattrib & AM_DIR); -} - int compareFileEntries(const void* a, const void* b) { const FileEntry* fa = (const FileEntry*)a; const FileEntry* fb = (const FileEntry*)b; @@ -349,10 +331,17 @@ void SCSITBService::ScanDirectoryRecursive(const char* fullPath, const char* rel // mounted now says why when you try, which is more use // than not being there. listIt = true; - } else if (!listIt && iequals(ext, ".bin")) { - // Hide the raw .bin of a cue/bin pair; its .cue is listed - listIt = !siblingWithExtExists(fullPath, fno.fname, ".cue"); } + // A .bin is never listed. Mounting one rewrites its extension + // and reads the .cue first, so a .bin whose cue is missing + // cannot be mounted at all, and one whose cue is present is + // already represented by that cue. + // + // The rule here used to be "list it unless a same-stem .cue + // exists", which is precisely backwards: it hid every .bin + // that could be mounted and offered every one that could not. + // On a split-track rip that meant a browser full of track + // files, none of them mountable. if (listIt) { size_t len = my_strnlen(fno.fname, MAX_FILENAME_LEN - 1); memcpy(m_FileEntries[m_FileCount].name, fno.fname, len); From 950d3ebb27f10777065c8f3f67c178481e6e7aa4 Mon Sep 17 00:00:00 2001 From: David Hauf Date: Tue, 28 Jul 2026 15:01:28 -0400 Subject: [PATCH 4/7] mount: stop reporting a disc that failed to load as the mounted one David mounted a split-track cue, got the refusal banner, and the page went on to say "Current File Loaded: Alien Trilogy (USA).cue" with that same file marked (Current) in the list. The host had never been given it - SetDevice() is not reached on a failed load - so the UI was reporting a disc that does not exist. Three causes, all of which the new error banner made visible rather than created. m_CurrentImagePath was filled in before the load was attempted and never put back when it failed, so a refused image immediately became the "current" path and its folder the current folder. current_cd is an index into the file list, and RefreshCache rebuilds that list without revisiting it. Any rescan that changes the ordering silently repoints it at a different file - and hiding .bin files, one commit ago, shifted every index on a card with cue/bin pairs. The mounted disc is now remembered by path and the index re-derived from it after each scan, which also means the UI reports nothing as current when the mounted file has genuinely gone away. The pick-something-to-mount fallback chose the first image in the list with no memory of what had just failed, so on a card whose only image is unmountable it retried the same one on every upload, delete and FTP change, re-raising the banner each time. Also widens the error buffer: the split-track message was being cut off mid-word at 160 characters. Re-deriving the index has a consequence that has to be handled in the same breath. current_cd is an int that is legitimately -1 when nothing is mounted; GetCurrentCD() returns it as size_t, so -1 becomes SIZE_MAX and GetName() answers an out-of-range index with nullptr. Both callers used that pointer where null is undefined - pagehandlerbase constructs a std::string from it on every page the web server serves, and the image-name API hands it to nlohmann's JSON. That was survivable only while current_cd was set once at the first successful mount and never cleared, so outside a narrow window at boot it was always valid. Re-deriving it makes "nothing is mounted" a state the UI can actually reach: an ejected drive that has not mounted anything this session, or a mounted file that is no longer in the list. GetCurrentCDName() now returns "" and both callers are guarded. Without this the device froze hard enough to need a power cycle after swapping to and from an image that refuses to mount. --- addon/scsitbservice/scsitbservice.cpp | 76 +++++++++++++++++--- addon/scsitbservice/scsitbservice.h | 16 ++++- addon/webserver/handlers/imagenameapi.cpp | 4 +- addon/webserver/handlers/pagehandlerbase.cpp | 7 +- 4 files changed, 89 insertions(+), 14 deletions(-) diff --git a/addon/scsitbservice/scsitbservice.cpp b/addon/scsitbservice/scsitbservice.cpp index 1352b322..22c3cf21 100644 --- a/addon/scsitbservice/scsitbservice.cpp +++ b/addon/scsitbservice/scsitbservice.cpp @@ -228,7 +228,17 @@ bool SCSITBService::IsEjected() const { } const char* SCSITBService::GetCurrentCDName() { - return GetName(GetCurrentCD()); + // Never nullptr. current_cd is an int that can legitimately be -1 (nothing + // mounted), GetCurrentCD() hands that back as a size_t - so SIZE_MAX - and + // GetName() answers an out-of-range index with nullptr. Callers feed the + // result straight into std::string and into JSON, both of which are + // undefined on a null pointer, and this runs on every web page load. + // + // That was survivable only while current_cd was set once at boot and never + // cleared. It is now re-derived after each rescan, so "nothing is mounted" + // became a state the UI can actually reach. + const char* name = GetName(GetCurrentCD()); + return name != nullptr ? name : ""; } bool SCSITBService::SetNextCDByName(const char* file_name) { @@ -393,6 +403,29 @@ bool SCSITBService::RefreshCache() { LOGNOTE("SCSITBService::RefreshCache() Found %d total entries", (int)m_FileCount); + // current_cd is an index into the list that was just rebuilt, so on its own + // it means nothing afterwards: entries move, and an index that used to name + // the mounted disc can end up naming some other file entirely. Re-derive it + // from the path that actually got mounted, and admit to nothing being + // current when that file is no longer there. + { + int resolved = -1; + if (m_MountedRelativePath[0] != '\0') { + for (size_t i = 0; i < m_FileCount; ++i) { + if (!m_FileEntries[i].isDirectory && + strcmp(m_FileEntries[i].relativePath, m_MountedRelativePath) == 0) { + resolved = (int)i; + break; + } + } + } + if (resolved != current_cd) { + LOGNOTE("SCSITBService::RefreshCache() current index %d -> %d after rescan", + current_cd, resolved); + } + current_cd = resolved; + } + // Find the current image in cache by matching relative path const char* searchPath = current_image; bool found = false; @@ -439,12 +472,21 @@ bool SCSITBService::RefreshCache() { // then persist "inserted" over the saved state. if (!found && m_FileCount > 0 && !IsEjected()) { for (size_t i = 0; i < m_FileCount; ++i) { - if (!m_FileEntries[i].isDirectory) { - LOGNOTE("SCSITBService::RefreshCache() Current image not found, using: %s", - m_FileEntries[i].relativePath); - next_cd = i; - break; + if (m_FileEntries[i].isDirectory) { + continue; } + // Skip the one we already know will not load. A rescan happens on + // every upload, delete and FTP change, so without this the same + // image is picked and fails again each time, and the error banner + // reappears for a disc the user never asked for. + if (m_LastFailedRelativePath[0] != '\0' && + strcmp(m_FileEntries[i].relativePath, m_LastFailedRelativePath) == 0) { + continue; + } + LOGNOTE("SCSITBService::RefreshCache() Current image not found, using: %s", + m_FileEntries[i].relativePath); + next_cd = i; + break; } } @@ -479,12 +521,19 @@ void SCSITBService::ProcessPendingMount() { next_cd = -1; return; } - snprintf(m_CurrentImagePath, sizeof(m_CurrentImagePath), "1:/%s", relativePath); + // Build the path locally. Writing it straight into m_CurrentImagePath + // before the load meant a failed mount renamed the "current" image to the + // one that had just refused to load, so the UI reported a disc the host + // had never been given. + char candidatePath[MAX_PATH_LEN]; + snprintf(candidatePath, sizeof(candidatePath), "1:/%s", relativePath); - IImageDevice* imageDevice = loadImageDevice(m_CurrentImagePath); + IImageDevice* imageDevice = loadImageDevice(candidatePath); if (imageDevice == nullptr) { - LOGERR("Failed to load image: %s", m_CurrentImagePath); + LOGERR("Failed to load image: %s", candidatePath); + strncpy(m_LastFailedRelativePath, relativePath, sizeof(m_LastFailedRelativePath) - 1); + m_LastFailedRelativePath[sizeof(m_LastFailedRelativePath) - 1] = '\0'; // Prefer the loader's own reason. "Unsupported or damaged" is true of // every failure and useful for none of them, so it is only the // fallback for a path that has not been given words yet. @@ -503,12 +552,19 @@ void SCSITBService::ProcessPendingMount() { } LOGNOTE("Loaded image: %s (format: %d, has subchannels: %s)", - m_CurrentImagePath, + candidatePath, (int)imageDevice->GetFileType(), imageDevice->HasSubchannelData() ? "yes" : "no"); cdromservice->SetDevice(imageDevice); + // Committed only now that the disc is really the one the host has. + strncpy(m_CurrentImagePath, candidatePath, sizeof(m_CurrentImagePath) - 1); + m_CurrentImagePath[sizeof(m_CurrentImagePath) - 1] = '\0'; + strncpy(m_MountedRelativePath, relativePath, sizeof(m_MountedRelativePath) - 1); + m_MountedRelativePath[sizeof(m_MountedRelativePath) - 1] = '\0'; + m_LastFailedRelativePath[0] = '\0'; + // Save relative path to config (without "1:/" prefix) configservice->SetCurrentImage(relativePath); diff --git a/addon/scsitbservice/scsitbservice.h b/addon/scsitbservice/scsitbservice.h index 5eaf52a2..237cc954 100644 --- a/addon/scsitbservice/scsitbservice.h +++ b/addon/scsitbservice/scsitbservice.h @@ -91,7 +91,21 @@ class SCSITBService : public CTask bool m_bPersistedEjected = false; // Set by ProcessPendingMount() on every failure path, cleared on success. - char m_LastMountError[160] = {0}; + // Wide enough for the loader's own wording plus the file name; at 160 the + // split-track message was cut off mid-word on screen. + char m_LastMountError[320] = {0}; + + // Relative path of the image that is ACTUALLY mounted, written only after + // a load succeeds. current_cd is an index into a list that RefreshCache + // rebuilds, so it cannot survive a rescan on its own - hiding .bin files + // shifted every index and left "Current File Loaded" naming a file that + // had never been mounted. This is what current_cd is re-derived from. + char m_MountedRelativePath[MAX_PATH_LEN] = {0}; + + // Relative path of the last image that failed to load, so the + // pick-something fallback in RefreshCache does not keep choosing it and + // failing again on every rescan. + char m_LastFailedRelativePath[MAX_PATH_LEN] = {0}; // Full path of currently mounted image (e.g., "1:/Games/game.iso") char m_CurrentImagePath[MAX_PATH_LEN]; diff --git a/addon/webserver/handlers/imagenameapi.cpp b/addon/webserver/handlers/imagenameapi.cpp index 965f36e8..d8954138 100644 --- a/addon/webserver/handlers/imagenameapi.cpp +++ b/addon/webserver/handlers/imagenameapi.cpp @@ -23,8 +23,10 @@ THTTPStatus ImageNameAPIHandler::GetJson(nlohmann::json& j, return HTTPInternalServerError; } + // Defensive: nlohmann's string construction is undefined on a null char*. + const char* name = svc->GetCurrentCDName(); j = { - {"name", svc->GetCurrentCDName()} + {"name", name != nullptr ? name : ""} }; // Mounting happens on the service task well after the mount request was diff --git a/addon/webserver/handlers/pagehandlerbase.cpp b/addon/webserver/handlers/pagehandlerbase.cpp index 31e397ee..01b7116d 100644 --- a/addon/webserver/handlers/pagehandlerbase.cpp +++ b/addon/webserver/handlers/pagehandlerbase.cpp @@ -61,8 +61,11 @@ THTTPStatus PageHandlerBase::GetContent(const char *pPath, if (!svc) return HTTPInternalServerError; - // Get current loaded image - std::string current_image = svc->GetCurrentCDName(); + // Get current loaded image. Constructing a std::string from a null + // char* is undefined, and this runs for every page the server serves, + // so it is checked here as well as at the source. + const char* current_image_name = svc->GetCurrentCDName(); + std::string current_image = current_image_name != nullptr ? current_image_name : ""; // If the last mount attempt failed, say so on every page. Mounting is // asynchronous - the request is answered "ok" as soon as the name is From 2dd27c88f54019523be72f4f973d2efe5e8e83fb Mon Sep 17 00:00:00 2001 From: David Hauf Date: Tue, 28 Jul 2026 15:44:27 -0400 Subject: [PATCH 5/7] mount: report what the mount did, not that the filename was found The web UI announced "Successfully mounted: Alien Trilogy (USA).cue" and then the very next page said the image needed re-ripping. Both were generated from the same request. Mounting is asynchronous - SetNextCDByName() hands an index to the service task and returns as soon as it has found the name in the catalogue - so the mount page was reporting that the file exists, in the words of a successful mount. SCSITBService now counts the requests its Run() loop retires, so a caller can wait for a specific one and find out what happened to it. MountByNameAndWait() does that and answers Success, Failed, NotFound or Timeout; the timeout is deliberately not called a failure, because a large CHD on a slow card takes a while and guessing wrong there would be worse than saying "still loading". The mount page now shows the refusal and its reason and stays put instead of bouncing to the homepage, and the mount API returns the reason rather than {"status":"ok"}. The HAT had the same problem in a worse form: it jumped to the homepage on any key press, so a disc that refused to load was indistinguishable from one the user had changed their mind about - the previously mounted image simply reappeared as current. It now waits, stays on the image list when the mount fails, and says so in the page title. Short, because that is all an OLED header holds; the reason is in the web UI and the log. The HAT cannot wait the obvious way, and this is the one thing to be careful of here. OnButtonPress does not run in task context on every display: PageManager::HandleButtonPress says so in a comment, and sh1106/display.cpp:299 and st7789/display.cpp:425 both call it straight from the GPIO interrupt handler. MountByNameAndWait() sleeps on the scheduler, so calling it there puts a task switch inside an interrupt handler and freezes the Pi hard enough to need a power cycle - on every mount, whatever the image, success or failure. ssd1306 escapes only because it already defers presses to ProcessPendingInput(); its own comment states the rule: the handler must do quick, lock-free work only. So the button handler only queues the request with SetNextCDByName() and remembers the retired-request counter, and ResolvePendingMount(), called from Refresh() in task context, waits for that counter to move and applies the outcome. Done on all three drivers rather than the two that would crash: on ssd1306 the wait was safe but still blocked the display loop for up to eight seconds, stalling the screen and the backlight timeout. MountByNameAndWait() is documented as task-context-only, naming the two handlers that are not. The vendor toolbox picker is deliberately left asynchronous. It runs inside a SCSI command handler, where sleeping is not acceptable, and DOS has nowhere to show a result anyway. --- addon/displayservice/sh1106/imagespage.cpp | 69 +++++++++++++++++++-- addon/displayservice/sh1106/imagespage.h | 17 +++++ addon/displayservice/ssd1306/imagespage.cpp | 69 +++++++++++++++++++-- addon/displayservice/ssd1306/imagespage.h | 17 +++++ addon/displayservice/st7789/imagespage.cpp | 67 ++++++++++++++++++-- addon/displayservice/st7789/imagespage.h | 17 +++++ addon/scsitbservice/scsitbservice.cpp | 34 ++++++++++ addon/scsitbservice/scsitbservice.h | 38 ++++++++++++ addon/webserver/handlers/mountapi.cpp | 18 +++++- addon/webserver/handlers/mountpage.cpp | 29 ++++++++- addon/webserver/pages/mount.html | 17 +++++ 11 files changed, 373 insertions(+), 19 deletions(-) diff --git a/addon/displayservice/sh1106/imagespage.cpp b/addon/displayservice/sh1106/imagespage.cpp index 682664af..31ea1af6 100644 --- a/addon/displayservice/sh1106/imagespage.cpp +++ b/addon/displayservice/sh1106/imagespage.cpp @@ -28,6 +28,10 @@ void SH1106ImagesPage::OnEnter() { ConfigService* config = ConfigService::Get(); bool flatFileList = config ? config->GetFlatFileList() : false; + // A mount request does not survive leaving and re-entering the page. + m_MountPending = false; + m_MountFailed = false; + const char* currentPath = m_Service->GetCurrentCDPath(); m_CurrentPath[0] = '\0'; @@ -125,10 +129,23 @@ void SH1106ImagesPage::OnButtonPress(Button button) { NavigateToFolder(relativePath); } else { const char* relativePath = m_Service->GetRelativePath(cacheIdx); - m_Service->SetNextCDByName(relativePath); - m_MountedIndex = m_SelectedIndex; - m_NextPageName = "homepage"; - m_ShouldChangePage = true; + // Queue the mount; do NOT wait for it here. This runs in + // the GPIO interrupt handler, and waiting means sleeping on + // the scheduler, which from IRQ context freezes the Pi hard + // enough to need a power cycle - on every mount, whatever + // the image. ResolvePendingMount() collects the outcome + // from Refresh(), which is task context. + m_MountRequestSeq = m_Service->GetMountSeq(); + if (m_Service->SetNextCDByName(relativePath)) { + m_MountRequestIndex = m_SelectedIndex; + m_MountPending = true; + m_MountWaitTicks = 0; + m_MountFailed = false; + } else { + m_MountPending = false; + m_MountFailed = true; + } + dirty = true; } } break; @@ -389,7 +406,47 @@ void SH1106ImagesPage::RefreshScroll() { } } +// Refresh() ticks every 50ms (displayservice.cpp), so this is roughly ten +// seconds - comfortably longer than a slow card needs, and longer than the +// service's own mount timeout. +static const unsigned MountWaitTickLimit = 200; + +// Collect the result of a mount the button handler queued. Runs in task +// context, because OnButtonPress cannot: it is a GPIO interrupt handler on this +// display, where sleeping would lock the machine up. +void SH1106ImagesPage::ResolvePendingMount() { + if (!m_MountPending || !m_Service) + return; + + if (m_Service->GetMountSeq() == m_MountRequestSeq) { + // Still queued. Give up eventually rather than sitting pending forever + // if the request never reaches the service task at all. + if (++m_MountWaitTicks > MountWaitTickLimit) { + m_MountPending = false; + m_MountFailed = true; + dirty = true; + } + return; + } + + m_MountPending = false; + dirty = true; + + if (m_Service->GetLastMountError()[0] == '\0') { + m_MountFailed = false; + m_MountedIndex = m_MountRequestIndex; + m_NextPageName = "homepage"; + m_ShouldChangePage = true; + } else { + // Stay on the image list. Jumping to the homepage showed the PREVIOUS + // image as current with nothing said about the failure. + m_MountFailed = true; + } +} + void SH1106ImagesPage::Refresh() { + ResolvePendingMount(); + if (dirty) { Draw(); return; @@ -407,7 +464,9 @@ void SH1106ImagesPage::Draw() { m_Graphics->ClearScreen(COLOR2D(0, 0, 0)); m_Graphics->DrawRect(0, 0, m_Display->GetWidth(), 10, COLOR2D(255, 255, 255)); - m_Graphics->DrawText(2, 1, COLOR2D(0, 0, 0), "Images", C2DGraphics::AlignLeft, Font8x8); + m_Graphics->DrawText(2, 1, COLOR2D(0, 0, 0), + m_MountFailed ? "Mount FAILED" : "Images", + C2DGraphics::AlignLeft, Font8x8); if (m_SelectedIndex != m_PreviousSelectedIndex) { m_ScrollOffsetPx = 0; diff --git a/addon/displayservice/sh1106/imagespage.h b/addon/displayservice/sh1106/imagespage.h index 1b14ca58..7c91e340 100644 --- a/addon/displayservice/sh1106/imagespage.h +++ b/addon/displayservice/sh1106/imagespage.h @@ -25,6 +25,7 @@ class SH1106ImagesPage : public IPage { private: void Draw(); + void ResolvePendingMount(); void MoveSelection(int delta); void NavigateToFolder(const char* path); void NavigateUp(); @@ -48,6 +49,22 @@ class SH1106ImagesPage : public IPage { CSH1106Display* m_Display; C2DGraphics* m_Graphics; SCSITBService* m_Service = nullptr; + // Set when a mount attempt fails, and shown in place of the page title. + // The HAT used to fall silently back to the previously mounted image, so a + // disc that refused to load looked exactly like one the user had changed + // their mind about. Short by necessity - the full reason is in the web UI + // and the log. + bool m_MountFailed = false; + + // A mount requested from the button handler and not yet resolved. + // OnButtonPress runs in the GPIO interrupt on this display, so it can only + // queue the request - m_MountRequestSeq is the service's retired-request + // counter as it stood when we asked, and Refresh() watches for it to move. + bool m_MountPending = false; + unsigned m_MountRequestSeq = 0; + size_t m_MountRequestIndex = 0; + unsigned m_MountWaitTicks = 0; + size_t m_SelectedIndex = 0; size_t m_MountedIndex = 0; int charWidth; diff --git a/addon/displayservice/ssd1306/imagespage.cpp b/addon/displayservice/ssd1306/imagespage.cpp index 03280fb0..ff47668e 100644 --- a/addon/displayservice/ssd1306/imagespage.cpp +++ b/addon/displayservice/ssd1306/imagespage.cpp @@ -28,6 +28,10 @@ void SSD1306ImagesPage::OnEnter() { ConfigService* config = ConfigService::Get(); bool flatFileList = config ? config->GetFlatFileList() : false; + // A mount request does not survive leaving and re-entering the page. + m_MountPending = false; + m_MountFailed = false; + const char* currentPath = m_Service->GetCurrentCDPath(); m_CurrentPath[0] = '\0'; @@ -125,10 +129,23 @@ void SSD1306ImagesPage::OnButtonPress(Button button) { NavigateToFolder(relativePath); } else { const char* relativePath = m_Service->GetRelativePath(cacheIdx); - m_Service->SetNextCDByName(relativePath); - m_MountedIndex = m_SelectedIndex; - m_NextPageName = "homepage"; - m_ShouldChangePage = true; + // Queue the mount; do NOT wait for it here. This runs in + // the GPIO interrupt handler, and waiting means sleeping on + // the scheduler, which from IRQ context freezes the Pi hard + // enough to need a power cycle - on every mount, whatever + // the image. ResolvePendingMount() collects the outcome + // from Refresh(), which is task context. + m_MountRequestSeq = m_Service->GetMountSeq(); + if (m_Service->SetNextCDByName(relativePath)) { + m_MountRequestIndex = m_SelectedIndex; + m_MountPending = true; + m_MountWaitTicks = 0; + m_MountFailed = false; + } else { + m_MountPending = false; + m_MountFailed = true; + } + dirty = true; } } break; @@ -389,7 +406,47 @@ void SSD1306ImagesPage::RefreshScroll() { } } +// Refresh() ticks every 50ms (displayservice.cpp), so this is roughly ten +// seconds - comfortably longer than a slow card needs, and longer than the +// service's own mount timeout. +static const unsigned MountWaitTickLimit = 200; + +// Collect the result of a mount the button handler queued. Runs in task +// context, because OnButtonPress cannot: it is a GPIO interrupt handler on this +// display, where sleeping would lock the machine up. +void SSD1306ImagesPage::ResolvePendingMount() { + if (!m_MountPending || !m_Service) + return; + + if (m_Service->GetMountSeq() == m_MountRequestSeq) { + // Still queued. Give up eventually rather than sitting pending forever + // if the request never reaches the service task at all. + if (++m_MountWaitTicks > MountWaitTickLimit) { + m_MountPending = false; + m_MountFailed = true; + dirty = true; + } + return; + } + + m_MountPending = false; + dirty = true; + + if (m_Service->GetLastMountError()[0] == '\0') { + m_MountFailed = false; + m_MountedIndex = m_MountRequestIndex; + m_NextPageName = "homepage"; + m_ShouldChangePage = true; + } else { + // Stay on the image list. Jumping to the homepage showed the PREVIOUS + // image as current with nothing said about the failure. + m_MountFailed = true; + } +} + void SSD1306ImagesPage::Refresh() { + ResolvePendingMount(); + if (dirty) { Draw(); return; @@ -407,7 +464,9 @@ void SSD1306ImagesPage::Draw() { m_Graphics->ClearScreen(COLOR2D(0, 0, 0)); m_Graphics->DrawRect(0, 0, m_Display->GetWidth(), 10, COLOR2D(255, 255, 255)); - m_Graphics->DrawText(2, 1, COLOR2D(0, 0, 0), "Images", C2DGraphics::AlignLeft, Font8x8); + m_Graphics->DrawText(2, 1, COLOR2D(0, 0, 0), + m_MountFailed ? "Mount FAILED" : "Images", + C2DGraphics::AlignLeft, Font8x8); if (m_SelectedIndex != m_PreviousSelectedIndex) { m_ScrollOffsetPx = 0; diff --git a/addon/displayservice/ssd1306/imagespage.h b/addon/displayservice/ssd1306/imagespage.h index d98be3d6..318ed329 100644 --- a/addon/displayservice/ssd1306/imagespage.h +++ b/addon/displayservice/ssd1306/imagespage.h @@ -25,6 +25,7 @@ class SSD1306ImagesPage : public IPage { private: void Draw(); + void ResolvePendingMount(); void MoveSelection(int delta); void NavigateToFolder(const char* path); void NavigateUp(); @@ -48,6 +49,22 @@ class SSD1306ImagesPage : public IPage { CSSD1306GfxDisplay* m_Display; C2DGraphics* m_Graphics; SCSITBService* m_Service = nullptr; + // Set when a mount attempt fails, and shown in place of the page title. + // The HAT used to fall silently back to the previously mounted image, so a + // disc that refused to load looked exactly like one the user had changed + // their mind about. Short by necessity - the full reason is in the web UI + // and the log. + bool m_MountFailed = false; + + // A mount requested from the button handler and not yet resolved. + // OnButtonPress runs in the GPIO interrupt on this display, so it can only + // queue the request - m_MountRequestSeq is the service's retired-request + // counter as it stood when we asked, and Refresh() watches for it to move. + bool m_MountPending = false; + unsigned m_MountRequestSeq = 0; + size_t m_MountRequestIndex = 0; + unsigned m_MountWaitTicks = 0; + size_t m_SelectedIndex = 0; size_t m_MountedIndex = 0; int charWidth; diff --git a/addon/displayservice/st7789/imagespage.cpp b/addon/displayservice/st7789/imagespage.cpp index 8c0a1478..bc986d66 100644 --- a/addon/displayservice/st7789/imagespage.cpp +++ b/addon/displayservice/st7789/imagespage.cpp @@ -28,6 +28,10 @@ void ST7789ImagesPage::OnEnter() { ConfigService* config = ConfigService::Get(); bool flatFileList = config ? config->GetFlatFileList() : false; + // A mount request does not survive leaving and re-entering the page. + m_MountPending = false; + m_MountFailed = false; + const char* currentPath = m_Service->GetCurrentCDPath(); m_CurrentPath[0] = '\0'; @@ -125,10 +129,23 @@ void ST7789ImagesPage::OnButtonPress(Button button) { NavigateToFolder(relativePath); } else { const char* relativePath = m_Service->GetRelativePath(cacheIdx); - m_Service->SetNextCDByName(relativePath); - m_MountedIndex = m_SelectedIndex; - m_NextPageName = "homepage"; - m_ShouldChangePage = true; + // Queue the mount; do NOT wait for it here. This runs in + // the GPIO interrupt handler, and waiting means sleeping on + // the scheduler, which from IRQ context freezes the Pi hard + // enough to need a power cycle - on every mount, whatever + // the image. ResolvePendingMount() collects the outcome + // from Refresh(), which is task context. + m_MountRequestSeq = m_Service->GetMountSeq(); + if (m_Service->SetNextCDByName(relativePath)) { + m_MountRequestIndex = m_SelectedIndex; + m_MountPending = true; + m_MountWaitTicks = 0; + m_MountFailed = false; + } else { + m_MountPending = false; + m_MountFailed = true; + } + dirty = true; } } break; @@ -388,7 +405,47 @@ void ST7789ImagesPage::RefreshScroll() { } } +// Refresh() ticks every 50ms (displayservice.cpp), so this is roughly ten +// seconds - comfortably longer than a slow card needs, and longer than the +// service's own mount timeout. +static const unsigned MountWaitTickLimit = 200; + +// Collect the result of a mount the button handler queued. Runs in task +// context, because OnButtonPress cannot: it is a GPIO interrupt handler on this +// display, where sleeping would lock the machine up. +void ST7789ImagesPage::ResolvePendingMount() { + if (!m_MountPending || !m_Service) + return; + + if (m_Service->GetMountSeq() == m_MountRequestSeq) { + // Still queued. Give up eventually rather than sitting pending forever + // if the request never reaches the service task at all. + if (++m_MountWaitTicks > MountWaitTickLimit) { + m_MountPending = false; + m_MountFailed = true; + dirty = true; + } + return; + } + + m_MountPending = false; + dirty = true; + + if (m_Service->GetLastMountError()[0] == '\0') { + m_MountFailed = false; + m_MountedIndex = m_MountRequestIndex; + m_NextPageName = "homepage"; + m_ShouldChangePage = true; + } else { + // Stay on the image list. Jumping to the homepage showed the PREVIOUS + // image as current with nothing said about the failure. + m_MountFailed = true; + } +} + void ST7789ImagesPage::Refresh() { + ResolvePendingMount(); + if (dirty) { Draw(); return; @@ -406,7 +463,7 @@ void ST7789ImagesPage::Draw() { m_Graphics->ClearScreen(COLOR2D(255, 255, 255)); - const char* pTitle = "CD Images"; + const char* pTitle = m_MountFailed ? "Mount FAILED - see web UI" : "CD Images"; // Draw header bar with blue background m_Graphics->DrawRect(0, 0, m_Display->GetWidth(), 30, COLOR2D(58, 124, 165)); diff --git a/addon/displayservice/st7789/imagespage.h b/addon/displayservice/st7789/imagespage.h index eae16282..292d130a 100644 --- a/addon/displayservice/st7789/imagespage.h +++ b/addon/displayservice/st7789/imagespage.h @@ -25,6 +25,7 @@ class ST7789ImagesPage : public IPage { private: void Draw(); + void ResolvePendingMount(); void MoveSelection(int delta); void NavigateToFolder(const char* path); void NavigateUp(); @@ -49,6 +50,22 @@ class ST7789ImagesPage : public IPage { SCSITBService* m_Service = nullptr; CST7789Display* m_Display; C2DGraphics* m_Graphics; + // Set when a mount attempt fails, and shown in place of the page title. + // The HAT used to fall silently back to the previously mounted image, so a + // disc that refused to load looked exactly like one the user had changed + // their mind about. Short by necessity - the full reason is in the web UI + // and the log. + bool m_MountFailed = false; + + // A mount requested from the button handler and not yet resolved. + // OnButtonPress runs in the GPIO interrupt on this display, so it can only + // queue the request - m_MountRequestSeq is the service's retired-request + // counter as it stood when we asked, and Refresh() watches for it to move. + bool m_MountPending = false; + unsigned m_MountRequestSeq = 0; + size_t m_MountRequestIndex = 0; + unsigned m_MountWaitTicks = 0; + size_t m_SelectedIndex = 0; size_t m_MountedIndex = 0; int charWidth; diff --git a/addon/scsitbservice/scsitbservice.cpp b/addon/scsitbservice/scsitbservice.cpp index 22c3cf21..5615ce35 100644 --- a/addon/scsitbservice/scsitbservice.cpp +++ b/addon/scsitbservice/scsitbservice.cpp @@ -503,7 +503,10 @@ void SCSITBService::ProcessPendingMount() { return; if ((size_t)next_cd >= m_FileCount) { + snprintf(m_LastMountError, sizeof(m_LastMountError), + "That image is no longer in the list; the card may have been rescanned."); next_cd = -1; + m_MountSeq++; return; } @@ -519,6 +522,7 @@ void SCSITBService::ProcessPendingMount() { snprintf(m_LastMountError, sizeof(m_LastMountError), "Path is too long to mount: %s", relativePath); next_cd = -1; + m_MountSeq++; return; } // Build the path locally. Writing it straight into m_CurrentImagePath @@ -548,6 +552,7 @@ void SCSITBService::ProcessPendingMount() { relativePath); } next_cd = -1; + m_MountSeq++; return; } @@ -570,6 +575,35 @@ void SCSITBService::ProcessPendingMount() { current_cd = next_cd; next_cd = -1; + m_MountSeq++; +} + +SCSITBService::MountOutcome SCSITBService::MountByNameAndWait(const char* file_name, + unsigned timeoutMs) { + const unsigned startSeq = m_MountSeq; + + if (!SetNextCDByName(file_name)) { + return MountOutcome::NotFound; + } + + // Run() picks the request up on its next pass and retires it, bumping the + // sequence exactly once whatever the result. Poll rather than block: this + // runs on the web server's or the display's task, and the service task + // needs the CPU to do the work being waited for. + unsigned waited = 0; + const unsigned step = 20; + while (m_MountSeq == startSeq && waited < timeoutMs) { + CScheduler::Get()->MsSleep(step); + waited += step; + } + + if (m_MountSeq == startSeq) { + // A large CHD on a slow card can genuinely take a while. Saying it + // failed would be a guess, and the wrong one more often than not. + return MountOutcome::Timeout; + } + + return m_LastMountError[0] == '\0' ? MountOutcome::Success : MountOutcome::Failed; } void SCSITBService::Run() { diff --git a/addon/scsitbservice/scsitbservice.h b/addon/scsitbservice/scsitbservice.h index 237cc954..02590c34 100644 --- a/addon/scsitbservice/scsitbservice.h +++ b/addon/scsitbservice/scsitbservice.h @@ -50,6 +50,40 @@ class SCSITBService : public CTask bool SetNextCD(size_t index); bool SetNextCDByName(const char* file_name); + /// What a mount attempt did. Anything that can tell the user should use + /// MountByNameAndWait() and report this, rather than SetNextCDByName(), + /// whose "true" only means the name was found in the catalogue. + enum class MountOutcome + { + Success, + NotFound, // no such entry; nothing was attempted + Failed, // the image was tried and would not load, see GetLastMountError() + Timeout // still loading; too big or the card is slow, not known to have failed + }; + + /// Queue an image and wait for the service task to finish loading it. + /// + /// Mounting is asynchronous - the request is handed to Run() and picked up + /// on its next pass - so a caller that returns as soon as it has queued the + /// request can only report that the file exists. The web UI announced + /// "Successfully mounted" for images that then failed to load, which is how + /// a split-track cue came to report success and refusal on two consecutive + /// pages. + /// + /// TASK CONTEXT ONLY. This sleeps on the scheduler, so calling it from an + /// interrupt handler freezes the machine hard enough to need a power cycle. + /// That is not hypothetical: the sh1106 and st7789 button handlers run in + /// the GPIO interrupt (see PageManager::HandleButtonPress), and calling + /// this from there locked up the Pi on every mount. Those pages queue with + /// SetNextCDByName() and watch GetMountSeq() from their refresh loop + /// instead. Also not with m_Lock held, and not from a SCSI command handler + /// - the vendor toolbox picker has no way to show a result anyway. + MountOutcome MountByNameAndWait(const char* file_name, unsigned timeoutMs = 8000); + + /// Counter of mount requests the service task has finished with, whatever + /// the result. Changes exactly once per retired request. + unsigned GetMountSeq() const { return m_MountSeq; } + // Eject / insert the current medium. These queue the request for the Run() // loop (task context), matching how SetNextCD defers the actual work. void SetPendingEject(); @@ -102,6 +136,10 @@ class SCSITBService : public CTask // had never been mounted. This is what current_cd is re-derived from. char m_MountedRelativePath[MAX_PATH_LEN] = {0}; + // Bumped once per mount request the service task retires, so a caller can + // tell "still queued" from "dealt with" without polling internal state. + volatile unsigned m_MountSeq = 0; + // Relative path of the last image that failed to load, so the // pick-something fallback in RefreshCache does not keep choosing it and // failing again on every rescan. diff --git a/addon/webserver/handlers/mountapi.cpp b/addon/webserver/handlers/mountapi.cpp index 8d709328..f0bb2958 100644 --- a/addon/webserver/handlers/mountapi.cpp +++ b/addon/webserver/handlers/mountapi.cpp @@ -33,10 +33,24 @@ THTTPStatus MountAPIHandler::GetJson(nlohmann::json& j, LOGNOTE("MountAPI: Mounting image with relative path: %s", file_param.c_str()); - // Use SetNextCDByName which now searches by relativePath in the cache - if (svc->SetNextCDByName(file_param.c_str())) { + // Report what the load actually did, not merely that the name was found. + switch (svc->MountByNameAndWait(file_param.c_str())) { + case SCSITBService::MountOutcome::Success: j = {{"status", "ok"}}; return HTTPOK; + + case SCSITBService::MountOutcome::Failed: + j = {{"status", "error"}, {"error", std::string(svc->GetLastMountError())}}; + return HTTPOK; + + case SCSITBService::MountOutcome::Timeout: + j = {{"status", "pending"}, + {"error", "Still loading; check the current image shortly."}}; + return HTTPOK; + + case SCSITBService::MountOutcome::NotFound: + default: + break; } return HTTPNotFound; diff --git a/addon/webserver/handlers/mountpage.cpp b/addon/webserver/handlers/mountpage.cpp index 59035ef1..619643e1 100644 --- a/addon/webserver/handlers/mountpage.cpp +++ b/addon/webserver/handlers/mountpage.cpp @@ -54,9 +54,34 @@ THTTPStatus MountPageHandler::PopulateContext(kainjow::mustache::data& context, return HTTPInternalServerError; } - // Use SetNextCDByName which now searches by relativePath in the cache - if (svc->SetNextCDByName(file_param.c_str())) { + // Wait for the load to actually happen before saying anything about it. + // Queueing the request and reporting success announced "Successfully + // mounted" for images that then refused to load, so the very next page + // contradicted this one. + switch (svc->MountByNameAndWait(file_param.c_str())) { + case SCSITBService::MountOutcome::Success: + context.set("mounted", true); return HTTPOK; + + case SCSITBService::MountOutcome::Failed: + context.set("mounted", false); + context.set("mount_failed", true); + context.set("mount_message", std::string(svc->GetLastMountError())); + // Stay put rather than bouncing to the homepage; the reason is here. + context.set("meta_refresh_url", ""); + return HTTPOK; + + case SCSITBService::MountOutcome::Timeout: + context.set("mounted", false); + context.set("mount_pending", true); + context.set("mount_message", + std::string("Still loading. Large images on a slow card can take a " + "while; the homepage will show it once it is ready.")); + return HTTPOK; + + case SCSITBService::MountOutcome::NotFound: + default: + break; } return HTTPNotFound; diff --git a/addon/webserver/pages/mount.html b/addon/webserver/pages/mount.html index a7f7b24b..70b4c6c3 100644 --- a/addon/webserver/pages/mount.html +++ b/addon/webserver/pages/mount.html @@ -1,8 +1,25 @@

Mounting File

+ +{{#mounted}}

Successfully mounted: {{image_name}}

Returning to homepage in {{meta_refresh_timeout}} seconds

+{{/mounted}} + +{{#mount_failed}} +
+

Could not mount: {{image_name}}

+

{{mount_message}}

+
+{{/mount_failed}} + +{{#mount_pending}} +
+

Mounting: {{image_name}}

+

{{mount_message}}

+
+{{/mount_pending}}
Return to File List From 7be0efc3a2b96aa260134acd83d7998624b2ec17 Mon Sep 17 00:00:00 2001 From: David Hauf Date: Tue, 28 Jul 2026 18:57:16 -0400 Subject: [PATCH 6/7] mount: come up empty when the saved image is gone, instead of a different disc An image that had been renamed, deleted, or left behind on another card was silently replaced by whichever file sorted first. The drive came up holding a game the user had never chosen, with nothing anywhere to say why, and the only clue was a log line nobody reads. Asked directly whether a missing image now gives an empty drive, the honest answer was no. RefreshCache now adopts a stand-in but presents the drive EMPTY, and records which image went missing so every web page can say so. The stand-in is still loaded behind the scenes because the gadget needs a geometry, and because it makes Insert instant if the user does want a disc. Three things this has to get right: - A fresh card must still auto-mount. GetCurrentImage() answers with "image.iso" whether the key is missing or genuinely set to that, so it cannot tell a remembered image from a card that has never mounted anything. Asking with an empty default can, and only a genuinely remembered image triggers the empty drive. - The stand-in must not be saved as the user's image. Recording it would lose the missing name, so the explanation would vanish on the next boot and putting the file back would no longer bring it up. - Adopting while ejected must stay ejected, and must still happen. SetDevice() clears the ejected latch unless the boot-eject is armed, so the arm is not optional here. And the fallback's !IsEjected() guard had to gain an exception for a gadget that has never come up: adopting is the only thing that initializes it, so refusing would have left the host seeing no USB device at all rather than an empty drive - reachable on the second boot after this code had itself come up empty and persisted the eject. An empty images partition still adopts nothing and still leaves the gadget uninitialized, which the QEMU boot test in tests/qemu-boot relies on: it boots a virgin card with the images partition swept, so m_FileCount is 0, the fallback never runs, and SetDevice() is never reached. CDROMService gained IsGadgetInitialized() to make that condition testable rather than inferred, and the deferred init is now commented as load-bearing rather than looking like an optimization someone could helpfully remove - naming the three symptoms validate_boot_log.py treats as FORBIDDEN_ANYWHERE, so the next person to touch it knows what depends on it. --- addon/cdromservice/cdromservice.cpp | 16 +++- addon/cdromservice/cdromservice.h | 7 ++ addon/scsitbservice/scsitbservice.cpp | 82 ++++++++++++++++++-- addon/scsitbservice/scsitbservice.h | 20 +++++ addon/webserver/handlers/pagehandlerbase.cpp | 10 +++ addon/webserver/pages/template.html | 7 ++ 6 files changed, 135 insertions(+), 7 deletions(-) diff --git a/addon/cdromservice/cdromservice.cpp b/addon/cdromservice/cdromservice.cpp index d25cf4f3..72ea4de8 100644 --- a/addon/cdromservice/cdromservice.cpp +++ b/addon/cdromservice/cdromservice.cpp @@ -56,7 +56,21 @@ void CDROMService::SetDevice(IImageDevice *pDevice) LOGNOTE("Device has subchannel data"); } - // We defer initialization of the CD Gadget until the first CD image is loaded + // We defer initialization of the CD Gadget until the first CD image is loaded. + // + // LOAD-BEARING, not just an optimization. This is the only path that brings + // the USB gadget up, so with no image there are no endpoints at all - and + // the QEMU boot test depends on exactly that, because QEMU cannot emulate + // dwc2 device mode. It keeps the images partition empty so this moment + // never arrives, and validate_boot_log.py lists the symptoms of the + // hardware path running - "does not support USB gadget mode", "dwgadget: + // Unknown vendor", "Failed to initialize CD Gadget" - as FORBIDDEN_ANYWHERE. + // See tests/qemu-boot/README.md. + // + // So: do not move initialization earlier, and do not make it unconditional. + // Note that ejected=1 does NOT avoid it either - the boot path still + // pre-loads the remembered image, and ArmBootEject() only hides the medium + // from the host after the gadget is already up. if (!isInitialized) { bool ok = m_CDGadget->Initialize(); diff --git a/addon/cdromservice/cdromservice.h b/addon/cdromservice/cdromservice.h index 7f8476c9..dcb845c5 100644 --- a/addon/cdromservice/cdromservice.h +++ b/addon/cdromservice/cdromservice.h @@ -46,6 +46,13 @@ class CDROMService : public CTask { // was last powered off. Must be armed before the first SetDevice(). void ArmBootEject(void); void DisarmBootEject(void); + + // Whether the USB gadget has been brought up yet. It is created lazily by + // the first SetDevice(), so "false" means the host currently sees no device + // at all - which callers deciding whether to adopt an image need to know, + // since an empty drive and an absent drive look nothing alike to a PC. + bool IsGadgetInitialized(void) const { return isInitialized; } + void Run(void); private: diff --git a/addon/scsitbservice/scsitbservice.cpp b/addon/scsitbservice/scsitbservice.cpp index 5615ce35..6d3960f1 100644 --- a/addon/scsitbservice/scsitbservice.cpp +++ b/addon/scsitbservice/scsitbservice.cpp @@ -377,8 +377,18 @@ bool SCSITBService::RefreshCache() { LOGNOTE("SCSITBService::RefreshCache() called"); m_Lock.Acquire(); - // Get current loaded image from config - const char* current_image = configservice->GetCurrentImage(DEFAULT_IMAGE_FILENAME); + // Get current loaded image from config. + // + // Asked with an empty default, this distinguishes "the user has an image + // remembered" from "this card has never mounted anything". They must not + // behave the same: a remembered image that has gone missing should leave + // the drive empty and say so, but a freshly written card has no remembered + // image at all and must still auto-mount, or first boot would look broken. + // GetCurrentImage(DEFAULT_IMAGE_FILENAME) cannot tell them apart, because + // it hands back "image.iso" in both cases. + const char* saved_image = configservice->GetCurrentImage(""); + const bool hadRememberedImage = (saved_image != nullptr && saved_image[0] != '\0'); + const char* current_image = hadRememberedImage ? saved_image : DEFAULT_IMAGE_FILENAME; LOGNOTE("SCSITBService::RefreshCache() loaded current_image %s from config.txt", current_image); // Store the current image path if not already set @@ -466,11 +476,23 @@ bool SCSITBService::RefreshCache() { } } - // Fallback to first image file if not found. Never while ejected: an empty + // Fallback to first image file if not found. Not while ejected: an empty // drive must stay empty, and this path also runs on rescans (upload, delete, // FTP), where auto-mounting a substitute would undo the user's eject and // then persist "inserted" over the saved state. - if (!found && m_FileCount > 0 && !IsEjected()) { + // + // The exception is a gadget that has never been brought up. Adopting an + // image is the ONLY thing that initializes it (cdromservice.cpp:59), so + // refusing here would leave the host seeing no USB device at all rather + // than an empty drive - and adopting cannot undo the eject anyway, because + // the boot-eject arm below keeps the medium hidden. Reachable whenever a + // remembered image goes missing while the drive is ejected, including the + // second boot after this code has itself come up empty and persisted it. + // + // m_FileCount == 0 still adopts nothing, which is what the QEMU boot test + // relies on - see tests/qemu-boot/README.md. + if (!found && m_FileCount > 0 && + (!IsEjected() || (cdromservice && !cdromservice->IsGadgetInitialized()))) { for (size_t i = 0; i < m_FileCount; ++i) { if (m_FileEntries[i].isDirectory) { continue; @@ -486,6 +508,30 @@ bool SCSITBService::RefreshCache() { LOGNOTE("SCSITBService::RefreshCache() Current image not found, using: %s", m_FileEntries[i].relativePath); next_cd = i; + + // A remembered image that is simply gone must not be swapped for a + // different disc in silence - that is how a renamed file turned + // into "some other game is in the drive" with nothing to explain + // it. Adopt this one so the gadget has a geometry and Insert is + // instant, but present the drive as EMPTY and record what went + // missing. A card that never had a remembered image (fresh write) + // keeps the plain auto-mount. + if (hadRememberedImage) { + strncpy(m_MissingSavedImage, current_image, sizeof(m_MissingSavedImage) - 1); + m_MissingSavedImage[sizeof(m_MissingSavedImage) - 1] = '\0'; + m_bAdoptAsEmpty = true; + LOGWARN("Saved image %s is not on the card; coming up empty with %s adopted", + current_image, m_FileEntries[i].relativePath); + } + + // Adopting while ejected MUST stay ejected. SetDevice() clears the + // ejected latch unless the boot-eject is armed, so without this the + // adoption we just allowed above would insert a disc the user had + // deliberately ejected - the exact thing the !IsEjected() guard + // exists to prevent. + if (IsEjected()) { + m_bAdoptAsEmpty = true; + } break; } } @@ -502,6 +548,11 @@ void SCSITBService::ProcessPendingMount() { if (next_cd <= -1) return; + // Consume the flag up front: every exit path below retires the request, so + // leaving it set would eject the next image the user deliberately mounts. + const bool adoptAsEmpty = m_bAdoptAsEmpty; + m_bAdoptAsEmpty = false; + if ((size_t)next_cd >= m_FileCount) { snprintf(m_LastMountError, sizeof(m_LastMountError), "That image is no longer in the list; the card may have been rescanned."); @@ -561,6 +612,15 @@ void SCSITBService::ProcessPendingMount() { (int)imageDevice->GetFileType(), imageDevice->HasSubchannelData() ? "yes" : "no"); + // This image is only standing in for one that is no longer on the card, so + // take its geometry but keep the drive empty to the host. Armed BEFORE + // SetDevice() deliberately: SetDevice() decides the ejected state itself + // and can yield, so ejecting afterwards would leave a window in which the + // gadget reports ready and the host mounts a disc nobody asked for. + if (adoptAsEmpty) { + cdromservice->ArmBootEject(); + } + cdromservice->SetDevice(imageDevice); // Committed only now that the disc is really the one the host has. @@ -570,8 +630,18 @@ void SCSITBService::ProcessPendingMount() { m_MountedRelativePath[sizeof(m_MountedRelativePath) - 1] = '\0'; m_LastFailedRelativePath[0] = '\0'; - // Save relative path to config (without "1:/" prefix) - configservice->SetCurrentImage(relativePath); + // Save relative path to config (without "1:/" prefix). + // + // Not for a stand-in: recording it would make the substitute the user's + // remembered image, so the explanation would vanish on the next boot and + // putting the missing file back would no longer bring it up. Leaving the + // old name in config means the notice persists until something is mounted + // deliberately, and the fallback does not fire again because the drive is + // now ejected. + if (!adoptAsEmpty) { + configservice->SetCurrentImage(relativePath); + m_MissingSavedImage[0] = '\0'; + } current_cd = next_cd; next_cd = -1; diff --git a/addon/scsitbservice/scsitbservice.h b/addon/scsitbservice/scsitbservice.h index 02590c34..5e46e33c 100644 --- a/addon/scsitbservice/scsitbservice.h +++ b/addon/scsitbservice/scsitbservice.h @@ -99,6 +99,16 @@ class SCSITBService : public CTask /// be mounted looks to the user exactly like one that can. const char* GetLastMountError() const { return m_LastMountError; } + /// The image config remembered, when it is no longer on the card, or "". + /// + /// A saved image that has been renamed, deleted or left on another card + /// used to be replaced by whichever file sorted first, with nothing said: + /// the drive came up holding a game the user had not asked for. Now the + /// drive comes up EMPTY and this says which image went missing. The + /// substitute is still adopted behind the scenes so the gadget knows a + /// geometry and Insert is instant - see ProcessPendingMount(). + const char* GetMissingSavedImage() const { return m_MissingSavedImage; } + // Task entry point void Run(void); @@ -140,6 +150,16 @@ class SCSITBService : public CTask // tell "still queued" from "dealt with" without polling internal state. volatile unsigned m_MountSeq = 0; + // The image config remembered when it turned out not to be on the card. + // Kept until the user mounts something deliberately, so the explanation + // survives a reboot rather than scrolling past once. + char m_MissingSavedImage[MAX_PATH_LEN] = {0}; + + // Set by RefreshCache when the image it is about to mount is only a + // stand-in for a missing one, and consumed by ProcessPendingMount, which + // arms the boot-eject so the drive presents empty. + bool m_bAdoptAsEmpty = false; + // Relative path of the last image that failed to load, so the // pick-something fallback in RefreshCache does not keep choosing it and // failing again on every rescan. diff --git a/addon/webserver/handlers/pagehandlerbase.cpp b/addon/webserver/handlers/pagehandlerbase.cpp index 01b7116d..8b6bbe4e 100644 --- a/addon/webserver/handlers/pagehandlerbase.cpp +++ b/addon/webserver/handlers/pagehandlerbase.cpp @@ -78,6 +78,16 @@ THTTPStatus PageHandlerBase::GetContent(const char *pPath, context.set("mount_error", std::string(mountError)); } + // The saved image was not on the card, so the drive came up empty + // rather than holding a disc the user never chose. Shown on every page + // and until something is mounted deliberately: this survives a reboot, + // so a one-shot message would be missed by exactly the person it is + // for. + const char* missingImage = svc->GetMissingSavedImage(); + if (missingImage != nullptr && missingImage[0] != '\0') { + context.set("missing_image", std::string(missingImage)); + } + // Get our config service ConfigService* config = static_cast(CScheduler::Get()->GetTask("configservice")); diff --git a/addon/webserver/pages/template.html b/addon/webserver/pages/template.html index d16af1f7..abc0cfb8 100644 --- a/addon/webserver/pages/template.html +++ b/addon/webserver/pages/template.html @@ -25,6 +25,13 @@

USB Optical Drive Emulator

{{/mount_error}} + {{#missing_image}} +
+ Drive is empty: {{missing_image}} is no longer on the card. + Mount an image to clear this. +
+ {{/missing_image}} + {{#cdrom}} {{>content}} {{/cdrom}} From f3001b99f612c0c401da551b362961243b10536e Mon Sep 17 00:00:00 2001 From: David Hauf Date: Tue, 28 Jul 2026 20:31:18 -0400 Subject: [PATCH 7/7] gadget: an ejected drive must not put a disc in by itself The empty-drive behaviour for a missing image worked on a reboot and silently did nothing on a running system. Renaming the mounted image over FTP left a different game mounted and readable, with no sign the saved one had gone. SetDevice() arms the disc-swap sequence whenever one device replaces another, and Update() drove NO_MEDIUM -> UNIT_ATTENTION without ever consulting the ejected latch. So adopting a stand-in put it in the drive 100 ms later. At boot m_pDevice is null, no swap is armed, and that transition never runs - which is exactly why the boot restore looked correct and hid this for as long as the boot restore has existed. The gadget was internally inconsistent while it happened: IsEjected() kept returning true the whole time it was serving READ(10) from the disc it had supposedly ejected. Update() now refuses the transition while ejected. Insert() is unaffected and still works - the drive is empty, not disabled. Test adopting_an_image_while_ejected_stays_ejected pins it, and fails without the fix on all three of the sense key, the additional sense code, and the READ(10) status. Also: the file list no longer names the stand-in as the current image. It is what the gadget holds open, not what the user mounted, and calling it "current" while the drive reads as empty is what made a vanished image look like an ordinary disc swap. m_CurrentImagePath still points at it so the FTP and web delete guards keep protecting it. --- addon/scsitbservice/scsitbservice.cpp | 22 ++++++-- addon/usbcdgadget/tcdstate_update.cpp | 17 ++++++ .../test-suite/test_mediachange.cpp | 53 +++++++++++++++++++ 3 files changed, 89 insertions(+), 3 deletions(-) diff --git a/addon/scsitbservice/scsitbservice.cpp b/addon/scsitbservice/scsitbservice.cpp index 6d3960f1..41102067 100644 --- a/addon/scsitbservice/scsitbservice.cpp +++ b/addon/scsitbservice/scsitbservice.cpp @@ -624,10 +624,24 @@ void SCSITBService::ProcessPendingMount() { cdromservice->SetDevice(imageDevice); // Committed only now that the disc is really the one the host has. + // + // m_CurrentImagePath is the file the gadget holds open, and is set either + // way: the delete/rename/overwrite guards key off it, and a stand-in is + // just as destructive to yank away as a chosen disc. strncpy(m_CurrentImagePath, candidatePath, sizeof(m_CurrentImagePath) - 1); m_CurrentImagePath[sizeof(m_CurrentImagePath) - 1] = '\0'; - strncpy(m_MountedRelativePath, relativePath, sizeof(m_MountedRelativePath) - 1); - m_MountedRelativePath[sizeof(m_MountedRelativePath) - 1] = '\0'; + + // m_MountedRelativePath is what the USER has mounted, and a stand-in is + // not that - the drive is empty. Leaving it clear resolves current_cd to + // -1, so the file list highlights nothing and GetCurrentCDName() returns + // "", which is what the host sees. Naming the stand-in as "current" read + // as an ordinary disc swap and hid the empty drive completely. + if (!adoptAsEmpty) { + strncpy(m_MountedRelativePath, relativePath, sizeof(m_MountedRelativePath) - 1); + m_MountedRelativePath[sizeof(m_MountedRelativePath) - 1] = '\0'; + } else { + m_MountedRelativePath[0] = '\0'; + } m_LastFailedRelativePath[0] = '\0'; // Save relative path to config (without "1:/" prefix). @@ -643,7 +657,9 @@ void SCSITBService::ProcessPendingMount() { m_MissingSavedImage[0] = '\0'; } - current_cd = next_cd; + // Nothing is current when the drive is empty, so the file list agrees with + // what the host can see rather than contradicting it. + current_cd = adoptAsEmpty ? -1 : next_cd; next_cd = -1; m_MountSeq++; } diff --git a/addon/usbcdgadget/tcdstate_update.cpp b/addon/usbcdgadget/tcdstate_update.cpp index ef08bbf5..7d1afdd6 100644 --- a/addon/usbcdgadget/tcdstate_update.cpp +++ b/addon/usbcdgadget/tcdstate_update.cpp @@ -37,6 +37,23 @@ void CUSBCDGadget::Update() switch (m_mediaState) { case MediaState::NO_MEDIUM: + // An ejected drive must never put a medium in by itself. + // + // SetDevice() arms this sequence whenever one device replaces + // another, and it used to run the transition below without + // consulting the ejected latch - so adopting an image while + // ejected re-inserted it 100 ms later and undid the eject. At + // boot m_pDevice is null, no swap is armed, and the path never + // runs, which is why the boot restore looked correct while the + // same thing on a running system silently put a disc back. + if (m_bEjected) + { + m_bPendingDiscSwap = false; + CDROM_DEBUG_LOG("CUSBCDGadget::Update", + "Disc swap: drive is ejected, staying NO_MEDIUM"); + break; + } + // Stage 2: Transition from NO_MEDIUM to UNIT_ATTENTION CTraceLab::Get()->TraceMediaState((u8)m_mediaState, (u8)MediaState::MEDIUM_PRESENT_UNIT_ATTENTION); m_CDReady = true; diff --git a/integration-tests/test-suite/test_mediachange.cpp b/integration-tests/test-suite/test_mediachange.cpp index 504df307..e3a3dcb3 100644 --- a/integration-tests/test-suite/test_mediachange.cpp +++ b/integration-tests/test-suite/test_mediachange.cpp @@ -176,3 +176,56 @@ TEST(gesn_async_notification_unsupported) CHECK_EQ(s.data[12], 0x24); // INVALID FIELD IN CDB CHECK_EQ(s.data[13], 0x00); } + +// Adopting an image while the drive is ejected must leave it ejected. +// +// This is the "saved image went missing" path: a stand-in image is loaded so +// the gadget has a geometry, with the boot-eject armed so the host still sees +// an empty drive. It worked at boot and silently failed at runtime, because at +// boot m_pDevice is null and SetDevice() therefore arms no disc swap - whereas +// replacing a device on a running system does, and Update() drove +// NO_MEDIUM -> UNIT_ATTENTION without ever consulting the ejected latch. The +// stand-in appeared in the host 100 ms later, so a renamed image looked like +// it had simply been swapped for a different game. +TEST(adopting_an_image_while_ejected_stays_ejected) +{ + CFakeImageDevice *discA = MakeDataISO(500); + CGadgetTestBench bench(discA); + bench.Activate(); + bench.RequestSense(); + + // Sanity: disc A is really readable before we eject. + CHECK_EQ(TestUnitReady(bench).csw.bmCSWStatus, 0); + + // Arm the boot-eject and adopt a stand-in, exactly as ProcessPendingMount + // does when the remembered image is no longer on the card. + bench.gadget->ArmBootEject(); + CFakeImageDevice *standIn = MakeDataISO(1500); + bench.gadget->SetDevice(standIn); + + // Empty drive: TEST UNIT READY fails with NOT READY / MEDIUM NOT PRESENT. + CHECK_EQ(TestUnitReady(bench).csw.bmCSWStatus, 1); + + // Now let the swap settle window elapse several times over. Nothing here + // may put the stand-in into the drive. + for (int i = 0; i < 5; i++) + SettleDiscSwap(bench); + + CHECK(bench.gadget->IsEjected()); + CHECK_EQ(TestUnitReady(bench).csw.bmCSWStatus, 1); + { + auto s = bench.RequestSense(); + CHECK(s.data.size() >= 14); + CHECK_EQ(s.data[2], 0x02); // NOT READY + CHECK_EQ(s.data[12], 0x3a); // MEDIUM NOT PRESENT + } + // And no data can be read out of a drive the user was told is empty. + CHECK_EQ(Read10LBA0(bench).csw.bmCSWStatus, 1); + + // A deliberate insert still works - the drive is empty, not broken. + bench.gadget->Insert(); + for (int i = 0; i < 5; i++) + SettleDiscSwap(bench); + bench.RequestSense(); // clear the medium-changed UNIT ATTENTION + CHECK_EQ(TestUnitReady(bench).csw.bmCSWStatus, 0); +}