Effect corrections part2 #107
Conversation
MmAaXx500
force-pushed
the
forcecorrect_p2
branch
from
2f2f7cb
to
58f4b63
Compare
- unified handling of all condition effects for uploads and updates - added new saturation parameters - added right, left, and right&left update type for saturation - added right&left update type for coefficient
MmAaXx500
force-pushed
the
forcecorrect_p2
branch
from
58f4b63
to
a78fbc2
Compare
|
Condition effects required more changes than I expected, there are 4 new fields and 4 new messages that I added to the documentation and implemented in the driver. Summary of what Windows and what this PR uses:
Nothing outstanding come in my mind about the changes, if something is not clear I'll clarify it. Spreadsheet: ffb.zip (had to be compressed to fit) |
|
thanks for your work, this looks great. |
On the charts, the X axis (horizontal) shows the OS specific API input value and the values on the Y axis (vertical) shows the value that was sent to the wheel. The X and Y values should match between Windows and Wine if the driver is working correctly, while the Linux values should match only on the Y axis exactly and be proportional on the X axis.
Yes, the corr (corrected) columns and lines are generated using this PR applied.
I'm not aware of any missing values, I think you just need to scroll down a fair bit to see them. There are about 65000 rows on each sheet. |
|
Thanks for explanation. |
|
Kind of interesting that the FEdit program I used to do most of the reverse engineering never even touched the saturation. I had a cursory glance at code, looks pretty good to me. I'll play around with it over the weekend, test out some games and have a closer look at the details before merging. In any case, really nice work, well done. |
I had tried FEdit, but I was unable to get it to work reliably under my Win 10 vm, so I have no experience with it. Maybe they forgot to implement it. The saturation fields of the I think I found something (apart from a missing title). There are 33 possible update packets. The type of the update is coded in the 4th and 5th bytes as bit fields. Do you want to have them all implemented? For me, it seems to be little benefit compared to the implementation complexity if you are not aware of any issue that caused by that these are not implemented. This PR in the current state already reduces the worst case of 5 update packets to 3 ( Here is the table as context (same in the docs) The original reason why I started to write this comment, to document here that these type of packets can be discovered by updating multiple values with one call of |
|
Came up with a simpler implementation than I thought before. It can be made even simpler if we only use the |
Would it be easier to just dump all parameters in a single message if one/multiple of them has changed? I don't necessarily think we have any massive bandwidth limitations, but sending fewer packets with more data is probably preferable over sending multiple smaller packets, and minimizing branches tends to make code easier to read. I don't think the wheel itself has significant issues dealing with a bit of 'unnecessary' data in the packets, but that should of course be tested. When I started this project, before I had done any proper USB reverse engineering, I was sure that update packets would just dump the new effect state as a whole because that just seemed like the sensible thing to do, and was sorely disappointed when the tools I was using seemed to just send a single parameter per packet. Looking at the data with new eyes, it seems like at least the periodic effect also uses a bitfield, and might also benefit from merging updates. The order in which fields are populated is a bit weird, I would've expected the packet to have fixed slots for each parameter, but apparently they act as a kind of priority stack? |
That's a good idea! I have just checked if this has any side effects like stutter or something on the not changed parameters, but I don't feel any, so it's probably safe to do that.
Yes, there is! It's
Yes, the order is fixed, the location in the packet is not. Probably easier for them to parse that way. |
MmAaXx500
force-pushed
the
forcecorrect_p2
branch
from
75f7dce
to
d9f3654
Compare
|
I saw some packets that merged duration update with the other parameters, I will check it tomorrow. |
|
I'm currently wondering about why does the duration updated with every update request, even it is unchanged. I saw 0e47624 that led me to #43. There is a comment from berarma that states:
I don't know what is expected under Linux or how other wheels behave, but SDL does not do any special calculations neither on Windows [1] nor Linux [2] while handling uploads [3,4] or updates [5,6]. The documentation for Direct Input states:
The wheel's behavior matches the Direct Input documentation: when a 10-second effect is uploaded and then updated to a 2-second effect after 5 seconds, it stops immediately instead of playing for an additional 2 seconds. What is your opinion about this? Can we include the duration update in the new [1]: windows conversion: https://github.com/libsdl-org/SDL/blob/main/src/haptic/windows/SDL_dinputhaptic.c#L656 |
Heh, now I'm a bit embarrassed. I thought that re-sending the duration would reset the length counter, but I just tried it out again and you're right, the effect stops once the internal length counter reaches the specified duration. I thought I tested it, but I must've done a really poor job of it. But the idea was to extend the effect duration for each update.
If Logitech wheels extend the duration, I suppose we should too. Just call it a de-facto standard ;) I'm still a bit annoyed how poorly documented this aspect of the Linux FFB subsystem is, I could easily see both approaches being the more 'intuitive' option. In addition to the SDL sources you linked, Wine doesn't seem to do any adjustments to the duration, possibly indicating that Wine assumes that the DInput duration handling would also apply to Linux: https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/dinput/joystick_hid.c?ref_type=heads#L2482 (kind of hard to link to code that doesn't exists, but you know)
Scratch that, I got them reversed, It might not even be that significant, I can't remember seeing any games that would require one or the other, generally most games just set effects to be of infinite length and just send a STOP signal when they should end. |
|
I have taken a look at some projects to see what they are doing. To make the things more interesting, the in kernel (I hope I read them correctly) Out of curiosity, do you know any driver that does the ff-memless way of the duration update and does not depend on ff-memless?
I feel it :) I'm in favor of keeping the current (Windows) behavior that maybe incorrect, but it does not break SDL, Wine, and probably more programs that expect the Windows behavior. Whatever is the decision is going to be, I want to push a commit to use the |
Logitech wheels are the only ones that I'm sure of.
I'm fine with that. @berarma Sorry to ping you this late into a thread but TL;DR: It seems like most projects assume DirectInput semantics w.r.t. effect duration updating, meaning that an effect update doesn't reset the length counter. Any thought on this? |
Don't worry. This is an interesting subject that we should talk about now that there's an increasing number of people collaborating on wheel drivers. I've already had a similar talk where someone patching a driver wants to fix any Wine/Proton/SDL/game bug in the driver, because since they're already modifying the driver, it's easier than having to create a fix for Wine/Proton/SDL. The problem with this is that we're fragmenting the Linux FFB API and we won't ever have a stable API if we work this way. And the drivers will become increasingly complex, having to deal with incompatible one-game fixes which might break other games. Trying to follow the Windows API in the Linux drivers has the same problems, I think there has to be a clear separation, and Wine/Proton is the compatibility layer. Any issue with Windows API to Linux API translations should be addressed in those projects. Since the Linux FFB API documentation is scarce, we must rely on the source code and previous drivers implementations. The most important one being ff-memless because it's probably the most used in practice, and I think the only one in use for wheels until recently. Native games and even Wine/Proton have been tested against it. In addition, I think the semantics of the Linux API are clear. There's no update operation for effects, only upload and play. When we upload an effect while it's playing, we're not only updating some parameters, we're uploading a new effect that will replace the one that is playing without having to stop and play again. In this regard, I think I didn't follow correctly these semantics in my new-lg4ff driver because they weren't clear to me at the time, but the effect length is updated. If we don't think about this carefully, we might end up with incompatible and over-complicated drivers. Applications will need to know which driver is in use to send the correct commands, breaking the separation of concerns of what a driver should do (handle the hardware) and what a user space application can expect (devices following the same conventions independently of the driver in use). |
|
I have searched the lkml in hope of some information. The earliest mention of
An interesting find is the proposed
however, the effect update is still using the There was a discussion on 2014-02-15 with the subject of "Questions about the documentation/specification of Linux ForceFeedback input.h" (lkml) Unfortunately the duration update was not brought up, only the infinite duration. Through the whole discussion, the DirectInput is used as a reference point. |
I'd take a pragmatic approach of breaking the least software, or even not breaking any if possible. I wouldn't mind much who did what first if it wasn't ever significantly used. ff-memless is used by many FFB devices and changing its API would break them. In this case, since the drivers which aren't following standard practice where hardly used (they weren't fully working even), like hid-pidff, I think we can easily avoid breaking what was already working. With new drivers like hid-tmff2 is even easier to avoid breakage. ff-memless-next is out of the question since it wasn't merged and I don't think it will for the same reasons I pointed above. I feel like we're discussing if we should all go our own separate ways changing the API as we please. I hope this is finally not the case because it would certainly make our lives harder, and merging into the kernel would be a no-no. |
|
From the LKML thread at https://lore.kernel.org/all/[email protected]/
Vague language of course, but it kind of seems like the Linux FFB is intended to match DInput, and a programming bug has just spread to become the most common behaviour (due to how many devices use I'm not opposed to making this driver behave as |
He was talking about the effect model, which is pretty close already, while this discussion was about semantics, or so I understand. I just want everyone to understand the important implications of going incompatible with other drivers. If this change isn't important, then we're just discussing this because we should be compatible with the Windows API. What's the reason when there's already a compatibility layer that takes care of that? MS might change or deprecate this interface for a new one any day. Should we be following their mood? I don't think making our drivers dependant on what MS does with Windows is good. While it might have been a good starting point and an inspiration, following what they do shouldn't be our end goal. |
I thought a model defines the semantics, but I guess we must have different definitions?
Sorry, must've expressed myself poorly. I don't mean that we should automatically follow what Windows does, the confusion here (on my part at least) lies in the fact that Wine and SDL, which I imagine together account for the vast majority of all FFB use, seem to assume that the duration parameter works the same on Linux and Windows. At least to me it's not obvious if the issue is drivers implemeting some (poorly defined) API incorrectly or users using it wrong but I'm fine with just picking one and documenting the choice clearly so other people don't have to worry avout this particular problem ever again, and right now I'm leaning towards extending durations with updates. Does that seem reasonable? |
|
For me, the effect model is the structs, the data. I think this is common terminology. Extending durations seems reasonable because we aren't breaking compatibility with existing drivers. I hope I'm making it clear it's not just my preference. If Wine isn't translating correctly or SDL isn't working properly, it will already be an issue for existing drivers that will have to be fixed at some point. If there never was a case for this use, maybe they are unaware of the issue. But it's no less important to keep consistency between drivers. |
|
Just to make sure everyone is on the same page. I may have described the situation inaccurately earlier, sorry for that. If we are going the |
What do you mean? All ff-memless devices are doing it this way, this is different to "no one wants to do it that way". Projects like Wine, Proton or SDL will do whatever works with the kernel drivers, and right now the only drivers in the kernel which are actually in use are ff-memless. If there are others it would be interesting to know. If this case isn't currently addressed correctly, the most logic reason could be that it rarely happens, making it even less convenient to make breaking changes in the kernel. As previously said, the most common case is setting the length infinite in the initial and later effect uploads. You could start a discussion involving Wine, Proton, SDL and kernel developers to change how ff-memless works, but I guess it'll be much more difficult than fixing Wine/Proton/SDL to work correctly with the current state. Wheel/FFB discussions usually get stalled because there's little interest, specially when they involve changing kernel drivers. Also, to start such a discussion there should be a rationale to break backwards compatibility for many FFB devices. Maybe I've missed some message but I think I haven't read the reasons yet. |
Probably good to bring those up as well, sure. Seems like I don't think I have any The FFB docs say 'Normally, the effect is not stopped and restarted' which I guess should be read as 'Normally, the effect is not stupped, and is restarted' assuming Alternatively, the quote could've been paraphrased from DInput, which states
This makes more sense to me, restarting the effect every time it is updated is a bit silly if the wheel itself supports partial updates. |
There's at least the Logitech HID++, which seems to just dump the effect buffer as a whole to the wheel during an update: I don't know how the wheel's firmware itself reacts to the delays, but could be cool to find someone with a G923 and ask. |
I mean that even if one of Wine, Proton, or SDL is opposed to adapt the *: The only project I found that clearly aware of
There is
In case of this driver ( |
I don't own the wheel but found the documentation directly from Logitech: https://opensource.logitech.com/wiki/force_feedback/Logitech_Force_Feedback_Protocol_V1.6.pdf In the
In |
|
So the reason is that it's easier to just let the firmware do whatever it does. Thus you'd really want the API to be open about it and not force any particular behavior. It just happens that this wheel behaves more closely to DInput for this case. If I didn't convince you then go ahead and take the easy path. I don't think an excuse is needed. It won't have consequences in the short term as far as we know. Given the state of neglect of this area of the kernel, I guess it will become a mess anyway, |
I didn't mean that, I only said how it is going to look like in
Well, that's how the first FFB driver born for I-Force wheels way before ff-memless, this is the first driver that introduced the term "update". I-Force has separate slots for the "core" effect and the "modifiers", changing the "core" requires a restart but the "modifiers" can be changed on the fly. I can't be sure about the exact behavior, but why would there be slots if they are restarting the whole effect? I think it works DInput-like. Also, it was not changed to ff-memless like other devices when ff-memless was introduced, and it's almost untouched since then over here: https://elixir.bootlin.com/linux/v6.9.8/source/drivers/input/joystick/iforce After the I-Force got its update capability, the "Dynamic update of an effect" section changed from
to
Some observations of mine: The difference between ff-memless and others are only visible when an effect is played with an envelope, delay or when the duration of the effect is important. Otherwise, the difference is invisible, I think. Most of the devices that are depends on ff-memless are gamepads with some rumble motors, and low-level chips that are lacking the concept of an "update". Gamepads are incapable of playing force feedback effect with precision because they are not designed for that, hence harder to notice something that is already rare. You asked previously:
It depends on how you define "actually in use" but here is what I found. I only include wheels that are probably capable of more than rumble. Depends on ff-memless
Works like ff-memless
Likely DInput-like
DInput-like
Based on these, I think we can't speak about some defined behavior on the kernel side either, so if we want something that's working the same for every device, then a change in the kernel is unavoidable. At this point, I see 3 possible decision for this discussion. We are not under pressure to do something, so I think all the 3 are equally valid. Leaving everything as-is
Adapt to ff-memless
Adapt to DInput
In both cases, when something is changed, closed-source applications should be considered for their current behavior because they cannot be changed easily. We can think that they are going to adapt, but when? Judging from a previous attempt to change one line of |
|
Right, I think we've let this stew for long enough. I at least haven't come up with anything and agree that probably the best course of action for now is to let things be the way they are. My opinion's been kind of flip-flopping over the course of this discussion, but I believe we uncovered some interesting aspects that I've never considered before and I'm happy it was brought up. It can always be resurrected if someone finds a real usecase that is affected. @MmAaXx500 Thanks for a sober and really well put together overview of the topic, again great work. Do you think you have everything you need to get this PR over the finish line? |
MmAaXx500
force-pushed
the
forcecorrect_p2
branch
from
6e0d76d
to
f595ade
Compare
Pushed the last commits to use the |
Alright, good to know. You mentioned previously
which I thought meant that there were still some changes coming. I previously ran some games and everything seemed to work fine, so added some final stylistic nitpicks, if you could sort them out I'd be happy. They're all really very minor and I could go in and fix them later myself, but that wouldn't be as hygienic so I'd prefer it if they were attended to before I merge. |
There was a commit below that comment, but I force pushed since then, and it is not visible there anymore (it's probably a bad practice on my side, sorry about that). It was the 6e0d76d and replaced by f595ade in the today's push. You can see there what I changed today: https://github.com/Kimplul/hid-tmff2/compare/6e0d76d71d21864a0b138b9c29e81a8c2a779e6d..f595adeeca291e37260ea731fbfaa90be376deb8
I don't see any. Did you submit them? |
src/tmt300rs/hid-tmt300rs.c
Outdated
| if(sat == 0) | ||
| return max; | ||
|
|
||
| return sat * max / 0xFFFF; |
There was a problem hiding this comment.
There's an extra space here as well, and the hex constant should be with small letters to fit into the existing code a bit better.
src/tmt300rs/hid-tmt300rs.c
Outdated
| { | ||
| /* max deadband value is 0x7FFF in either direction */ | ||
| /* deadband is the width of the deadzone, one direction is half of it */ | ||
| *out_rband = clamp(offset + (deadband / 2), -0x7FFF, 0x7FFF); |
There was a problem hiding this comment.
Hex letters and I probably should've mentioned earlier but line length should be ~80 characters, maybe split the signature between out_lband and deadband.
|
|
||
| memcpy(&packet_damper->damper_start, damper_values, ARRAY_SIZE(damper_values)); | ||
| t300rs_fill_timing(&packet_damper->timing, duration, offset); | ||
| t300rs_fill_timing(&packet_condition->timing, duration, offset); | ||
|
|
||
| ret = t300rs_send_int(t300rs); | ||
| if (ret) |
There was a problem hiding this comment.
"failed uploading spring" should probably be changed to "failed uploading condition"
(GitHub won't let me comment on unchanged code apparently, couple lines below here)
src/tmt300rs/hid-tmt300rs.c
Outdated
| { | ||
| if(effect_type == FF_SPRING) | ||
| return 0x6aa6; | ||
|
|
There was a problem hiding this comment.
There are extra spaces on this line. Empty lines should generally be truly empty, not have invisible characters. Some editors leave extra spaces around for whatever reason.
src/tmt300rs/hid-tmt300rs.c
Outdated
| input_level = 100; | ||
| break; | ||
| } | ||
|
|
src/tmt300rs/hid-tmt300rs.c
Outdated
|
|
||
| } | ||
| t300rs_calculate_deadband(&right_deadband, &left_deadband, cond.deadband, cond.center); | ||
| t300rs_calculate_deadband(&right_deadband_old, &left_deadband_old, cond_old.deadband, cond_old.center); |
There was a problem hiding this comment.
These two lines are a bit on the long side, suggest splitting arguments 2+2.
src/tmt300rs/hid-tmt300rs.c
Outdated
|
|
||
| t300rs_fill_header(&packet_damper->header, effect.id, 0x64); | ||
| memcpy(&packet_condition->hardcoded, condition_values, ARRAY_SIZE(condition_values)); |
There was a problem hiding this comment.
Just a bit too long here as well, suggest putting ARRAY_SIZE(...) on new line.
src/tmt300rs/hid-tmt300rs.c
Outdated
| uint8_t damper_start[17]; | ||
| struct t300rs_packet_timing timing; | ||
| } *packet_damper = (struct t300rs_packet_damper *)t300rs->send_buffer; | ||
| t300rs_calculate_deadband(&right_deadband, &left_deadband, cond.deadband, cond.center); |
There was a problem hiding this comment.
Suggest splitting arguments 2+2.
Apparently not, thanks for pointing it out. I thought I was just leaving comments but I must've accidentally started a review at some point, should be visible now. |
|
Addressed all the requested changes. |
|
Great, thank you! |
Part1: #105
Updated the documentation that I forgot in part1, all the indicated values are sent by the Windows driver.
Same for part2.