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/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..555e2b4a 100644 --- a/addon/discimage/util.cpp +++ b/addon/discimage/util.cpp @@ -25,8 +25,31 @@ #include "mdsfile.h" #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'); @@ -200,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; } @@ -207,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; } @@ -246,11 +271,31 @@ 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; } 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); + 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; + } + // Switch to BIN file for data change_extension_to_bin(fullPath); } @@ -260,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; @@ -293,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; } @@ -378,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); @@ -392,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/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 09acde5a..41102067 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; @@ -246,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) { @@ -339,13 +331,27 @@ 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"); - } 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"); + // 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; } + // 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); @@ -371,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 @@ -397,6 +413,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; @@ -437,18 +476,63 @@ 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) { - 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; + + // 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; } } @@ -464,41 +548,148 @@ 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."); next_cd = -1; + m_MountSeq++; 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; + m_MountSeq++; 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. + 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; + m_MountSeq++; return; } LOGNOTE("Loaded image: %s (format: %d, has subchannels: %s)", - m_CurrentImagePath, + candidatePath, (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); - // Save relative path to config (without "1:/" prefix) - configservice->SetCurrentImage(relativePath); + // 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'; + + // 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). + // + // 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; + // 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++; +} + +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 f0830290..5e46e33c 100644 --- a/addon/scsitbservice/scsitbservice.h +++ b/addon/scsitbservice/scsitbservice.h @@ -50,12 +50,65 @@ 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(); 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; } + + /// 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); @@ -81,6 +134,37 @@ class SCSITBService : public CTask bool m_bBootEjectPending = false; bool m_bPersistedEjected = false; + // Set by ProcessPendingMount() on every failure path, cleared on success. + // 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}; + + // 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; + + // 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. + 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/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/addon/webserver/handlers/imagenameapi.cpp b/addon/webserver/handlers/imagenameapi.cpp index 072b3b84..d8954138 100644 --- a/addon/webserver/handlers/imagenameapi.cpp +++ b/addon/webserver/handlers/imagenameapi.cpp @@ -23,9 +23,21 @@ 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 + // 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/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/handlers/pagehandlerbase.cpp b/addon/webserver/handlers/pagehandlerbase.cpp index ca046ebe..8b6bbe4e 100644 --- a/addon/webserver/handlers/pagehandlerbase.cpp +++ b/addon/webserver/handlers/pagehandlerbase.cpp @@ -61,8 +61,32 @@ 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 + // 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)); + } + + // 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/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 diff --git a/addon/webserver/pages/template.html b/addon/webserver/pages/template.html index bac2dd14..abc0cfb8 100644 --- a/addon/webserver/pages/template.html +++ b/addon/webserver/pages/template.html @@ -19,6 +19,19 @@

USB Optical Drive Emulator

+ {{#mount_error}} +
+ Last mount failed: {{mount_error}} +
+ {{/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}} 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)); +} 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); +}