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

battery block: fallback for determining power consumption #653

Merged
merged 4 commits into from
May 10, 2020

Conversation

ammgws
Copy link
Collaborator

@ammgws ammgws commented May 9, 2020

Resolves #429 by implementing the simple approach mentioned here: #429 (comment). This could be worked on in the future for better hysteresis.

I don't have a battery operated device to test with at the moment so would appreciate if @travankor, @GladOSkar could check this out if they have time.

@ammgws
Copy link
Collaborator Author

ammgws commented May 9, 2020

One thing I'm not sure about is how these changes affect this block:

       let status = self.status()?;
        match status.as_str() {
            "Full" => Ok(((full as f64 / usage) * 60.0) as u64),
            "Discharging" => Ok(((fill / usage) * 60.0) as u64),
            "Charging" => Ok((((full as f64 - fill) / usage) * 60.0) as u64),
            _ => {
                // TODO: What should we return in this case? It seems that under
                // some conditions sysfs will return 0 for some readings (energy
                // or power), so perhaps the most natural thing to do is emulate
                // that.
                Ok(0)
            }
        }

Since it seems that depending on the context, usage has two different values (power or current). Might need to add another if guard to check the current status before setting usage.

@GladOSkar
Copy link
Contributor

Will test in ~4-5 hrs

@GladOSkar
Copy link
Contributor

GladOSkar commented May 9, 2020

One thing I'm not sure about is how these changes affect this block:

       let status = self.status()?;
        match status.as_str() {
            "Full" => Ok(((full as f64 / usage) * 60.0) as u64),
            "Discharging" => Ok(((fill / usage) * 60.0) as u64),
            "Charging" => Ok((((full as f64 - fill) / usage) * 60.0) as u64),
            _ => {
                // TODO: What should we return in this case? It seems that under
                // some conditions sysfs will return 0 for some readings (energy
                // or power), so perhaps the most natural thing to do is emulate
                // that.
                Ok(0)
            }
        }

Since it seems that depending on the context, usage has two different values (power or current). Might need to add another if guard to check the current status before setting usage.

Oh wait - i remember writing that part of the code now, i added it via #214 exactly because my system doesn't have energy[Wh] and power[W] but charge[Ah] and current[A].

The idea was that usage, fill and full always represent the abstract values in terms of the current system, independent of what units the device driver was using (Amps or Watts), since apparently the device driver always uses either one or the other for both the now and the full values.

Meaning, to be clear: if the device driver uses the combination of energy_full, energy_now and power_now, all values (full, fill, and usage) are in Watts, while if it uses charge_full, charge_now and current_now, they're in Amps.

It works out anyways because in all 3 equations in the cited code block the units cancel out and we're left with a time value.

So, in conclusion, your changes in fn time_remaining() actually break ERT calculation. fn power_consumption() is correct.

With upower = true, everything works as usual:
image

With upower = false though (which this PR addresses afaik), both values are messed up:
image

Firstly, the sysfs value is apparently in μW and needs to be adjusted for that. Secondly, the timer doesn't work.

When removing all changes to fn time_remaining() and dividing the fn power_consumption() result (for the voltage*current case) by 1000000), it's perfect:

image

@travankor
Copy link
Contributor

travankor commented May 9, 2020

Seems like power is returned in μW, while the value is normally returned in W.

Also the battery status reports FULL instead of DISCHARGING

@travankor
Copy link
Contributor

It does not work with upower = false though:

Are you using the sysfs driver?

@GladOSkar
Copy link
Contributor

Hey @travankor i revised my original test because i missed a couple of things. I usually use upower but now focused on sysfs because that's what this PR is for. I saw the μW issue as well.

src/blocks/battery.rs Outdated Show resolved Hide resolved
src/blocks/battery.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@GladOSkar GladOSkar left a comment

Choose a reason for hiding this comment

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

Added my suggestions as reviews, feel free to adjust to your liking. This github review thing seems to be fighting me, it's weird

src/blocks/battery.rs Outdated Show resolved Hide resolved
ammgws and others added 2 commits May 10, 2020 08:11
Co-authored-by: Oskar <okkathe97th@gmail.com>
Co-authored-by: Oskar <okkathe97th@gmail.com>
@ammgws
Copy link
Collaborator Author

ammgws commented May 9, 2020

OK so I believe it should be good to go now.

@travankor Can you please test again?

@GladOSkar
Copy link
Contributor

FYI, works fine on my end now.

@ammgws ammgws merged commit 8b440fe into greshake:master May 10, 2020
@ammgws ammgws deleted the pwrfallback branch May 10, 2020 13:06
@ammgws
Copy link
Collaborator Author

ammgws commented May 10, 2020

Thanks!

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.

Battery: sysfs driver can't get the power draw
3 participants