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

Termux support #33

Closed bbigras closed 4 years ago

bbigras commented 4 years ago
error: Support for this target OS is not implemented yet!
 You may want to create an issue: https://github.com/svartalf/rust-battery/issues/new
  --> /home/builder/.cargo/registry/src/github.com-1ecc6299db9ec823/battery-0.7.4/src/platform/mod.rs:27:9
   |
27 | /         compile_error!("Support for this target OS is not implemented yet!\n \
28 | |             You may want to create an issue: https://github.com/svartalf/rust-battery/issues/new");
   | |___________________________________________________________________________________________________^

error[E0432]: unresolved import `crate::platform::Device`
 --> /home/builder/.cargo/registry/src/github.com-1ecc6299db9ec823/battery-0.7.4/src/types/battery.rs:5:5
  |
5 | use crate::platform::Device;
  |     ^^^^^^^^^^^^^^^^^^^^^^^ no `Device` in `platform`

error[E0432]: unresolved import `crate::platform::Iterator`
 --> /home/builder/.cargo/registry/src/github.com-1ecc6299db9ec823/battery-0.7.4/src/types/iterator.rs:3:5
  |
3 | use crate::platform::Iterator as PlatformIterator;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `Iterator` in `platform`

error[E0432]: unresolved import `crate::platform::Iterator`
 --> /home/builder/.cargo/registry/src/github.com-1ecc6299db9ec823/battery-0.7.4/src/types/manager.rs:5:5
  |
5 | use crate::platform::Iterator as PlatformIterator;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `Iterator` in `platform`

error[E0432]: unresolved import `crate::platform::Manager`
 --> /home/builder/.cargo/registry/src/github.com-1ecc6299db9ec823/battery-0.7.4/src/types/manager.rs:6:5
  |
6 | use crate::platform::Manager as PlatformManager;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `Manager` in `platform`
svartalf commented 4 years ago

So, I poked termux a little bit on my phone and I don't think that supporting it in the battery crate would be a good idea:

In a long-term run it would be better for battery to support Android as OS (which is mostly blocked by https://issuetracker.google.com/issues/37095733), so it can be used within termux.

As for your case with starship, you can rewrite the starship::modules::battery::get_battery_status to use battery for OSes supported by it and call termux-battery-status for termux directly

bbigras commented 4 years ago

Do you know if there is a way to know if Termux:API is installed? termux-battery-status hangs without it and I would prefer to predict that the command would not hang.

svartalf commented 4 years ago

@bbigras sorry, I'm not aware of any way to check this (and personally I think that this is a very strange decision to hang the process instead of failing immediately).

Maybe you can spawn the process, await for stdout with some timeout and then store some kind of flag (ex. in a static var) that termux-battery-status can be called in the current session?

bbigras commented 4 years ago

and personally I think that this is a very strange decision to hang the process instead of failing immediately

Just in case you are curious, it seems to send an Android broadcast and wait for the reply on a Unix abstract socket. Maybe it hangs before the reply never comes.

Thanks for the help.

alexanderadam commented 3 years ago

So maybe this issue can be reopened? Because you're not against adding termux support per se, right @svartalf?

svartalf commented 3 years ago

@alexanderadam I'm not up for supporting termux in its current state, see https://github.com/svartalf/rust-battery/issues/33#issuecomment-529193904 for reasons

alexanderadam commented 3 years ago

I've read that comment but I don't see any 'unfixable. points there. And you've just mentioned that it's just the 'current state' that's difficult for you.

But maybe I've should have gone into a detailed answer:

  • battery binds to OS APIs, while termux is an emulator and providing support for all emulators possible seems to be excessive

So, I'm not sure whether this is clear already but termux is a terminal emulator. Like the xterm, iTerm. It's not an emulator for an operating system. Furthermore it's IMHO the most popular one. I'm not sure whether there any any relevant alternatives.

  • termux-battery-status output hardly can be mapped to what battery::Battery should provide

So the productive way of handling this, would be to mention what exactly is needed. This way it would be possible to open an issue in termux-api and fix it. You somehow missed what info is necessary to map and in which form it is needed.

svartalf commented 3 years ago

@alexanderadam you can check battery::Battery struct documentation to see what information I would expect to get from the target OS, all exposed methods are required to provide correct information without any stubs or zeroed values. termux, of course, is not an "OS", but that just adds an extra question β€” how to ensure at compile time that we are targeting termux environment and not the usual linux or android targets, I'm not aware of any solution to do that in an elegant way (add a feature for that maybe?).

I'm definitely up for adding functionality needed to run battery (and starship as a consequence) inside of the termux env, but it is out of my scope to work on that directly; if you want to raise an issue for termux-api and communicate with its developers to extend provided API, that would be great.

alexanderadam commented 3 years ago

@svartalf thank you for your response. I created issue 364 for it. Please add information in case I forgot something.

UPDATE: Are all fields necessary or would it also be possible to return NULL for some? See also this comment and the one below.

alexanderadam commented 3 years ago

@svartalf just to be sure: did you see the questions in the termux issue?

alexanderadam commented 3 years ago

Also, would you please be so kind and reopen this issue then? :wink: Thank you in advance!

svartalf commented 3 years ago

Hi, @alexanderadam!

To this point I don't see how we can add reasonable Termux support into the battery; please, check previous messages in this thread for reasons.

alexanderadam commented 3 years ago

check previous messages in this thread for reasons.

@svartalf, I did. I mean… we wrote earlier, remember? This was the reason why I asked you for further information and opened issue 364 at the termux-api in the first place.

Can you please answer the question I asked earlier: whether all fields are obligatory or whether some of them might be optional?

For instance the fields technology, vendor, model and serial_number doesn't seem so relevant for many cases. I guess they aren't used for for calculations and have probably only a informational value. Thus something like Android as vendor could be suitable as well, I guess?

Did you have a look at the already mentioned issue 364 of the termux-api?

svartalf commented 3 years ago

Can you please answer the question I asked earlier: whether all fields are obligatory or whether some of them might be optional?

I'm sorry, I assumed it was obvious from the Battery struct documentation. Basically, any method, which returns Option<T> is, well, optional, as in pub fn temperature(&self) -> Option<ThermodynamicTemperature>. That means that temperature, cycle_count, vendor, model and serial_number are optional in a current implementation and other methods are not.

Thus something like Android as vendor could be suitable as well, I guess?

There is a subtle difference: "vendor" in here is a manufacturer of the battery itself, in my laptop it is the SMP.

Did you have a look at the already mentioned issue 364 of the termux-api?

Yes, I checked and to this point I don't see how Android' BatteryManager data can be mapped to Battery from this crate.

nikelborm commented 6 months ago

Hi, @svartalf just checking out are there still no plans to support termux? πŸ‘‰πŸ» πŸ‘ˆπŸ»