v1993 / cemuhook-protocol

Mostly complete cemuhook protocol reference
https://v1993.github.io/cemuhook-protocol/
The Unlicense
53 stars 3 forks source link

Add controller rumble #7

Closed breeze2 closed 1 year ago

breeze2 commented 1 year ago

I extended this protocol with two message type:

And I will implement them in Dolphin Emulator. By the way, I am developing a mobile application about DSU Server, I can vibrate the phone to simulate a rumble.

breeze2 commented 1 year ago

Thanks for your reply, I made some changes based on your suggestion. I marked the new message type as "Unofficial". Do them need to be distanced from official values? For example, starting from 0x110001 ? And should I create a new table called "Unofficial extension message types", after the official type table?

v1993 commented 1 year ago

I marked the new message type as "Unofficial". Do them need to be distanced from official values? For example, starting from 0x110001 ?

Yeah, this sounds like a good idea :+1: . If further extensions are introduced, they can utilize 0x11XX00-0x11XXFF value ranges - 256 message types per extension is much more than enough and so is potential for 256 extensions. If we ever somehow hit that, something has gone very wrong :laughing:.

And should I create a new table called "Unofficial extension message types", after the official type table?

Shouldn't be needed for now. In case more extensions are proposed we can change page structure as needed (which doesn't apply to protocol values, hence "namespacing" them in advance).

breeze2 commented 1 year ago

I marked the new message type as "Unofficial". Do them need to be distanced from official values? For example, starting from 0x110001 ?

Yeah, this sounds like a good idea 👍 . If further extensions are introduced, they can utilize 0x11XX00-0x11XXFF value ranges - 256 message types per extension is much more than enough and so is potential for 256 extensions. If we ever somehow hit that, something has gone very wrong 😆.

And should I create a new table called "Unofficial extension message types", after the official type table?

Shouldn't be needed for now. In case more extensions are proposed we can change page structure as needed (which doesn't apply to protocol values, hence "namespacing" them in advance).

Use 0x110001 and 0x110002 now.

breeze2 commented 1 year ago

You are welcome. Next I will go to Dolphin to implement it 😊.

iwubcode commented 1 year ago

Shouldn't the protocol version have been bumped for this change? I know these are new message types (so non-breaking) but still.

Also, calling it "Unofficial" is strange if it's a supported value..

v1993 commented 1 year ago

@iwubcode Protocol version negotiation is non-existent in real world implementations, thus bumping protocol version is pretty likely to accidentally break compatibility. Same goes for calling values "Unofficial" - DSU was implementation-driven since its very beginning and it does not seem likely to change, although, admittedly, "unofficial" may not be the best term here.

iwubcode commented 1 year ago

@v1993 - what do you mean by "protocol version negotiation"? Do you just mean most implementations aren't checking the version?

I don't think updating the version will break anything, unless the implementation is checking the version. In which case, they can add this to support these new messages.

The version number just makes things more explicit and allows everyone to reference things more easily if more messages get added in the future (or if the fields in a message changes).

Same goes for calling values "Unofficial" - DSU was implementation-driven since its very beginning and it does not seem likely to change, although, admittedly, "unofficial" may not be the best term here.

I would push for avoiding the word "Unofficial" as it puts this repo into questionable authority. I understand the background and that this was just a thrown together document to explain the outstanding protocol, rather than a place of record that reflects what apps/emus should be using. But still calling it "Unofficial" seems wrong to me. And I think a "version" would be a better reflection of added details.

iwubcode commented 1 year ago

Additionally, as an aside...

We've talked a number of times amongst the Dolphin team about missing features from the DSU protocol (such as button names). There's been discussion about introducing a new protocol. However, I personally question that. I think it would add additional complexity to emus (having to support both DSU and the new protocol) and also split the implementations for the actual apps that generate the data. If we can make things work with the version number, I feel like that'd cause less friction.

breeze2 commented 1 year ago

Additionally, as an aside...

We've talked a number of times amongst the Dolphin team about missing features from the DSU protocol (such as button names). There's been discussion about introducing a new protocol. However, I personally question that. I think it would add additional complexity to emus (having to support both DSU and the new protocol) and also split the implementations for the actual apps that generate the data. If we can make things work with the version number, I feel like that'd cause less friction.

I agree that supporting both cemuhook protocol and one new protocol increases the complexity of the emulator, so currently it seems better to extend the cemuhook protocol to get more features.

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 commented 1 year ago

I know it breaks compatibility for these new features. But I continue to say that is the correct thing to do. If you've ever looked at formal specifications, they won't just add new message types or new data out of a whim, they will actually have a formal document that is versioned, there will be a field in the specification that matches that version. That way users can base their application of that to know when messages are added or fields modified.

The fact that the existing software does not support new features is not a surprise and the software needs to be updated to support the version if they want those features. In particular, emulators (and DSU servers) should see this specification as an evolving document and the version number should dictate when new functionality is available.

Anyway, that is just my thoughts. I will leave it up to @v1993 to decide how to proceed. I will echo these thoughts on the Dolphin PR, someone else will have to decide what they think is appropriate.

v1993 commented 1 year ago

@iwubcode Considering development of a new protocol - what do you think about making a stand-alone program acting as a bridge between DSU servers and new protocol's clients? This would allow to make the transition smoother both for emulator and motion server developers. Alternatively, if latency turns out to be a problem in this case, it's possible to make a C++ library that supports both protocols, making life of emulator devs easier overall.

Ultimately, for now I'd rather abstain from changing version number, precisely to avoid breaking existing software. While version number change is actually known to break things, same can'b be said about servers receiving unknown message types. It that turns out to actually be an issue, I'll be far more willing to bump protocol version.

iwubcode commented 1 year ago

I don't agree but if that's your decision, then so be it. Appreciate you weighing in.

I personally think both options you suggested are needlessly complex and I don't really understand the reason. As mentioned before - what is a new protocol accomplishing? It seems you are afraid of breaking existing software but the solutions you have presented will be even more cumbersome for teams to pick up.

I'd suggest reaching out to some key emulator developers on various projects and get their opinions on your suggestion. I don't mind weighing in but understand my views are not the only one. I can't even speak for the entire Dolphin team. Billiard is our main input developer and has stronger views than me on DSU.

mbc07 commented 1 year ago

I think bumping the protocol version is fundamental, not doing so is a huge flaw and a recipe for disaster.

Yes, it would break non-updated apps, but this can (and should) be handled by the DSU Servers themselves (e.g. add a protocol version selector on them, and enable the new rumble reports accordingly -- that way they can work with non-updated apps if desired).

FWIW those new message types could break non-updated apps the same way they would break by bumping the protocol version, so just go with the latter before this spreads out to the existing DSU Servers and becomes unmanageable...

jordan-woyak commented 1 year ago

Upping the version number is objectively the correct thing to do. That's entirely the point of the version number. If poorly implemented software can't cope with version negotiation then version selectors can be added, as mentioned.

Anyways, we can make our own DSU protocol with blackjack and rumble. It can be documented elsewhere if management of this github is unwilling.

cobalt2727 commented 1 year ago

Considering development of a new protocol - what do you think about making a stand-alone program acting as a bridge between DSU servers and new protocol's clients?

I'm speaking from an end user point of view here - this sounds awful to work with. Aim for having less programs open at once to use a controller, not more. Bump the version number and existing software will update their implementations accordingly.