Come up empty when the saved image is gone, instead of a different disc

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();

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:

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)) {

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);

addon/discimage/util.cpp

@@ -25,8 +25,31 @@
 #include "mdsfile.h"
 #include "chdfile.h"
 
+#include <cueparser/cueutil.h>
+#include <stdarg.h>
+
 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,13 +223,15 @@ 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;
     }
 
     // Create MDS device
     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;
     }
 }

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);

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;

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;