D25018: Move ACPI battery information from /proc/acpi to /sys

[Arjen Hiemstra]

ahiemstra added a comment.


  @jjorge: I had to manually edit your author information into the commit, 
since that got lost somewhere along the way and replaced with our localization 
script info. I think it is OK now, but please check 
https://commits.kde.org/ksysguard/94f9bbc246743f5eed7eac0fc9be791f43ba5052

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: alexde, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[Arjen Hiemstra]

This revision was automatically updated to reflect the committed changes.
Closed by commit R106:9022a93738df: Move ACPI battery information from 
/proc/acpi to /sys (authored by l10n daemon script scri...@kde.org, 
committed by ahiemstra).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D25018?vs=69357=70569#toc

REPOSITORY
  R106 KSysguard

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25018?vs=69357=70569

REVISION DETAIL
  https://phabricator.kde.org/D25018

AFFECTED FILES
  ksysguardd/Linux/acpi.c
  ksysguardd/Linux/acpi.h
  ksysguardd/modules.h

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: alexde, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[José JORGE]

jjorge added a comment.


  In D25018#562834 , @ahiemstra 
wrote:
  
  > Do you have a developer account to push the changes? Otherwise I can do 
that for you.
  
  
  I don't, so yes please do the push.

REPOSITORY
  R106 KSysguard

BRANCH
  acpi-move-battery-to-sysfs

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: alexde, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[Arjen Hiemstra]

ahiemstra added a comment.


  KSysGuard is part of Plasma, so these changes will not be released until the 
next plasma release, which is 5.18 and should be released I think January next 
year.
  
  Do you have a developer account to push the changes? Otherwise I can do that 
for you.

REPOSITORY
  R106 KSysguard

BRANCH
  acpi-move-battery-to-sysfs

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: alexde, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[José JORGE]

jjorge added a comment.


  How will this be merged? Will it be in time for Kde 19.12 ?

REPOSITORY
  R106 KSysguard

BRANCH
  acpi-move-battery-to-sysfs

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: alexde, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[José JORGE]

jjorge added a comment.


  > How can you interpret these weird charge_now values correctly? If the range 
is [1/1000, 901] Wh, while the real range is [0, 45] Wh, how does the charge 
curve look like while charging or discharging? Are there (continues) values in 
[46, 901] Wh? Do you assume all of those as 100%?
  
  The only correct interpretation is that a battery chan't have 2000% charge, 
neither -45%. So we assume the 0-100 range for a percentage ;-)
  
  Edit: Is the charge_now or the maximum value incorrect due a faulty battery?
  
  Yes, that's the reason. The BIOS tells me I should junk it, but I can still 
work nearly two hours more when it is plugged in ;-)

REPOSITORY
  R106 KSysguard

BRANCH
  acpi-move-battery-to-sysfs

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: alexde, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[Alex Debus]

alexde added a comment.


  In D25018#561268 , @jjorge wrote:
  
  > > Are states > 100 or < 0 really well defined? Is it save to assume that a 
state > 100 can be associated with 100?
  >
  > Yes : this is a percentage. States out of this value are bad hardware : I 
have a travel battery which shows crazy values along time, from 0,001Wh 
capacity till 901Wh (Real capacity is near 45Wh).
  
  
  How can you interpret these weird charge_now values correctly? If it goes 
from [1/1000, 901] Wh, while the real range is [0, 45] Wh, how does the charge 
curve look like while charging or discharging? 
  Are there (continues) values in [46, 901] Wh? Do you assume all of those as 
100%?

REPOSITORY
  R106 KSysguard

BRANCH
  acpi-move-battery-to-sysfs

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: alexde, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[José JORGE]

jjorge marked an inline comment as done.
jjorge added a comment.


  > Are states > 100 or < 0 really well defined? Is it save to assume that a 
state > 100 can be associated with 100?
  
  Yes : this is a percentage. States out of this value are bad hardware : I 
have a travel battery which shows crazy values along time, from 0,001Wh 
capacity till 901Wh (Real capacity is near 45Wh).
  
  > This should maybe be a part of a separate patch as it seems unrelated.
  
  You're right, this is already in master, I have badly also selected this 
commit in my patch.

REPOSITORY
  R106 KSysguard

BRANCH
  acpi-move-battery-to-sysfs

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: alexde, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[Alex Debus]

alexde added inline comments.

INLINE COMMENTS

> acpi.c:129
> +} else if (state < 0) {
> +state = 0; /* prevent insane numbers with bad hardware */
>  }

Are states > 100 or < 0 really well defined? Is it save to assume that a state 
> 100 can be associated with 100?

> acpi.c:161
> +} else if (state < 0) {
> +state = 0; /* prevent insane numbers with bad hardware */
> +}

Are states > 100 or < 0 really well defined? Is it save to assume that a state 
> 100 can be associated with 100 for example?

> nvidia.json:12
>  "Description[nn]": "Statistikk til prosessorspesifikke GPU-ressursar 
> for NVIDIA-kort",
> +"Description[pl]": "Statystyka dla zasobów GPU na proces dla kart 
> Nvidii",
>  "Description[pt]": "Estatísticas dos recursos do GPU por processo 
> nas placas Nvidia",

This should maybe be a part of a separate patch as it seems unrelated.

REPOSITORY
  R106 KSysguard

BRANCH
  acpi-move-battery-to-sysfs

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: alexde, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[José JORGE]

jjorge added a comment.


  Thanks. Just some thoughts : designcharge is usefull if you want to see the 
percentage of charge you reach against what it should be with a new battery. 
For some adicional sensors, of course, even if I dont see what ones you are 
thinking about.

REPOSITORY
  R106 KSysguard

BRANCH
  acpi-move-battery-to-sysfs

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[Arjen Hiemstra]

ahiemstra accepted this revision.
ahiemstra added a comment.
This revision is now accepted and ready to land.


  So, with this my system lists battery in ksysguard again, which is really 
nice. I do wonder about the usefulness of the "DesignCharge" sensor and would 
maybe like to see some additional sensors, but those can be discussed in a 
follow up.

REPOSITORY
  R106 KSysguard

BRANCH
  acpi-move-battery-to-sysfs

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[José JORGE]

jjorge updated this revision to Diff 69357.
jjorge marked 6 inline comments as done.
jjorge added a comment.


  - hopefully correct all problems found in nice ahiemstra's revision
  - - indentation corrected to 4 chars
  - - function braces in newline
  - - We can calculate charge even if current is <= zero.

REPOSITORY
  R106 KSysguard

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25018?vs=68919=69357

BRANCH
  acpi-move-battery-to-sysfs

REVISION DETAIL
  https://phabricator.kde.org/D25018

AFFECTED FILES
  ksysguardd/Linux/acpi.c
  ksysguardd/Linux/acpi.h
  ksysguardd/modules.h
  plugins/process/nvidia/nvidia.json

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[Arjen Hiemstra]

ahiemstra added a comment.


  You are right, sorry. I was confusing /sys with /proc, which does have a lot 
of "put all of this information in a single file" stuff going on. It would 
still be nice if there was a better way of getting this information, but that 
is not related to this change. So then I mostly have style nitpicks as comment.

INLINE COMMENTS

> acpi.c:96
>  
> -  if ( ( d = opendir( "/proc/acpi/battery" ) ) == NULL ) {
> - AcpiBatteryNum = -1;
> - AcpiBatteryOk = 0;
> - return;
> -  } else {
> - AcpiBatteryNum = 0;
> - AcpiBatteryOk = 1;
> -while ( ( de = readdir( d ) ) )
> -  if ( ( strcmp( de->d_name, "." ) != 0 ) && ( strcmp( de->d_name, ".." 
> ) != 0 ) ) {
> -   strncpy( AcpiBatteryNames[ AcpiBatteryNum ], de->d_name, 8 );
> -   snprintf( s, sizeof( s ), "acpi/battery/%d/batterycharge", 
> AcpiBatteryNum );
> -   registerMonitor( s, "integer", printAcpiBatFill, 
> printAcpiBatFillInfo, sm );
> -   snprintf( s, sizeof( s ), "acpi/battery/%d/batteryusage", 
> AcpiBatteryNum );
> -   registerMonitor( s, "integer", printAcpiBatUsage, 
> printAcpiBatUsageInfo, sm);
> -   AcpiBatteryCharge[ AcpiBatteryNum ] = 0;
> -   AcpiBatteryNum++;
> -   }
> +  d = opendir("/sys/class/power_supply/");
> +  if (d != NULL) {

Can you clean up the indentation of this function? It should use 4 spaces for 
indentation.

> acpi.c:112
>  
> -
> -int updateAcpiBattery( void )
> -{
> -  int i, fd;
> -  char s[ ACPIFILENAMELENGTHMAX ];
> -  size_t n;
> -  char AcpiBatInfoBuf[ ACPIBATTERYINFOBUFSIZE ];
> -  char AcpiBatStateBuf[ ACPIBATTERYSTATEBUFSIZE ];
> -  char *p;
> -  int AcpiBatCapacity = 1;
> -  int AcpiBatRemainingCapacity = 0;
> -
> -  if ( AcpiBatteryNum <= 0 )
> -return -1;
> -
> -  for ( i = 0; i < AcpiBatteryNum; i++ ) {
> -/* get total capacity */
> -snprintf( s, sizeof( s ), "/proc/acpi/battery/%s/info", 
> AcpiBatteryNames[ i ] );
> -if ( ( fd = open( s, O_RDONLY ) ) < 0 ) {
> -  print_error( "Cannot open file \'%s\'!\n"
> -   "Load the battery ACPI kernel module or\n"
> -   "compile it into your kernel.\n", s );
> -  AcpiBatteryOk = 0;
> -  return -1;
> -}
> -if ( ( n = read( fd, AcpiBatInfoBuf, ACPIBATTERYINFOBUFSIZE - 1 ) ) ==
> - ACPIBATTERYINFOBUFSIZE - 1 ) {
> -  log_error( "Internal buffer too small to read \'%s\'", s );
> -  close( fd );
> -  AcpiBatteryOk = 0;
> -  return -1;
> -}
> -close( fd );
> -p = AcpiBatInfoBuf;
> -if ( p && strstr(p, "ERROR: Unable to read battery") )
> -return 0;  /* If we can't read the battery, reuse the last value 
> */
> -while ( ( p!= NULL ) && ( sscanf( p, "last full capacity: %d ",
> -   ) != 1 ) ) {
> -  p = strchr( p, '\n' );
> -  if ( p )
> -p++;
> -}
> -/* get remaining capacity */
> -snprintf( s, sizeof( s ), "/proc/acpi/battery/%s/state", 
> AcpiBatteryNames[ i ] );
> -if ( ( fd = open( s, O_RDONLY ) ) < 0 ) {
> -  print_error( "Cannot open file \'%s\'!\n"
> -   "Load the battery ACPI kernel module or\n"
> -   "compile it into your kernel.\n", s );
> -  AcpiBatteryOk = 0;
> -  return -1;
> +void printSysBatteryCharge(const char *cmd) {
> +int zone = 0;

Braces on newline for function definitions please.

> acpi.c:151
> +int state = 0;
> +if (current > 0 && maximum > 0) {
> +state = current * 100 / maximum;

I don't see why current needs to be >0 ? What if I have an empty battery?

> acpi.c:170
>  
> -void printAcpiBatUsage( const char* cmd)
> -{
> - int i;
> +void printSysBatteryRate(const char *cmd) {
> +int zone = 0;

Braces on newline for function definitions please.

> acpi.c:328
>  
> -static int getSysFileValue(const char *group, int value, const char *file) {
> +static int getSysFileValue(const char *class, const char *group, int value, 
> const char *file) {
>  static int shownError = 0;

Using `class` as name here seems a bit dangerous to me, if this code ever gets 
compiled with a C++ compiler it will trip over this name. Maybe use className 
instead?

> acpi.c:332
>  char input_buf[ 100 ];
> -snprintf(th_file, sizeof(th_file), "/sys/class/thermal/%s%d/%s",group, 
> value, file);
> +snprintf(th_file, sizeof(th_file), "/sys/class/%s/%s%d/%s",class, group, 
> value, file);
>  int fd = open(th_file, O_RDONLY);

Please add a space between , and class

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[José JORGE]

jjorge added a comment.


  In D25018#558503 , @ahiemstra 
wrote:
  
  > Hmm, I would like to see some smarter sys file handling for this. Right 
now, multiple calls to getSysFileValue result in the same file being read from 
disk over and over. While the original implementation may not have been ideal 
either, the update function at least made sure the ACPI stuff got only read 
once per update. If you need some inspiration, Memory.c has a fairly decent 
implementation.
  
  
  Are you sure this is a good idea? While previous update function readed only 
one file in /proc, like memory.c does, we read different files in /sys : one 
for each information. So this is not the same file readed over and over (and it 
is not read from disk as it is a virtual fs exposed by the kernel). Thanks.

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[José JORGE]

jjorge requested review of this revision.
jjorge added a comment.


  So asking again for feedback, if I understand well how phabricator works ...

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[Arjen Hiemstra]

ahiemstra requested changes to this revision.
ahiemstra added a comment.
This revision now requires changes to proceed.


  Hmm, I would like to see some smarter sys file handling for this. Right now, 
multiple calls to getSysFileValue result in the same file being read from disk 
over and over. While the original implementation may not have been ideal 
either, the update function at least made sure the ACPI stuff got only read 
once per update. If you need some inspiration, Memory.c has a fairly decent 
implementation.

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D25018

To: jjorge, davidedmundson, #plasma, ahiemstra
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25018: Move ACPI battery information from /proc/acpi to /sys

[José JORGE]

jjorge created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
jjorge requested review of this revision.

REVISION SUMMARY
  Since kernel 2.6.24 in 2008, the /proc/acpi/battery interface is deprecated.
  There is for years no updated kernel which wouldn't provide the 
/sys/class/power_supply/BAT0 in a laptop 
  This patch removes the useless old path, and uses the sysfs to provide 3 
informations :
  
  - current flow in mA (charge or discharge)
  - charge percent against last completed full charge
  - charge percent against battery by design charge
  
  Ref:
  https://cateee.net/lkddb/web-lkddb/ACPI_PROCFS_POWER.html
  https://bbs.archlinux.org/viewtopic.php?id=97761

REPOSITORY
  R106 KSysguard

BRANCH
  acpi-move-battery-to-sysfs

REVISION DETAIL
  https://phabricator.kde.org/D25018

AFFECTED FILES
  ksysguardd/Linux/acpi.c
  ksysguardd/Linux/acpi.h
  ksysguardd/modules.h

To: jjorge
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart