Refactor time remaining calculation and integrate estimator - #18
Refactor time remaining calculation and integrate estimator#18DinanathDash wants to merge 4 commits into
Conversation
|
"Hey! @srimanachanta Just a quick heads-up: I've submitted 6 modular PRs (PRs #17 through #22) to break down a larger set of features and make the review process much easier for you. Because a few of these PRs touch the same core files (like Once you merge a PR, if GitHub flags any of my remaining open PRs with conflicts, just let me know or leave them be—I will actively monitor the repo and locally rebase my remaining branches against your updated
Thanks for maintaining Stasis! Let me know if you need any adjustments. |
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
|
|
||
| let formatted = formatTimeRemaining(minutes: metrics.timeRemaining) | ||
| timeRemainingText = formatted.isEmpty ? "Calculating..." : formatted | ||
| timeRemainingText = formatTimeRemaining(minutes: metrics.timeRemaining, powerSource: derivedPowerSource, isCharging: metrics.isCharging) |
There was a problem hiding this comment.
self.adapterConnected isn't assigned to the new value until line 145 of this method, so this call evaluates against the previous tick's adapter state.
| uptimeText = "\(hours)H \(minutes)M" | ||
| } else { | ||
| uptimeText = "\(minutes)M" | ||
| } |
There was a problem hiding this comment.
Prefer Foundation's Duration.UnitsFormatStyle over manual interpolation.
let duration = Duration.seconds(uptime)
uptimeText = duration.formatted(
.units(allowed: [.days, .hours, .minutes], width: .abbreviated)
)|
|
||
| func formatTimeRemaining( | ||
| reportedMinutes: Int, | ||
| powerSource: PowerSource, |
There was a problem hiding this comment.
unused, should be dropped.
| @@ -0,0 +1,104 @@ | |||
| import Foundation | |||
|
|
|||
| @MainActor | |||
There was a problem hiding this comment.
This class should not be @MainActor. No UI deps, no async, no shared mutable state outside self. Isolation flow is guaranteed as this is only called by the MenuViewModel regardless.
| import Foundation | ||
|
|
||
| @MainActor | ||
| class TimeRemainingEstimator { |
There was a problem hiding this comment.
Should be a struct, not a class. One stored property, no identity, no inheritance. Use mutating func formatTimeRemaining(...).
|
|
||
| @MainActor | ||
| class TimeRemainingEstimator { | ||
| private var trendSample: (date: Date, percentage: Int, isCharging: Bool)? |
There was a problem hiding this comment.
Convert this to a struct instead, more readable and easier to expand later (if needed).
private struct TrendSample { let date: Date; let percentage: Int; let isCharging: Bool }
| batteryPercentage: Int, | ||
| targetPercentage: Int | ||
| ) -> Int { | ||
| guard reportedMinutes >= 0 else { return -1 } |
There was a problem hiding this comment.
This class should just return Int? aswell. estimateMinutesFromTrend already does so this should too to follow Swift BP.
| if targetPercentage >= 100 || batteryPercentage >= 100 { | ||
| return reportedMinutes | ||
| } |
There was a problem hiding this comment.
Why is this needed? The batteryPercentage >= 100 half of this OR is unreachable: the earlier batteryPercentage >= targetPercentage guard already returned 0 for that case (since targetPercentage ≤ 100).
|
|
||
| let remainingToTarget = max(0, targetPercentage - batteryPercentage) | ||
| let remainingToFull = max(1, 100 - batteryPercentage) | ||
| let scaled = Double(reportedMinutes) * Double(remainingToTarget) / Double(remainingToFull) |
There was a problem hiding this comment.
This assumption is incorrect. Charging a laptop battery is rarely linear, where macOS charges noticeably slower in the top ~20% (CC→CV transition). For a user with charge limit at 80% this will tend to over-estimate time-to-target. Some form of a non-linear model needs to be formed based on a charging history and combined with battery health data to estimate a proper time to target. I would rather just give the macOS value than a crude estimation like this.
|
@srimanachanta I've pushed updates addressing all of your feedback.
|
|
This is still a linear model to estimate time. We're going to need a nonlinear model for this feature to be useful. |
I did not understand, can you tell me how it will be possible, what does the logic mean? |
Currently, you estimate the time remaining as a linear relationship where the time it takes to gain 10 percent was the same time it took to gain the last 10 percent. For some parts of the charging model, this is accurate (say 30 -> 40 and 40 -> 50), but for other parts (70 -> 80 and 80 -> 90), this is wildly inaccurate. This is because of the way lithium-ion batteries work at a chemical level and the way that the charge management controller inside the laptop actually handle charging the laptop (minimize heat, maximize efficiency). This leads to different charging policies such as CC -> CV transition where the charge controller switches from charging as fast as possible to as efficiently as possible as we approach a certain threshold. The second factor is chemistry specific which is that no two batteries are exactly the same. How long it might take your laptop to charge from 20 -> 80% may be 1/2 the time it takes mine just due to the chemical wear and age of the battery, adapter used to charge it, temperature, etc. This is why macOS has the whole "Calculating..." where the BMS or "Battery Management System" is testing the battery's chemical state and the adapter to determine a reasonable estimate of how long it will take to charge the battery. For us to arbitrarily change the end goal with a charge limiter but then not account for all of these other variables when calculating a new time, we are giving a very inaccurate estimate. The proper solution for this issue would be to query the BMS for all of this relevant information (through the SMC) and feed it into our own mathematical model with an arbitrary end-charge to determine the remaining time. Given the amount of fine-tuning required for this and inability to verify the results, I choose to not implement this when I first made the app. I would rather we give the macOS time than a wildly inaccurate time. If you want to pursue this, I suggest you look through the attached links in smc_power to try and find the relevant keys and build this estimator out, but it is a complex endeavor. |
…eat/fix-battery-uptime-standalone # Conflicts: # Stasis/ViewModels/MenuViewModel.swift
|
@srimanachanta Can you check the latest commit, and tell me if I was able to fix it or not? |
The diff I see in this PR is that formatting is improved and the nil check is in place to stop showing "Calculating..." when charging is stopped. Is that correct? Can you remove the formatting changes as they are crowding the diff. |
2 participants
This pull request introduces a new
TimeRemainingEstimatorclass to improve the estimation and formatting of battery time remaining, and refactors how this information is displayed in the menu. It also enhances the display of system uptime to show days, hours, and minutes for better readability.Battery time estimation improvements:
TimeRemainingEstimatorclass (Stasis/Models/TimeRemainingEstimator.swift) that calculates and formats time remaining, taking into account charging status, power source, charging targets, and trends in battery percentage. This provides more accurate and user-friendly time remaining estimates.MenuViewModelto use the newTimeRemainingEstimatorfor displaying time remaining, passing relevant parameters such as power source, charging status, and charging target. [1] [2] [3]User interface enhancements:
MenuViewModelto display days, hours, and minutes, making it clearer for users with long uptimes.