WiimoteEmu: Cleanup ack handling. Improve accuracy of report handling with unusual values. Eliminated outdated comments.

Source/Core/Core/HW/WiimoteCommon/WiimoteConstants.h

@@ -52,9 +52,9 @@ enum class WiimoteAddressSpace : u8
 enum class WiimoteErrorCode : u8
 {
   SUCCESS = 0,
+  INVALID_SPACE = 6,
   NACK = 7,
-  // TODO: Can there be a better name for this?
+  INVALID_ADDRESS = 8,
-  INVALID = 8,
 };
 
 constexpr u8 MAX_PAYLOAD = 23;

Source/Core/Core/HW/WiimoteCommon/WiimoteReport.h

@@ -16,6 +16,7 @@
 typedef std::vector<u8> Report;
 
 // Report defines
+// TODO: enum classes
 enum ReportType
 {
   RT_RUMBLE = 0x10,
@@ -184,6 +185,7 @@ union wm_nc
 };
 static_assert(sizeof(wm_nc) == 6, "Wrong size");
 
+// TODO: kill/move these:
 union wm_classic_extension_buttons
 {
   u16 hex;
@@ -375,10 +377,12 @@ struct wm_report
     u8 data[0];
     struct
     {
-      u8 rumble : 1;  // enable/disable rumble
+      // Enable/disable rumble. (Valid for ALL output reports)
-      // only valid for certain reports
+      u8 rumble : 1;
-      u8 ack : 1;     // respond with an ack
+      // Respond with an ack. (Only valid for certain reports)
-      u8 enable : 1;  // enable/disable certain features
+      u8 ack : 1;
+      // Enable/disable certain features. (Only valid for certain reports)
+      u8 enable : 1;
     };
   };
 };
@@ -387,7 +391,6 @@ static_assert(sizeof(wm_report) == 2, "Wrong size");
 struct wm_leds
 {
   u8 rumble : 1;
-  // real Wii also sets bit 0x2 (unknown purpose)
   u8 : 3;
   u8 leds : 4;
 };
@@ -396,8 +399,7 @@ static_assert(sizeof(wm_leds) == 1, "Wrong size");
 struct wm_report_mode
 {
   u8 rumble : 1;
-  // unsure what "all_the_time" actually is, the real Wii does set it (bit 0x2)
+  u8 : 1;
-  u8 all_the_time : 1;
   u8 continuous : 1;
   u8 : 5;
   u8 mode;
@@ -419,7 +421,7 @@ struct wm_status_report
   u8 speaker : 1;
   u8 ir : 1;
   u8 leds : 4;
-  u8 padding2[2];  // two 00, TODO: this needs more investigation
+  u8 padding2[2];
   u8 battery;
 };
 static_assert(sizeof(wm_status_report) == 6, "Wrong size");

Source/Core/Core/HW/WiimoteEmu/EmuSubroutines.cpp

@@ -49,15 +49,13 @@ void Wiimote::ReportMode(const wm_report_mode* const dr)
 
   // TODO: A real wiimote sends a report immediately.
   // even on REPORT_CORE and continuous off when the buttons haven't changed.
+  // But.. it is sent after the ACK
 
   // INFO_LOG(WIIMOTE, "Set data report mode");
   // DEBUG_LOG(WIIMOTE, "  Rumble: %x", dr->rumble);
   // DEBUG_LOG(WIIMOTE, "  Continuous: %x", dr->continuous);
-  // DEBUG_LOG(WIIMOTE, "  All The Time: %x", dr->all_the_time);
   // DEBUG_LOG(WIIMOTE, "  Mode: 0x%02x", dr->mode);
 
-  // TODO: what does the 'all_the_time' bit really do?
-  // m_reporting_auto = dr->all_the_time;
   m_reporting_auto = dr->continuous;
   m_reporting_mode = dr->mode;
 }
@@ -86,52 +84,50 @@ void Wiimote::HidOutputReport(const wm_report* const sr, const bool send_ack)
 
   switch (sr->wm)
   {
-  case RT_RUMBLE:  // 0x10
+  case RT_RUMBLE:
-    // this is handled above
+    // This is handled above.
-    return;  // no ack
+    // A real wiimote never ACKs a rumble report:
+    return;
     break;
 
-  case RT_LEDS:  // 0x11
+  case RT_LEDS:
     // INFO_LOG(WIIMOTE, "Set LEDs: 0x%02x", sr->data[0]);
     m_status.leds = sr->data[0] >> 4;
     break;
 
-  case RT_REPORT_MODE:  // 0x12
+  case RT_REPORT_MODE:
     ReportMode(reinterpret_cast<const wm_report_mode*>(sr->data));
     break;
 
-  case RT_IR_PIXEL_CLOCK:  // 0x13
+  case RT_IR_PIXEL_CLOCK:
     // INFO_LOG(WIIMOTE, "WM IR Clock: 0x%02x", sr->data[0]);
     // Camera data is currently always updated. Ignoring pixel clock status.
-    // m_ir_clock = sr->enable;
-    if (false == sr->ack)
-      return;
     break;
 
-  case RT_SPEAKER_ENABLE:  // 0x14
+  case RT_SPEAKER_ENABLE:
-    // ERROR_LOG(WIIMOTE, "WM Speaker Enable: %02x", sr->enable);
+    // INFO_LOG(WIIMOTE, "WM Speaker Enable: %02x", sr->enable);
-    // PanicAlert( "WM Speaker Enable: %d", sr->data[0] );
     m_status.speaker = sr->enable;
-    if (false == sr->ack)
-      return;
     break;
 
-  case RT_REQUEST_STATUS:  // 0x15
+  case RT_REQUEST_STATUS:
     RequestStatus(reinterpret_cast<const wm_request_status*>(sr->data));
-    return;  // sends its own ack
+    // No ACK:
+    return;
     break;
 
-  case RT_WRITE_DATA:  // 0x16
+  case RT_WRITE_DATA:
     WriteData(reinterpret_cast<const wm_write_data*>(sr->data));
-    return;  // sends its own ack
+    // Sends it's own ACK conditionally:
+    return;
     break;
 
-  case RT_READ_DATA:  // 0x17
+  case RT_READ_DATA:
     ReadData(reinterpret_cast<const wm_read_data*>(sr->data));
-    return;  // sends its own ack/reply
+    // No ACK:
+    return;
     break;
 
-  case RT_WRITE_SPEAKER_DATA:  // 0x18
+  case RT_WRITE_SPEAKER_DATA:
     // Not sure if speaker mute stops the bus write on real hardware, but it's not that important
     if (!m_speaker_mute)
     {
@@ -140,29 +136,27 @@ void Wiimote::HidOutputReport(const wm_report* const sr, const bool send_ack)
         PanicAlert("EmuWiimote: bad speaker data length!");
       SpeakerData(sd->data, sd->length);
     }
-    return;  // no ack
+    // No ACK:
+    return;
     break;
 
-  case RT_SPEAKER_MUTE:  // 0x19
+  case RT_SPEAKER_MUTE:
     m_speaker_mute = sr->enable;
-    if (false == sr->ack)
-      return;
     break;
 
-  case RT_IR_LOGIC:  // 0x1a
+  case RT_IR_LOGIC:
     // Camera data is currently always updated. Just saving this for status requests.
     m_status.ir = sr->enable;
-    if (false == sr->ack)
-      return;
     break;
 
   default:
     PanicAlert("HidOutputReport: Unknown channel 0x%02x", sr->wm);
-    return;  // no ack
+    return;
     break;
   }
 
-  SendAck(sr->wm);
+  if (sr->ack)
+    SendAck(sr->wm);
 }
 
 /* This will generate the 0x22 acknowledgement for most Input reports.
@@ -219,8 +213,6 @@ void Wiimote::HandleExtensionSwap()
 
 void Wiimote::RequestStatus(const wm_request_status* const rs)
 {
-  // INFO_LOG(WIIMOTE, "Wiimote::RequestStatus");
-
   // update status struct
   m_status.extension = m_extension_port.IsDeviceConnected();
   // Battery levels in voltage
@@ -229,8 +221,7 @@ void Wiimote::RequestStatus(const wm_request_status* const rs)
   //   0x33 - 0x54: level 3
   //   0x55 - 0xff: level 4
   m_status.battery = (u8)(m_battery_setting->GetValue() * 0xff);
-  // TODO: this right?
+  m_status.battery_low = m_status.battery < 0x20;
-  m_status.battery_low = m_status.battery < 0x33;
 
   // set up report
   TypedHidPacket<wm_status_report> rpt;
@@ -251,18 +242,13 @@ void Wiimote::WriteData(const wm_write_data* const wd)
 {
   u16 address = Common::swap16(wd->address);
 
-  if (0 == wd->size)
-  {
-    // Ignore requests of zero size
-    return;
-  }
-
   INFO_LOG(WIIMOTE, "Wiimote::WriteData: 0x%02x @ 0x%02x @ 0x%02x (%d)", wd->space,
            wd->slave_address, address, wd->size);
 
-  if (wd->size > 16)
+  if (0 == wd->size || wd->size > 16)
   {
-    PanicAlert("WriteData: size is > 16 bytes");
+    WARN_LOG(WIIMOTE, "WriteData: invalid size: %d", wd->size);
+    // A real wiimote silently ignores such a request:
     return;
   }
 
@@ -276,21 +262,23 @@ void Wiimote::WriteData(const wm_write_data* const wd)
 
     if (address + wd->size > WIIMOTE_EEPROM_SIZE)
     {
-      ERROR_LOG(WIIMOTE, "WriteData: address + size out of bounds!");
+      WARN_LOG(WIIMOTE, "WriteData: address + size out of bounds!");
-      PanicAlert("WriteData: address + size out of bounds!");
+      error_code = WiimoteErrorCode::INVALID_ADDRESS;
-      return;
     }
-    memcpy(m_eeprom + address, wd->data, wd->size);
+    else
-
-    // write mii data to file
-    if (address >= 0x0FCA && address < 0x12C0)
     {
-      // TODO Only write parts of the Mii block
+      std::copy_n(wd->data, wd->size, m_eeprom + address);
-      std::ofstream file;
+
-      File::OpenFStream(file, File::GetUserPath(D_SESSION_WIIROOT_IDX) + "/mii.bin",
+      // Write mii data to file
-                        std::ios::binary | std::ios::out);
+      if (address >= 0x0FCA && address < 0x12C0)
-      file.write((char*)m_eeprom + 0x0FCA, 0x02f0);
+      {
-      file.close();
+        // TODO: Only write parts of the Mii block
+        std::ofstream file;
+        File::OpenFStream(file, File::GetUserPath(D_SESSION_WIIROOT_IDX) + "/mii.bin",
+                          std::ios::binary | std::ios::out);
+        file.write((char*)m_eeprom + 0x0FCA, 0x02f0);
+        file.close();
+      }
     }
   }
   break;
@@ -312,7 +300,9 @@ void Wiimote::WriteData(const wm_write_data* const wd)
   break;
 
   default:
-    PanicAlert("WriteData: unimplemented parameters!");
+    WARN_LOG(WIIMOTE, "WriteData: invalid address space: 0x%x", wd->space);
+    // A real wiimote gives error 6:
+    error_code = WiimoteErrorCode::INVALID_SPACE;
     break;
   }
 
@@ -325,7 +315,7 @@ void Wiimote::ReadData(const wm_read_data* const rd)
   if (m_read_request.size)
   {
     // There is already an active read request.
-    // a real wiimote ignores the new one.
+    // A real wiimote ignores the new one.
     return;
   }
 
@@ -375,14 +365,14 @@ bool Wiimote::ProcessReadDataRequest()
   case WiimoteAddressSpace::EEPROM:
   {
     // Read from EEPROM
-    if (m_read_request.address + m_read_request.size >= WIIMOTE_EEPROM_FREE_SIZE)
+    if (m_read_request.address + m_read_request.size > WIIMOTE_EEPROM_FREE_SIZE)
     {
       if (m_read_request.address + m_read_request.size > WIIMOTE_EEPROM_SIZE)
       {
-        ERROR_LOG(WIIMOTE, "ReadData: address + size out of bounds");
+        WARN_LOG(WIIMOTE, "ReadData: address + size out of bounds!");
       }
 
-      // generate a read error, even if the start of the block is readable a real wiimote just sends
+      // Generate a read error. Even if the start of the block is readable a real wiimote just sends
       // error code 8
 
       // The real Wiimote generate an error for the first
@@ -390,8 +380,7 @@ bool Wiimote::ProcessReadDataRequest()
       // read the calibration data at the beginning of Eeprom. I think this
       // error is supposed to occur when we try to read above the freely
       // usable space that ends at 0x16ff.
-      //INFO_LOG(WIIMOTE, "Responding with read error 8.");
+      error_code = WiimoteErrorCode::INVALID_ADDRESS;
-      error_code = WiimoteErrorCode::INVALID;
     }
     else
     {
@@ -417,8 +406,7 @@ bool Wiimote::ProcessReadDataRequest()
   case WiimoteAddressSpace::I2C_BUS:
   case WiimoteAddressSpace::I2C_BUS_ALT:
   {
-    // Read from Control Register
+    // Read from I2C bus
-
     // Top byte of address is ignored on the bus, but it IS maintained in the read-reply.
     auto const bytes_read = m_i2c_bus.BusRead(
         m_read_request.slave_address, (u8)m_read_request.address, bytes_to_read, reply->data);
@@ -436,7 +424,9 @@ bool Wiimote::ProcessReadDataRequest()
   break;
 
   default:
-    PanicAlert("Wiimote::ReadData: invalid address space (space: 0x%x)!", m_read_request.space);
+    WARN_LOG(WIIMOTE, "ReadData: invalid address space: 0x%x", m_read_request.space);
+    // A real wiimote gives error 6:
+    error_code = WiimoteErrorCode::INVALID_SPACE;
     break;
   }
 
@@ -449,7 +439,7 @@ bool Wiimote::ProcessReadDataRequest()
   }
   else
   {
-    // Modify the read request, zero size == complete
+    // Modify the active read request, zero size == complete
     m_read_request.address += bytes_to_read;
     m_read_request.size -= bytes_to_read;
   }

Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.cpp

@@ -314,14 +314,15 @@ static const char* const named_buttons[] = {
 
 void Wiimote::Reset()
 {
-  // TODO: Is a wiimote in CORE or DISABLED reporting mode on sync?
+  // Wiimote starts in non-continuous CORE mode:
-  m_reporting_mode = RT_REPORT_DISABLED;
+  m_reporting_mode = RT_REPORT_CORE;
   m_reporting_channel = 0;
   m_reporting_auto = false;
 
   m_rumble_on = false;
   m_speaker_mute = false;
 
+  // TODO: Can probably just set this to the desired extension right away?
   m_extension->active_extension = 0;
 
   // eeprom
@@ -889,8 +890,7 @@ void Wiimote::Update()
       return;
   }
 
-  u8 data[MAX_PAYLOAD];
+  u8 data[MAX_PAYLOAD] = {};
-  memset(data, 0, sizeof(data));
 
   Movie::SetPolledDevice();
 
@@ -939,7 +939,6 @@ void Wiimote::Update()
     // acceleration
     if (rptf.accel_size)
     {
-      // TODO: GetAccelData has hardcoded payload offsets..
       s16 x, y, z;
       GetAccelData(&x, &y, &z);
 
@@ -983,8 +982,12 @@ void Wiimote::Update()
     if (rptf.ir_size)
     {
       if (!m_status.ir)
+      {
+        // TODO: Does a real wiimote send 0xFFs in this case?
         WARN_LOG(WIIMOTE, "Game is reading IR data without enabling IR logic first.");
+      }
 
+      // The real wiimote reads camera data from the i2c bus at offset 0x37:
       u8 camera_data_offset = offsetof(IRCameraLogic::RegData, camera_data);
 
       if (RT_REPORT_INTERLEAVE2 == m_reporting_mode)
@@ -1218,7 +1221,10 @@ void Wiimote::ExtensionLogic::Update()
 template <typename T>
 void SetBit(T& value, u32 bit_number, bool bit_value)
 {
-  bit_value ? value |= (1 << bit_number) : value &= ~(1 << bit_number);
+  if (bit_value)
+    value |= (1 << bit_number);
+  else
+    value &= ~(1 << bit_number);
 }
 
 void Wiimote::MotionPlusLogic::Update()
@@ -1303,13 +1309,13 @@ void Wiimote::MotionPlusLogic::Update()
   {
     switch (GetPassthroughMode())
     {
-    case PassthroughMode::PASSTHROUGH_DISABLED:
+    case PassthroughMode::DISABLED:
     {
       // Passthrough disabled, always send M+ data:
       mplus_data.is_mp_data = true;
       break;
     }
-    case PassthroughMode::PASSTHROUGH_NUNCHUK:
+    case PassthroughMode::NUNCHUK:
     {
       if (ext_amt == i2c_bus.BusRead(ext_slave, ext_addr, ext_amt, data))
       {
@@ -1336,7 +1342,7 @@ void Wiimote::MotionPlusLogic::Update()
       }
       break;
     }
-    case PassthroughMode::PASSTHROUGH_CLASSIC:
+    case PassthroughMode::CLASSIC:
     {
       if (ext_amt == i2c_bus.BusRead(ext_slave, ext_addr, ext_amt, data))
       {

Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.h

@@ -413,6 +413,9 @@ private:
     struct RegData
     {
       // Contains sensitivity and other unknown data
+      // TODO: Do the IR and Camera enabling reports write to the i2c bus?
+      // TODO: does disabling the camera peripheral reset the mode or sensitivity?
+      // TODO: break out this "data" array into some known members
       u8 data[0x33];
       u8 mode;
       u8 unk[3];
@@ -475,12 +478,12 @@ private:
       // Check if encrypted reads is on
       if (0xaa == reg_data.encryption)
       {
-        //INFO_LOG(WIIMOTE, "Encrypted read.");
+        // INFO_LOG(WIIMOTE, "Encrypted read.");
         WiimoteEncrypt(&ext_key, data_out, addr, (u8)count);
       }
       else
       {
-        //INFO_LOG(WIIMOTE, "Unencrypted read.");
+        // INFO_LOG(WIIMOTE, "Unencrypted read.");
       }
 
       return result;
@@ -509,11 +512,12 @@ private:
 
   struct SpeakerLogic : public I2CSlave
   {
+    // TODO: enum class
     static const u8 DATA_FORMAT_ADPCM = 0x00;
     static const u8 DATA_FORMAT_PCM = 0x40;
 
     // TODO: It seems reading address 0x00 should always return 0xff.
-
+#pragma pack(push, 1)
     struct
     {
       // Speaker reports result in a write of samples to addr 0x00 (which also plays sound)
@@ -522,14 +526,17 @@ private:
       u8 format;
       // seems to always play at 6khz no matter what this is set to?
       // or maybe it only applies to pcm input
+      // Little-endian:
       u16 sample_rate;
       u8 volume;
+      u8 unk_5;
       u8 unk_6;
+      // Reading this byte on real hardware seems to return 0x09:
       u8 unk_7;
-      u8 play;
+      u8 unk_8;
-      u8 unk_9;
+      u8 unknown[0xf6];
-      u8 unknown[0xf4];
     } reg_data;
+#pragma pack(pop)
 
     static_assert(0x100 == sizeof(reg_data));
 
@@ -557,7 +564,6 @@ private:
       }
       else
       {
-        // TODO: should address wrap around after 0xff result in processing speaker data?
         return RawWrite(&reg_data, addr, count, data_in);
       }
     }
@@ -626,9 +632,9 @@ private:
 
     enum class PassthroughMode : u8
     {
-      PASSTHROUGH_DISABLED = 0x04,
+      DISABLED = 0x04,
-      PASSTHROUGH_NUNCHUK = 0x05,
+      NUNCHUK = 0x05,
-      PASSTHROUGH_CLASSIC = 0x07,
+      CLASSIC = 0x07,
     };
 
     bool IsActive() const { return ACTIVE_DEVICE_ADDR << 1 == reg_data.ext_identifier[2]; }
@@ -726,7 +732,7 @@ private:
             // Activate motion plus:
             reg_data.ext_identifier[2] = ACTIVE_DEVICE_ADDR << 1;
             // TODO: kill magic number
-            //reg_data.cert_ready = 0x2;
+            // reg_data.cert_ready = 0x2;
 
             // A real M+ is unresponsive on the bus for some time during activation
             // Reads fail to ack and ext data gets filled with 0xff for a frame or two

Source/Core/Core/HW/WiimoteReal/WiimoteReal.cpp

@@ -98,7 +98,6 @@ void Wiimote::DisableDataReporting()
   // This probably accomplishes nothing.
   wm_report_mode rpt = {};
   rpt.mode = RT_REPORT_CORE;
-  rpt.all_the_time = 0;
   rpt.continuous = 0;
   rpt.rumble = 0;
   QueueReport(RT_REPORT_MODE, &rpt, sizeof(rpt));
@@ -110,7 +109,6 @@ void Wiimote::EnableDataReporting(u8 mode)
 
   wm_report_mode rpt = {};
   rpt.mode = mode;
-  rpt.all_the_time = 1;
   rpt.continuous = 1;
   QueueReport(RT_REPORT_MODE, &rpt, sizeof(rpt));
 }
@@ -445,7 +443,6 @@ void Wiimote::EmuResume()
 
   wm_report_mode rpt = {};
   rpt.mode = wm->m_reporting_mode;
-  rpt.all_the_time = 1;
   rpt.continuous = 1;
   QueueReport(RT_REPORT_MODE, &rpt, sizeof(rpt));
 
@@ -460,7 +457,6 @@ void Wiimote::EmuPause()
 
   wm_report_mode rpt = {};
   rpt.mode = RT_REPORT_CORE;
-  rpt.all_the_time = 0;
   rpt.continuous = 0;
   QueueReport(RT_REPORT_MODE, &rpt, sizeof(rpt));