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

Expose Joystick power level #759

Merged
merged 2 commits into from
Mar 31, 2018
Merged

Conversation

ticky
Copy link
Contributor

@ticky ticky commented Mar 7, 2018

This PR adds a power_level method to the Joystick type. The method’s signature looks like this:

pub fn power_level(&self) -> Result<PowerLevel, IntegerOrSdlError>

At this stage, due to limitations in SDL2 itself, this method only returns a useful value for XInput devices on Windows, but hopefully SDL2 will improve that in the future! On unsupported platforms, this will currently consistently return Ok wrapping sdl2::joystick::PowerLevel::Unknown with no error.

On a supported platform, it will return Ok wrapping one of sdl2::joystick::PowerLevel::{Unknown, Empty, Low, Medium, Full, Wired}, depending on the type and status of the controller. These values mirror those in the underlying SDL2 enum, discarding the “MAX” value which is not used (this appeared to be the norm on other Rust versions of SDL enums)

It should not be possible for this method to return an IntegerOrSdlError, as the only case in which the underlying SDL2 method should emit an error is if it is called on a joystick object which is not yet open (the check governing this is that the object passed through to the underlying SDL_PrivateJoystickValid function is NULL), which should be impossible using the Rust API, however, for consistency with other methods like this, I have implemented it this way. If it would be preferred to discard the error states altogether given they should be impossible using the safe API, I’d be up for making the change.

I’ve also added a power level reading to the joystick example. It prints once when a valid joystick has been found.

Of note is that this method is suitable for use on GameController objects via SDL_GameControllerGetJoystick, but given the rest of the Joystick API is not mirrored in GameController I have held off on doing something so rash until discussion in #758 :)

Looking forward to your feedback! :)

#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
#[repr(i32)]
pub enum PowerLevel {
Unknown = sys::SDL_JoystickPowerLevel::SDL_JOYSTICK_POWER_UNKNOWN as i32,
Copy link
Member

Choose a reason for hiding this comment

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

Why do you have Unknown here since it is never returned in the SDL2 wrapper? We would be matching against Unknown from the return of powel_level(), but it's not possible for power_level to return Unknown given your impl. I think we should just remove Unknown from the SDL2 enum.

Copy link
Contributor Author

@ticky ticky Mar 10, 2018

Choose a reason for hiding this comment

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

Unknown is actually returned plenty of the time on several platforms!

As I said in the original post:

At this stage, due to limitations in SDL2 itself, this method only returns a useful value for XInput devices on Windows, but hopefully SDL2 will improve that in the future! On unsupported platforms, this will currently consistently return Ok wrapping sdl2::joystick::PowerLevel::Unknown with no error.

Copy link
Member

Choose a reason for hiding this comment

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

That I agree with, but you have this line in the Rust code: if result != sys::SDL_JoystickPowerLevel::SDL_JOYSTICK_POWER_UNKNOWN, meaning that technically, the method cannot ever return Ok(Unknown). We basically have two choices here:

  • Remove Unknown from the SDL2 enum since it can't ever be returned from power_level anyway, and it isn't used anywhere else.
  • Mention that the power_level method cannot ever return Ok(Unknown) and so this pattern is unreachable.

I'd tend to go towards solution 1), and Have to_ll() return an Option<PowerLevel> (None would be the equivalent of SDL_JoystickPowerLevel::SDL_JOYSTICK_POWER_UNKNOWN)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we’re talking past each other here.

The method will return Ok(Unknown) if on a platform where there is no power data available; in the “else” block it checks if there is an error, and if there is no error, it will return Ok(Unknown).

I like your proposal of using Option<PowerLevel> but I put it together this way as it matches the pattern for dealing with potential error states as established by the axis method below.

If moving away from that pattern is acceptable, I’d be happy to update it! Given the error state this intends to handle shouldn’t be accessible if used from safe code, this should be safe to do.

Copy link
Member

Choose a reason for hiding this comment

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

You're totally right, I guess I wasn't focused enough in this code review and misread what your PR did exactly.

There is no need to change something that works perfectly fine already, so we're going to keep everything else as it is :) Thank you for the PR!

@Cobrand Cobrand merged commit 809fee0 into Rust-SDL2:master Mar 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants