hid: tmff2: add Thrustmaster T500RS wheel base driver#186
hid: tmff2: add Thrustmaster T500RS wheel base driver#186cazzoo wants to merge 14 commits intoKimplul:masterfrom
Conversation
2ae18a3 to
bd43bb9
Compare
|
Hi @Kimplul , Here is a new version of the driver, fully rebuilt from scratch. I've been trying to do my best to get it done the right way, steering its content the best I could (with my knowledge). I'll discard the previous PR that is not optimal. This one should be supporting more effects, and should be better designed. I've been also working on documentation to make it more comprehensive and based on SDL2 real examples. Again, I am fully open to your feedback (and also @MmAaXx500 ones) that were really valuable by the other PR, so when you will have time, I'd really happy to get to your review. Thanks |
Kimplul
left a comment
There was a problem hiding this comment.
Thanks for continuing on this project, I was getting worried that we scared you off :)
I did a quick overview. The documentation in particular looks a lot better, having clear examples of what each packet does and how they build up to a full effect upload helped greatly. I haven't done any cross-referencing between the docs and implementation, as I still found quite a lot of smaller issues I'd like taken care of first. I'll do a more thorough review after that.
Not entirely sure what you mean by 'from scratch', for instance the comment explaining the set_gain rationale is identical between this and the previous PR.
|
Got the code and the PR updated based on all your feedback. All were valid, the only ones I haven't worked on are the SQUARE effect I plan to work on later and the git version (that I can easily remove). |
Add support for the Thrustmaster T500RS wheel base in the hid-tmff2 driver. This driver has been built from scratch based the previous dirty driver (see Kimplul#175) on captures from ffbsdl tool ran in windows for all possible effects (through SDL2 library).
- Simplified effect slot system - Added support of direction - Tidy-up code here and there - Reduced complexity overhead - Code standardization (in regards of the base repo) - Documentation updates
Add support for FF_SQUARE periodic waveform based on FFEdit USB captures
d258e05 to
9e6319b
Compare
Corrected packet structure and scaling formulas Added ramp support
9e6319b to
b7d188d
Compare
|
Hi @Kimplul , I've been working again on the PR, responding to all your points (almost all -- if not all -- were relevant), and introducing some missing features (FF_SQUARE missing and 0x05 conditional effect that was incorrectly done). If you mind having another pass, I'd be really happy. |
|
Sorry about the delay, bit busy before the end of the year. I had a look through the resolved comments, most looked good but there were a few that I decided to unresolve, please have a second look at them. I still haven't cross-referenced documentation to the code, I'll try and get that done by the end of next week, but based on a cursory look the code looks a lot better now. Seems you managed to shave off ~400 lines of code, well done. Please also add a copyright statement to the start of the files you created, something like https://elixir.bootlin.com/linux/v6.18.1/source/drivers/hid/hid-retrode.c for example. I really should do that as well to the bits I wrote, but this is the first 'major' new addition to the driver so I've never really had any reason to think about it before. |
|
No worries, thank you for the feedback you provided. Take the necessary time for whatever need review. I am not in the hurry, and this driver does work for me already so It can last as long as necessary from your end. I'm happy we trend to get somewhat a stable mergeable version. Cheers |
408f5bf to
1c5af2c
Compare
That's the responsibility of |
It is indeed not working with the current code. I'll revert this. Not sure how I should update the init driver yet. Maybe just specifying the boot mode, will test. EDIT: I just open a PR against hid-tminit for TSCP branch (even though it's t500rs code update): Kimplul/hid-tminit#2 I tested and it appears to work fine with that code and my driver (and the mode switch reverted from within the driver) |
Yeah, I intended to merge |
|
There we are @Kimplul , I've been addressing all the points I guess, and introduced some fix also here or there (you can check latest commits, small scoped). Take your time for the documentation cross-check and let me know, in parallel I'm trying to improve the bits where I can |
+ Removal of trailing whitespace, unnecessary unicode chars, small formatting changes (though a larger kernel style check should probably be performed)
Kimplul
left a comment
There was a problem hiding this comment.
Sorry about the delay, I've been busier than expected. I mainly focused on the docs and found some inaccuracies and points of improvement, please have a look at those.
I applied some style fixes myself, please cherry-pick them from https://github.com/Kimplul/hid-tmff2/tree/cazzoo-hardening/t500rs-driver. I don't have push access to your branch and opening a PR for a PR seemed a bit silly, but the changes were trivial enough that I didn't really want to request them explicitly. Kind of a clumsy part of the PR flow, but eh.
Regarding style, I've tried to follow the Linux Kernel style guide in this repo. Most significantly, it dictates a tab width of 8 spaces, whereas this has a width of 2, please fix that. You might also want to try running your code through clang-format with the kernel's style configuration, see https://www.kernel.org/doc/html/next/dev-tools/clang-format.html
- Unicode char replacement - Documented duration/delays with packet upload - Code tidy-up (parameters renaming) - Set structs for init sequence - Invalid conditional effect types now cause an error instead of silently defaulting - Ported check for modified parameters to all effects - Fix saturation handling in T500RS driver & update documentation to reflect it - Update documentation to clarify dynamic packet code values - Update examples to use hardware ID 1 instead of 0 - Moved Subtype system section to the bottom - Hardened documentation style - Provide details for "DC bias" terminology - Do not discard 100% gain set but kept warning, more explicit.
1bf936c to
5c31c0e
Compare
|
Thank you @Kimplul for all the feedback. No worries for the delay, I also had a lot of parallel work to take care of in the meantime. Now I believe the code and much more ready, and the documentation updated appropriately and more accurately. Please have another pass and I'd be happy to change anything that's not fine by your POV. |
9c50dc4 to
5015d21
Compare
5015d21 to
3e8c027
Compare
Kimplul
left a comment
There was a problem hiding this comment.
At least the documentation still needs a bit of work. I would suggest maybe to read through everything a couple of times from a perspective of someone who doesn't know anything about the wheel, reading from top to bottom to get a good overall picture of what the wheel does and how. Present things in the order that they're needed, lots of examples, try to maintain a consistent way to present information, stuff like that.
docs/T500RS_FFBEFFECTS.md
Outdated
| 41 00 41 01 - START command | ||
| ``` | ||
| - **Packet Type:** 0x41 (Command) | ||
| - **Effect ID:** 0x00 (always 0x00 for T500RS) |
There was a problem hiding this comment.
I believe we've already established that the effect ID varies depending on what effects are loaded, no?
docs/T500RS_FFBEFFECTS.md
Outdated
| 41 00 00 01 - STOP command | ||
| ``` | ||
| - **Packet Type:** 0x41 (Command) | ||
| - **Effect ID:** 0x00 (always 0x00 for T500RS) |
docs/T500RS_FFBEFFECTS.md
Outdated
|
|
||
| **Important Notes:** | ||
| - **Envelope Support:** Envelope packets (0x02) have limited support. Non-zero envelope values cause EPROTO errors on periodic and constant effects. Always send zeros for envelope parameters on these effect types. | ||
| - **Runtime Updates:** Effect updates (via `update_effect` callback) only modify parameter-specific packets (0x03, 0x04, 0x05). Duration and delay changes require re-uploading the entire effect. |
There was a problem hiding this comment.
Please clarify that this is only a limitation of this driver, and not of the FFB subsystem in general. Just in case someone reference this documentation when trying to figure out how FFB works.
docs/T500RS_FFBEFFECTS.md
Outdated
| 4 | 2 | duration_ms | Duration in milliseconds, little-endian | ||
| 6 | 2 | delay_ms | Delay before start, little-endian | ||
| 8 | 1 | reserved1 | 0x00 | ||
| 9 | 2 | packet_code_1 | Code for subsequent packet type (variable!) |
There was a problem hiding this comment.
packet_code_1 and packet_code_2 are the same as param_sub and envelope_sub, are they not? I think param_sub and envelope_sub are more descriptive terms, the code uses them as well.
| | 0x40 | Spring | Windows driver captures | | ||
| | 0x41 | Damper/Friction/Inertia | Windows driver + FFEdit captures | | ||
|
|
||
| **Note:** Square wave (0x20) was discovered in FFEdit captures. The Windows driver may not expose this effect type through the standard API. |
There was a problem hiding this comment.
The windows driver most definitely exposes the square wave. FFEdit uses the Windows API, so I'm not entirely sure what this is referencing anyway?
docs/T500RS_FFBEFFECTS.md
Outdated
|
|
||
| **Parameter Details:** | ||
| - Magnitude: 0-127 (scaled from Linux 0-32767) | ||
| - Offset: s8 (-128 to +127, DC bias (Direct Current bias - a constant force offset), scaled from Linux -32768 to +32767) |
There was a problem hiding this comment.
Direct Current bias - a constant force offset I mean I know what you're trying to say, but you're still not saying very well and I'm not sure a first-time reader would really get what you're trying to say.
A graphical example would probably be pretty good here, maybe you could try for some ASCII art? At the very least, I would like to see it explained out in full, something like A constant offset means that the periodic effect is stronger when twisting the wheel to one side and weaker to the other. Feel free to come with your own suggestions, I'm not much of a wordsmith :)
docs/T500RS_FFBEFFECTS.md
Outdated
| - Magnitude: 0-127 (scaled from Linux 0-32767) | ||
| - Offset: s8 (-128 to +127, DC bias (Direct Current bias - a constant force offset), scaled from Linux -32768 to +32767) | ||
| - Phase: 0-255 (256 steps for 360 degrees, scaled from Linux 0-35999) | ||
| - Period: Direct milliseconds (no Hz conversion!) |
| - effect_type = 0x21 (triangle wave) | ||
| - Same envelope and periodic packet structure as sine wave | ||
|
|
||
| **Note:** Waveform type determined by effect_type in main packet, not in periodic packet parameters. |
There was a problem hiding this comment.
Would it be correct to say that each waveform type is its own effect_type?
| | 6 | 0x00b6 | 0x00c4 | | ||
|
|
||
| ### Driver Implementation Notes | ||
| - **Effect ID Handling:** The driver uses hardware IDs 1-15 to avoid quirky behavior with hardware index 0 (only valid for constant effects). |
There was a problem hiding this comment.
This is already mentioned above. Sometimes repetition can be useful, but there's only like 15 lines between each occurence so one or the other should probably be removed.
docs/T500RS_FFBEFFECTS.md
Outdated
| - **Device Format:** 16-bit little-endian (0-35999 in 0.01 degree units) | ||
| - **Conversion:** `device_dir = (os_ffb_dir * 36000) / 65536` | ||
| - **Examples:** | ||
| - 0degrees = 0x0000 |
There was a problem hiding this comment.
I believe it's customary to put a space between the number and degrees, presumably because degree is a whole word. Short forms, such as ms for millisecond, can be written without a space between.
|
I have been following this development with some excitement and just tried to build and run the current PR. I do not know how to properly mention files in a comment, but in the hid-tmt500rs-usb.c file there is a capital i after a semicolon. |
|
Good catch! Thank you very much for the report |
|
@SeitzB I think you have the wrong branch checked out, |
Kimplul
left a comment
There was a problem hiding this comment.
I added some comments about the most recent stylistic changes. Looks like clang-format did something weird with the whitespacing in multi-line comments, or maybe they were weird to begin with and clang-format just choked. Dunno.
src/tmt500rs/hid-tmt500rs.c
Outdated
| u8 phase, u16 period_ms) | ||
| { | ||
| /* Byte order per Windows USB captures (example: 04 2a 00 06 00 3f 0a 00): | ||
| * b0=T500RS_PKT_PERIODIC, b1=code, b2=reserved1, b3=mag, b4=offset, |
There was a problem hiding this comment.
Quick nitpick: Something funky with the comments, seems to use mixed spaces and tabs?
| /* Wheel handles positive magnitudes only */ | ||
| projected = -projected; | ||
|
|
||
| /* Add 180 degrees to phase to maintain correct force direction. |
src/tmt500rs/hid-tmt500rs.c
Outdated
| offset = (s8)((end_level - start_level) / 512); | ||
|
|
||
| /* | ||
| * Phase encodes ramp direction per FFEdit captures: |
| */ | ||
| phase = (start_level < end_level) ? 0x7f : 0x00; | ||
|
|
||
| /* Byte order per USB captures: b0=id, b1=code, b2=reserved1, b3=mag, |
| p->id = 0x02; | ||
| p->subtype = subtype; | ||
|
|
||
| /* |
|
|
||
| T500RS_DBG(t500rs, "Sending initialization sequence...\n"); | ||
|
|
||
| /* Report 0x42 - Init/status commands (2 bytes each) |
| hid_warn(t500rs->hdev, "Init command 0x42 0x00 failed: %d\n", | ||
| ret); | ||
|
|
||
| /* Report 0x40 - Enable FFB (4 bytes) |
| hid_warn(t500rs->hdev, | ||
| "Init command 3 (0x40 config) failed: %d\n", ret); | ||
|
|
||
| /* Report 0x43 - Set global gain (2 bytes) |
src/tmt500rs/hid-tmt500rs.c
Outdated
| u8 right_sat = (cond->right_saturation * 100) / 65535; | ||
| u8 left_sat = (cond->left_saturation * 100) / 65535; | ||
|
|
||
| struct t500rs_pkt_r05_condition *p = |
There was a problem hiding this comment.
Not required to get merged, but the high level of indent here makes the code a bit annoying to read, some kind of refactoring could be useful here, such as instead of t500rs_build_r05_condition and t500rs_send_hid you could instead just have t500rs_send_condition.
| break; | ||
| } | ||
|
|
||
| case T500RS_SEQ_CONDITION_Y: { |
There was a problem hiding this comment.
Does anyone use the second axis? The T300 only uses effect->u.condition[0], and I don't remember seeing kernel drivers use condition[1].
|
Commit c6d27c3 says:
That's not true. Most of the issues I pointed out still remain. Did you push the commit by accident or what's the deal here? |
|
That was indeed not intended to be pushed straight away, I'm still having a few time to work on it and probably pressed the wrong button. Sorry for the inconvenience |
c106fae to
cb6305e
Compare
…d examples Major changes: - Add QUICK START section for immediate practical learning - Add Understanding Effects section with real-world analogies - Reorganize documentation: practice-first, reference-later - Standardize terminology: param_sub/env_sub instead of packet_code - Fix degrees spacing and improve parameter descriptions - Add consistent example format with packet-by-packet breakdowns - Fix code comment formatting to kernel style - Remove redundant & 0xff casts when casting to u8 The cast to u8 already truncates to 8 bits, making & 0xff redundant - Fix SDL references to Linux FFB subsystem in header file comments - Verify no unicode characters in source files - Remove unnecessary braces - Added helper functions for build&send packets
cb6305e to
ab8801b
Compare
This driver has been built from scratch based the previous dirty driver (see #175) and on captures from ffbsdl tool ran in windows for all possible effects (through SDL2 library).