CLI mode, benchmark from movie etc.

mirror / dolphin-emu

ARCHIVED Moved to *dolphin-emu/dolphin*

https://github.com/dolphin-emu/dolphin

14 stars 14 forks source link

CLI mode, benchmark from movie etc. #1

Open john-peterson opened 11 years ago

rog9 commented 11 years ago

Why did you leave out BootManager.cpp?

Same reason i left out all of your cli fixes: i didn't feel like going through your commits, and remaking them without all the other unrelated changes. Make a single patch that does this, and only this, and maybe it can be committed. These changes are completely unrelated to wiimote savestate fixes

rog9 commented 11 years ago

not sure if you saw this: http://code.google.com/p/dolphin-emu/source/detail?r=470a4eee8b9c75d231c2f6840b0b8559325b1375

rog9 commented 11 years ago

I don't see the flaw, i just know it still happens (rarely, but it does). Regardless of that, it still doesn't save in the proper order when using it too quickly. When trying it, more than half of my saves end up being to slot 8, often 5+ in a row, if i just hold it down.

And that guy who said it was crashing is using a real wiimote. my guess is that's the problem. I don't have one to test with, so i can't reproduce it.

john-peterson commented 11 years ago

CoreTiming state function

Problem

Running benchmarks return a "Access violation reading location" in VS from f.e. these events (printed in CoreTiming::EventDoState) with the pointer >= 518958768

./dolphin -BC log -e twist_road.dtm
40:10:395 Src\CoreTiming.cpp:183 W[CONSOLE]: TransferComplete, ev 123424528, ev->type 18
40:10:395 Src\CoreTiming.cpp:181 E[CONSOLE]: DecCallback, ev 518958768, ev->type 0
40:10:395 Src\CoreTiming.cpp:181 E[CONSOLE]: SetToken, ev 550736496, ev->type 1414744320
40:10:395 Src\CoreTiming.cpp:181 E[CONSOLE]: SetFinish, ev 550736352, ev->type 1312904275

because ev->type is undefined for these events

Solution

Read ev->type when it definitely exists, when p.GetMode() != PointerWrap::MODE_READ

john-peterson commented 11 years ago

Skipping `DoMarker` section

470a4ee causes a crash on state loading in IWII_IPC_HLE_Device::DoStateShared(PointerWrap& p).

This incorrect code was written because of an incorrect assumption that

state loading skip a missing DoMarker section

This is wrong because

DoMarker write 0x42 rather than the section string (which is inadequate information about which section is missing)
state loading doesn't skip a missing DoMarker section

john-peterson commented 11 years ago

Cheat manager

Why did you remove this from https://github.com/mirror/dolphin-emu/commit/983d5d1f733e6961daec258a9fab8595efd87019?

Placing the Gecko codes tab before the AR codes tab because it's more popular

[17:20] <_RachelB> i removed it because i have never once used gecko codes, and use AR codes extensively, so it would be massively inconvenient to me

john-peterson commented 11 years ago

Log

LogManager

The benefit with

log to the launching console (in Windows too)

the log can be read in the console after the program is terminated

log to OutputDebugString when the process is not debugged too

the message can be read in DebugView which can be more efficient than reading it in the program's log window

log OSD messages to LogManager too because that make the message (f.e. about state version)

permanent (for future reference) rather than temporary (f.e. 5 s)
visible to CLI mode if it occurs before window creation

fflush(stderr)

print directly rather than with a delay

IsNonCmdConhost

detect a non-cmd conhost f.e. mintty that support "\033[" rather than a cmd conhost f.e. conemu that doesn't support it

Loading log settings for CLI

The benefit with

loading log settings for CLI is

allow disabling logs (rather an log all messages) for CLI

The code

m_Log[i]->RemoveListener(m_debuggerLog);

is run regardless of IsDebuggerPresent() because it's possible that debugging has been released since

m_Log[i]->AddListener(m_debuggerLog);

was run

Discussion

@NeoBrainX

Why would we want OSD messages to be in the logfile?

Because

it might be sent when there's no window
the OSD message is temporary i.o.w. it can't be read again later

john-peterson commented 11 years ago

More deterministic dual thread movie input

Problem

Dual thread recording desync, sometimes because the recorded frame differ from the played frame

The problem is important for a benchmark when it causes the lap completion percentage to be low because the movie isn't representative of play that draw a whole lap

The movies have frame desync messages similar to this

51:07:192 Src\Movie.cpp:1001 W[PAD]: Frame at input 1867: cur - rec  20, rec   1823, cur   1843

Solution

Updating dual thread input directly after frame draw because

that makes it more deterministic in several games
doesn't make it less deterministic in any game

The change is conditional on dual threading because

single threading is deterministic from a tick interval too
the lack of intra-frame input is a cost because some games process input between frames
- the GC vesion of RE4 prcess input at 60 Hz despite a 30 FPS rate
- majora's mask for VC has a minigame which on the japanese version require 1/60 s input precision despite a 20 FPS rate

There's no frame mismatch in any of the movies but some still desync

This doesn't solve the problem that GC and Wii input order isn't deterministic from boot, but result in the message

./dolphin -B -e dkcr_boot.dtm
44:32:859 Src\MsgHandler.cpp:80 E[*]: Warning: Type at input     26: rec 0, cur 1

Comparison

The test builds are

dolphin this branch at "Synchronising dual thread movie input with frame drawing instead of a tick interval"
dolphin-sync this branch at "Changing the dual thread movie Wii input update from 200 Hz to VideoInterface::TargetRefreshRate"

The tested commands are f.e. for the movie twist_road

./dolphin -BC normal -e twist_road.dtm
./dolphin-sync -BC normal -e twist_road_sync.dtm

The "movie" column is the movie name without the suffix ".dtm", "_sync.dtm"

The "lap" column is lap completion percentage for the test programs in the order: dolphin_sync, dolphin

The "fps" column is target FPS (no value mean 60)

The "frame" column is frame mismatch information (the "cur - rec" value in parentheses)

The "problem" column describe the column for the movie that has less than 100% lap completion

movie           lap             fps frame           problem
twist_road      0   100         many                vehicle hit wall
luigi_circuit   30  100         many                vehicle hit wall
xg3             20  100         many (-1, 1)        vehicle hit wall
lm_1            50  100     30  many (-1, 1)        desyncs before the key is obtained
re4_1           20  100     30  few (1)             character become stuck in a house
ct_1            100 100         many (-1, 1)
mx_1            100 100     30  many (-1, 1)
sf_1            100 100         many (-1)
nsmb_1-1        100 100         few (-1, 1)
jungle_hijinxs  100 100         few (-3 to 1)
ski_school      50  80          many                racer hit wall
dolphin_park    10  0       30  many (-40 to -1)    vehicle hit wall
ikaruga_1       80  50          many (-17 to 2)     continue isn't selected at the right time
bo2_1           10  10      30  few (-1)            menu option is selected at the wrong time
sms_1           30  30      30  many (-1, 1)        desyncs and subsequently hangs
nfsu_1          30  20      30  many (1 to ~260)    vehicle hit wall
nfsu2_1         20  20      30  many (~-500 to -1)  vehicle hit wall
n05_1           60  50          many (-1)           vehicle hit wall
srw_1           0   0       30  many (~-500 to -1)  vehicle miss target
turok_1         50  50      30  many (-13 to -1)    move in the wrong direction
cars_1          50  70          many (-9 to -1)     vehicle hit wall
pr_1            20  20          many (-5 to -1)     vehicle hit wall
4x4e2           5   5           many (-50 to ~100)  vehicle hit wall

Discussion

[10:52] <_RachelB> also i got no warnings about desyncs

Run the movies in the topic "Problem" because they create frame desync warnings

<_RachelB> JPeterson: still desyncs

It desyncs less than with a tick interval input update as described by the different lap completion percentage

[10:49] <_RachelB> JPeterson: that's not okay anyway, since some games accept input between frames [10:50] <_RachelB> you really can't change the polling rates [02:23] <_RachelB> it is wrong because that is not how it works on real hardware [the real GC hardware update input at VideoInterface::TargetRefreshRate and the real Wii hardware at 200 Hz]
<_RachelB> gcpad must be 60 hz, and wiimote 200 hz. you cannot change that for any reason <_RachelB> it breaks games because some games require you to press buttons in between frame draws, which you have made impossible

The cost of losing dual thread intra-frame input is lower than the utility of the synchronisation improvement because

single thread movies are unchanged
the input rate for dual thread 60 (rather than 30 or 20) FPS GC movies is unchanged at once per frame
the input deterministicity for dual threaded movies is better or unchanged
a desynchronized movie has little value despite intra-frame input update

<_RachelB> if it synced perfectly, i'd maybe consider it, but it doesn't, …

Why should the synchronisation be perfect to outweigh the loss of intra-frame movie input? considering that a desynchronized movie has little value despite intra-frame input update

[03:14] <_RachelB> as it is, the [cost of the] time spent checking sync will easily outweigh the speed gains of using dual core, so unless dual core syncs perfectly, there's no benefit of using dual core anyway, while single core has been shown to never desync

If a perfect sync is required to give a dual thread movie a value, the example movies with 100% lap completion indicate a perfect sync i.o.w. a value

It's not correct that the < 100% lap completion example movies are without value because a benchmark movie's value increase with increased lap completion

… and allowing it to be committed anyway it is likely to remove any motivation to find a real fix

It doesn't reduce the motivation for improving the dual thread movie sync because it's still a problem that some dual threaded example movies desync

Create a dual thread movie that doesn't desync significantly and benefit from intra-frame input because it's not clear that it exists

john-peterson commented 11 years ago

Movie synchronisation data

Problem

There's no movie desynchronisation detection

Solution

The frame count is a measure of synchronisation

The tick count isn't a good measure of synchronisation because it's different in a single thread movie (despote having deterministic input)

The frame log print this message when the recorded tick is different from the current

51:07:120 Src\Movie.cpp:1001 W[PAD]: Frame at input 1863: cur - rec  20, rec   1819, cur   1839

Size is moved from the Wii data to a input packet header (i.o.w. store for GC input to despite that always being 8) because that's better organisation because it's easier to read

The wrong type end the movie because

GC and Wii input is called from a CoreTiming scheduled event that's deterministic also for dual thread, i.o.w. the played order can't differ from the recorded order (unless f.e. a state variable is unsaved)

Discussion

<_RachelB> oh hey, i got the tick warning by playing it from the start without restarting dolphin, lets see if it actually noticeably desyncs

Tick appear to be a flawed way to determine synchronisation because both with "CPUThread = True" and "CPUThread = False" is there

a point tick difference of up to around 50 and a cumulative tick difference of up to around 500

but

with "CPUThread = True" the desync is noticebable because the vehicle hit the wall
with "CPUThread = False" there's no noticeable desync

The frame sync is

no frame desynchronisation

./dolphin -BC normal -e twist_road_sc.dtm
FPS:          24,62

CPU
Recompile: 1
Skip idle: 1

Video
Thread: 0

two frame desynchronisation (one frame per desynchronisation)

./dolphin -BC sync -e twist_road_sync.dtm
FPS:          25,52

Recompile: 1
Skip idle: 1

Video
Backend: 1
Thread: 1
Synchronized: 1

44:07:952 Src\Movie.cpp:1002 W[PAD]: Frame at     34: cur - rec   1, cum   1, rec 34, cur 35
45:04:105 Src\Movie.cpp:1002 W[PAD]: Frame at  1▒564: cur - rec   1, cum   2, rec 1▒561, cur 1▒562

all frames are desynchronised

./dolphin -BC normal -e twist_road.dtm
FPS:          45,39

CPU
Recompile: 1
Skip idle: 1

Video
Thread: 1
Synchronized: 0

52:34:347 Src\Movie.cpp:1002 W[PAD]: Frame at  1▒417: cur - rec -17, cum -24▒203, rec 1▒413, cur 1▒396

<_RachelB> gc never desyncs, so that seems entirely unnecessary GC input desyncs when dual threading is used, i.o.w. when "CPUThread = True" and "SyncGPU = False" > <_RachelB> well, that's not worth doubling the size, … It's worth additonal size because data is needed to determine synchronisation which is important > … and making it impossible to neatly view it in a hex editor It's not impossible because the data column width can be changed from 8 to 9 + 8 to show the data and data header in separate columns > <_RachelB> not in the one i use :( In 010 Editor the column width is changed with "View → Line Width → Custom Width" The movie header can be removed with this command to show data in the first cell ``` tail -c +257 file.dtm>_noheader.dtm ``` > [05:45] <_RachelB> trying to test it and i can't get a desync.. A GC desync can be created by recording F-Zero with the files in the topic "File" in [benchmark](https://github.com/mirror/dolphin-emu/issues/26) This file has desync that's noticed because the controlled object hit a wall repeatedly ``` ./dolphin -BC normal -e twist_road.dtm ``` > <_RachelB> JPeterson: without dual core, or idling skipping? This can be seen in benchmark.txt in the topic "File" in [benchmark](https://github.com/mirror/dolphin-emu/issues/26) ``` ./dolphin -BC normal -e twist_road CPU Skip idle: 1 Video Thread: 1 Synchronized: 0 ./dolphin -BC normal -e twist_road_sc.dtm CPU Skip idle: 1 Video Thread: 0 Synchronized: 0 ./dolphin -BC normal -e twist_road_idle.dtm CPU Skip idle: 0 Video Thread: 1 Synchronized: 0 ./dolphin -BC normal -e twist_road_sc_idle.dtm CPU Skip idle: 0 Video Thread: 0 Synchronized: 0 ```

john-peterson commented 11 years ago

Wii input shake

Problem

A peak Wiimote g-force of 3 is almost too low in Pucca's Kisses Game at the end of level 1-2 (where shake is used to run to catch up with another character) with the result that the character

catch up close to the last moment (which is at the third tree after the four section red wall)
around 1/3 tries doesn't catch up before the last moment

because

the maximum theoretical accelerometer value is

Wiimote accelerometer max

max(zero / (one - zero), (255.f - zero) / (one - zero))
max(128 / (154 - 128), (255 - 128) / (154- 128))
max(4,92, 4,88)
4,92

Nunchuk accelerometer max

max(128 / (179- 128), (255 - 128) / (179 - 128))
max(2,51, 2,49)
2,51

this output (from first party hardware) of maximum accelerometer values show that the Wiimote reach a maxium value in one direction and the Nunchuk in both directions, meaning that it's likely that software is designed to expect that maximum values can be reached

Wiimote accelerometer min, max [ 0, 238]

--- wiimote ---
gx    = -0.04 [-5.13, 5.17]
gy    = -0.04 [-5.13, 5.22]
gz    = 1.04 [-4.88, 5.04]
x     = 117 [  0, 237] [118] [ 23]
y     = 117 [  0, 238] [118] [ 23]
z     = 142 [  0, 238] [117] [ 24]

Nunchuk accelerometer min, max [ 0, 255]

--- nunchuk ---
gx    = 0.28 [-0.72, 0.69]
gy    = 0.02 [-0.70, 0.71]
gz    = 0.08 [-0.71, 0.71]
x     = 180 [  0, 255] [130] [181]
y     = 131 [  2, 255] [128] [179]
z     = 141 [  0, 255] [127] [180]

Solution

Using the the maximum force from calibration rather than a constant because

the character catch up every time, at the one section red wall
it creates a maximum force, for both Wiimote and Nunchuck despite their different calibration

Discussion

<_RachelB> JPeterson: btw your shake code works fine, but not as fast as it can be. I finish 1-2 about 4 seconds faster by setting shake_intensity to 6.7

If it's desirable to make shake more efficient

it should be considered that the most efficient shake pattern can be different for different games depending on how they read shake intensity, i.o.w. that there might not be one pattern that's the best pattern in all games

it might be better to reduce shake_step_max instead of increasing shake_intensity to create more data reports with high accelerometer values because the bottom output below might not be more efficient than the top output in all games

WARN_LOG(CONSOLE, "x %.1f, trim %.1f, zero %u, one %u", m_accel.x, double(cx - calib->zero_g.x) / double(calib->one_g.x-calib->zero_g.x), calib->zero_g.x, calib->one_g.x);

shake_intensity = max(zero / (one - zero), (255.f - zero) / (one - zero));

52:58:668 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -4,9, trim -4,9, zero 128, one 154
52:58:672 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -4,7, trim -4,7, zero 128, one 154
52:58:674 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -3,7, trim -3,7, zero 128, one 154
52:58:676 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -2,0, trim -2,0, zero 128, one 154
52:58:678 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 0,0, trim 0,0, zero 128, one 154
52:58:681 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 2,0, trim 2,0, zero 128, one 154
52:58:683 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 3,7, trim 3,7, zero 128, one 154
52:58:686 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 4,7, trim 4,7, zero 128, one 154
52:58:688 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 4,9, trim 4,9, zero 128, one 154
52:58:690 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 4,3, trim 4,3, zero 128, one 154
52:58:692 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 2,9, trim 2,9, zero 128, one 154
52:58:694 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 1,0, trim 1,0, zero 128, one 154
52:58:696 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -1,0, trim -1,0, zero 128, one 154
52:58:698 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -2,9, trim -2,9, zero 128, one 154
52:58:701 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -4,3, trim -4,3, zero 128, one 154

shake_intensity = max(zero / (one - zero), (255.f - zero) / (one - zero)) * 2.f;

54:02:798 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 9,8, trim 4,9, zero 128, one 154
54:02:802 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 8,5, trim 4,9, zero 128, one 154
54:02:805 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 5,8, trim 4,9, zero 128, one 154
54:02:809 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 2,0, trim 2,0, zero 128, one 154
54:02:812 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -2,0, trim -2,0, zero 128, one 154
54:02:816 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -5,8, trim -4,9, zero 128, one 154
54:02:819 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -8,5, trim -4,9, zero 128, one 154
54:02:821 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -9,8, trim -4,9, zero 128, one 154
54:02:823 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -9,4, trim -4,9, zero 128, one 154
54:02:825 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -7,3, trim -4,9, zero 128, one 154
54:02:828 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x -4,0, trim -4,0, zero 128, one 154
54:02:831 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 0,0, trim 0,0, zero 128, one 154
54:02:834 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 4,0, trim 4,0, zero 128, one 154
54:02:837 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 7,3, trim 4,9, zero 128, one 154
54:02:840 Src\HW\WiimoteEmu\WiimoteEmu.cpp:432 W[CONSOLE]: x 9,4, trim 4,9, zero 128, one 154

john-peterson commented 11 years ago

Unsaved Wii input state value

Patch

The commit in this branch with the same title as this post

Problem

In a (deterministic) single thread emulation

there's a (m_WiiMotes.size()-1)/m_WiiMotes.size() (4/5) probability that the Wii input state value wiimote_to_update isn't 0 when the state is saved with the result that

the frame and GC and Wii input order (if the GC pad is enabled ("SIDevice0 = 6")) is unsynchronized, resulting in the message

./dolphin -B -e nsmb_menu.dt
22:45:740 Src\Movie.cpp:1015 W[PAD]: Frame at input      3: cur - rec  -1, rec      1, cur      0
22:46:074 Src\MsgHandler.cpp:80 E[*]: Warning: Type at input      4: rec 0, cur 1

Solution

Result

Making the Wii input equally deterministic as the GC input by

removing dependency on the Wiimote state variable wiimote_to_update
i.o.w. changing the emulated Wii input update simultaneity from non-simultaneous to simultaneous for all Wiimotes

because that

make movie play deterministic
allow recording GC and Wii input simultaneously in the same array

Why not save `wiimote_to_update`?

Removing emulated Wii input use of wiimote_to_update rather than saving it because

updating the state version has a cost because

it invalidate old states which is a cost because states for the purpose of
- benchmarks must be re-recorded
- playing must be replaced by using the game's save function to load the game state

non-simultaneous Wiimote update doesn't have meaning because

the variable wiimote_to_update (and a non-simultaneous Wiimote update) added in a80429b (for the reason that it reduce the peak m_queue size) isn't meaningful because the m_queue size isn't large with simultaneous updates as described in the topic "Test"

a simultaneous update is better for a movie because

that allow exact input control (by pausing emulation) of all Wiimotes at one time rather than one time per Wiimote

Outdated argument

The following argument is outdated because it's based on an incorrect belief that non-simultaneous update has a meaning for real Wiimotes

saving the real Wiimote state doesn't have meaning because

it's not meaningful that the real Wiimote input is deterministic because it's less important for recording because
- it's not important to use a real Wiimote for recording
- the real Wiimote isn't recorded, i.o.w. Movie::CheckWiimoteStatus isn't called from WiimoteReal.cpp
- the real Wiimote doesn't support re-recording because it require m_WiimoteReconnectOnLoad because there's no code to create a real Wiimote connection (with the state's Wiimote channel and reporting mode) between state load and CCPU::Run to remove the need for m_WiimoteReconnectOnLoad

Test

ACL queue size

The largest ACL queue size with emulated Wiimotes in

Mario Kart Wii
Wii Party 4 player mini-game
DKCR
Rhythm Heaven Fever
Just Dance

during

regular operation is [connected Wiimotes - 1], f.e.

19:11:152 Src\IPC_HLE\WII_IPC_HLE_Device_usb.cpp:557 W[WII_IPC_WIIMOTE]: ACL queue size 3

peak (when connection messages are sent f.e. at boot)

04:36:176 Src\IPC_HLE\WII_IPC_HLE_Device_usb.cpp:532 W[WII_IPC_WIIMOTE]: ACL queue size 6

Simultaneous (compared to non-simultaneous) Wii input updates funtion without a noticeable difference in these games

Real Wiimote

This result isn't differrent if 1 Wiimote is real and the other 3 are emulated

It's a low probability that the ACL queue size is significantly different if 4/4 instead of 1/4 Wiimotes are real

Frequency threshold for audio

The minimum input update frequency where the speaker create audio is

Mario Kart Wii (battle mode because it has much audio): no audio at 165 Hz, audio at 170 Hz, at 300 Hz a temporary state of delayed (around 10 s) audio occured but it might be a coincidence
DKCR, tested by changing between the "New Game" menu items: at 160 Hz there's sometimes faint audio
DKCR "Select Game" menu: below around 170 Hz audio is disabled and it sends around 20 WM_SPEAKER_MUTE alternating between 0 and 1, about every 5 s

200 Hz problem

Should this patch keep the Wii input update frequency at 160 Hz or change it to 200 Hz?, considering f.e. the arguments

the Wiimote hardware has been determined to send data reports at 200 Hz because that return the best value from WPADGetRadioSensitivity in libogc
I request that the WPADGetRadioSensitivity return value and code that test the native data report frequency is added to TestSuite WPAD because that information is important to decide the frequency
it appears that the audio quality is good at 200 Hz because at around 170 Hz the audio is faint or absent and at around 300 Hz a temporary state of audio delay can occur
200 rather than 160 Hz disconnect the real Wiimote in DKCR if m_WiimoteEnableSpeaker = true (so that speaker data is sent do the Wiimote), however at 160 Hz the audio is faint or absent

This patch can create Wiimote disconnects (perhaps only when speaker data is enabled) because it changes the Wiimote input update frequency from 160 to 200

The Wii input update frequency was unintentionally changed from 200 to 160 [200 * (4/5)] Hz in 86b4a87 when m_WiiMotes.size() was changed from 4 to 5

When using 200 rather than 160 Hz in DKCR with

one real (but not when emulated) Wiimote
speaker data enabled (but not when disabled)

audio is enabled (because 200 Hz is above the audio enable threshold) with the result that

the connection status is changed to "Not connected" coinciding with the screen message "Communications with the Wii Remote have been interrrupted" after

around 3 seconds after progressing past the "Press 2 to play" message

Data

Facts meaningful for the problem

changing the IPC_HLE frequency (current is 1 500 Hz) to 15 000, 3 000, 1000, 500 Hz doesn't allow DKCR to use 200 Hz with speaker data enabled without disconnecting
if speaker data is enabled during a running emulation the disconnect occur within 3 s, if the speaker data is disabled the message wont disappear until the Wiimote is reconnected

The real Wiimote frequency for different IPC frequencies

100 samples moving average [min max]

Data report (WM_REPORT_CORE) in Wii Sports (before pressing a button)

// IPC_HLE_INPUT_PERIOD = 100, IPC_HLE_PERIOD = 1500
30:54:685 Src\HW\Wiimote.cpp:538 W[Wiimote]: Hz: 40,5 [9,1 647,5]
// IPC_HLE_INPUT_PERIOD = 200, IPC_HLE_PERIOD = 1500
29:34:554 Src\HW\Wiimote.cpp:538 W[Wiimote]: Hz: 40,2 [9,7 874,0]
// IPC_HLE_INPUT_PERIOD = 400, IPC_HLE_PERIOD = 3000
31:36:868 Src\HW\Wiimote.cpp:538 W[Wiimote]: Hz: 41,4 [9,7 683,8]
// IPC_HLE_INPUT_PERIOD = 1000, IPC_HLE_PERIOD = 6000
32:24:080 Src\HW\Wiimote.cpp:538 W[Wiimote]: Hz: 42,6 [10,1 583,4]

IOWrite in DKCR (before pressing a button)

// IPC_HLE_INPUT_PERIOD = 100, IPC_HLE_PERIOD = 1500
39:56:819 Src\HW\WiimoteReal\WiimoteReal.cpp:260 W[Wiimote]: Hz: 0.8 [1.$ 0.0]
// IPC_HLE_INPUT_PERIOD = 200, IPC_HLE_PERIOD = 1500
38:03:569 Src\HW\WiimoteReal\WiimoteReal.cpp:260 W[Wiimote]: Hz: 41.1 [21.6 41.4]
// IPC_HLE_INPUT_PERIOD = 400, IPC_HLE_PERIOD = 3000
36:24:442 Src\HW\WiimoteReal\WiimoteReal.cpp:260 W[Wiimote]: Hz: 45.4 [28.3 97.7]
// IPC_HLE_INPUT_PERIOD = 1000, IPC_HLE_PERIOD = 6000
34:54:753 Src\HW\WiimoteReal\WiimoteReal.cpp:260 W[Wiimote]: Hz: 53.0 [24.8 54.5]

Merge status

[11:08] <_RachelB> JPeterson: was https://github.com/john-peterson/dolphin-emu/commit/4f4c4d3a572bd8138d2a6edd3e1485729fc253b8 ever tested with 4 real wiimotes? …

There's a comment about 4 real Wiimotes in the topic "Test → Real Wiimote"

… I'd like to get that merged if possible

I oppose a merge of this patch if it's not merged together with Removing muted Wiimote audio because of the arguments in the topic "Merge status" in that post

Discussion

[03:38] @skid_au i think we should make it 200.. but even better would be to fix it properly so that the enable speaker option does not affect the connectivity

[04:34] @Billiard disconnects happen in DKCR because the real wiimote code delays packets when there is audio data

[04:39] @Billiard JPeterson: and fyi, the disconnects are only in DKCR (and a few random others) because DKCR (and a few random others) continuously send speaker data, even when sound is not audible

Which should be limited to 200 Hz because the speaker IOWrite otherwise interrupt other IOWrite

Which other game send WM_WRITE_SPEAKER_DATA continuously?

[06:25] @Billiard and do 4 wiimotes in mkwii still work

[06:55] @skid_au yer gotta test Rhythm Heaven Fever Remix 6, DKCR, Just Dance 3, MKWii and Wii Party 4 player mini-game for a full test

The test is described in the topic "Test"

@Billiard why do we still have wiimote_to_update for real wiimotes! @Billiard afaik, wiimote_to_update was used to get input from wiimotes at separate times so as to not overflow the silly internal buffer [06:33] @Billiard if that isn't needed, great, but I really do not see how this has anything to do with real vs. emu

Do you refer to m_queue?

How do I produce the m_queue overflow problem?

@Billiard yeah, so like, it shouldn't really be an issue anymore probably

Why isn't it an issue anymore probably?

@Billiard JPeterson: not an issue anymore probably because the IPC HLE "update" happens often enough, and that buffer goes up to 100 now @Billiard change one of those to make it overflow

The patch is changed to update real Wiimotes simultaneously too because

m_queue size isn't large (described above)
it's expected that the m_queue size when using real (compared to emulated) Wiimotes isn't significantly different

@Billiard but I don't see why you don't update all 4 real wiimotes at the same time as well

I can't test 4 real Wiimotes because I have 1

Why can I know that the ACL queue isn't significantly larger from a real than emulated Wiimote? I.o.w. why can I know that the real Wiimote doesn't result in more m_queue packages?

Outdated communication

The following communication is outdated because it's based on an incorrect belief that non-simultaneous update has a meaning for real Wiimotes

[21:24] @Billiard JPeterson: why are you handling read wiimotes differently than emu'd?

The real Wiimote should be read

separately from the emulated Wiimote because of the arguments in the topic "Solution", f.e.

it's less important that the real (compared to emulated) Wiimote input is deterministic because it's less important for recording
the real Wiimotes are read with a (maximum) distance between each other rather than simultaneously (which appear to be without meaning for en emulated Wiimote) since a80429b without an explanation (perhaps because it might reduce wireless interference between Wiimote signals) which encourage keeping that code because its purpose isn't known

[22:43] <_RachelB> JPeterson: the first wiimote patch is fine

[03:33] @Billiard JPeterson: "Unsaved Wii input state value" is probably fine as long as everything still works

john-peterson commented 11 years ago

Dual thread GC and Wii input order

Problem

Playing a Wii movie with

GC pad enabled ("SIDevice0 = 6")

and these settings that increase the probability of the occurence

"CPUThread = True" and "SyncGPU = False"

play Wii input from GC input and vice versa with the message

./dolphin -B -e wii_sports_menu.dtm
44:32:859 Src\MsgHandler.cpp:80 E[*]: Warning: Type at input     26: rec 0, cur 1

because

the dual thread GC and Wii input update order in't deterministic if their update rate isn't evenly divisible ' the tick distance between CoreTiming scheduled events vary in dual thread which it doesn't do in single thread

f.e. the unevenly divisible frequencies

Wii input: 200 Hz
GC input: 60 Hz

result in a GC and Wii recording eventually being inside the tick uncertainty region

with the result that the played et_IPC_HLE and et_SI scheduled event order differ from the recorded

Solution

Keeping the Wii and GC input update GC outside the tick uncertainty region by updating them at the same rate (at a large tick distance)

because that allow recording GC and Wii input simultaneously in the same array

Changing the emulated Wii input update

frequency from 200 Hz to VideoInterface::TargetRefreshRate

This is the only even Wii input update frequency because the IPC_HLE update frequency is VideoInterface::TargetRefreshRate * 25

If the multiplier is changed from 25 to 30 there are two even update frequencies, VideoInterface::TargetRefreshRate and VideoInterface::TargetRefreshRate * 2

because

that's better for a movie because
- that allow exact input control by pausing emulation at a frame rather than at the Wiimote update
- it's a constant number per frame
it's not known that
- altering the emulated Wiimote status update rate alters the speaker data rate (from CWII_IPC_HLE_WiiMote::ExecuteL2capCmd) (rather than the rate at which the real Wiimote consider the speaker data) which is mentioned as a reason to update it at 200 Hz
- any Wii program read the Wiimote status more often than at 60 Hz

Removing WII_IPC_HLE_Interface::Update() from Write32 because

that's called from the emulation which can't be controlled
it doesn't have a benefit because the update rate VideoInterface::TargetRefreshRate * 25 per second is adequate

The alternative solution

record and play GC and Wii input separately

is less good because a deterministic Wii input is better because it's necessary for an exact movie

Even when IPC_HLE and SI update has the same tick interval a dual threaded movie recorded from boot won't be deterministic because the tick when Wiimote connection occur varies, with this message as result

./dolphin -B -e dkcr_boot.dt
39:00:798 Src\MsgHandler.cpp:80 E[*]: Warning: Type at input     92: rec 0, cur 1

Test

It's tested with benchmark

./dolphin -BC normal -e jungle_hijinxs.dtm

these ISOs in order of sensitivity for Wiimote disconnect when the Wiimote update is wrong

DKCR: Wiimote disconnect when IPC_HLE_PERIOD < 25 * ,4 (around 600 Hz)
Zelda - TP: discussed in fa27a97
Wii Sports
Mario Kart Wii
Punch Out

Discussion

@_RachelB

[00:05] <_RachelB> JPeterson: wiimotes still desync

does desync mean the messages in code blocks in the topic "Problem"?

place the build, configuration and movie in Dropbox because that allow me to run the same file and configuration as you

run it with the files in the topic "Files" in benchmark

run the movie ./dolphin -BC normal -e nsmb_1-1.dtm with your and those files because that show if both give the same result

describe the movie because that allow me to record a similar movie

<_RachelB> if you record a movie, making a save state at the start, then play it back repeatedly by loading the save state with read only, it should eventually desync

Despite that, you should place the build, configuration and movie that desync in Dropbox because

there's no explanation for how the GC and Wii input order can be non-deterministic
it's valuable to know that
this command has been running to 50 (during 50 * 25 s) without a run ending before "Movie End."

i=0; while :; do ./dolphin -BC normal -e nsmb_1-1.dtm; ((i++)); echo $i; done

That's what I understand to mean desync. What do you mean with desync

<_RachelB> but despite playing back in the appropriate order, the outcome of the movie is sometimes different than it was when recorded when using save states <_RachelB> for example, if you record jumping over a goomba, it might instead run into the goomba and die <_RachelB> the Wii input update should be more frequent than once per `VideoInterface::TargetRefreshRate` <_RachelB> JPeterson: it makes the parts that require shaking in pucca's kisses game impossible, change it back

You should

commit that change rather than me
if it's different from the GC input rate, explain why the GC and Wii input rate should be different

because the emulated Wiimote update is changed from 200 Hz to VideoInterface::TargetRefreshRate (60 Hz) because

I'm not convinced that a faster update is meaningful i.o.w. that a game process input more often
the GC and Wii input update rate should be the same unless there's an opposing reason for that, because that's consistent
the frequency 200 Hz
- isn't based on knowledge about its meaning for input
- is based on discussion about the real Wii frequency, real Wiimote latency or speaker audio quality

[21:24] @Billiard JPeterson: why are you handling read wiimotes differently than emu'd?

The real Wiimote should be read

at 200 Hz rather than 60 Hz because

the real Wiimote by its (rather than the Wii's) initiative send input status at 200 Hz (in the code it's also been written that this occur at 100 and 150 Hz) which affect the speaker data rate (CWII_IPC_HLE_WiiMote::ExecuteL2capCmd) and audio quality
however, there's no indication that a game read input status more often than 60 Hz

What's the source for the information that the Wii read the Wiimote at 200 Hz?

@Billiard JPeterson: uh, there is homebrew somewhere that shows the connection "quality". it approaches 100% as input approaches 200hz, and audio doesn't work until well over 100hz? @Billiard actually it might not even be homebrew, might be nintendo thingy

How do i call WPADGetRadioSensitivity from libogc (f.e. in TestSuite WPAD)?

@skid_au

[14:46] @skid_au JPeterson, yeh, commit the deterministic wii input into a branch, judging by the code, i think that's gonna break 4 wiimote connections [14:47] @skid_au changing: IPC_HLE_PERIOD = GetTicksPerSecond() / (freq * VideoInterface::GetNumFields()); [14:47] @skid_au to SystemTimers::GetTicksPerSecond() / (VideoInterface::TargetRefreshRate * SystemTimers::Get_IPC_HLE_Period()); @skid_au it's not going to update often enough @skid_au the wiimotes will disconnect in various games

The Wiimote connection

isn't affected for NTSC because

the IPC_HLE_UpdateCallback frequency is still 1 500 Hz because VideoInterface::TargetRefreshRate * 25 is 1 500

probably not affected for PAL because

the PAL IPC_HLE_UpdateCallback frequency is 1 250 Hz (instead of 1 500 Hz)
the highest tested frequency where the Wiimote disconnect (within 30 s) is around 600 Hz in DKCR NTSC
if PAL games run at 50/60 of the NTSC speed the PAL IPC_HLE_UpdateCallback frequency should be 50/60 of the NTSC frequency because that makes the frequency relative to the game speed equal for NTSC and PAL

@rdragoon

... or does it, and it was just that check which you disabled that was causing it to stop?

Yes.

it doesn't make sense to break support for changing discs while playing back gc+wiimote at the same time I agree, but even without disc change support it's an improvement.

why not just use "if (!tmpHeader.bWii)"?

Fixed.

It should just work as is for wii games, so long as it uses a gc controller

I agree. I'm guessing there's an unzeroed buffer recorded from the gc pad that result in unwanted input during playback.

@NeoBrainX

Random "// TODO: fix this" comments in 0680e0052ec5?

It's necessary for a Wii movie f.e. ./dolphin -B -e file.dtm to play to completion

john-peterson commented 11 years ago

Saving real Wiimote state

Commit

The commit in this branch with the same title as this post

Problem

Reconnect after state load isn't functional

Reconnecting when loading a state create an unhandled Wiimote state where m_EventQueue.empty() != 0 and m_HCIEndpoint.IsValid() = false so that m_acl_pool isn't emptied which result in this message

./dolphin -e RMCE01.s04
19:32:939 Src\IPC_HLE\WII_IPC_HLE_Device_usb.cpp:538 E[WII_IPC_WIIMOTE]: ACL queue size reached 100 - current packet will be dropped!

The code

m_WiiMotes[i].Activate(false);
m_WiiMotes[i].Activate(true);

doesn't reconnect the Wiimote as it's used CWII_IPC_HLE_Device_usb_oh1_57e_305::DoState because

m_HCIEndpoint.IsValid() = false so that SendEventRequestConnection isn't called

Hang when reconnecting often

The code

m_WiiMotes[i].Activate(false);
m_WiiMotes[i].Activate(true);

is unreliable when run often as observed by

running Wii Sports to the "press A an B" message
using one (because that's simpler than multiple) emulated Wiimote
placing this in CFrame::ConnectWiimote

    GetUsbPointer()->AccessWiiMote(wm_idx | 0x100)->Activate(false);
    GetUsbPointer()->AccessWiiMote(wm_idx | 0x100)->Activate(true);

holding the reconnection hotkey for around 10 s until the picture freeze

and observing that "Video Thread" is at g_cpu_thread.join() and "Main Thread" at g_EmuThread.join() (apparently as a result of a terminated emulation) which is a deadlock condition because the threads can't be joined from themselves (rather than from the interface thread)

No meaning with reconnecting

There's no meaning with reconnecting the real Wiimote after state load

because its state can be saved instead

Solution

Saving the real Wiimote state because that's simpler than reconnecting

The Wiimote state is saved in WiimoteEmu through the existing hybrid Wiimote function that save a mirror of the real Wiimote state

If this means an issue compared to not using the hybrid Wiimote function that's because of a bug in this functions that it's beneficial to locate

With this patch, the only difference betwen WIIMOTE_SRC_REAL and WIIMOTE_SRC_HYBRID is that emulated Wiimote input is disabled for the previous

The loaded state is channel and reporting mode because that's adequate

Connection

Not changed

This doesn't affect the real Wiimote operation because a difference in the real Wiimote operation between the real and hybrid operation isn't observed in

test
code analysis
diff of WiimoteEmu::Spy output

after resetting the Wiimote state with

turning off: holding the "POWER" button until the light is disabled
turning on: pressing "POWER"
pairing with Dolphin: open Dolphin. starting the Wiimote scanner by opening the Dolphin Wiimote Configuration dialog. open the Wiimote battery cover, press red button, Wiimote lights flash, "Bluetooth HID Device" systray notice is shown, Dolphin vibrate the Wiimote

Removing reading of the emulated Wiimote state in the real Wiimote mode

idan...@gmail.com

in

SMNP01 - smb wii SUKE01 - kirby

the wiimote doesn't connect at times, disconnects randomly

a video show a repeated "connection with the Wii remote was lost" dialog at the start of a NSMB world 2 level at 60 FPS

in the nsmb + other 2 games that require you hold the wiimote HORIZONTALLY , it doesnt work. it keeps on working like VERTICALLY held wiimote.

This is solved in the commit with the same name as the title of this topic by

not handling WM_REQUEST_STATUS and WM_READ_DATA in the real Wiimote mode

i.o.w. by making the emulated Wiimote object write only rather than read and write

Problem description

The hybrid Wiimote read the emulated Wiimote object with the intention of

synchronising the Wiimote extension with the emulated Wiimote

This doesn't have meaning for the real Wiimote mode because the emulated Wiimote isn't used

The problem was created in the commit "Saving real Wiimote state" because it doesn't solve the problem as described in this topic

Adding read data reply to the real Wiimote

This solution is an inadequate version of the solution in the topic "Removing read emulated Wiimote data from real Wiimote"

This problem is solved in the commit with the title "Adding read data reply to the real Wiimote" by

adding a read data acknowledgement 0x21 to the real Wiimote

There's a link to a branch that contain this commit in the topic "More information"

There's a link to a folder that contain a build of this branch (in the file Dolphin.exe) in the topic "Log"

The read data acknowledgement was unintentionally disabled for the real Wiimote in the commit with the title "Saving real Wiimote state"

Merge status

[17:18] <_RachelB> JPeterson: seems ok, i'll merge it

Problem description

The problem that the emulated program detect input (f.e. the B button in the NSMB level start screen) but still report that the Wiimote is disconnected can be a result of missing or unsynchronised

read data acknowledgement 0x21
output report acknowledgement 0x22

because

the emulated program expect these reports in addition to data reports from a functional connection

Removing wait for real Wiimote connection

The wiimote screen takes forever to appear …

This is solved in the commit "Removing wait for real Wiimote connection" by

not waiting for Wiimote::Initialize when

opening the Wiimote configuration dialog
booting from the emulated program entry point rather than a state

because

it doesn't have meaning in these scenarios but has meaning when booting from a state

Reason for wait

The commit "Saving real Wiimote state" add a wait function to Wiimote::Initialize because

the state shouldn't be loaded before the Wiimote connection is established because the Wiimote connection is used to apply the real Wiimote state

Merge status

[17:19] <_RachelB> JPeterson: the removing wait patch is good too, i'll merge that as well

Hybrid IR

I have to use "hybrid" to even have functionality in game (bypass wiimote screens) BUT even then I have no IR

The hybrid IR isn't implemented because of a challenge in combining the emulated and real IR input

Connecting more than one Wiimote

Also, I can only connect one wiimote. I've tried four wiimotes, new and old, and all have the same problem

Do you mean that the log write the message

55:34:559 Src\HW\WiimoteReal\WiimoteReal.cpp:458 N[Wiimote] Wiimote scanning has started.
55:34:559 Src\HW\WiimoteReal\WiimoteReal.cpp:577 N[Wiimote] WiimoteReal::Initialize
55:37:316 Src\HW\WiimoteReal\WiimoteReal.cpp:650 N[Wiimote] Connected to Wiimote 1.

when you expect it to also write

55:37:366 Src\HW\WiimoteReal\WiimoteReal.cpp:650 N[Wiimote] Connected to Wiimote 2.
…

because there's more than one Wiimote in the Bluetooth range?

This problem isn't caused by the commit "Saving real Wiimote state" because

the log output in this topic is created with this commit

Settings

Communicate the User/Config/WiimoteNew.ini settings through Dropbox because that information has meaning because WiimoteScanner don't attempt to connect 2 Wiimotes if less than 2 Wiimotes has the Source 2 or 3, as in this example

[Wiimote1]
; real
Source = 2
; hybrid
; Source = 3

[Wiimote2]
; real
Source = 2
; hybrid
; Source = 3

Connection time

… and has a lot of trouble initially syncing up with wiimote

This isn't a result of the commit with the title "Saving real Wiimote state" because

it doesn't change WiimoteScanner

Reproduce

Place the Dolphin folder in Dropbox (and paste the link to the Dropbox folder) because

that can make it easier to reproduce the problem because it allow

running the same code and settings
reading User/Logs/dolphin.log

Log

Run Dolphin.exe from this folder

because

it has the log function WiimoteEmu::Spy enabled which is beneficial to identify the problem

State

Run the states from the folder linked in the topic "Log"

with command

./dolphin -e SMNE01.s01
./dolphin -e SMNE01.s02
…

./dolphin -e SUKE01.s01
./dolphin -e SUKE01.s02
…

or with the GUI in "File > Open"

and

communicate if the Wiimote input works directly after and a longer while after loading the state
share the Dolphin folder from this test with Dropbox because that allow reading User/Logs/dolphin.log from this test

Real compared to hybrid

Communicate if the real Wiimote operation in hybrid mode

that's enabled with the User/Config/WiimoteNew.ini settings

[Wiimote1]
; real
Source = 2
; hybrid
; Source = 3

is different because

if the hybrid mode is functional the real mode can easily be made functional

Frame rate

Communicate if there's a difference in how the real Wiimote operates between the User/Config/Dolphin.ini settings

[Core]
; off
FrameLimit = 0x00000000
; auto
FrameLimit = 0x00000001
; 30
FrameLimit = 0x00000007
; 60
FrameLimit = 0x0000000d

because

the frame rate is meaningful because the emulated program is sensitive to the Wiimote communication rate

More information

There's more information about this patch at https://github.com/mirror/dolphin-emu/pull/1#issuecomment-18724064 that can be useful to analyse the problem

State reliability

Read reply state not saved

The real Wiimote states are potentially unreliable because

m_read_requests isn't written
if the state is saved between sending a read request to the Wiimote and the Wiimote returning a read reply the connection can be reported as disconnected without a way to continue the emulated program (f.e. at the NSMB level start screen)

This problem is mitigate because

it's a low probability to save such a state because registry read is infrequent

Pausing data reporting

Disabling Wiimote data reporting during CORE_UNINITIALIZED and CORE_PAUSE because that

reduce battery use
pause Spy logging (if logData = true too) so that the recent log can more easily be inspected because the Log window auto scroll

Bit comparison

[09:52] @Matt_P you are treating it like a bitfield, which it is not, it is just an enum member

The expression const & g_wiimote_sources will be true for these combinations

// const = WIIMOTE_SRC_NONE

// WIIMOTE_SRC_EMU
g_wiimote_sources = WIIMOTE_SRC_EMU
WIIMOTE_SRC_HYBRID

// WIIMOTE_SRC_REAL
WIIMOTE_SRC_REAL
WIIMOTE_SRC_HYBRID

// WIIMOTE_SRC_HYBRID
WIIMOTE_SRC_EMU
WIIMOTE_SRC_REAL
WIIMOTE_SRC_HYBRID

State version

[08:45] <_RachelB> er, don't forget to bump the state version

The state version shouldnt be changed because the state isn't changed

Merge status

[14:20] @skid_au JPeterson, looks ok

[08:44] <_RachelB> i'm ok with the first one

[18:15] @Billiard idunno, I hate everything related to hybrid wiimote, it was done (by me) in such a hacky manner

Test

It's tested with a real Wiimote

DKCR: world 1 and 2
NSMB: level 1-1, 2-1
Kirby's Return to Dream Land: 1-1, part of 7-1

with the settings

without extension

Wii Sports: menu
Mario Kart Wii: battle mode level

with the settings

without extension
with Nunchuk extension

Wii Sports: menu

with the settings

two real Wiimote

with

http://www.cpubenchmark.net/cpu.php?cpu=Intel+Core+i7-3770K+%40+3.50GHz

Refactor

Moving swap24 to Common because

it's used in Spy
it's better organisation

Log

Adding a WiimoteEmu::Spy function that log a Wiimote connection because that allow

observing the Wiimote connection

This is useful because

it allow
- emulating the real Wiimote
- identify problems in the real Wiimote connection
without this log the knowledge about the Wiimote connection is limited

It's appropriate to merge this with this patch because

it's a non-zero probability that this patch change the real Wiimote operation because of a bug or feature in the hybrid Wiimote operation (the topic "Solution → Real Wiimote" comment on this)
if this patch change the real Wiimote operation the difference can be observed by comparing (in f.e. diff or WinMerge) the output in WiimoteEmu::Spy when using a real and hybrid Wiimote

[12:17] <_RachelB> JPeterson: what exactly is the purpose of the 650 lines of #if 0'd code in WiimoteEmu::Spy() again?

This is described in this topic

john-peterson commented 11 years ago

Removing muted Wiimote audio

Patch

The commit in this branch with the same title as this post

Problem

_IOWrite has limited capacity

DKCR continuously send audio reports at 41.1 [21.6 41.4] Hz (this information is in the topic "Unsaved Wii input state value") that delay WM_ACK_DATA so that the game detect a disconnect around 8 s after it's connected with the message

08:26:364 Src\Main.cpp:770 N[Wiimote]: Wiimote Connected
08:34:589 Src\Main.cpp:770 N[Wiimote]: Not connected

Just Dance 4 disconnect after around 7 s with the message

54:02:963 Src\Main.cpp:770 N[Wiimote]: Wiimote Connected
54:09:832 Src\Main.cpp:770 N[Wiimote]: Not connected

Solution

Removing muted audio because

that won't disconnect the Wiimote during normal play when audio is enabled

It's possible that the Wii hardware doesn't send muted audio

Test

The patch is tested in

DKCR: world 1 and 2
Just Dance 4: one song

With settings

WiimoteEnableSpeaker = True

Normal play in DKCR often doesn't disconnect the Wiimote because the audio often isn't continuous for a long period

Sending audio data continuously by changing frequently between the "New Game" options

disconnect the Wiimote after around 17 s, after which delayed audio continue for around 5 s

Merge status

These commits should be merged at the same time

Removing Wii input use of an unsaved state value
Saving real Wiimote state
Removing muted Wiimote audio

because

"Removing Wii input use of an unsaved state value" change the Wii input update frequency from 160 to 200 Hz which enable audio in DKCR which disconnect the real Wiimote (if WiimoteEnableSpeaker = True) without the patch in this post
it's a significant probability that "Removing Wii input use of an unsaved state value" will be opposed because of this problem if it's not merged together with "Removing muted Wiimote audio"

[11:50] @skid_au wiimote mute code looks ok to me

Billiard's approval is appropriate because

Billiard has written part of the code that's changed

Discussion

[19:19] @Billiard it would maybe work when you are using hybrid wiimote, even then I don't know if you can safely drop speaker packets whent he speaker is muted, what about the the wiimote's decoder state? [20:35] @Billiard the adpcm decoder in the wiimote [20:35] @Billiard adpcm decoder has state which is affected by samples [20:48] @Billiard because each sample affects the wiimote's adpcm decoder state [20:47] @Billiard I expect your speaker packet dropping to not be foolproof when it is 0x00 (adpcm) [20:47] @Billiard (and it's always adpcm) [20:49] @Billiard and you are dropping samples

This means that m_adpcm_state (when m_reg_speaker.format is 0x00) can become desynchronised so that audio is altered

There isn't meaningful indication of that in the games in the topic "Test" because audio isn't perceived to be altered or missing

[20:59] @Billiard perhaps the games are (sanely) not sending packets that alter the state (zeros or w/e) when muted

[21:02] @Billiard I think you should check that the samples are all zeros maybe

This is the first muted WM_WRITE_SPEAKER_DATA from DKCR after boot

16:07:479 52 18 a0 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08
16:07:480 52 18 a0 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08
16:07:485 52 18 a0 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08
16:07:490 52 18 a0 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08
16:07:496 52 18 a0 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08

[21:14] @Billiard dunno what they mean [21:15] @Billiard I mean, dunno if it's stuff to maintain the same decoder state or what

[03:31] @Billiard JPeterson: doesn't the wiimote audio thing still have the "only working with hybrid wiimote" problem?

No because it's solved in https://github.com/mirror/dolphin-emu/pull/1#issuecomment-18724064 "Saving real Wiimote state"

john-peterson commented 11 years ago

Moving thread name function to a class

Problem

These features are missing

thread name list
deadlock detection
thread functions organised

Solution

moving the thread naming to a class
moving Core threads to a class
adding a thread std::map

because

that's better organisation

The reason that Thread

don't allow running a thread without a name is

all threads should have a name because that allow locating deadlocks

don't allow running a thread from its creator

to ensure class pointer creation before the thread is started (which can't be ensured if the thread is started in the creator)

Joining one thread at a time because

it's simpler
it can reduce the number of deadlock threads from 2 to 1 as described here

std::thread::id

The std::thread::id type is

VC, xthrcommon.h: unsigned int
libstdc++, thread: std::thread::native_handle_type
Apple LLVM, XcodeDefault.xctoolchain/usr/lib/c++/v1/thread: pthread_t

john-peterson commented 11 years ago

Creating a namespace for memory utilities

Patch

The commit in this branch with the same title as this post

Problem

The memory utilities doesn't use a namespace

Solution

Creating a namespace for memory utilities because

f.e.

Memory::AllocateExecutable
Memory::AllocatePages

is better organisation than

AllocateExecutableMemory
AllocateMemoryPages

Memory message

Problem

A lack of memory messages during start and stop

makes memory allocation and duration of start and stop less clear

Solution

Adding memory messages

john-peterson commented 11 years ago

Copying fork to origin branch

Purpose

@skid_au you can commit all of your stuff into a branch so other people can test it

I've copied this branch to origin/cli because

that makes it easier to checkout the branch compared to checking out my fork branch

Origin branch removed

I've removed origin/cli because

this branch is changed (by moving code about logging to a separate commit because it's not directly related to the other changes in the commit)
rebasing a non-master origin branch is opposed
it's therefore better to remove it than
- rebase it
- keep it unchanged because it's different from this branch and this branch should be considered instead

`push -f` opposed

[15:47] <_delroth> [don't use push --force in an origin branch] because it's confusing for users, it's confusing for developers who want to check out your branch, …

Why?

[15:47] … it's confusing for our tools …

@neobrain JPeterson: force-pushing might make the build bot sad …

Why?

<_Bh44L> you shouldnt cause trouble for people looking at or basing stuff on your branch by rebasing every other day

There's no description in this topic of a trouble with push --force (i.o.w. there's no description of the problems "confusing for users", "confusing for our tools")

<_Bh44L> confusing for users, you break their parent references - they're basing off a commit that doesnt exist anymore

It's not a significant problem for

git pull because

git pull remote/branch

give a message when remote/branch has been rebased

Automatic merge failed; fix conflicts and then commit the result.

informing the user to run

git merge --abort; git pull --rebase remote/branch

<_Bh44L> confusing for tools, replays/fast-forwards dont work if parent commits are gone - a build tool because it can use `git pull --rebase` instead of `git pull` - a mirror because it can use `git clone --mirror /home/user/repo` and update with `git remote update` @neobrain … and given that there's no reason to do tests like this in the main repo you shouldn't challenge it

If rebasing a non-master origin branch is opposed it's better for me to not copy my fork branch to origin because

it's beneficial for me to change the commit content and order so that it's as logical as it can be

[16:54] <_Sonicadvance1> JPeterson, Are you still trying to accumulate reasoning to use --force?

Yes, it's my opinion that push --force should be allowed in a non-master origin branch because of the reasons in this topic

But it's not important for me because my branch can easily be tested by pulling it from my fork (compared to from origin)

It's beneficial to use push --force in a test branch to change commit order or commits because

having one logical change in one functional commit (rather than more than one commit) is beneficial because

its changes can then be viewed from one (rather than more than one) commit

<_delroth> and a branch merge is also one functional commit - it's correct that if a branch has one logical change all changes from it can be showed by showing all commits in a branch merge (with the command below), but a branch doesn't always have one logical change because it's sometimes beneficial to create more than one logical change in a branch because they're related

git show merge_hash
# copy the hash from "Merge: hash hash"
git log hash..hash -p

reverting one logical change involves one (rather than more than one) commit

<_delroth> reverting a branch merge is still only one commit

It's correct that all commits from a merge can be removed by removing the merge commit, but all commits in a merge isn't always wrong if one commit in a merge is

[17:03] <_Bh44L> humm, your reasoning is good for a local branch, but if you must use --force and push it out, something went wrong in your workflow (if you have to reorder AFTER pushing them)

If the opinion is that it's wrong to change opinion (about commit order or content) after git push I disagree because

git push is necessary to receive opinion about the code and that opinion can make it clear that another commit order or content is more logical
thinking and knowledge (without another opinion) after git push can make it clear that another commit order or content is better, despite the presence of adequate thinking and knowledge before git push

john-peterson commented 11 years ago

Adding CLI mode to GUI build etc.

The benefits with

CLI mode in GUI build

CLI and GUI mode can be run from the same program file
it's easier to automatically build one rather than two (GUI and CLI program) targets

running without GUI

remove potential performance impact of GUI

booting from a movie or state

expose missing state values
quickly test code that fix issues at a particular location in an ISO

benchmark

accurately test performance impact of changes

check state version before boot because

otherwise loading a state with incorrect version from CLI might not be communicated to the user because the notice is only printed to DisplayMessage which isn't visible yet

john-peterson commented 11 years ago

Changing Wiimote logging from logging all to logging changed data reports

Commit messages

because unchanged data reports

don't contain meaningful information
use space in the log history

Logging all communication

Problem

All Wiimote communication isn't logged in WiimoteEmu::Spy

Not logging all data reports

Problem

Logging all data reports is a problem because

it moves preceding communication in the log

Solution

Logging data reports that are

preceded by a non-data report
a different mode

john-peterson commented 11 years ago

Adding IR to hybrid Wiimote

Problem

IR isn't enabled for the hybrid Wiimote

This is a problem because

the hybrid Wiimote allow faster change between real and emulated input than changing Wiimote source between Real and Hybrid

john-peterson commented 11 years ago

Adding wait for real Wiimote connection

Commit message

when

booting from the emulated program entry point

because

the hybrid Wiimote connection isn't established automatically if the real Wiimote connection isn't present on boot

Problem

The problem is

the hybrid Wiimote connection isn't established automatically if the real Wiimote connection isn't present on boot
a manual disconnect/connect is required to enable it

The Wiimote connection sohuld be enabled on boot because

the hybrid connection is more complex if it can't rely on that
the wait time isn't increased significantly because the Wiimote connection is established in around 3 s

Real Wiimote mode

The real Wiimote mode wait for a Wiimote connection before booting to because

despite that the connection works (without a manual disconnect/connect)

there's no increase in wait time because the connection with the emulated program must also wait for the Wiimote connection before it becomes functional
it's conceivable that some emulated programs can't handle this or that a change in Dolphin unintentionally makes a connection fail if the Wiimote connection isn't avaliable on boot
it's less complex to not rely on the fact that Dolphin or the emulated program can handle a non-present Wiimote connection when booting a program
it's less complex to not differentiate between the hybrid and real mode when waiting

Analysis

The last connection messages when booting to a non-functional hybrid connection is

23:33:286 Src\HW\WiimoteReal\WiimoteReal.cpp:645 N[Wiimote] Connected to Wiimote 1.
23:33:287 Src\Thread.cpp:124 N[COMMON] Created "Wiimote" 10360
23:33:288 Src\Core.cpp:126 N[CONSOLE] Wiimote 1 Connected
23:33:288 com[R] WM_ACK_DATA 12 03
23:33:288 data[E] | id  | a1 30  | c  | a | -4.92 -4.92 -4.92 | ir  | ext 
23:33:290 com[R] WM_STATUS_REPORT
23:33:290 extension: 0
23:33:290 data[E] | id  | a1 30  | c  | a | -4.92 -4.92 -4.92 | ir  | ext

It's not clear from this which report is missing

The missing report can be 0x21 or 0x22 messages is missing because

that's a common source of a non-functional connection

Test

Tested by booting

Wii Sports

at the FPS

60
unlimited

with the CPU

http://www.cpubenchmark.net/cpu.php?cpu=Intel+Core+i7-3770K+%40+3.50GHz

john-peterson commented 11 years ago

Adding option to disable Wiimote connection motor notification

Commit message

by disabling motor

because

it makes a noise

Problem

the Wiimote connection notification makes a noise
a motor notification isn't always meaningful because there's a "Bluetooth HID Device" systray notification when the device is connected

Solution

Adding an option to disable the motor notification by disabling the motor

john-peterson commented 11 years ago

Synchronising the hybrid Wiimote calibration

Commit message

because

it makes control more accurate

Problem

The emulated Wiimote don't use the same calibration as the real Wiimote in hybrid mode

This is a problem because

the emulated program expect stick input in (rather than outside) the calibration range

Extension calibration

It's a problem that the emulated rather than real calibration is used because

this emulated calibration that's sent to the emulated program

Nunchuk calibration
zero x: 128
zero y: 128
zero z: 128
g x: 179
g y: 179
g z: 179
j x center: 128
j x max: 255
j x min: 0
j y center: 128
j y max: 255
j y min: 0

makes data reports from a Nunchuk with f.e. this calibration (from a RVL-004 Nunchuk) less accurate

Nunchuk calibration
zero x: 130
zero y: 128
zero z: 127
g x: 181
g y: 179
g z: 180
j x center: 126
j x max: 227
j x min: 32
j y center: 124
j y max: 226
j y min: 31

Test

This commit can be tested with

use Hybrid Wiimote
enable WiimoteEmu::Spy
use the emulated Nunchuk stick and observe the Nunchuk stick value in the log

the WPAD test

Missing calibration

When the Nunchuk return 0xff as calibration, as f.e.

Logic3 Wireless Nunchuk NW806 (not Motion Plus compatible)

that output

j x center: 125
j x max: 255
j x min: 0
j y center: 126
j y max: 239
j y min: 0

the emulated calibration is used for the real Nunchuk stick

Dynamic calibration

This implementation should be considered

devkitPro solve this by using a dynamic calibration that set

mininimum to the mimium output
maximum to the maximum output

Start values

A problem in the implementation is the start values

27, 128, 255
30, 128, 255

because

the start value for maximum should be lower than 255, f.e. 128, because that allow it to adjust to the maximum input

john-peterson commented 11 years ago

Synchronising the hybrid Wiimote extension

Commit message

by controlling the extension from the real rather than emulated Wiimote in hybrid mode

because

the real can control the emulated Wiimote extension
the opposite isn't true because it involve physical attachment

Connected extension

It's possible to control the extension from the emulated Wiimote because

the real Wiimote accept sending extension data reports without an extension

But there are problems because

it might not be considered in the desing

Frozen extension data

The extension data is sometimes 0xff and sometimes frozen at the last value f.e.

25:36:245 data[R] | a1 37  | ir 1023 1023 1023 1023 | ext 1 1,  33   5, -0.75 [-1.00, 1.00] -0.96 [-1.00, 1.00],   91  154  132

Data report cease

There's a problem when changing the real extension because

the Wiimote cease reporting when changing extension until receiving WM_REPORT_MODE

this can be done at

void Wiimote::Update()
    case WM_STATUS_REPORT :
        wm_status_report &status = *(wm_status_report*)(real_data + 2);
        if (status.extension && m_extension->active_extension > 0)
            WiimoteReal::g_wiimotes[m_index]->EnableDataReporting(m_reporting_mode);

but the problem is

how should the extension reporting be restored when the Nunchuk is attached again? more preciely than with reconnecting
is requesting extension repoting without a connected extension an unhandled scenario that disable extension reporting regardless of message beside reconnecting?

Removing emulated non-data input reports in hybrid mode

This commit remove non-data input reports in hybrid mode

before the commit a non-data input reply to these output reports is created

WM_REQUEST_STATUS
WM_READ_DATA

Sending emulated non-data input reports in hybrid mode has the advantage

more control
give knowledge about Wiimote communication which is beneficial

not sending emulated has the advantage

less complex

john-peterson commented 11 years ago

Adding option to disable log window automatic scrolling

Commit messages

by scrolling to another line than the last

because

automatic scrolling is disruptive when observing previous messages because it change the view from the view with the observed messages to another view

john-peterson commented 11 years ago

Changing the Nunchuk stick axis from center to center + 1

Message

This message is at https://github.com/mirror/dolphin-emu/pull/1#issuecomment-21289093

Commit

In this branch with the same title as this post

Dependency

This commit depend on the commit

Adding Nunchuk stick calibration

Commit message

if the other axis isn't at center

because

it's expected by some emulated programs

Reference

This is discussed in

that mention

a60ad41
a5cde0d

Problem

Produce the problem in A Boy and His Blob

play to the level where jellybean selection is avaliable (start of level 2)
hold Nunchuk Z to display jellybean selection
press Nunchuk stick right to select a black jellybean

Problem description

The selection direction (← red jellybean, ↑ blue, → black) isn't changed if the other axis is at center

f.e. the red or black jellybean isn't selected if

wm_extension::jy == nu_cal::jy.center

Solution

Changing the Nunchuk stick axis from center

if the other axis isn't at center

More information

The black jellybean is selected at

wm_extension::jx >= 188 at angle 0,5π rad

with calibration

0, 128, 255

Discussion

does the commit "Changing the Nunchuk stick radius" fix http://code.google.com/p/dolphin-emu/issues/detail?id=6427

No because

it's not related to the stick radius

will it fix it? JP specifically said it wouldn't

John Peterson, in that link you state:

does "Changing the Nunchuk stick radius" fix http://code.google.com/p/dolphin-emu/issues/detail?id=6427? No because: it's not related to the stick radius

the message mean that the commit "Changing the Nunchuk stick radius" doesn't solve the problem

shouldn't + 1 be the initial state?

No because

it doesn't have a benefit
emulated Nunchuk stick activity is detected when an axis is != center in the commit "Adding Nunchuk to hybrid Wiimote"

Test

Run

./dolphin -e SBLE5G.s02

from this folder

jellybean selection is available in the state

select a red, blue and black jellybean as described in the topic "Problem"

john-peterson commented 11 years ago

Input configuration problem

Non-default input not detected

Produce

use this profile

[Profile]
Device = XInput/0/Gamepad
Buttons/A = `DInput/0/Keyboard Mouse:A`

press A on the keyboard input device

the input A isn't detected (doesn't change "A" in the "Buttons" bitmap from a white to a red background)

"| OR" append to the left instead of to the right

Produce

open the "Dolphin Emulated Wiimote Configuration" dialog

change "Device" to a pad input device (f.e. called "XInput/0/Gamepad")
set a control to Left Y+

right click input configuration to open the "Configure Control" dialog

change to a keyboard device (f.e. called "DInput/0/Keyboard Mouse")
detect W
click "| OR"
the control is | WLeft Y+instead ofLeft Y+| W

john-peterson commented 11 years ago

Adding Nunchuk stick calibration

Problem

The Nunchuk stick doesn't consider its calibration

This is a problem in the hybrid mode that's described in the topic "Synchronising the hybrid Wiimote calibration"

Solution

Adding calibration consideration to the Nunchuk stick

Implementation

The attachment is given access to its registry with

a pointer to WiimoteEmu::m_reg_ext

because compared to copying it to the existing Attachment::reg

use less memory because there's 1 rather than 1 + [number of attachment objects] extension registers
less complex because WiimoteEmu::m_reg_ext isn't copied from/to Attachment::reg

Merge status

[05:42] <_RachelB> skid_au: that one is ok too? I haven't looked at it [05:43] <_skid_au> yeh, that's ok too [05:44] <_RachelB> ok

john-peterson commented 11 years ago

Deterministic multi-Wiimote movie

[16:20] <_RachelB> JPeterson: hey, maybe you can help with this. I'm working on adding wiimote support to netplay, but one problem i am having is that wiimotes do not all connect at the same time, or in any order that can be determined ahead of time. Do you think you would be able to do something about that?

Yes I should analyse that

mirror / dolphin-emu

CLI mode, benchmark from movie etc. #1

CoreTiming state function

Problem

Solution

Skipping DoMarker section

Cheat manager

Log

LogManager

Loading log settings for CLI

Discussion

More deterministic dual thread movie input

Problem

Solution

Comparison

Discussion

Movie synchronisation data

Problem

Solution

Discussion

Wii input shake

Problem

Solution

Discussion

Unsaved Wii input state value

Patch

Problem

Solution

Result

Why not save wiimote_to_update?

Outdated argument

Test

ACL queue size

Real Wiimote

Frequency threshold for audio

200 Hz problem

Data

Merge status

Discussion

Outdated communication

Dual thread GC and Wii input order

Problem

Solution

Test

Discussion

Saving real Wiimote state

Commit

Problem

Reconnect after state load isn't functional

Hang when reconnecting often

No meaning with reconnecting

Solution

Connection

Not changed

Removing reading of the emulated Wiimote state in the real Wiimote mode

Problem description

Adding read data reply to the real Wiimote

Merge status

Problem description

Removing wait for real Wiimote connection

Reason for wait

Merge status

Hybrid IR

Connecting more than one Wiimote

Settings

Connection time

Reproduce

Log

State

Real compared to hybrid

Frame rate

More information

State reliability

Read reply state not saved

Pausing data reporting

Bit comparison

State version

Merge status

Test

Refactor

Skipping `DoMarker` section

Why not save `wiimote_to_update`?

`push -f` opposed