SI_DeviceGBA: refactor and make GBASockServer a member rather than parent #5169

ligfx · 2017-03-26T04:58:23Z

Inspired by, and a replacement for, #5147.

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

+  };
+
+  ConnectionState m_state = ConnectionState::AcceptCommand;
+  u8 m_cmd;


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

    m_timestamp_sent = CoreTiming::GetTicks();
-    m_waiting_for_response = true;
+    m_state = ConnectionState::WaitForTransferTime;


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

+    }
+    else
+    {
+      num_data_received = 5;


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

-
-  if (m_cmd != CMD_STATUS)
-    m_booted = true;
+    status = m_client->send(send_data.data(), 1);


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

  void Send(const u8* si_buffer);
  int Receive(u8* si_buffer);

 private:
+  void Disconnect();


ligfx · 2017-05-04T22:46:59Z

@skidau @endrift I see you've worked on this before, would you mind taking a glance if you have the time?

endrift · 2017-05-04T23:18:13Z

I can't look at this right now, but please don't merge it until I get a chance to look over it. Add me as a reviewer, etc.

ligfx · 2017-05-04T23:20:06Z

@endrift I don't think I can add reviewers, but if you post a review comment I think GitHub will automatically add you.

endrift

This is pretty dang different from the fixes I had for this, but it's worth pointing out that SI_DeviceGBA is quite buggy and incorrect atm. I didn't bother with a PR because I was probably going to yank out a lot of the changes when I got around to Orca, and it would break VBA-M too, but if you're here already, now's a good time to fix them.

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

-
-  if (m_cmd != CMD_STATUS)
-    m_booted = true;
+    status = m_client->send(send_data.data(), 1);


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

+    }
+    if (num_data_received > 0)
+      m_state = ConnectionState::AcceptCommand;
+    return num_data_received;


endrift · 2017-05-05T00:19:21Z

Relevant commits from Orca:
endrift@ed68c3c
endrift@1dba026

endrift

Revised my review with the understanding that you don't want to update the visible logic.

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

@@ -136,39 +128,29 @@ void GBAConnectionWaiter_Shutdown()
    s_connection_thread.join();
 }

-static bool GetAvailableSock(std::unique_ptr<sf::TcpSocket>& sock_to_fill)
+template <typename T>
+static std::unique_ptr<T> MoveFromFront(std::queue<std::unique_ptr<T>>& ptrs)


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

-  if (!m_client)
-    if (!GetAvailableSock(m_client))
-      return;
+  return m_client || (m_client = GetNextSock());


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

-    m_num_data_received = 0;
-    ClockSync();
-    Send(buffer);
+  case ConnectionState::AcceptCommand:


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

+                 buffer[2], buffer[1], buffer[0], buffer[7]);
+#endif
+      m_sock_server.Send(buffer);
+    }


shuffle2 · 2017-06-16T00:17:55Z

Just to state the obvious here, but please make sure to actually test it :D

ligfx · 2017-06-16T00:21:00Z

@shuffle2 tested on Four Swords Adventures (multiple GBAs), Crystal Chronicles (multiple GBAs), Metroid Prime, and Billy Hatcher and the Giant Egg :)

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

+    }
+    else
+    {
+      constexpr u32 reply = SI_ERROR_NO_RESPONSE;


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

+    }
+    else
+    {
+      num_data_received = RECV_MAX_SIZE;


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

  {
-    return 0;
+    int num_data_received = m_sock_server.Receive(buffer);
+    if (!m_sock_server.IsConnected())


endrift · 2017-07-14T00:36:12Z

Ship it

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

-      m_client->receive(m_recv_data.data(), m_recv_data.size(), num_received);
-  if (recv_stat == sf::Socket::Disconnected)
+      m_client->receive(recv_data.data(), recv_data.size(), num_received);
+  if (recv_stat == sf::Socket::Disconnected || recv_stat == sf::Socket::NotReady ||


Inspired by "dolphin-emu#5147: GBASockServer: remove m_device_number (fixes warning)," trying to wrap my head around how this file works.

Slight behavior change, but fills a gaping hole in the state logic.

Rather than returning 0 / not creating an expected SI interrupt. You can test this by running VBA-M in a debugger and stopping it while it's connected to Dolphin: on current master, Dolphin will freeze-up until it gets a response. With this PR, Dolphin will gracefully disconnect the device, and reconnect if it starts responding again.

In the refactore done in dolphin-emu#5169, the logic was slightly altered to no longer wait in case the answer from the GBA is delayed. This broke some games like Final Fantasy Crystal Chronicles. This fixes Final Fantasy Crystal Chronicles GBA connection on trunk.

ligfx force-pushed the sidevicegbastate branch 3 times, most recently from df8fcd6 to 217f989 Compare March 26, 2017 05:10

BhaaLseN reviewed Mar 26, 2017

View reviewed changes

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

};

ConnectionState m_state = ConnectionState::AcceptCommand;

u8 m_cmd;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ligfx mentioned this pull request Mar 31, 2017

GBASockServer: remove m_device_number (fixes warning) #5147

Closed

ligfx force-pushed the sidevicegbastate branch from 217f989 to bf30da7 Compare March 31, 2017 21:30

ligfx changed the title ~~SI_DeviceGBA: clarify request-response state machine~~ SI_DeviceGBA: refactor and make GBASockServer a member rather than parent Mar 31, 2017

ligfx force-pushed the sidevicegbastate branch 6 times, most recently from 193b068 to c2b2be2 Compare March 31, 2017 23:15

BhaaLseN reviewed Apr 1, 2017

View reviewed changes

endrift suggested changes May 5, 2017

View reviewed changes

endrift reviewed May 5, 2017

View reviewed changes

ligfx force-pushed the sidevicegbastate branch 3 times, most recently from 7c97e98 to c2503a4 Compare May 17, 2017 22:06

ligfx force-pushed the sidevicegbastate branch from c2503a4 to df3bdb6 Compare June 15, 2017 23:28

endrift reviewed Jul 13, 2017

View reviewed changes

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

}

else

{

constexpr u32 reply = SI_ERROR_NO_RESPONSE;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

endrift reviewed Jul 13, 2017

View reviewed changes

ligfx force-pushed the sidevicegbastate branch from df3bdb6 to 0978ab1 Compare July 13, 2017 23:16

endrift reviewed Jul 13, 2017

View reviewed changes

ligfx force-pushed the sidevicegbastate branch from 0978ab1 to 61a6a83 Compare July 14, 2017 00:34

endrift approved these changes Jul 14, 2017

View reviewed changes

endrift reviewed Jul 14, 2017

View reviewed changes

ligfx added 10 commits July 14, 2017 13:57

SI_DeviceGBA: clarify request-response state machine

72946a4

Inspired by "dolphin-emu#5147: GBASockServer: remove m_device_number (fixes warning)," trying to wrap my head around how this file works.

Move GBASockServer logging into SI_DeviceGBA

7513772

GBASockServer: cleanup GetNextSock and GetNextClock

89ca32d

GBASockServer: clean up Receive() return value

d30e0ea

SI_DeviceGBA: clean up GetTransferTime()

e6cfc3a

SI_DeviceGBA: remove duplication of GBASockServer destructor logic

ff78327

SI_DeviceGBA: make GBASockServer a member instead of parent

05ab03a

SI_DeviceGBA: extract magic numbers SEND_MAX_SIZE, RECV_MAX_SIZE

8dee8e6

SI_DeviceGBA: use SI_ERROR_NO_RESPONSE when client isn't connected

f004dfa

Slight behavior change, but fills a gaping hole in the state logic.

ligfx force-pushed the sidevicegbastate branch from 61a6a83 to becb1a7 Compare July 14, 2017 20:57

leoetlino merged commit 284aa99 into dolphin-emu:master Aug 4, 2017

Steelskin mentioned this pull request Sep 8, 2020

SI_DeviceGBA: Wait for an answer from the GBA #9069

Closed

SI_DeviceGBA: refactor and make GBASockServer a member rather than parent #5169

SI_DeviceGBA: refactor and make GBASockServer a member rather than parent #5169

Conversation

ligfx commented Mar 26, 2017 • edited

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

ligfx commented May 4, 2017

endrift commented May 4, 2017

ligfx commented May 4, 2017

endrift left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

endrift commented May 5, 2017

endrift left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

shuffle2 commented Jun 16, 2017

ligfx commented Jun 16, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

endrift commented Jul 14, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

ligfx commented Mar 26, 2017 •

edited