Skip to content
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

DualShockUDPClient: add motors for rumble #11545

Closed
wants to merge 1 commit into from

Conversation

breeze2
Copy link

@breeze2 breeze2 commented Feb 6, 2023

cemuhook-protocol has been updated, two unofficial message types about rumble were added, so I implemented them.
By the way, I am developing a mobile application about DSU Server, I can vibrate the phone to simulate a rumble.

@iwubcode
Copy link
Contributor

iwubcode commented Feb 6, 2023

While it might work, I'm not sure how I feel about tacking on additional message types like this to the existing protocol. I've commented on the cemuhook-protocol about how version should have been bumped for that change.

@breeze2
Copy link
Author

breeze2 commented Feb 7, 2023

As for bumping version, it does break compatibility.

Let's look at the implementation of Dolphin DSU Client:

E6640365-AC65-4A40-A8F6-A72D08EC74B9

if (m_message.header.protocol_version > CEMUHOOK_PROTOCOL_VERSION)
    return std::nullopt;

The Dolphin DSU Client does not handle the messages from a higher version DSU Server. All features of the higher version DSU Server do not work properly with current or previous Dolphin (version <= 5.0-18515), including the original features.

But what we expect should be that the previous version of Dolphin gets the original features, and the new version of Dolphin gets more features.
This problem also exists in other clients and servers. So I think the version number should not be bumped now, maybe later.

@iwubcode
Copy link
Contributor

iwubcode commented Feb 7, 2023

I said most of my thoughts on the cemuhook PR.

To reiterate, adding messages without rolling the version feels wrong to me. I would expect there to be incompatibilities and that the implementation + the version change would be implemented when new features are formalized in the cemuhook specification. Whether that actually happens, will be up to the cemuhook protocol maintainer(s).

@v1993
Copy link
Contributor

v1993 commented Feb 7, 2023

This queries motor count only once, which might fail due to unreliable nature of UDP. Also not sure why call those virtual motors, since they are often going to be perfectly real.

@breeze2 breeze2 changed the title DualShockUDPClient: add virtual motors DualShockUDPClient: add motors for rumble Feb 7, 2023
@breeze2
Copy link
Author

breeze2 commented Feb 7, 2023

This queries motor count only once, which might fail due to unreliable nature of UDP. Also not sure why call those virtual motors, since they are often going to be perfectly real.

I removed the word "virtual",and made device query motor count every 1 second.

Copy link
Member

@mbc07 mbc07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm strongly against merging this to Dolphin without a bump to the protocol version.

This protocol nowadays is used across several projects, not just Dolphin, if we don't do things correctly now (like bumping the protocol version since additions were made -- like any sane, versioned protocol would do), it'll become a mess in the future.

I would rather make breaking changes now, while (AFAICT) no existing DSU Servers has adopted the new rumble extension yet, than later when DSU Servers would have potentially adopted the new message types with the old protocol version and becomes unpredictable in how they'll interact with DSU Clients not updated to handle the new rumble extension...

@lioncash
Copy link
Member

lioncash commented Mar 2, 2023

Given the protocol author's refusal (based on what I've read so far) to Just Increase The Version like any normal spec or protocol out there (for the record, I agree with increasing the version), should this just be closed, considering this has been sitting for 3 weeks with no input or discussion?

@lioncash
Copy link
Member

lioncash commented Mar 3, 2023

Opting to close given 24 hours has passed

@lioncash lioncash closed this Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants