svartalf / rust-battery

Rust crate providing cross-platform information about the notebook batteries.
https://crates.io/crates/battery
Apache License 2.0
354 stars 40 forks source link

Batteries iterator order #65

Open lucassardois opened 4 years ago

lucassardois commented 4 years ago

Is the batteries iterator having a fixed order? From my tests it seem to ne be the case. acpi gives me:

Battery 0: Discharging, 15%, 00:28:40 remaining
Battery 1: Full, 100%

But the batteries iterator first give me Battery 1 then Battery 0. I didn't find anything about this in the documentation.

svartalf commented 4 years ago

Hey, @loustak, great question!

There are no guarantees provided right now, I clarified that in 9b7b41238d82864d820f21a3aa717b0b9a3392df, thanks for pointing that out.

In general, I don't think that Batteries iterator should yield them in some particular order, because some underline OS implementations are not providing it too, so it will require some kind of internal buffering at least. Additionaly, it is not required by current battery crate consumers right now, so introducing it will hit their usage performance with no gain in return.

We can potentially expose an OS-specific identifier, which could be used for sorting, if it might be helpful somehow:

OS Type and source
Linux Can be parsed from the /sys/class/power_supply/BAT0 path
macOS IOKit objects have some id field of i32 type, I think?
Windows BatteryTag field of u32 type from BATTERY_QUERY_INFORMATION struct
FreeBSD unit field of libc::c_int type from ACPI data

And of course, it should be future-proof, as some OSes (which can be implemented later) might not have any identifier of that kind at all.

Do you have a specific case where you need the guaranteed order?

lucassardois commented 4 years ago

Thanks for the answer!

My need is not much to have the iterator already sorted but more of a way to sort it.

You crate is beeing used in the following project to list batteries available on the system and print informations in the status bar. The idea is to print the batteries informations, and I wanted to sort them so the order stay consistent whatever happen.

svartalf commented 4 years ago

I see, thanks for explaining it. So far I see two ways to solve this issue:

  1. Introduce OS-specific traits similar to what std does (unix' FileExt trait, for example) with some kind of fn id(&self) -> {tag} method, where {tag} is the type from the table above.

For Linux it will look similar to this then:

#[cfg(target_os = "linux")]
pub trait BatteryExt {
    fn id(&self) -> u64;  // Not sure about the exact device id type right now
}

impl BatteryExt for crate::Battery {
    fn id(&self) -> u64 {
        self.inner_os_specific_implementation.id
    }
}

so you will need to use them with #[cfg] attributes, let's say, with cfg-if crate help:

let battery_id = cfg_if::cfg_if! {
    if cfg!(target_os = "linux") {
        use battery::os::linux::BatteryExt;
        battery.id()
    } elif cfg!(target_os = "windows") {
        use battery::os::windows::BatteryExt;
        battery.id()
    } elif /* and so on… */
  1. Another option I see is to introduce some opaque Tag newtype, which will contain OS-specific battery unique id and implements Hash + PartialOrd + Ord (and maybe something else):
impl Battery {
    pub tag(&self) -> Tag {
        // random fake code
        Tag(self.linux_implementation.sysfs_path.name.parse_int_from_it())
    }
}

It seems better from the UX point, but I'm not sure if it is fully portable yet, so it needs to be investigated first.

lucassardois commented 4 years ago

I like idea 2 because, as you said, it make it opaque to the user of the crate.