Skip to content

Reinstate battery info in Kodi #8603

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

Closed

Conversation

GavinDarkglider
Copy link

This PR does 2 things.

  1. adds back UPower, which was removed in 2013 even though it has always been needed by kodi to show battery statistic info.
  2. Fixes issues with kodi UPower implementation, and removes patch that removed battery info from system info.

This addresses many issues across the web, with workarounds, why do that, when the fix is easy. A few examples:
https://forum.libreelec.tv/thread/22536-solution-for-battery-status-in-kodi-libreelec/
https://www.reddit.com/r/kodi/comments/cypvvz/how_can_i_add_a_battery_indicator_to_the_interface/

I tested this in my nintendo switch build, and everything appears working. feel free to test on other builds before merging.

@chewitt
Copy link
Member

chewitt commented Feb 10, 2024

I'd like others on staff to comment too, but I would not be in favour of this because:

a) LE targets static HTPC hardware not portable devices so ommission is intentional and there is no bug to fix
b) Users do recycle old laptops into HTPCs but most are likely PSU tethered due to dead/dying batteries
c) It's fixing a problem on a hardware platform that we don't support (Nintendo Switch)

In terms of patch content I'd want to see package add/remove to be individual commits and if addition restores a previously removed package the copyright dates should reflect the original package not date of readdition. And All commits need to follow repo package: description of change house-style to keep the git log (and summaries extracted from it) readable. The PR also adds dbus patches for Kodi which look like they should be upstreamed or Kodi should be adapted to handle a wider set of dbus messaging use-cases. It's no problem to temporarily backport things from Kodi, but we don't want to accumulate more technical debt.

@chewitt chewitt changed the title Fix Battery info Reinstate battery info in Kodi Feb 10, 2024
@GavinDarkglider
Copy link
Author

GavinDarkglider commented Feb 11, 2024

I'd like others on staff to comment too, but I would not be in favour of this because:

a) LE targets static HTPC hardware not portable devices so ommission is intentional and there is no bug to fix b) Users do recycle old laptops into HTPCs but most are likely PSU tethered due to dead/dying batteries c) It's fixing a problem on a hardware platform that we don't support (Nintendo Switch)

In terms of patch content I'd want to see package add/remove to be individual commits and if addition restores a previously removed package the copyright dates should reflect the original package not date of readdition. And All commits need to follow repo package: description of change house-style to keep the git log (and summaries extracted from it) readable. The PR also adds dbus patches for Kodi which look like they should be upstreamed or Kodi should be adapted to handle a wider set of dbus messaging use-cases. It's no problem to temporarily backport things from Kodi, but we don't want to accumulate more technical debt.

A. Wait, What? Static? What..... This might just be an opinion, but removing support instead of fixing, because you dont have a use for something, is not a reason to not let others fix it for you, and apply the patches for those that might want that info available. Also, I have a new renditition of the patch, that expands on things, like if there is a battery, that which makes keeping the system summary info the same on current devices, if there is no battery detected, so behavior will be the same. I should also mention that one of those issues I mentioned when opening this thread comes from the libreelec forum. To ignore that people using platforms that you support, might want support for something, you didnt want to implement, then someone else did, but you dont want to merge, because you dont see the point, because I did it for a device the main project doesnt support..... Where is the logic in that. I did the work, why I did it shouldnt matter, either the patchset fixes a problem or it doesnt. Just because you dont have that problem, doesnt mean that others do not.

B. Some People might want to keep those tethered to a PowerSupply, you cant say you know every situation, or every use case.

C. Just because I did it for a platform you do not support, doesnt change how UPower works. This litterally is a simple patch that fixes how kodi handles dbus stuff with u-power, because it was completely broken in the backend, because they have everything upstream is based on an ancient u-power..... I am actually in the process of cleaning it all up, and making it more diverse for upstream as well......

D. Why whould I licence a package 2013-2024 that doesnt exist currently in tree, and when it did, was a completely different format, and uses none of the same code? That is more of a curiousity question?

I should also mention having proper battery backend comes in handy for kodi game stuff as well, since controllers, and other devices you might connect to your HTPC might have batteries you might want to get the battery info from. I have no idea if there is currently an implementation for that in peripheral stuff in general. UPower can handle all of that in the power manager backend. As I said, I have already started reworking the backend, so it is a matter of getting the battery info for those batteries, which is really just time and implementation.

Here is the commit in question, where they removed upower: 3a130cd#diff-2ab764032a55b2d8f7cf6b5f60385f933c41f86a833518bebd64580f5e5a260f

if you notice the reason, it was because systemd was supposed to replace it. Currently, Systemd/logind handle power states, Upower handles the battery/power supply stuff. It wasnt removed because they didnt want battery stuff. It was probably removed because whomever did it didnt understand that it was still needed, but optional in the upower battery stuff.

Also, I have the full patchset for the Switch, and I am happy to push it upstream. I have been talking about doing that for months, it is minimally invasive to other builds, and builds off of the pi Image build system script. That being said, that is a 20+ commit(Over 200,000 lines changed) patchset, and would take a long time to review..... as it has taken me 4 years to get it right in the Lakka-LibreELEC tree. Honestly, I would have started here(Would have made lakka patchset cleaner), but at the time no widevine, so no hope.

@chewitt
Copy link
Member

chewitt commented Feb 12, 2024

LE (and OE before it) has long worked to avoid "the death of a thousand cuts" that occurs when niche things few people will use accumlate over time and bloat the distro. We aim to run great on common HTPC hardware and popular SBC boards, and we are intentionally minimalist and not a general purpose purpose distro that seeks to support everything everywhere or for all time. As 99%+ of HTPC devices are non-portable and tethered by cables to non-portable TV's our support for portable and handheld kit or old hardware is rather lacking, and deliberate. Where you see missing fuctionality others see focus and an avoidance of the unnecessary.

So 11-years ago the core capabilities of Upower we required for shutdown/suspend were replaced with systemd functions that exist and work fine for that purpose. You are wrongly assuming an intent to fully replace all Upower capabilities with systemd equivalents. Upower things that were not needed were not a factor in that change.

I had a brief look at the Switch packaging in Lakka and there are some objectionable things that would need to be reworked, e.g. we do not allow firmware blobs in the buildsystem or git-cloned sources that avoid versioning, and it appears to need a Linux 4.9 vendor kernel. These days we champion upstream, so devices that can only use a downstream bsp source would be hard to accept. And so I come back to the question; do we really need Upower changes for devices that aren't common target hardware and we don't support?

The option to support battery info from game controller devices would be nice to have, but if Kodi does not support this today it would need to be added before LE considers including the surrounding things to support it; we do not add things on the speculative hope and whims of it might be useful someday in the future. We also care about image-size impact: growth has been kept to around ~30MB in 10-years (and most of that is firmware) so if you seek to add; what are you adding?

@GavinDarkglider
Copy link
Author

GavinDarkglider commented Feb 12, 2024

LE (and OE before it) has long worked to avoid "the death of a thousand cuts" that occurs when niche things few people will use accumlate over time and bloat the distro. We aim to run great on common HTPC hardware and popular SBC boards, and we are intentionally minimalist and not a general purpose purpose distro that seeks to support everything everywhere or for all time. As 99%+ of HTPC devices are non-portable and tethered by cables to non-portable TV's our support for portable and handheld kit or old hardware is rather lacking, and deliberate. Where you see missing fuctionality others see focus and an avoidance of the unnecessary.

Not really sure what you mean by this comment. What I am getting from it is if I cant see the point, or the goal, then it must not have a potential userbase. I dont know if you have checked the lakka statistics, but the switch is the 2nd most downloaded build in the last 3 years. I dont understand why you wouldnt want to bring that userbase, or members of that development team over here to help fix/solve issues all around. Who cares why we do it, just if it is a helpful piece of code, that serves a decent purpose. I should also mention that kodi started on a game system, and has always been a nice interface for devices with controllers, since that has been a feature of sorts since 2002(?) dont remember when XBMC first released of the top of my head..... Do remember compiling it for a decent media player on my Xbox...... That was a long time ago. lol.

So 11-years ago the core capabilities of Upower we required for shutdown/suspend were replaced with systemd functions that exist and work fine for that purpose. You are wrongly assuming an intent to fully replace all Upower capabilities with systemd equivalents. Upower things that were not needed were not a factor in that change.

This is an invalid opinion imo, because as I said, your use case isnt always everyone elses use, and excluding support for something useful is a bad practice, especially if at one point it was supported, even if accedentally. This is a PR, and not a feature request. I offered a fix for a problem I saw. The Split the changes into seperate commits is a valid reason. Licence stuff valid, Needs descussion because I dont see the use in a built in feature, because that is not relevant for my idea of what I am targeting personally as a dev is not a valid reason in a project like this. From a development point of view, I try to include as many options as possible for users, because I cant always anticipate their needs. On niche devices like the switch, this is easier, but even then.....

I had a brief look at the Switch packaging in Lakka and there are some objectionable things that would need to be reworked, e.g. we do not allow firmware blobs in the buildsystem or git-cloned sources that avoid versioning, and it appears to need a Linux 4.9 vendor kernel. These days we champion upstream, so devices that can only use a downstream bsp source would be hard to accept. And so I come back to the question; do we really need Upower changes for devices that aren't common target hardware and we don't support?

First I chose to go with 4.9, as that has full hardware support for things like the GPU, Audio, PD, CEC, Docking, HW encoding/Decoding, etc and works on all current revisions of the switch..... Second, by definition the Switch is a common platform like the Pi that will be around forever. So not including it because of the kernel stuff is old/vender specific is ? to me for 2 reasons. 1. I have spent the time working out the patchsets needed to make it work in the current tree with the only difference being a script that puts all of the kernel repos together for you, then builds it using the existing package.mk. and 2. It since that opens up dev for any tegra device on the market t210+. I have already layed the framework in my patchset for the whole platform, up to the newer versions which are based on mainline.

As for the firmware, I can host that else where, and have the BSP packages download it. that isnt an issue. As for git sources, that is a joke right? Are you telling me you keep packaged version sources of all of the downloads in the tree for every possible version of libreelec I could think to build? I seriously doubt if I chose to go back to libreELEC 3 I could still get your downloads. IMO Git is the better option, as it is versioned with a commit hash, from the master tree. As for the kernel, if you look at the script that gets the sources, it downloads nvidia's official sources from git(If building a main L4T board)..... As I said, there is a lot to be cleaned up in that build, before it gets pushed here..... It meets the standards of the lakka devs, who have been using git downloading since before I started adding support for things there.

Also, to say it isnt worth the effort to include, clean up, and make easier for downstream distro ports to maintain in their stuff, based on your stuff because you dont see the use, is meh...... Just look at the lakka download statistics, and it might change your mind a bit.

https://nightly.builds.lakka.tv/stats/index.php

So 11-years ago the core capabilities of Upower we required for shutdown/suspend were replaced with systemd functions that exist and work fine for that purpose. You are wrongly assuming an intent to fully replace all Upower capabilities with systemd equivalents. Upower things that were not needed were not a factor in that change.

This is true, but also irrelevant, since people are sharing work arounds on your own forum, because you dont care enough to add support, because of your "death of 1000 cuts therory"...... At one point it worked for user, and the next it didnt....... but that wasnt a regression, that was a choice, that no one included the users in...... if they did, there wouldnt be work arounds. Also, you call it just enough OS to run kodi, but then intentionally choose to disable kodi features that you deem not neccesary.

The option to support battery info from game controller devices would be nice to have, but if Kodi does not support this today it would need to be added before LE considers including the surrounding things to support it; we do not add things on the speculative hope and whims of it might be useful someday in the future. We also care about image-size impact: growth has been kept to around ~30MB in 10-years (and most of that is firmware) so if you seek to add; what are you adding?

I am working on adding support, once I finish up the battery stuff. Hince why I am not pushing it upstream yet, and pushed it here to fix the battery issues in the current implementation upstream. I have a much more indepth patchset I am working on for that.

In the other sense, the switch patchset from lakka, comes with many advantages on top of the switch stuff:

  1. Comes with proper stuff to combine joycons on any linux device.
  2. Allows you to make images that run from fat32, without effecting usablitity. Which I have found useful on a couple of the boards I have ported.
  3. Allows for running kodi as a non root user, so things that adjust permissions in the background(Joycond/Udev) those permissions are respected in userspace, and not treated like they never change. Joycond does this to hide single joycons while combined. That being said, it was mentioned in this PR about attempting to do exactly that: Writeable /etc #7597 (There is one issue with this though, it makes logind dbus break with
    out polkit. On my list of things to patch in systemd for libreelec/lakka. I have hacked a workaround for this for now in kodi.)

Dont take this as a personal thing, I appreciate what you all are doing here, which is why I bothere to try to merge these changes as far upstream as I can, to get support into as many builds as possible. I just dont see the point in turning your nose up at code that has been proven/tested, and is stable(LibreELEC Switch Patchset), and will bring a whole new userbase to this project as well.

@HiassofT
Copy link
Member

Thanks a lot for your contribution, but, like @chewitt already mentioned, this isn't beneficial to the majority of our userbase.

For such a niche feature a strict requirement would be to make the dependencies optional, see eg CEC_SUPPORT, ALSA_SUPPORT etc in https://github.com/LibreELEC/LibreELEC.tv/blob/master/distributions/LibreELEC/options so they don't unnecessarily bloat the images of devices without battery support.

As I also don"t see much value in adding the upower feature to LE, which would put the maintenance burden on us, I think it"s best to keep the changes downstream to Lakka.

@HiassofT HiassofT closed this Feb 12, 2024
@GavinDarkglider
Copy link
Author

Thanks a lot for your contribution, but, like @chewitt already mentioned, this isn't beneficial to the majority of our userbase.
Since there have been issues reported by your users, and addon workarounds for this issue, I dont agree with this statement.

You are welcome I try to add things I find useful to project.

For such a niche feature a strict requirement would be to make the dependencies optional, see eg CEC_SUPPORT, ALSA_SUPPORT etc in https://github.com/LibreELEC/LibreELEC.tv/blob/master/distributions/LibreELEC/options so they don't unnecessarily bloat the images of devices without battery support.

I would argue that 548K is hardly bloat in the image(Where full image size is ~200mb), even if it isnt gated or used on all devices. While not all devices have a battery pretty much any generic X86 device can have a battery, so fixing the backend for that is never a bad idea, and having that be optional is dumb, since it doesnt fix the issue, which you arent considering an issue, if it doesnt get installed in things like generic builds by default.

As I also don"t see much value in adding the upower feature to LE, which would put the maintenance burden on us, I think it"s best to keep the changes downstream to Lakka.

What maitenence burdon? Upower gets updated like 2 times a year, the maitenence is minimal at best, and really shouldnt need updating, once it is in tree if you dont want to. Currently it is close to latest, but really, changing a link, and sha must be a ton of work for you?...... Also. I maintain my own changes, in all the projects I contribute to. Which I would also do here. So you wouldnt even have to worry about the maitenence burdon. But if you want to go there. Emuelec(Not really, because they only support rockchip devices, but, that shouldnt be too hard to fix for other devices..... just have to work out their image script, and integrate the changes properly into the current version), and Lakka, ..... all base their code off of your versioned trees. Which means every time they update stuff, I spend days fixing things. I would rather maintain a base in the highest tree possible, and diseminate fixes for specific tree into that tree's patchset as needed, vs having to rebase changes multiple times....... Half the reason it took 4 years to complete lakka is because of this maitenence burdon, which I have taken on for the whole community, and I have had to port it to multiple versions of the lakka tree, dev versions as well, that have long since been dropped, while also trying to fix issues. If these changes were here, I would have been able to continue fixing issues there, and not rebasing stuff that was mangled in a poor merge.

On top of that, if you werent so strict about stuff, we could start merging these distro's into one tree, and everyone could benifit, not just device developers, but even you. For example the lakka tree is focused on retroarch, and bringing that to all sorts of new devices. Things like the switch build are completely stable. And people use it. A lot of them. updated so much more, than the retroplayer cores here. It would not be hard to turn those into addon depends, which can be easily updated seperatly from the actual addon package. That way they can be used for multiple distros, and keep one package. Which is just one of many examples of how bringing together these split off projects together would help everyone. But that is a whole different issue.

But here is the real truth of it...... I have users who are interested..... A lot of them, and have had them since 2018. https://gbatemp.net/threads/32-bit-gentoo-buiild-with-kodi-18-and-netflix-support.540307/

This was a hack build I did with the broken mainline kernel, so most HW didnt work right, optimizations were way off, ..... but it allowed people to do widevine streaming on the switch....... for the longest time it was the only option, so it gets users. I have had people asking me for libreelec port since before I started working on lakka. My big restraints at the time was the HW decoding wasnt working in kodi, and no widevine for arm64.

@cobalt2727
Copy link

kind of wild as a user from the outside looking in to see "devices that have a battery" being categorized as a niche use case.

@bottmint
Copy link

bottmint commented Feb 15, 2024

b) Users do recycle old laptops into HTPCs but most are likely PSU tethered due to dead/dying batteries

This is stated with such conviction like replacing a battery is entirely out of the realm of possibility. Not only that, but taking your setup with you when you go out of town for vacation or other purposes is also an alien concept? This was an extremely baffling take on a useful feature. How do you ignore your own users in your own forums raising this issue and shoot down a PR that fixes the issue and call it useless and niche?

Having accurate battery stats for less than 1MB is hardly bloat, and you're making an awfully large assumption about your userbase or their individual use cases when you say it isn't useful for the community at large. How do you quantify how useful it is to what portion of your users? Did you poll them? It's a useful feature, even if you don't see it and refuse to acknowledge it.

@HiassofT
Copy link
Member

We are not making any assumptions about our user base, we have anonymized statistics that provide us with hard data about the (actively used!) LE installations.

And for the past couple of years the distribution of active installations has been rather constant at about 80% RPi, 10% x86 and 10% other ARM SBCs/boxes.

As neither RPi nor the ARM SBCs are portable devices with a battery you can easily imagine that adding that feature would add unnecessary bloat to 90% of our userbase. If you take into account the vast majority of x86 installation are using NUCs and the likes you very are close to 100% of user which don't need that feature.

We also monitor our forums for feature requests and if you search them for "battery" you find exactly two posts asking for that feature, so this is hardly a feature that our target userbase wants.

@GavinDarkglider
Copy link
Author

GavinDarkglider commented Feb 15, 2024

And for the past couple of years the distribution of active installations has been rather constant at about 80% RPi, 10% x86 and 10% other ARM SBCs/boxes.

That is a BS argument, when I offered to give you the code to change that...... The switch build is so versitile it works on all models of switch, and as I said, the Lakka build sits at the second most downloaded lakka build ever, and that is the dev build statistics.... so, just thinking about that, I have come to the conclusion that your own policies about bloat, and bsp kernels blocks half the userbase/devs that might jump on this project. What ever though....... I saw an issue I thought needed fixed for my purposes, saw others(even if it is only 2 on your forum, there is more on reddit, and other places) who wanted the same thing, and pushed a change for it. All of the arguements other than maybe gating it are bad, imo. and even that one is bad considering <512k isnt bloat in the days of 8+tb drives. All I worry about with the switch build is keeping the build to a minimum to run what is needed, and keep everything working. So there is no real extra bloat...... Need to keep image size under 4gb, since it runs from fat32. so bloat is a huge issue. Final size of the installed image is 230Mb. For what it is worth, switching from pulseaudio to pipewire adds 5 mb, and you seem(If you enable pipewire it causes issues in LibreELEC kodi package: https://github.com/LibreELEC/LibreELEC.tv/blob/master/packages/mediacenter/kodi/package.mk#L93 && https://github.com/LibreELEC/LibreELEC.tv/blob/master/distributions/LibreELEC/options#L106-L106 && https://github.com/LibreELEC/LibreELEC.tv/blob/master/distributions/LibreELEC/options#L160 kills the whole dependency graph) to support both. So 5MB for another audio management service isnt bloat(That you cant build at all and is still included/maintained) but 512k is for proper battery info is? On top of that once I finish my kodi changes and have the controller battery alert stuff finished, you will need upower, because I am basing my changes off of the code I already fixed. And most of that is done, just need to link the Power Managment subsystem into the joystick plugin, so it can handle alerts for registered controllers.

@HiassofT
Copy link
Member

You are certainly barking up the wrong tree.

If you have a fix for kodi's battery handling, submit it to the kodi repo, not here,

If you want to add a new feature to Lakka builds, submit it to the Lakka repo.

If you want to add a new feature to LibreELEC you are free to PR it but you are also required to address our comments.

Repeatedly insisting how important a feature is for your device and completely ignoring review comments isn't helping and as I don't see this going anywhere I'm locking the conversation.

@LibreELEC LibreELEC locked as too heated and limited conversation to collaborators Feb 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants