-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(linux/input): don't pass unknown battery values #2820
fix(linux/input): don't pass unknown battery values #2820
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2820 +/- ##
=========================================
- Coverage 9.24% 9.23% -0.01%
=========================================
Files 97 97
Lines 17433 17440 +7
Branches 8314 8314
=========================================
Hits 1611 1611
- Misses 12939 12946 +7
Partials 2883 2883
Flags with carried forward coverage won't be shown. Click here to find out more.
|
case LI_BATTERY_STATE_UNKNOWN: | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-looking at this, we should probably add LI_BATTERY_STATE_NOT_PRESENT
and LI_BATTERY_STATE_NOT_CHARGING
so that we cover all cases?
case LI_BATTERY_STATE_NOT_CHARGING:
state = inputtino::PS5Joypad::CHARGHING_ERROR;
break
case LI_BATTERY_STATE_UNKNOWN:
case LI_BATTERY_STATE_NOT_PRESENT:
return; // We can't set it, let's return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LI_BATTERY_STATE_NOT_PRESENT
makes sense, but I'm not sure about mapping LI_BATTERY_STATE_NOT_CHARGING
to a charging error. We don't really have a specific value in the protocol to indicate that a device is actually broken. "Not charging" could simply indicate the device is holding at 80% or whatever to preserve the life of the battery cells.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, makes sense. To be fair I'm not even sure what the actual meaning for CHARGING_ERROR
originally is or how (if?) that will be picked up by clients, but I can see the subtle difference that you mention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I changed the logic here to just ignore status values that the code doesn't understand and those that don't have good mappings to inputino.
If there was a way to just report a percentage without a battery status, maybe we could support LI_BATTERY_STATE_NOT_CHARGING
.
efffa1d
to
a006100
Compare
Description
The
battery.percentage
value is a real percentage (0..100
), but it has a special value ofLI_BATTERY_PERCENTAGE_UNKNOWN
(0xFF) to indicate the percentage is unknown. We shouldn't pass this value or theLI_BATTERY_STATE_UNKNOWN
status intoinputino
.cc: @ABeltramo
Screenshot
Issues Fixed or Closed
Fixes #2715
Type of Change
.github/...
)Checklist
Branch Updates
LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.