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

WIP - Wiimote emu motion plus #7589

Open
wants to merge 21 commits into
base: master
from

Conversation

5 participants
@jordan-woyak
Contributor

jordan-woyak commented Nov 24, 2018

Cleanup Wiimote emu code to more closely simulate the real thing.
Simulate Motion Plus attachment.
Eliminated some dead code.

@BhaaLseN

Yay for cleanup!

// hybrid Wiimote stuff
if (WIIMOTE_SRC_REAL & g_wiimote_sources[m_index] && (m_extension->switch_extension <= 0))
{
using namespace WiimoteReal;

This comment has been minimized.

@BhaaLseN

BhaaLseN Nov 24, 2018

Member

Are those WiimoteReal.h includes still needed with all those removals? Goes for all the WiimoteEmu files changed in this commit.

Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/EmuSubroutines.cpp Outdated
Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/Speaker.cpp Outdated
Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/Speaker.cpp Outdated
Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/EmuSubroutines.cpp
Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/EmuSubroutines.cpp
Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.h Outdated
Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.h
Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.h
Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.h
Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/EmuSubroutines.cpp Outdated
@@ -202,53 +209,54 @@ void Wiimote::HandleExtensionSwap()
void Wiimote::RequestStatus(const wm_request_status* const rs)
{
HandleExtensionSwap();
INFO_LOG(WIIMOTE, "Wiimote::RequestStatus");

This comment has been minimized.

@riking

riking Nov 26, 2018

Contributor

Consider removing or moving to DEBUG level before merge - this might get really noisy depending on the game.

This comment has been minimized.

@jordan-woyak

jordan-woyak Nov 26, 2018

Contributor

I do plan on that.

// ignore the 0x010000 bit
address &= ~0x010000;
INFO_LOG(WIIMOTE, "Wiimote::WriteData: 0x%02x @ 0x%02x @ 0x%02x (%d)", wd->space, wd->slave_address, address, wd->size);

This comment has been minimized.

@riking

riking Nov 26, 2018

Contributor

Same "probably a DEBUG log" comment.

This comment has been minimized.

@jordan-woyak

jordan-woyak Nov 26, 2018

Contributor

👍

Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/EmuSubroutines.cpp Outdated
Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/EmuSubroutines.cpp Outdated
@@ -620,4 +478,4 @@ void Wiimote::RealState()
g_wiimotes[m_index]->EnableDataReporting(m_reporting_mode);

This comment has been minimized.

@riking

riking Nov 26, 2018

Contributor

has this code (savestate restore on real wiimote) been verified to still be adequate?

@@ -135,21 +135,37 @@ void Pause()
// An L2CAP packet is passed from the Core to the Wiimote on the HID CONTROL channel.
void ControlChannel(int number, u16 channel_id, const void* data, u32 size)

This comment has been minimized.

@BhaaLseN

BhaaLseN Nov 26, 2018

Member

Wouldn't it be kinda nice if we could just call s_config.GetController(number)->ControlChannel(...) and not worry about whether it is a real, emulated or...disabled/non-existant Wiimote? Goes for both ControlChannel and InterruptChannel.

Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.cpp
Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.h
class I2CBus
{
public:
void AddSlave(I2CSlave* slave) { m_slaves.emplace_back(slave); }

This comment has been minimized.

@riking

riking Nov 27, 2018

Contributor

Given ExtensionPort::SetAttachment(nullptr) to disconnect the attachment, shouldn't we be filtering nullptr args here?

This comment has been minimized.

@jordan-woyak

jordan-woyak Nov 27, 2018

Contributor

Yeah, either that or a RemoveAttachment function. Didn't yet get to the point of allowing user to disable motion plus and extensions.

Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.cpp Outdated
static const u8 c1[16] = {0x78, 0xd9, 0x78, 0x38, 0x77, 0x9d, 0x2f, 0x0c,
0xcf, 0xf0, 0x31, 0xad, 0xc8, 0x0b, 0x5e, 0x39};
static const u8 c2[16] = {0x6f, 0x81, 0x7b, 0x89, 0x78, 0x51, 0x33, 0x60,
0xc9, 0xf5, 0x37, 0xc1, 0x2d, 0xe9, 0x15, 0x8d};

This comment has been minimized.

@riking

riking Nov 27, 2018

Contributor

Would be nice to be able to decode the actual numbers being used here so that we can be sure the right values are being sent and it's not being slightly offset.

This comment has been minimized.

@jordan-woyak

jordan-woyak Nov 27, 2018

Contributor

Yeah. I plan on it. I was just trying to hack something together to make a game boot.

// Address 0x58
m_i2c_bus.AddSlave(&m_camera_logic);
// TODO: only add to bus when enabled

This comment has been minimized.

@riking

riking Nov 27, 2018

Contributor

shouldn't the motion+ still be added to the bus, despite the TODO?

This comment has been minimized.

@jordan-woyak

jordan-woyak Nov 27, 2018

Contributor

I was suggesting a situation where a user disabled motion+ entirely in the UI to disable motion+ emulation. But maybe that shouldn't even be an option if motion+ is foolproof.

This comment has been minimized.

@riking

riking Nov 28, 2018

Contributor

I was actually referring to the fact that the code as reviewed doesn't ever add M+ to the bus, no matter what the settings are :P

{
++times_updated_since_activation;
// TODO: wtf is this value actually..

This comment has been minimized.

@riking

riking Nov 27, 2018

Contributor

The Switch joycons have a timer value that increments faster than 60hz and is included in the packets - probably the same kind of thing here?

Probably just use (getEmulatedTimeMillis() / 10) & 0xF or something.

This comment has been minimized.

@jordan-woyak

jordan-woyak Nov 27, 2018

Contributor

It's not just a timer. At least it's not behaving like one on real hardware. And Sports Resort disconnects the motion+ after reading it. I'm still figuring it out.

// For some reason the first read seems to have garbage data
// is_mp_data and extension_connected are set, but the data is junk
// it does seem to have some sort of pattern though, byte 5 is always 2
// something like: d5, b0, 4e, 6e, fc, 2

This comment has been minimized.

@riking

riking Nov 27, 2018

Contributor

if it's actually reads from uninitialized memory, replace the values with 0xA5A5A5 or something.

This comment has been minimized.

@jordan-woyak

jordan-woyak Nov 27, 2018

Contributor

Sorry. I mean the REAL hardware returns weird data for the first frame. Consistently.

Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.cpp
return result;
// It seems a write of any value triggers deactivation.
if (0xf0 == addr)

This comment has been minimized.

@riking

riking Nov 27, 2018

Contributor

This if statement is after a return.

This comment has been minimized.

@jordan-woyak

jordan-woyak Nov 27, 2018

Contributor

Sorry.. leftovers from debugging. I've already removed it locally.

auto const result = RawWrite(&reg_data, addr, count, data_in);
// It seems a write of any value triggers activation.
if (0xfe == addr)

This comment has been minimized.

@riking

riking Nov 27, 2018

Contributor

The two if branches have different values for the addr check. Please fix and move to a named constant.

This comment has been minimized.

@jordan-woyak

jordan-woyak Nov 27, 2018

Contributor

I can add some named constants. But yes. activation and deactivation are different addresses.

@Warepire

This comment has been minimized.

Warepire commented Nov 27, 2018

Reading through this code I am missing the handing of the read/write bit in i2c addressing. Am I missing the relevant code blocks or is the addressing handling wrong?

If necessary, this document describes the i2c bus: https://www.nxp.com/docs/en/user-guide/UM10204.pdf

@jordan-woyak

This comment has been minimized.

Contributor

jordan-woyak commented Nov 27, 2018

The read/write bit (lsb of slave_address) is ignored by a real wiimote. I chop it off in Read/WriteData calls (>> 1) and the high level i2c bus simulation just makes use of the 7 bit address.

@Warepire

This comment has been minimized.

Warepire commented Nov 28, 2018

Ah, that makes sense.

Would it be meaningful to name the operation using a small constexpr function?

@jordan-woyak jordan-woyak force-pushed the jordan-woyak:wiimote-emu-motion-plus branch from da8e649 to 7369a84 Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment