BBA: Added BuiltIn device that allow BBA emulation without the need o… #10690

schthack · 2022-05-22T16:39:49Z

…f a TapDevice Configuration include a dns server setting

This is the squish of all the previous requests in the previous PR. I will try to not mess up the rebase on nexts requested changes....

sepalani

A quick first pass at this PR. I'll check the more technical bit, asap, such as:

the concern about the potential UB: BBA: Tap-less/Built-in network translation #10564 (comment)
the missing assert regarding the trivially copiable struct
the use of u8[] rather than a vector
Etc.

Source/Core/Core/HW/EXI/BBA/BuiltIn.h

sepalani · 2022-05-22T17:08:42Z

Source/Core/Core/HW/EXI/EXI_DeviceEthernet.cpp

@@ -18,6 +18,8 @@
 #include "Core/HW/EXI/EXI.h"
 #include "Core/HW/Memmap.h"

+//#define BBA_TRACK_PAGE_PTRS


This should be removed or added in Dolphin's config system.

The #ifdef for this is preexisting so you don't have to convert this to a setting, but yeah probably remove this commented out line at least.

sepalani · 2022-05-22T17:12:53Z

Source/Core/DolphinQt/Settings/BroadbandAdapterSettingsDialog.cpp

@@ -48,6 +48,15 @@ void BroadbandAdapterSettingsDialog::InitControls()
    window_title = tr("Broadband Adapter MAC Address");
    break;

+  case Type::BuiltIn:
+    address_label = new QLabel(tr("Enter the DNS server to use:"));
+    address_placeholder = QString::fromStdString("8.8.8.8");


This should be QStringLiteral("8.8.8.8").

sepalani · 2022-05-22T17:17:18Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+#include "Core/HW/EXI/EXI_Device.h"
+#include "Core/HW/EXI/EXI_DeviceEthernet.h"
+
+// #define BBA_TRACK_PAGE_PTRS


Should be removed or part of Dolphin's config system.

sepalani · 2022-05-22T17:29:11Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+  m_current_ip = htonl(ip);
+  m_router_ip = (m_current_ip & 0xFFFFFF) | 0x01000000;
+  // clear all ref
+  for (int i = 0; i < std::size(network_ref); i++)


Since network_ref is now a std::array, you can use a range-based for loop:

for (auto& ref : network_ref) ref.ip = 0;

sepalani · 2022-05-22T17:34:28Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+void CEXIETHERNET::BuiltInBBAInterface::WriteToQueue(const u8* data, int length)
+{
+  queue_data[queue_write].resize(length);
+  std::memcpy(&queue_data[queue_write][0], data, length);


queue_data[queue_write].data() should work.

sepalani · 2022-05-22T17:37:20Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+    for (int i = 0; i < std::size(network_ref); i++)
+    {
+      if (network_ref[i].ip != 0 && network_ref[i].local == port)
+        return &network_ref[i];
+    }


A range-based for loop can used.

sepalani · 2022-05-22T17:37:31Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+  for (int i = 0; i < std::size(network_ref); i++)
+  {
+    if (network_ref[i].ip == 0)
+      return &network_ref[i];
+  }


A range-based for loop can used.

sepalani · 2022-05-22T17:37:45Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+  for (int i = 0; i < std::size(network_ref); i++)
+  {
+    if (network_ref[i].ip == ip && network_ref[i].remote == dst_port &&
+        network_ref[i].local == src_port)
+    {
+      return &network_ref[i];
+    }
+  }


A range-based for loop can used.

sepalani · 2022-05-22T17:42:51Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+    if (self->queue_read != self->queue_write)
+    {
+      datasize = self->queue_data[self->queue_read].size();
+      std::memcpy(self->m_eth_ref->mRecvBuffer.get(), &self->queue_data[self->queue_read][0],


self->queue_data[self->queue_read].data() should work.

AdmiralCurtiss · 2022-05-22T18:01:06Z

Source/Core/Core/HW/EXI/EXI_DeviceEthernet.h

@@ -413,6 +418,60 @@ class CEXIETHERNET : public IEXIDevice
 #endif
  };

+  class BuiltInBBAInterface : public NetworkInterface


The member order in this class doesn't match our coding style guide, so unless there's a specific reason you have them like this please reorder them.

Also all of the member variables should have a prefixed m_, only some of them do currently.

AdmiralCurtiss · 2022-05-22T18:13:30Z

Source/Core/Core/CMakeLists.txt

@@ -706,4 +712,4 @@ endif()
 if(MSVC)
  # Add precompiled header
  target_link_libraries(core PRIVATE use_pch)
-endif()
+endif()


You removed the end-of-file newline here.

AdmiralCurtiss · 2022-05-22T18:20:53Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+  ref->ip = 0x08080808;  // change for ip
+  ref->local = htons(port);
+  ref->remote = htons(port);
+  ref->type = 17;


This looks like a magic number, can you name what this is?

AdmiralCurtiss · 2022-05-22T18:21:04Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+    ref->ip = *(u32*)ipdata->destination_addr;  // change for ip
+    ref->local = udpdata->source_port;
+    ref->remote = udpdata->destination_port;
+    ref->type = 17;


AdmiralCurtiss · 2022-05-22T18:21:26Z

Source/Core/Core/HW/EXI/EXI_DeviceEthernet.h

+    void HandleARP(Common::EthernetHeader* hwdata, Common::ARPHeader* arpdata);
+    void HandleDHCP(Common::EthernetHeader* hwdata, Common::UDPHeader* udpdata,
+                    Common::DHCPBody* request);
+    StackRef* GetAvaibleSlot(u16 port);


Typo, Available.

AdmiralCurtiss · 2022-05-22T18:24:12Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+    InitUDPPort(26512);                                 // MK DD and 1080
+    InitUDPPort(26502);                                 // Air Ride


Shouldn't you check/handle if these succeeded? Also, does it make sense to always open both of these?

they are opened only if port 1900 is used, there is no way to tell the game playing from the BBA point of view. This is also a hack to facilitate the communication, it's not 100% required to work. if you have a way to popup a warning without killing dolphin i would add a check. for now only an error log is displayed

I think you want PanicAlertFmt().

AdmiralCurtiss · 2022-05-22T18:28:35Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+}
+
+std::tuple<Common::EthernetHeader*, Common::IPv4Header*, Common::TCPHeader*>
+getTcpHeaders(u8* data, Common::MACAddress dest, Common::MACAddress src)


GetTCPHeaders() per our coding guidelines.

AdmiralCurtiss · 2022-05-22T18:28:45Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+}
+
+std::tuple<Common::EthernetHeader*, Common::IPv4Header*, Common::UDPHeader*>
+getUdpHeaders(u8* data, Common::MACAddress dest, Common::MACAddress src)


GetUDPHeaders() per our coding guidelines.

Source/Core/Core/HW/EXI/BBA/BuiltIn.h

AdmiralCurtiss · 2022-05-22T18:31:50Z

Source/Core/Core/HW/EXI/EXI_DeviceEthernet.cpp

+      // raise back if there is still data
+      if (page_ptr(BBA_RRP) != page_ptr(BBA_RWP))
+      {
+        if (mBbaMem[BBA_IMR] & INT_R)
+        {
+          mBbaMem[BBA_IR] |= INT_R;
+
+          exi_status.interrupt |= exi_status.TRANSFER;
+        }
+      }
+


This and a few other changes in this file look like bugfixes to the existing Ethernet device implementation, rather than directly part of your new built-in BBA implementation. Can you make a separate PR or at least a separate commit for this?

AdmiralCurtiss · 2022-05-22T18:35:08Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+  StackRef* ref = GetAvaibleSlot(htons(port));
+  if (ref == nullptr || ref->ip != 0)
+    return;
+  ref->ip = 0x08080808;  // change for ip


Should this use the configured DNS instead of a fixed IP...?

nolrinale · 2022-05-22T22:53:51Z

Several other users and myself had been testing these builds throughly this whole month and it has been super stable on macOS, Linux and Windows as most of the deal breaker bugs have been squashed already weeks ago, functionality wise I think the feature is working as intended without side effects. And i've been running this for almost 24 hours permanently connected online most days just to make sure.

As a suggestion, in the current state, can this be merged into master as a first version? with the possibility to iterate from there in further PR's refining and optimising the code even more later on?

I would like to hear your thoughts about this

JMC47 · 2022-05-23T00:03:33Z

Regardless of if it works or not, code quality issues need to be taken care of before it is merged.

Arlisbloxer05 · 2022-05-23T00:56:59Z

yeah but like, at this point this is starting to seem more like suggestions rather than proper bug checking...

AdmiralCurtiss · 2022-05-23T01:10:49Z

It's about maintainability, not just bug checking. You want to be able in 5 years to look back on this change and still understand why the code was written like it was, what it is doing, and why it is doing it. And while you can never be sure that this is true, since it's hard to predict the future and all, there's several places here that could stand to be improved with those criteria in mind.

AdmiralCurtiss · 2022-05-23T01:35:43Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+void CEXIETHERNET::BuiltInBBAInterface::HandleARP(Common::EthernetHeader* hwdata,
+                                                  Common::ARPHeader* arpdata)
+{
+  std::memset(m_in_frame.get(), 0, 0x30);
+  Common::EthernetHeader* hwpart = (Common::EthernetHeader*)m_in_frame.get();
+  *hwpart = Common::EthernetHeader(ARP_PROTOCOL);
+  hwpart->destination = *(Common::MACAddress*)&m_eth_ref->mBbaMem[BBA_NAFR_PAR0];
+  hwpart->source = fake_mac;
+
+  Common::ARPHeader* arppart = (Common::ARPHeader*)&m_in_frame[14];
+  Common::MACAddress bba_mac = *(Common::MACAddress*)&m_eth_ref->mBbaMem[BBA_NAFR_PAR0];
+  if (arpdata->target_ip == m_current_ip)
+  {
+    // game asked for himself, reply with his mac address
+    *arppart = Common::ARPHeader(arpdata->target_ip, bba_mac, m_current_ip, bba_mac);
+  }
+  else
+  {
+    *arppart = Common::ARPHeader(arpdata->target_ip, fake_mac, m_current_ip, bba_mac);
+  }
+
+  WriteToQueue(&m_in_frame[0], 0x2a);
+}


Like, take this function for example. Regardless of if it works or not, there's several issues here that make it difficult to understand and possibly change in the future:

Magic numbers. Why is this memsetting 0x30 bytes to zero, but then only writing 0x2a of them? What do those bytes represent, even?

Lots of type punning that violates the C++ spec.

Even if it were allowed, it's hard to mentally process. The writes to hwpart and arppart do indirectly affect the data stored in m_in_frame!

And last but not least, why is this even writing to m_in_frame? Does something else rely on those 0x30 bytes being written? If so, where, and why is there no comment explaining that? If not, why is this not just a 0x30 bytes array on the stack instead?

This code is (unfortunately) full of things like this. These need to be fixed before it can be merged, or we're just creating headaches for future maintainers of this project.

i understands all those issue, sadly im far from being a guru of C++ and the code im digging in right now is far from your todays standard. Since it look like there is still tones of changes to do, I will push one last commit for most of what was requested today, then it would be nice if someone more knoledgeable in C++ could take over....
I can provide information on what it does, like in this case 0x30 fill is to clear the memory to ensure you get the proper value, the original reply was 0x30 and got reduced in the revisions, so 0x30 can now beacome 0x2a, i will review the size.

Most of the code was based on the tapdevice code as a starting point, why i use m_in_frame? beacause it was like that in the tap or xlink device. Im not saying this is the the right way, and i could of just reallocate a new buffer every time, just tought it was faster cpu side to not free/alloc memory, and it looked like this was used like that elsewhere too.

Yeah, that's totally fair! The TAP code in particular is probably 10 years old and our standards were very different back then. And genuinely, I'm impressed you managed to get this to work at all if you're not very C++ knowledgeable.

If you want, someone else can definitely take this over and fix it up for you as long as you're willing to answer any questions we might have about the code.

All valid points guys, thank you all for your hard work.

I can provide my Discord id to the team or the one taking over. I will be happy to awnser and fallow the result to learn more about c++.

Devs usually hang out on IRC (Libera.Chat, #dolphin-dev), but if you prefer Discord join the unofficial Dolphin server and ask for some devs there, some of us are also there.

sepalani

Some more comments regarding the code. I'll try find alternatives to get rid of these undefined behaviours.

sepalani · 2022-05-23T06:55:26Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+{
+  int offset = 0;
+
+  std::memcpy(m_out_frame.get(), frame, size);


m_out_frame is only used here, so it doesn't need to be a member of the structure. This code should be equivalent:

int offset = 0; std::vector<u8> out_frame(frame, frame+size);

The size is also never checked when inspecting the frame bytes so a buffer overflow can occur. I'll try to find a way to address this and the UB parts this code.

sepalani · 2022-05-23T07:00:57Z

Source/Core/Core/HW/EXI/EXI_DeviceEthernet.h

+    defined(__OpenBSD__) || defined(__NetBSD__) || defined(__HAIKU__)
+    std::array<StackRef, 10> network_ref{};  // max 10 at same time, i think most gc game had a
+                                             // limit of 8 in the gc framework
+    std::unique_ptr<u8[]> m_in_frame;


m_in_frame is only used in Handle* methods when creating the frame to be sent. It can be created (as std::vector<u8>) in these Handle* methods instead of being a member variable.

sepalani · 2022-05-23T08:31:42Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+    // process queue file first
+    if (self->m_queue_read != self->m_queue_write)
+    {
+      datasize = self->m_queue_data[self->m_queue_read].size();


AFAICT, mRecvBuffer size is BBA_RECV_SIZE and datasize isn't checked against it.

sepalani · 2022-05-23T08:34:13Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+  switch (ref->type)
+  {
+  case IPPROTO_UDP:
+    ref->udp_socket.receive(&buffer[0x2a], 1500, datasize, ref->target, remote_port);


Where does 1500 come from?

sepalani · 2022-05-23T08:36:22Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+    datasize = 0;
+    const bool can_go = (GetTickCountStd() - ref->poke_time > 100 || ref->window_size > 2000);
+    if (tcp_buffer != nullptr && ref->ready && can_go)
+      st = ref->tcp_socket.receive(&buffer[0x36], 440, datasize);


Where does 440 come from?

AdmiralCurtiss · 2022-05-23T15:54:23Z

Tip: You can rebase this on current master (which now has the #10691 merged) by doing

git fetch upstream
git rebase upstream/master

(if you don't have upstream set first do git remote add upstream https://github.com/dolphin-emu/dolphin.git)

Then verify that it rebased properly, and if so

git push --force

to push it to this PR.

sepalani · 2022-05-23T21:45:05Z

@AdmiralCurtiss @schthack
What do you think of this approach (master...sepalani:pr10690) to address the strict-aliasing and type-puning issues?

I used the PacketView class to read specific bytes in the stream. When enough information is gathered about the type of packet, BitCastPtr is used to return the specialised structure. For simplicity, I haven't implemented the options fields for the IPv4 and TCP header. I also tried my best not to change the code logic and refactor only the part using out_frame to illustrate this approach.

The code compiles, unfortunately, I have no way to test it to make sure I didn't introduce regressions. I also spotted endianness issues when accessing some structures like the EthernetHeader::ethertype and TCPHeader::properties that were accessed but not converted to the host endianness.

schthack · 2022-05-23T22:40:16Z

So i guess something similar would be done to write the frame too?
IP and TCP header have variable size so some of the test need to be adjusted i think, and nice find for the endianness

AdmiralCurtiss · 2022-05-24T01:20:39Z

Yes, I think that should work. I've made a few comments on the commit itself, too, and I think I spotted one behavior change, though it's unclear to me whether that behavior was intentional in the first place.

sepalani · 2022-05-25T19:01:16Z

Yes, I think that should work. I've made a few comments on the commit itself, too, and I think I spotted one behavior change, though it's unclear to me whether that behavior was intentional in the first place.

Good catch, that was an oversight on my end.

So i guess something similar would be done to write the frame too?
IP and TCP header have variable size so some of the test need to be adjusted i think

Yeah, ideally something similar should be done when writing a frame. Regarding the TCP and IP header variable size, they can be taken into account. However, by doing so, it will make increase the code complexity (due to the variable size structure). If you're sure both are needed, I can implement them.

schthack · 2022-05-25T23:30:27Z

However, by doing so, it will make increase the code complexity (due to the variable size structure). If you're sure both are needed, I can implement them.

including the extra data isnt needed, but adjusting the size test and data begining is needed. i havent ran into a special sized IP header but it can happen, for the TCP header i had multiple issue where the data is pushed futher beacause of partial ack and other TCP options

also if you want i can probably take that new code and test/fix

sepalani · 2022-05-28T19:14:55Z

including the extra data isnt needed, but adjusting the size test and data begining is needed. i havent ran into a special sized IP header but it can happen, for the TCP header i had multiple issue where the data is pushed futher beacause of partial ack and other TCP options

also if you want i can probably take that new code and test/fix

Sure feel free to do so as I'm unable to test it unfortunately. You should be able to git cherry-pick and amend it if needed.

schthack · 2022-06-04T01:08:20Z

@AdmiralCurtiss @sepalani what do you think of those changes?

sepalani

Yeah, the reading approach is better. We came with similar approach (master...sepalani:pr10690) which makes me believe we are toward the right direction. I think I spotted some typos/mistakes (see my branch or the PR review).

I do believe that the packet building part can be improved (I haven't reach that part yet on my branch). For instance, when creating a TCP packet, we can fill the IP header with a lot of information since we know the IPProto (TCP) and might have the data to compute most fields. Ditto for the UDP packet building.

Source/Core/Common/Network.cpp

Source/Core/Common/Network.h

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

sepalani

Thank you for the time and effort you're putting into this. I found some other points that need to be addressed.

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

sepalani · 2022-06-05T18:45:41Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+          datasize = socket_data.value().size();
+          std::memcpy(self->m_eth_ref->mRecvBuffer.get(), socket_data.value().data(), datasize);


socket_data->size() and socket_data->data() should work.

tryed, but VS is piss off

What is your error? The following piece of code works fine on my VS:

datasize = socket_data->size(); std::memcpy(self->m_eth_ref->mRecvBuffer.get(), socket_data->data(), datasize);

'data' n'est pas membre de 'std::optional<std::vector<u8,std::allocatorsf::Uint8>>'

in english: data is not a member of std.....

Make sure you're using socket_data->data() instead of socket_data.data(). The former gets the vector contained inside the optional and then calls vector's data() function (which is what you want), while the latter tries to call optional's non-existent data() function.

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

nolrinale · 2022-06-05T21:17:00Z

Just to let you know, I continue testing rebuilding Dolphin with the latest changes of this branch every time a new commit appears, and so far, the BBA tapless mode has been working without issues with PSO Online even with all the refactoring you guys are doing.

As sepalani mentioned earlier, thank you for your hard work and patience.

nolrinale · 2022-06-08T22:14:28Z

There's a crash that happens with PSO very randomly (only with PSO Episode 1&2 btw) where the game freezes and cannot continue normal operation but Dolphin continues operating as usual.

I was told it could be useful for you to post these pics here in the exact memory pointers as it might be related to something that was included within the commits of this tapless PR

Frozen Frame (in-game) (notice the 0 FPS, the sound goes brrrrrrrrrrrrrrrrrrrrrrrrrrrr)

Affected Memory Address

Affected Register

For the record this is from the latest binary I built from your very latest commit under macOS 12.4 0b9de57

AdmiralCurtiss · 2022-06-18T00:15:12Z

So what's the status here?

Kayak555 · 2022-06-20T22:36:28Z

Good question, we have many users using this and wondering when a merge is happening.

schthack · 2022-06-20T22:38:23Z

havent had the time to work much on it, still have to change the way dhcp work + all those changes apear to have introduce some issue that we need to hunt

sepalani

Here are some improvements based on the latest commit changes.

I'll try to find the remaining lines of code infringing Dolphin coding style, asap. After that, I believe the code would be good enough for a some final review/comments before being tested and merged.

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

Source/Core/Common/Network.h

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

Source/Core/Common/Network.cpp

schthack · 2022-06-23T20:47:05Z

change made. Also this morning i was looking at what someone did a while ago on android. and the "tap less" work fine

If it get merge at some point adding a menu for BBA might be nice

nolrinale · 2022-06-23T21:08:12Z

Maybe you can add it to the [...] button under the Gamecube settings tab of the tapless adapter? Where you set the DNS's and such?

plenty of space to add the extra functionality that you currently have hidden such as the Mac Address setup, the settings for the local LAN gameplay, etc.

It gives the final user more granularity to adjust settings related to the Tapless BBA inner workings.

sepalani

I think I found most coding style issues and some concerns about parts of the network code. Feel free to ask if you need help on some of these parts, BTW.

Source/Core/Common/Network.cpp

sepalani · 2022-06-24T20:47:03Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+      {
+        Common::IPv4Header* ipdata = (Common::IPv4Header*)&b[14];
+        ipdata->identification = ntohs(++self->m_ip_frame_id);
+        ipdata->header_checksum = 0;
+        ipdata->header_checksum = htons(Common::ComputeNetworkChecksum(ipdata, 20));
+      }
+      self->m_eth_ref->mRecvBufferLength = datasize > 64 ? (u32)datasize : 64;


I have mixed feelings about this part, there might be an aliasing issue and a checksum issue.

const std::size_t ip_offset = EthernetHeader::SIZE; const std::size_t frame_id_offset = ip_offset + offsetof(Common::IPv4Header, identification); Common::BitCastPtr<u16>(recv_buffer + frame_id_offset) = ntohs(++self->m_ip_frame_id); const std::size_t ip_checksum_offset = ip_offset + offsetof(Common::IPv4Header, header_checksum); // (...) Common::BitCastPtr<u16>(recv_buffer + ip_checksum_offset) = ntohs(new_checksum);

Won't your current approach clobber the actual checksum and won't take into account the data stored after the IP header? Moreover, only a single uint16 which was zero initially was changed, can't a cheaper computation be done rather than computing the whole header checksum again?

The packet can be a resend, so the value isnt always 0, you have to recompute if it isnt. Also i will add support for options but those packet are made by the code and will never have options

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

AdmiralCurtiss · 2022-07-01T22:14:51Z

Uh, I dunno what you did but that was definitely not a rebase. The diff between this and current master is pretty clean at least so you could just take that and push that as a single commit, I guess?

schthack · 2022-07-01T23:44:23Z

@AdmiralCurtiss idid the fallowing:
git fetch upstream
git rebase upstream/master

nvm, i forgot to use git push --force instead of git push....
any idea on what can be done to fix that?

AdmiralCurtiss · 2022-07-02T19:25:44Z

3d664454cbd52a0f9fc3bfea21a09059699aa04a is the last good commit I've seen, so starting from there:

git checkout BBA-tapless
git reset --hard 3d664454cbd52a0f9fc3bfea21a09059699aa04a
git cherry-pick f43a35b8e803c8cb3499d8ba92d2deb461fac61f

Then verify that you're in a correct state. If yes, rebase/push as normal. If no, you can go back to the current state with git reset --hard c330680bba2ddd95c7880bfeeccee66306543c0d.

…f a TapDevice Configuration include a dns server setting

Source/Core/Common/Network.cpp

AdmiralCurtiss · 2022-07-02T23:35:28Z

Note for merge: I would prefer if at least 01ada38 stayed in the history of the merged PR so we have a reference to how this looked before the refactoring. So please don't squash into that.

Source/Core/Common/Network.cpp

Kayak555 · 2022-07-06T02:34:40Z

So, what is the status of this?

schthack · 2022-07-06T19:06:25Z

@AdmiralCurtiss @sepalani Anything else you think need reworks?

sepalani

Beside these nitpicks and the commit history mentioned by @AdmiralCurtiss, the code looks mostly good to me.

Source/Core/Core/HW/EXI/EXI_DeviceEthernet.cpp

Source/Core/Common/Network.cpp

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

AdmiralCurtiss · 2022-07-07T21:00:02Z

To be clear, I don't really care about how the rest of the commit history looks (there's a lot of commits in there that could certainly be squashed but whatever), but I do care that 01ada38 stays in the commit history as-is to have a pre-refactor reference in case we end up discovering a bug in the refactored code.

AdmiralCurtiss · 2022-07-08T17:22:24Z

@sepalani Anything else? Otherwise we just need someone to test the current state and then I think we're good to go.

Source/Core/Common/Network.cpp

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

sepalani · 2022-07-08T19:42:42Z

Source/Core/Core/HW/EXI/BBA/BuiltIn.cpp

+void ChangeIPId(u8* buffer, const std::size_t size, const u16 new_val)
+{
+  if (size < Common::EthernetHeader::SIZE + Common::IPv4Header::SIZE)
+    return;
+  Common::IPv4Header ip_header =
+      Common::BitCastPtr<Common::IPv4Header>(buffer + Common::EthernetHeader::SIZE);
+  ip_header.identification = htons(new_val);
+  ip_header.header_checksum = 0;
+  ip_header.header_checksum =
+      htons(Common::ComputeNetworkChecksum(&ip_header, ip_header.DefinedSize()));
+  Common::BitCastPtr<Common::IPv4Header>(buffer + Common::EthernetHeader::SIZE) = ip_header;
+}


By having it there you won't need to pass the buffer and size parameters and can use m_ptr/m_size directly. This would give something like this:

void PacketView::SetIPIdentification(u16 value) { if (m_size < Common::EthernetHeader::SIZE + Common::IPv4Header::SIZE) return; u8* ip_ptr = m_ptr + Common::EthernetHeader::SIZE; Common::IPv4Header ip_header = Common::BitCastPtr<Common::IPv4Header>(ip_ptr); ip_header.identification = htons(value); ip_header.header_checksum = 0; ip_header.header_checksum = htons(Common::ComputeNetworkChecksum(&ip_header, ip_header.DefinedSize())); Common::BitCastPtr<Common::IPv4Header>(ip_ptr) = ip_header; }

I have one concern regarding the current approach, though. Won't this clobber the IPv4 options and result to an invalid checksum? Won't an approach using offsetof be safer?

nolrinale · 2022-07-08T21:19:21Z

@sepalani Anything else? Otherwise we just need someone to test the current state and then I think we're good to go.

Tested heavily after rebuilding this afternoon with the latest changes and it was still running as intended. No errors to report from my side.

At this point, several people had already tested this for several weeks in all 3 big platforms and from a functionality perspective is pretty solid, so it's more of a code compliance thing the three of you have to find a common ground on and decide the breakpoint between "good (and safe) enough" and what can be "Iterated/Improved at a later time" to proceed with the merge into master.

sepalani

I unfortunately can't test this PR. The code SGTM regarding the coding style and I don't think it's UB anymore.

AdmiralCurtiss · 2022-07-09T18:57:53Z

Can you squash the last commit into the one before it?

Changed access for offsetof + some clean up

JMC47 · 2022-07-09T21:35:05Z

Congratulations on getting it merged, and thank you for your work on this.

schthack · 2022-07-09T23:34:09Z

@AdmiralCurtiss @sepalani Thank you for helping and your patience!

JMC47 · 2022-07-24T02:54:36Z

I know this is a bit late, but I was testing this to get screenshots for the Progress Report, and I can't get Kirby Air Ride to connect. It errors out once the two GameCubes see one another. I'm using LAN because of the port issue when using a single computer. Any advice would be appreciated. I reported an issue with it if anyone else is interested. If there's any logging or testing I could do, or a way to test it on a single computer more easily instead of having to hook up other computers.

JMC47 · 2022-07-24T08:46:22Z

A second issue has cropped up that is possibly my fault. I had my Laptop, my Desktop, and my phone all together working, and I accidentally unlimited the framerate from my desktop and that predictably disconnected them.

But now my laptop and my desktop won't see each other period and I don't know why. My laptop will connect to my phone and my desktop will connect to my phone, but laptop refuses to even see the desktop. If I try to link all three, Laptop + Desktop see 2 devices, where as the phone will see 3.

sepalani suggested changes May 22, 2022

View reviewed changes

AdmiralCurtiss reviewed May 22, 2022

View reviewed changes

AdmiralCurtiss mentioned this pull request May 23, 2022

Common/Network: Use std::array in IPv4Header. #10691

Merged

AdmiralCurtiss reviewed May 23, 2022

View reviewed changes

sepalani reviewed May 23, 2022

View reviewed changes

schthack force-pushed the BBA-tapless branch from 6477648 to f81a71b Compare May 23, 2022 16:59

sepalani suggested changes Jun 4, 2022

View reviewed changes

sepalani suggested changes Jun 5, 2022

View reviewed changes

sepalani suggested changes Jun 23, 2022

View reviewed changes

sepalani suggested changes Jun 24, 2022

View reviewed changes

BBA: Added BuiltIn device that allow BBA emulation without the need o…

01ada38

…f a TapDevice Configuration include a dns server setting

schthack force-pushed the BBA-tapless branch from c330680 to a15f3e5 Compare July 2, 2022 22:04

AdmiralCurtiss reviewed Jul 2, 2022

View reviewed changes

Source/Core/Common/Network.cpp Outdated Show resolved Hide resolved

AdmiralCurtiss reviewed Jul 2, 2022

View reviewed changes

Source/Core/Common/Network.cpp Outdated Show resolved Hide resolved

AdmiralCurtiss reviewed Jul 2, 2022

View reviewed changes

Source/Core/Common/Network.cpp Show resolved Hide resolved

AdmiralCurtiss reviewed Jul 2, 2022

View reviewed changes

Source/Core/Common/Network.cpp Show resolved Hide resolved

AdmiralCurtiss reviewed Jul 2, 2022

View reviewed changes

Source/Core/Common/Network.cpp Show resolved Hide resolved

AdmiralCurtiss reviewed Jul 3, 2022

View reviewed changes

Source/Core/Common/Network.cpp Outdated Show resolved Hide resolved

sepalani suggested changes Jul 7, 2022

View reviewed changes

schthack force-pushed the BBA-tapless branch from dfafbc3 to e6efb95 Compare July 8, 2022 14:20

sepalani suggested changes Jul 8, 2022

View reviewed changes

sepalani approved these changes Jul 9, 2022

View reviewed changes

Refactoring of the code to match dolphin style

087020b

Changed access for offsetof + some clean up

schthack force-pushed the BBA-tapless branch from fd22ded to 087020b Compare July 9, 2022 21:01

AdmiralCurtiss merged commit 5a7759e into dolphin-emu:master Jul 9, 2022
11 checks passed

JosJuice mentioned this pull request Jul 23, 2022

Android: Add "Broadband Adapter (Built In)" to settings #10884

Merged

		InitUDPPort(26512); // MK DD and 1080
		InitUDPPort(26502); // Air Ride

		datasize = socket_data.value().size();
		std::memcpy(self->m_eth_ref->mRecvBuffer.get(), socket_data.value().data(), datasize);

BBA: Added BuiltIn device that allow BBA emulation without the need o… #10690

BBA: Added BuiltIn device that allow BBA emulation without the need o… #10690

Conversation

schthack commented May 22, 2022

sepalani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdmiralCurtiss May 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdmiralCurtiss May 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolrinale commented May 22, 2022

JMC47 commented May 23, 2022

Arlisbloxer05 commented May 23, 2022

AdmiralCurtiss commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sepalani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sepalani May 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdmiralCurtiss commented May 23, 2022 • edited

sepalani commented May 23, 2022

schthack commented May 23, 2022

AdmiralCurtiss commented May 24, 2022 • edited

sepalani commented May 25, 2022

schthack commented May 25, 2022

sepalani commented May 28, 2022

schthack commented Jun 4, 2022

sepalani left a comment

Choose a reason for hiding this comment

sepalani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolrinale commented Jun 5, 2022

nolrinale commented Jun 8, 2022 • edited

AdmiralCurtiss commented Jun 18, 2022

Kayak555 commented Jun 20, 2022

schthack commented Jun 20, 2022

sepalani left a comment

Choose a reason for hiding this comment

schthack commented Jun 23, 2022

nolrinale commented Jun 23, 2022

sepalani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdmiralCurtiss commented Jul 1, 2022

schthack commented Jul 1, 2022 • edited

AdmiralCurtiss May 22, 2022 •

edited

AdmiralCurtiss May 22, 2022 •

edited

sepalani May 23, 2022 •

edited

AdmiralCurtiss commented May 23, 2022 •

edited

AdmiralCurtiss commented May 24, 2022 •

edited

nolrinale commented Jun 8, 2022 •

edited

schthack commented Jul 1, 2022 •

edited

AdmiralCurtiss commented Jul 2, 2022 •

edited

schthack commented Jul 6, 2022 •

edited

AdmiralCurtiss commented Jul 7, 2022 •

edited

nolrinale commented Jul 8, 2022 •

edited