New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UDS: Add non zero mac address to the shared page #3444

Merged
merged 5 commits into from Mar 5, 2018

Conversation

Projects
None yet
9 participants
@jroweboy
Member

jroweboy commented Feb 17, 2018

Apparently several games check the shared page mac address and wifi
link level values, and will refuse to start UDS if they are not correct.
This change gives a default value (in case you aren't connected to a
network) and will read the value from Room if you are connected.

This pr is known to fix Tri Force Heroes multiplayer. Please test other games that used to have the "Cannot connect" issue when starting a lobby, and report success here. Some games that I know are on that list are: Mario Party Star Rush, The Legend of Zelda: TriForce Heroes, Monster Hunter Generations, Monster Hunter X, Monster Hunter XX, Dragon Quest Monsters: Terry, PokemonXY, and VirtualConsole games (such as pokemon red/blue/gold/silver)

clang format will likely bark at me.


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented Feb 17, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-02-17T01:51:19Z: 775fb38...jroweboy:49131ecff4a9565e38d25b1772ab3ee54cdb8dd4

@LogPod

This comment has been minimized.

LogPod commented Feb 17, 2018

X/Y multiplayer is working now, nice.

@FearlessTobi

This comment has been minimized.

Contributor

FearlessTobi commented Feb 17, 2018

Mario Party Star Rush is showing a kinda weird behavior now. You can create a room just fine. But when you try to join the room with another instance, you are able to see your created room, but the button to join it keeps flickering and you can't use it.

The "host" instance is spamming: core/hle/kernel/svc.cpp:CallSVC:1414: unimplemented SVC function GetHandleInfo(..)

Log of the "client" instance: https://pastebin.com/fpTQfxyQ

Look at the second screen:
ezgif-1-04ceabaeb9

@valentinvanelslande

This comment has been minimized.

Contributor

valentinvanelslande commented Feb 17, 2018

acnl broken
ACNL still does not work

@jroweboy

This comment has been minimized.

Member

jroweboy commented Feb 17, 2018

@valentinvanelslande

This comment has been minimized.

Contributor

valentinvanelslande commented Feb 17, 2018

now it works for some reason

@B3n30

Great work in finding the solution to the multiplayer issue

@@ -569,6 +570,12 @@ static void InitializeWithVersion(Interface* self) {
connection_status.status = static_cast<u32>(NetworkStatus::NotConnected);
}
if (auto room_member = Network::GetRoomMember().lock()) {
if (room_member->IsConnected()) {
SharedPage::SetMacAddress(static_cast<SharedPage::MacAddress>(room_member->GetMacAddress()));

This comment has been minimized.

@B3n30

B3n30 Feb 17, 2018

Contributor

Imo this shouldn't be in InitializeWithVersion since on 3ds (according to 3dbrew) the mac address get written when NWM gets initialized. And games don't expect that value to get changed afterwards, so this might lead to issues. I think a better way is to move this part to the NWM init. But then we have the problem that useres might connect after the init which would lead to a wrong mac address in shared page. I have two ideas for that solution:

  1. if the user is not connected, generate a random mac, write it to shared page and store it in the libnetwork::RoomMember so it can be used when the user connects
  2. Don't allow connecting when a game is running

I personally prefer option 1

enum class WifiLinkLevel : u8 {
POOR = 0,
GOOD = 1,
BEST = 2,

This comment has been minimized.

@B3n30

B3n30 Feb 17, 2018

Contributor

According to 3dbrew WifiLinkLevel can have the values between 0-3 and not 0-2 with 0: no connection, 1: poor, 2: good, and 3:best

@@ -83,6 +83,17 @@ void Init() {
update_time_event =
CoreTiming::RegisterEvent("SharedPage::UpdateTimeCallback", UpdateTimeCallback);
CoreTiming::ScheduleEvent(0, update_time_event);
SetWifiLinkLevel(WifiLinkLevel::POOR);

This comment has been minimized.

@B3n30

B3n30 Feb 17, 2018

Contributor

nit: Add a comment why we arbitrarily set the value to POOR?

@Andrix44

This comment has been minimized.

Contributor

Andrix44 commented Feb 17, 2018

MH Gen and MHXX now works.

@MojoJojoDojo

This comment has been minimized.

MojoJojoDojo commented Feb 17, 2018

Pokemon Red dont work : it disconnects after both connections.
here is the log :

red .txt

@cluezbot

This comment has been minimized.

cluezbot commented Feb 18, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-02-18T05:19:58Z: 775fb38...jroweboy:34456600fc0100a9d8604fc8dad0bfa12924e9af

@jroweboy

This comment has been minimized.

Member

jroweboy commented Feb 18, 2018

@FearlessTobi it might not be related, but the first version changed the mac address in the shared page on init, which is probably a bad idea in retrospect. Doubt it fixed your issue, but now it only ever gets one mac in the shared page.

@B3n30 i went ahead and moved it to nwm init. Its a little scary putting it there imo, since it has a dependency on kernel being inited before service but i can't ever see us deciding to change the init order...

@wwylele

Its a little scary putting it there imo, since it has a dependency on kernel being inited before service but i can't ever see us deciding to change the init order...

Kernel is guaranteed initialized first and there are many services having this assumption because they need to create kernel objects on init

@@ -22,6 +25,21 @@ void Init() {
AddService(new NWM_SOC);
AddService(new NWM_TST);
AddService(new NWM_UDS);
std::random_device rd;

This comment has been minimized.

@wwylele

wwylele Feb 18, 2018

Member

Friendly reminder that std::random_device is broken in MinGW, plus I don't think a random generator should be seeded every time NWM initializes. Since this is in the core, feel free to use Crypto++'s RNG.

@@ -22,6 +22,7 @@
#include "core/hle/service/nwm/uds_beacon.h"
#include "core/hle/service/nwm/uds_connection.h"
#include "core/hle/service/nwm/uds_data.h"
#include "core/hle/shared_page.h"

This comment has been minimized.

@B3n30

B3n30 Feb 18, 2018

Contributor

isn't needed anymore, besides that and wwylele's comment: LGTM

@wwylele

This comment has been minimized.

Member

wwylele commented Feb 22, 2018

Please resolve the conflict.

jroweboy added some commits Feb 17, 2018

UDS: Add non zero mac address to the shared page
Apparently several games check the shared page mac address and wifi
link level values, and will refuse to start UDS if they are not correct.
This change gives a default value (in case you aren't connected to a
network) and will read the value from Room if you are connected.
@cluezbot

This comment has been minimized.

cluezbot commented Feb 22, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-02-22T17:39:27Z: e2eab46...jroweboy:752cfcaaaeef6993fc9a58cde2c994e80d1f3c48

@jroweboy jroweboy force-pushed the jroweboy:fix-multiplayer branch from 3445660 to 752cfca Feb 22, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Feb 23, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-02-23T05:32:07Z: e2eab46...jroweboy:1f87766b8673756973d02c90e05bd29fde77e466

@jroweboy

This comment has been minimized.

Member

jroweboy commented Feb 23, 2018

@wwylele ready for merge, captain!

@wwylele

LGTM. Just some nit-picking

if (auto room_member = Network::GetRoomMember().lock()) {
if (room_member->IsConnected()) {
mac = static_cast<SharedPage::MacAddress>(room_member->GetMacAddress());

This comment has been minimized.

@wwylele

wwylele Feb 23, 2018

Member

This static_cast is useless IMO. If Network and Core defines mac address as the same underlying type (which is the case now), the cast is a no-op and no need to be explicit; otherwise, if the types are different, the cast will fail anyway regardless you put a static_cast here.

CryptoPP::AutoSeededRandomPool rng;
auto mac = SharedPage::DefaultMac;
// Keep the Nintendo 3DS MAC header and randomly generate the last 3 bytes
rng.GenerateBlock(static_cast<CryptoPP::byte*>(mac.data() + 3), 3);

This comment has been minimized.

@wwylele

wwylele Feb 23, 2018

Member

A more logical approach to me would be putting the random mac generation in the else block of the if block below, but I am fine with this if you insist

This comment has been minimized.

@jroweboy

jroweboy Mar 5, 2018

Member

the issue is there are two if blocks so i'd need to rewrite it to set a bool "failed" and then if (failed) or duplicate the call twice. neither sounded appealing, and this default init is only done once so it doesn't hurt.

@wwylele

This comment has been minimized.

Member

wwylele commented Feb 23, 2018

I wonder, though, that if we should always let core/uds generate a random mac and feed it to the network module.

@cluezbot

This comment has been minimized.

cluezbot commented Mar 5, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-05T03:20:51Z: e2eab46...jroweboy:878217372b5af21655ba59e3e4ec5686be3baeea

@cluezbot

This comment has been minimized.

cluezbot commented Mar 5, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-05T03:28:42Z: e2eab46...jroweboy:d26cf11399969b77e3082334e40723d92c42d009

@jroweboy jroweboy merged commit ce725f2 into citra-emu:master Mar 5, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment