SI: Clarify length fields for manual commands

Fix up the calculation of the length fields and check that the returned
response is the expected length. This touches many files because it
converts a parameter name from the SI_Device interface from 'length' to
'request_length'. Prior, this field seemed to be used as request length
sometimes, as response length sometimes, and usually just totally ignored.

Source/Core/Core/HW/SI/SI.cpp

@@ -8,7 +8,9 @@
 #include <array>
 #include <atomic>
 #include <cstring>
+#include <iomanip>
 #include <memory>
+#include <sstream>
 
 #include "Common/ChunkFile.h"
 #include "Common/CommonTypes.h"
@@ -280,31 +282,51 @@ static void GenerateSIInterrupt(SIInterruptType type)
   UpdateInterrupts();
 }
 
+constexpr u32 SI_XFER_LENGTH_MASK = 0x7f;
+
+// Translate [0,1,2,...,126,127] to [128,1,2,...,126,127]
+constexpr u32 ConvertSILengthField(u32 field)
+{
+  return ((field - 1) & SI_XFER_LENGTH_MASK) + 1;
+}
+
 static void RunSIBuffer(u64 user_data, s64 cycles_late)
 {
   if (s_com_csr.TSTART)
   {
-    // Math in_length
-    int in_length = s_com_csr.INLNGTH;
-    if (in_length == 0)
-      in_length = 128;
-    else
-      in_length++;
-
-    // Math out_length
-    int out_length = s_com_csr.OUTLNGTH;
-    if (out_length == 0)
-      out_length = 128;
-    else
-      out_length++;
+    u32 request_length = ConvertSILengthField(s_com_csr.OUTLNGTH);
+    u32 expected_response_length = ConvertSILengthField(s_com_csr.INLNGTH);
+    std::vector<u8> request_copy(s_si_buffer.data(), s_si_buffer.data() + request_length);
 
     std::unique_ptr<ISIDevice>& device = s_channel[s_com_csr.CHANNEL].device;
-    int numOutput = device->RunBuffer(s_si_buffer.data(), in_length);
+    u32 actual_response_length = device->RunBuffer(s_si_buffer.data(), request_length);
 
-    DEBUG_LOG(SERIALINTERFACE, "RunSIBuffer  chan: %d  inLen: %i  outLen: %i  processed: %i",
-              s_com_csr.CHANNEL, in_length, out_length, numOutput);
+    DEBUG_LOG(SERIALINTERFACE,
+              "RunSIBuffer  chan: %d  request_length: %u  expected_response_length: %u  "
+              "actual_response_length: %u",
+              s_com_csr.CHANNEL, request_length, expected_response_length, actual_response_length);
+    if (expected_response_length != actual_response_length)
+    {
+      std::stringstream ss;
+      for (u8 b : request_copy)
+      {
+        ss << std::hex << std::setw(2) << std::setfill('0') << (int)b << ' ';
+      }
+      ERROR_LOG(
+          SERIALINTERFACE,
+          "RunSIBuffer: expected_response_length(%u) != actual_response_length(%u): request: %s",
+          expected_response_length, actual_response_length, ss.str().c_str());
+    }
 
-    if (numOutput != 0)
+    // TODO:
+    // 1) Wait a reasonable amount of time for the result to be available:
+    //    request is N bytes, ends with a stop bit
+    //    response in M bytes, ends with a stop bit
+    //    processing controller-side takes K us (investigate?)
+    //    each bit takes 4us ([3us low/1us high] for a 0, [1us low/3us high] for a 1)
+    //    time until response is available is at least: K + ((N*8 + 1) + (M*8 + 1)) * 4 us
+    // 2) Investigate the timeout period for NOREP0
+    if (actual_response_length != 0)
     {
       s_com_csr.TSTART = 0;
       GenerateSIInterrupt(INT_TCINT);

Source/Core/Core/HW/SI/SI_Device.cpp

@@ -37,15 +37,16 @@ SIDevices ISIDevice::GetDeviceType() const
   return m_device_type;
 }
 
-int ISIDevice::RunBuffer(u8* buffer, int length)
+int ISIDevice::RunBuffer(u8* buffer, int request_length)
 {
 #ifdef _DEBUG
-  DEBUG_LOG(SERIALINTERFACE, "Send Data Device(%i) - Length(%i)   ", m_device_number, length);
+  DEBUG_LOG(SERIALINTERFACE, "Send Data Device(%i) - Length(%i)   ", m_device_number,
+            request_length);
 
   std::string temp;
   int num = 0;
 
-  while (num < length)
+  while (num < request_length)
   {
     temp += StringFromFormat("0x%02x ", buffer[num]);
     num++;

Source/Core/Core/HW/SI/SI_Device.h

@@ -73,7 +73,7 @@ public:
   SIDevices GetDeviceType() const;
 
   // Run the SI Buffer
-  virtual int RunBuffer(u8* buffer, int length);
+  virtual int RunBuffer(u8* buffer, int request_length);
   virtual int TransferInterval();
 
   // Return true on new data

Source/Core/Core/HW/SI/SI_DeviceDanceMat.cpp

@@ -17,24 +17,19 @@ CSIDevice_DanceMat::CSIDevice_DanceMat(SIDevices device, int device_number)
 {
 }
 
-int CSIDevice_DanceMat::RunBuffer(u8* buffer, int length)
+int CSIDevice_DanceMat::RunBuffer(u8* buffer, int request_length)
 {
   // Read the command
   EBufferCommands command = static_cast<EBufferCommands>(buffer[0]);
-
   if (command == CMD_RESET)
   {
-    ISIDevice::RunBuffer(buffer, length);
+    ISIDevice::RunBuffer(buffer, request_length);
 
     u32 id = Common::swap32(SI_DANCEMAT);
     std::memcpy(buffer, &id, sizeof(id));
+    return sizeof(id);
   }
-  else
-  {
-    return CSIDevice_GCController::RunBuffer(buffer, length);
-  }
-
-  return length;
+  return CSIDevice_GCController::RunBuffer(buffer, request_length);
 }
 
 u32 CSIDevice_DanceMat::MapPadStatus(const GCPadStatus& pad_status)

Source/Core/Core/HW/SI/SI_DeviceDanceMat.h

@@ -16,7 +16,7 @@ class CSIDevice_DanceMat : public CSIDevice_GCController
 public:
   CSIDevice_DanceMat(SIDevices device, int device_number);
 
-  int RunBuffer(u8* buffer, int length) override;
+  int RunBuffer(u8* buffer, int request_length) override;
   u32 MapPadStatus(const GCPadStatus& pad_status) override;
   bool GetData(u32& hi, u32& low) override;
 };

Source/Core/Core/HW/SI/SI_DeviceGBA.cpp

@@ -290,7 +290,7 @@ CSIDevice_GBA::CSIDevice_GBA(SIDevices device, int device_number) : ISIDevice(de
 {
 }
 
-int CSIDevice_GBA::RunBuffer(u8* buffer, int length)
+int CSIDevice_GBA::RunBuffer(u8* buffer, int request_length)
 {
   switch (m_next_action)
   {

Source/Core/Core/HW/SI/SI_DeviceGBA.h

@@ -45,7 +45,7 @@ class CSIDevice_GBA : public ISIDevice
 public:
   CSIDevice_GBA(SIDevices device, int device_number);
 
-  int RunBuffer(u8* buffer, int length) override;
+  int RunBuffer(u8* buffer, int request_length) override;
   int TransferInterval() override;
   bool GetData(u32& hi, u32& low) override;
   void SendCommand(u32 command, u8 poll) override;

Source/Core/Core/HW/SI/SI_DeviceGCAdapter.cpp

@@ -49,7 +49,7 @@ GCPadStatus CSIDevice_GCAdapter::GetPadStatus()
   return pad_status;
 }
 
-int CSIDevice_GCAdapter::RunBuffer(u8* buffer, int length)
+int CSIDevice_GCAdapter::RunBuffer(u8* buffer, int request_length)
 {
   if (!Core::WantsDeterminism())
   {
@@ -66,7 +66,7 @@ int CSIDevice_GCAdapter::RunBuffer(u8* buffer, int length)
       return 4;
     }
   }
-  return CSIDevice_GCController::RunBuffer(buffer, length);
+  return CSIDevice_GCController::RunBuffer(buffer, request_length);
 }
 
 bool CSIDevice_GCAdapter::GetData(u32& hi, u32& low)

Source/Core/Core/HW/SI/SI_DeviceGCAdapter.h

@@ -16,7 +16,7 @@ public:
   CSIDevice_GCAdapter(SIDevices device, int device_number);
 
   GCPadStatus GetPadStatus() override;
-  int RunBuffer(u8* buffer, int length) override;
+  int RunBuffer(u8* buffer, int request_length) override;
 
   bool GetData(u32& hi, u32& low) override;
 

Source/Core/Core/HW/SI/SI_DeviceGCController.cpp

@@ -36,17 +36,17 @@ CSIDevice_GCController::CSIDevice_GCController(SIDevices device, int device_numb
   m_origin.substick_y = GCPadStatus::C_STICK_CENTER_Y;
 }
 
-int CSIDevice_GCController::RunBuffer(u8* buffer, int length)
+int CSIDevice_GCController::RunBuffer(u8* buffer, int request_length)
 {
   // For debug logging only
-  ISIDevice::RunBuffer(buffer, length);
+  ISIDevice::RunBuffer(buffer, request_length);
 
   GCPadStatus pad_status = GetPadStatus();
   if (!pad_status.isConnected)
   {
     u32 reply = Common::swap32(SI_ERROR_NO_RESPONSE);
     std::memcpy(buffer, &reply, sizeof(reply));
-    return 4;
+    return sizeof(reply);
   }
 
   // Read the command
@@ -60,21 +60,21 @@ int CSIDevice_GCController::RunBuffer(u8* buffer, int length)
   {
     u32 id = Common::swap32(SI_GC_CONTROLLER);
     std::memcpy(buffer, &id, sizeof(id));
-    break;
+    return sizeof(id);
   }
 
   case CMD_DIRECT:
   {
-    INFO_LOG(SERIALINTERFACE, "PAD - Direct (Length: %d)", length);
+    INFO_LOG(SERIALINTERFACE, "PAD - Direct (Request length: %d)", request_length);
     u32 high, low;
     GetData(high, low);
-    for (int i = 0; i < (length - 1) / 2; i++)
+    for (int i = 0; i < 4; i++)
     {
       buffer[i + 0] = (high >> (24 - (i * 8))) & 0xff;
       buffer[i + 4] = (low >> (24 - (i * 8))) & 0xff;
     }
+    return sizeof(high) + sizeof(low);
   }
-  break;
 
   case CMD_ORIGIN:
   {
@@ -85,8 +85,8 @@ int CSIDevice_GCController::RunBuffer(u8* buffer, int length)
     {
       buffer[i] = *calibration++;
     }
+    return sizeof(SOrigin);
   }
-  break;
 
   // Recalibrate (FiRES: i am not 100 percent sure about this)
   case CMD_RECALIBRATE:
@@ -98,8 +98,8 @@ int CSIDevice_GCController::RunBuffer(u8* buffer, int length)
     {
       buffer[i] = *calibration++;
     }
+    return sizeof(SOrigin);
   }
-  break;
 
   // DEFAULT
   default:
@@ -110,7 +110,7 @@ int CSIDevice_GCController::RunBuffer(u8* buffer, int length)
   break;
   }
 
-  return length;
+  return 0;
 }
 
 void CSIDevice_GCController::HandleMoviePadStatus(GCPadStatus* pad_status)

Source/Core/Core/HW/SI/SI_DeviceGCController.h

@@ -84,7 +84,7 @@ public:
   CSIDevice_GCController(SIDevices device, int device_number);
 
   // Run the SI Buffer
-  int RunBuffer(u8* buffer, int length) override;
+  int RunBuffer(u8* buffer, int request_length) override;
 
   // Return true on new data
   bool GetData(u32& hi, u32& low) override;

Source/Core/Core/HW/SI/SI_DeviceGCSteeringWheel.cpp

@@ -20,10 +20,10 @@ CSIDevice_GCSteeringWheel::CSIDevice_GCSteeringWheel(SIDevices device, int devic
 {
 }
 
-int CSIDevice_GCSteeringWheel::RunBuffer(u8* buffer, int length)
+int CSIDevice_GCSteeringWheel::RunBuffer(u8* buffer, int request_length)
 {
   // For debug logging only
-  ISIDevice::RunBuffer(buffer, length);
+  ISIDevice::RunBuffer(buffer, request_length);
 
   // Read the command
   EBufferCommands command = static_cast<EBufferCommands>(buffer[0]);
@@ -36,14 +36,11 @@ int CSIDevice_GCSteeringWheel::RunBuffer(u8* buffer, int length)
   {
     u32 id = Common::swap32(SI_GC_STEERING);
     std::memcpy(buffer, &id, sizeof(id));
-    break;
+    return sizeof(id);
+  }
   }
 
-  default:
-    return CSIDevice_GCController::RunBuffer(buffer, length);
-  }
-
-  return length;
+  return CSIDevice_GCController::RunBuffer(buffer, request_length);
 }
 
 bool CSIDevice_GCSteeringWheel::GetData(u32& hi, u32& low)

Source/Core/Core/HW/SI/SI_DeviceGCSteeringWheel.h

@@ -13,7 +13,7 @@ class CSIDevice_GCSteeringWheel : public CSIDevice_GCController
 public:
   CSIDevice_GCSteeringWheel(SIDevices device, int device_number);
 
-  int RunBuffer(u8* buffer, int length) override;
+  int RunBuffer(u8* buffer, int request_length) override;
   bool GetData(u32& hi, u32& low) override;
   void SendCommand(u32 command, u8 poll) override;
 

Source/Core/Core/HW/SI/SI_DeviceKeyboard.cpp

@@ -21,10 +21,10 @@ CSIDevice_Keyboard::CSIDevice_Keyboard(SIDevices device, int device_number)
 {
 }
 
-int CSIDevice_Keyboard::RunBuffer(u8* buffer, int length)
+int CSIDevice_Keyboard::RunBuffer(u8* buffer, int request_length)
 {
   // For debug logging only
-  ISIDevice::RunBuffer(buffer, length);
+  ISIDevice::RunBuffer(buffer, request_length);
 
   // Read the command
   EBufferCommands command = static_cast<EBufferCommands>(buffer[0]);
@@ -37,21 +37,21 @@ int CSIDevice_Keyboard::RunBuffer(u8* buffer, int length)
   {
     u32 id = Common::swap32(SI_GC_KEYBOARD);
     std::memcpy(buffer, &id, sizeof(id));
-    break;
+    return sizeof(id);
   }
 
   case CMD_DIRECT:
   {
-    INFO_LOG(SERIALINTERFACE, "Keyboard - Direct (Length: %d)", length);
+    INFO_LOG(SERIALINTERFACE, "Keyboard - Direct (Request Length: %d)", request_length);
     u32 high, low;
     GetData(high, low);
-    for (int i = 0; i < (length - 1) / 2; i++)
+    for (int i = 0; i < 4; i++)
     {
       buffer[i + 0] = (high >> (24 - (i * 8))) & 0xff;
       buffer[i + 4] = (low >> (24 - (i * 8))) & 0xff;
     }
+    return sizeof(high) + sizeof(low);
   }
-  break;
 
   default:
   {
@@ -60,7 +60,7 @@ int CSIDevice_Keyboard::RunBuffer(u8* buffer, int length)
   break;
   }
 
-  return length;
+  return 0;
 }
 
 KeyboardStatus CSIDevice_Keyboard::GetKeyboardStatus() const

Source/Core/Core/HW/SI/SI_DeviceKeyboard.h

@@ -18,7 +18,7 @@ public:
   CSIDevice_Keyboard(SIDevices device, int device_number);
 
   // Run the SI Buffer
-  int RunBuffer(u8* buffer, int length) override;
+  int RunBuffer(u8* buffer, int request_length) override;
 
   // Return true on new data
   bool GetData(u32& hi, u32& low) override;

Source/Core/Core/HW/SI/SI_DeviceNull.cpp

@@ -14,11 +14,11 @@ CSIDevice_Null::CSIDevice_Null(SIDevices device, int device_number)
 {
 }
 
-int CSIDevice_Null::RunBuffer(u8* buffer, int length)
+int CSIDevice_Null::RunBuffer(u8* buffer, int request_length)
 {
   u32 reply = Common::swap32(SI_ERROR_NO_RESPONSE);
   std::memcpy(buffer, &reply, sizeof(reply));
-  return 4;
+  return sizeof(reply);
 }
 
 bool CSIDevice_Null::GetData(u32& hi, u32& low)

Source/Core/Core/HW/SI/SI_DeviceNull.h

@@ -15,7 +15,7 @@ class CSIDevice_Null final : public ISIDevice
 public:
   CSIDevice_Null(SIDevices device, int device_number);
 
-  int RunBuffer(u8* buffer, int length) override;
+  int RunBuffer(u8* buffer, int request_length) override;
   bool GetData(u32& hi, u32& low) override;
   void SendCommand(u32 command, u8 poll) override;
 };