Rust fixes #18
Rust fixes #18
Conversation
Added test using SDL
bindings/rust/src/common.rs
Outdated
| } | ||
|
|
||
| impl DeviceDefinition { | ||
| pub fn new(name: &str, vendor_id: u16, product_id: u16, version: u16, phys: &str, uniq: &str) -> Self { |
There was a problem hiding this comment.
Public functions and types should have docstrings.
Also, you can use impl AsRef<str> and impl Into<u16> to give callers more flexibility. For example, that would allow you to use the evdev enums without converting to a raw u16 first.
bindings/rust/src/common.rs
Outdated
| pub fn new(name: &str, vendor_id: u16, product_id: u16, version: u16, phys: &str, uniq: &str) -> Self { | ||
| let name = CString::new(name).unwrap(); | ||
| let phys = CString::new(phys).unwrap(); | ||
| let uniq = CString::new(uniq).unwrap(); |
There was a problem hiding this comment.
Instead of unwrapping, you should return Result<Self, Something>.
bindings/rust/src/joypad_nintendo.rs
Outdated
| } | ||
|
|
||
| impl SwitchJoypad { | ||
| pub fn new(device: &DeviceDefinition) -> Result<Self, String> { |
There was a problem hiding this comment.
At a minimum, the error type should be pub struct InputtinoError(String). Better would be an enum using thiserror or similar.
bindings/rust/tests/joypads.rs
Outdated
| assert_eq!(sdl_js.name(), "Xbox One Controller"); | ||
| assert!(sdl_js.has_rumble()); | ||
|
|
||
| { |
There was a problem hiding this comment.
This is a perfect case for a helper macro (I suggest the name assert_events).
bindings/rust/tests/joypads.rs
Outdated
| } | ||
| sdl2::event::Event::JoyButtonDown { button_idx, .. } => { | ||
| assert_eq!(button_idx, sdl2::controller::Button::A as u8); | ||
| break; |
There was a problem hiding this comment.
This test passes on the following combinations:
ControllerButtonDown, JoyButtonDown
JoyButtonDown
ControllerButtonDown, ControllerButtonDown, ControllerButtonDown, JoyButtonDown
It seems like we expect both events, but in any order - is that right? If so this needs tweaking.
There was a problem hiding this comment.
It seems like we expect both events, but in any order - is that right?
That is correct, SDL has two APIs for joypads: SDL_Joystick is the legacy one whilst SDL_GameController is the newest which support more features.
The test here is trying to make sure that we only get ControllerButtonDown and/or JoyButtonDown for the right button that we expect to be pressed. Any other event (ex: ControllerAxisMotion) should fail the test.
There's no way to control the order of the events AFAIK, and it might even be that after some time the events "repeat" since the button state is "kept pressed" (not sure how SDL handles that) so I wouldn't introduce a hard requirement that we want one and only one event of each type.
Would you like to elaborate what kind of tweaking is needed?
There was a problem hiding this comment.
Is it really not deterministic for a given SDL version? At least in terms of the number of events?
I would suggest using gilrs or evdev to test for events coming out, if SDL is too weird/difficult to pin down. By pinning a rust library, you can be sure that your tests are deterministic, and it would also reduce test compile time by a bunch as a bonus.
There was a problem hiding this comment.
I've got unit tests with both and there are a few reasons why I really like SDL:
evdev(and by extension I thinkgilrssince it's only using that) can't access all the features that we expose in the DualSense pad like LED and battery that are instead properly handled and supported by SDL (you can only access them viahidrawor manually via/sys/devices/...)- There's a lot more logic involved than just reading the events via
evdevespecially for stuff like Gyro and Acceleration. You might think that you are sending the right value, but in fact the expected representation is different (ex:radvsdeg). Having a test like
joypad.set_motion(inputtino::PS5Joypad::ACCELERATION,
acceleration_data[0],
acceleration_data[1],
acceleration_data[2]);
REQUIRE(event.type == SDL_CONTROLLERSENSORUPDATE);
REQUIRE(event.csensor.sensor == SDL_SENSOR_ACCEL);
REQUIRE_THAT(event.csensor.data[0], WithinAbs(acceleration_data[0], 0.9f));
REQUIRE_THAT(event.csensor.data[1], WithinAbs(acceleration_data[1], 0.9f));
REQUIRE_THAT(event.csensor.data[2], WithinAbs(acceleration_data[2], 0.9f));it's extremely helpful because it's checking what the game will receive and interpret and not what we've sent "over the wire".
bindings/rust/src/joypad.rs
Outdated
| } | ||
|
|
||
| #[repr(u8)] | ||
| pub enum Joypad { |
There was a problem hiding this comment.
repr(u8) makes no sense here, and I'm surprised it even compiles.
I think a public trait would be more idiomatic than an enum.
There was a problem hiding this comment.
Ah haha it doesn't make sense indeed. It is a leftover from me trying to combine JoypadKind and Joypad in one, but ultimately decided not to.
I was considering a trait vs enum, and chose an enum currently. The reason is that you likely want to store a list of any type of joypad. With a trait you would need to store a Vec of Box with a bunch of traits. On top of that, you might want to do some specific logic if a joypad is a PS5 controller (like LED settings), for example. With an enum this becomes a simple match statement.
I'm curious what your thoughts are on this, as I'm not 100% sure my reasoning is correct :).
There was a problem hiding this comment.
Could you elaborate on the trait vs enum consideration?
There was a problem hiding this comment.
Sorry for the radio silence. I think it comes down to how much unique behavior each of the gamepad types has.
If each gamepad struct has unique functionality, but share some common behavior, that should be a trait. If all the gamepad types have the same methods, then I would say there should be a pub struct Gamepad with a vendor method or field, returning the Gamepad enum, rather than a separate struct XoneGamepad, etc. (From a quick skim, this seems to be the case.)
I wouldn't really export trait enum like this - it just feels unidiomatic. The stdlib very rarely uses enums for API-central receiver objects like this.
There was a problem hiding this comment.
I believe it's only the DualSense controller that is unlike the others. This could be a separate impl for DualSense.
I do like the trait implementation the more I think of it. At first I wasn't sure how to handle DualSense specific features if you are dealing with a Box<dyn Joypad>, but Joypad can implement something like:
fn as_dual_sense_mut(&mut self) -> Option<&mut DualSense> {
None // Default to `None` for non-DualSense gamepads
}There was a problem hiding this comment.
This doesn't work as I had hoped, because on_rumble_fn takes a on_rumble_fn: impl FnMut(i32, i32), which means the trait isn't eligible for dynamic dispatching. The rust compiler suggests to use an enum xD
inputtino/bindings/rust/inputtino/src/joypad.rs:12:8
|
12 | fn set_on_rumble(&mut self, on_rumble_fn: impl FnMut(i32, i32) + 'static);
| ^^^^^^^^^^^^^ the trait cannot be made into an object because method `set_on_rumble` has generic type parameters
= help: the following types implement the trait, consider defining an enum where each variant holds one of these types, implementing `Joypad` for this new enum and using it instead:
inputtino::PS5Joypad
inputtino::SwitchJoypad
inputtino::XboxOneJoypad
Any recommendations?
There was a problem hiding this comment.
Ugh, right.
I understand your point about matching the C++ API, but I still think a single Joypad struct is the best option. Future Joypads could also support LEDs, so I don't see a problem with having that method be a noop if the Joypad has none.
There was a problem hiding this comment.
In general, the callbacks are not very rusty. It would be better IMO to expose a pollable handle and not invert control. But that may be departing quite a bit from the C++ interface.
|
Thanks @colinmarc , your review is appreciated, you raised some good points 👍 . Are you considering using this in magic-mirror? |
Yes, I think I will need to provide a (maybe optional, maybe default) uinput backend for device emulation. The FUSE approach I've taken so far is just too edge-casey. |
Sweet :). I'm sure @ABeltramo likes it if more projects use this. |
|
Just a small commit: I've fixed the joypad SDL tests. I hope that doesn't conflict with any change you might have locally. On a different note, on my machine, I haven't looked into it, just wondering if it's my setup, or you can replicate it on your side. |
No problem, I wasn't looking at the unit tests yet. Hmm strange regarding the linking errors, considering I don't get that issue when using it in my project. I'm not super familiar with |
|
I just pushed some smaller changes. I also had a look at the tests, they got me puzzled 😄 . I'm not quite sure why they don't want to link properly.. I'll try to look into it some more later. |
bindings/rust/src/joypad_nintendo.rs
Outdated
| get_nodes(inputtino_joypad_switch_get_nodes, self.joypad) | ||
| } | ||
|
|
||
| pub fn set_pressed(&self, buttons: i32) { |
There was a problem hiding this comment.
The fact that each of these types has the exact same methods, but that is not enforced by the compiler, indicates that they should just be one type, imo - either a trait with many impls or just one Joypad type.
There was a problem hiding this comment.
I think a trait with many impls is closer to the C API. Without changing the C/C++ API too, I think that makes more sense.
|
@hgaiser sorry to bother you, is there anything else missing from this PR? |
|
No problem at all. Aside from some polishing (documentation and an error type, for instance), I think only the advanced dualsense features need to be tested (rumble, battery and LED). I briefly tested rumble but the callback was never called. I hope to continue in a week or two. If you want, I can focus on cleaning up basic usage in order to get this PR merged (without things like rumble)? |
|
There's absolutely no rush to merge this one in, let's polish it properly first. |
|
Ah yeah it would be great if you can have a look at the tests. They didn't seem to pass for me. I'm a bit fuzzy on the details but the error is: |
This PR does the following:
stablebranch.inputtino, instead ofinputtino_rs.Inputtinoprefixes in the Rust binding.Joypadenum that is similar toJoypadin the C++ API.impl Sendfor Rust binding objects.inputtino_joypad_ps5_set_ledtoinputtino_joypad_ps5_set_on_led(since it sets a callback, it doesn't set the LED like the name suggested). This is the only change that changes any of the other APIs.The tests don't pass currently, but this was already the case before I made any changes. I do still want to look into that.