Update status code handling in initiator tools and error handling

uweseimet · Jan 6, 2025 · e695a7a · e695a7a
1 parent 4429db4
commit e695a7a
Show file tree

Hide file tree

Showing 7 changed files with 29 additions and 34 deletions.
diff --git a/cpp/buses/bus.cpp b/cpp/buses/bus.cpp
@@ -4,7 +4,7 @@
 //
 // Copyright (C) 2001-2006 ＰＩ．([email protected])
 // Copyright (C) 2014-2020 GIMONS
-// Copyright (C) 2022-2024 Uwe Seimet
+// Copyright (C) 2022-2025 Uwe Seimet
 //
 //---------------------------------------------------------------------------
 
@@ -20,7 +20,7 @@ bool Bus::Init(bool target)
     return true;
 }
 
-int Bus::CommandHandShake(vector<uint8_t> &buf)
+int Bus::CommandHandShake(span<uint8_t> buf)
 {
     DisableIRQ();
 
@@ -47,7 +47,6 @@ int Bus::CommandHandShake(vector<uint8_t> &buf)
     // Most other host adapters (e.g. LINK96/97 and the one by Inventronik) and also several devices (e.g.
     // UltraSatan or GigaFile) that can directly be connected to the Atari's ACSI port also support ICD
     // semantics. In fact, these semantics have become a standard in the Atari world.
-    // SCSi2Pi becomes ICD compatible by ignoring the prepended $1F byte before processing the CDB.
     if (buf[0] == 0x1f) {
         SetREQ(true);
 
@@ -75,19 +74,15 @@ int Bus::CommandHandShake(vector<uint8_t> &buf)
         return 0;
     }
 
-    int offset = 0;
-
     int bytes_received;
     for (bytes_received = 1; bytes_received < command_byte_count; ++bytes_received) {
-        ++offset;
-
         SetREQ(true);
 
         ack = WaitSignal(PIN_ACK, true);
 
         WaitBusSettle();
 
-        buf[offset] = GetDAT();
+        buf[bytes_received] = GetDAT();
 
         SetREQ(false);
 

diff --git a/cpp/buses/bus.h b/cpp/buses/bus.h
@@ -4,7 +4,7 @@
 //
 // Copyright (C) 2001-2006 ＰＩ．([email protected])
 // Copyright (C) 2014-2020 GIMONS
-// Copyright (C) 2022-2024 Uwe Seimet
+// Copyright (C) 2022-2025 Uwe Seimet
 //
 //---------------------------------------------------------------------------
 
@@ -116,7 +116,7 @@ class Bus // NOSONAR The number of convenience methods is justified
 
     virtual bool WaitSignal(int, bool);
 
-    int CommandHandShake(vector<uint8_t>&);
+    int CommandHandShake(span<uint8_t>);
     int MsgInHandShake();
     int ReceiveHandShake(uint8_t*, int);
     int SendHandShake(const uint8_t*, int, int = SEND_NO_DELAY);

diff --git a/cpp/initiator/initiator_executor.cpp b/cpp/initiator/initiator_executor.cpp
@@ -84,7 +84,7 @@ int InitiatorExecutor::Execute(span<uint8_t> cdb, span<uint8_t> buffer, int leng
     }
 
     if (enable_log) {
-        LogStatus();
+        initiator_logger.warn(GetStatusString(status_code));
     }
 
     return status_code;
@@ -337,18 +337,3 @@ void InitiatorExecutor::SetTarget(int id, int lun, bool s)
     sasi = s;
 }
 
-void InitiatorExecutor::LogStatus() const
-{
-    if (status_code) {
-        if (const auto &it = STATUS_MAPPING.find(static_cast<StatusCode>(status_code)); it != STATUS_MAPPING.end()) {
-            initiator_logger.warn("Device reported {0} (status code ${1:02x})", it->second, status_code);
-        }
-        else if (status_code != 0xff) {
-            initiator_logger.warn("Device reported an unknown status (status code ${:02x})", status_code);
-        }
-        else {
-            initiator_logger.warn("Device did not respond");
-        }
-    }
-}
-
diff --git a/cpp/initiator/initiator_executor.h b/cpp/initiator/initiator_executor.h
@@ -72,8 +72,6 @@ class InitiatorExecutor
     bool WaitForFree() const;
     bool WaitForBusy() const;
 
-    void LogStatus() const;
-
     void Sleep(const timespec &ns) const
     {
         nanosleep(&ns, nullptr);

diff --git a/cpp/s2pexec/s2pexec_core.cpp b/cpp/s2pexec/s2pexec_core.cpp
@@ -485,16 +485,18 @@ tuple<SenseKey, Asc, int> S2pExec::ExecuteCommand()
         }
     }
 
-    const int status = executor->ExecuteCommand(cdb, buffer, timeout, true);
-    if (status) {
-        if (status != 0xff) {
+    const int status_code = executor->ExecuteCommand(cdb, buffer, timeout, true);
+    if (status_code) {
+        if (status_code != 0xff) {
             if (request_sense) {
                 return executor->GetSenseData();
             }
+
+            throw execution_exception(GetStatusString(status_code));
         }
         else {
-            throw execution_exception(fmt::format("Can't execute command {}",
-                CommandMetaData::Instance().GetCommandName(static_cast<ScsiCommand>(cdb[0]))));
+            throw execution_exception(fmt::format("Can't execute command {0} (${1:2x})",
+                CommandMetaData::Instance().GetCommandName(static_cast<ScsiCommand>(cdb[0])), cdb[0]));
         }
     }
 

diff --git a/cpp/shared/s2p_util.cpp b/cpp/shared/s2p_util.cpp
@@ -245,6 +245,19 @@ string s2p_util::GetScsiLevel(int scsi_level)
     }
 }
 
+string s2p_util::GetStatusString(int status_code)
+{
+    if (const auto &it = STATUS_MAPPING.find(static_cast<StatusCode>(status_code)); it != STATUS_MAPPING.end()) {
+        return fmt::format("Device reported {0} (status code ${1:02x})", it->second, status_code);
+    }
+    else if (status_code != 0xff) {
+        return fmt::format("Device reported an unknown status (status code ${:02x})", status_code);
+    }
+    else {
+        return "Device did not respond";
+    }
+}
+
 string s2p_util::FormatSenseData(span<const byte> sense_data)
 {
     const auto flags = static_cast<int>(sense_data[2]);

diff --git a/cpp/shared/s2p_util.h b/cpp/shared/s2p_util.h
@@ -2,7 +2,7 @@
 //
 // SCSI2Pi, SCSI device emulator and SCSI tools for the Raspberry Pi
 //
-// Copyright (C) 2021-2024 Uwe Seimet
+// Copyright (C) 2021-2025 Uwe Seimet
 //
 //---------------------------------------------------------------------------
 
@@ -70,6 +70,8 @@ tuple<string, string, string> GetInquiryProductData(span<const uint8_t>);
 
 string GetScsiLevel(int);
 
+string GetStatusString(int);
+
 string FormatSenseData(span<const byte>);
 string FormatSenseData(SenseKey, Asc, int = 0);