search

nefarius / ViGEmClient

ViGEm Client SDK for feeder development.
https://docs.nefarius.at/projects/ViGEm/
MIT License
134 stars 65 forks source link

Removed NotificationPool and boost references. Created a new notification thread handler logic #7

Closed mika-n closed 5 years ago

mika-n commented 5 years ago

, which is simpler and more straight forward than use of BOOST:ASIO library.

At the same time fixed a thread cleanup bug in unregister_notification functions (sometimes the old code crashed when a notifications was unregistered. There was some multithread cleanup issue).

This fork of ViGemClient was tested with ViGem.NET library in DS4Windows application. No more phantom threads left behind when a connection is closed to a controller object and no more random crashes in contr.Disconnect calls.

nefarius commented 5 years ago

Love getting rid of Boost ASIO, thank you very much.

Was this tested against the issue of out of order calls of the rumble feedback functions though (like happened with Dolphin)?

Then I'd merge immediately.

Cheers

nefarius commented 5 years ago

See #3

mika-n commented 5 years ago

This patch needs further tweaking because it was just found out that sometimes it is possible that an application may flood the FFB event queue and out-of-order issue may happen.

However, the solution is already known and I will change this patch diff soon to include that fix. Anyway, the proposed change does fix the orphanage thread issue and random crashes during contObj.Disconnect calls if FFB event was received "at wrong time" during the unregister_notification.

mika-n commented 5 years ago

@nefarius Now this PR contains a tweaked ViGemClient "notification callback" modification which handles incoming FFB/notification events in correct order even when an application would flood the notification IO stream. The new version of the code uses now only one background thread with a ring buffer to pool DeviceIO requests.

This makes it easier to make sure IO events are handled in correct order without using any multithread synchronization techniques and the ring buffer is efficient and easy way to pool IO requests. Also the buffer makes it easy to increase the size of IO pool if necessary (NOTIFICATION_OVERLAPPED_QUEUE_SIZE). Now the ring buffer size is 6 pooled IO requests. For example Dolphin emulator (and RetroArch/N64 emulator based on tests made by Ryochan7) seems to sometimes flood the notification queue and 3 pooled IO requests seemed to be enough. Pool size 6 adds a bit more room because any other app I have tested doesn't seem to flood the IO queue as much as Dolphin emulator.

I have run several tests using DS4Windows app by forcing it to change profiles few times per second (this makes frequent contrObj.Disconnect and contrlObj.Connect calls and stress tests the IO and thread cleanup procedures). No random crashes and orphanage threads as it happens with the current commited VigemClient branch.

Also, stress tested notification queue with various applications and especially with Dolphin. Notification callbacks were no longer called in wrong order (if bad luck stroke with thread syncing) and no lost events.

To me this looks good. Feel free to test/review/comment the change once you find limited resource called Time. Thanks for the good work with Vigem.

nefarius commented 5 years ago

Thank you kindly, this issue was lingering and bothering me for way too long, time to get this BS rectified, thanks a bunch for the contribution 👍