unstable parsing of battery state

[nephros]

Hi,

trying to use the script from sfos-upgrade-3.5-5.noarch.rpm on a Gemini PDA under SFOS 3.1.0.11 fails with the script exiting silently (exit code 127).

bash -x tracing reveals that:

1. line 412 battery_path="/sys/class/power_supply/*battery*" is not evaluated in the loop following it, when there is only one file, /sys/class/power_supply/battery. I worked around this by removing the asterisks. Not sure why this happens though, AFAICS it shouldn't

2. after this in Line 415, the sourcing fails. This is because when the charger is not connected, the uevent file has:

   POWER_SUPPLY_STATUS=Not charging

   the space causing an incorrect sourcing.
   I worked around this by rewriting that section to:

   if [ -e "${battery_uevents}/uevent" ] then sed 's/STATUS=(.*)/STATUS="\1"/' "${battery_uevents}/uevent" > /tmp/battuevent$$ source /tmp/battuevent$$ battery_info="sourced" break fi

which is a dirty, dirty hack but works for me.

[Olf0]

1. Many thanks for reporting and analysis!
2. Those sysfs battery uevent entries seem to vary a lot between device kernels.
3. Unfortunately reading the battery information on various devices has been full of issues in the past, see issue #26. This is the reason, why the code is already a bit complicated in this regard.

Specifically WRT your analysis:

line 412 battery_path="/sys/class/power_supply/battery" is not evaluated in the loop following it, when there is only one file, /sys/class/power_supply/battery. I worked around this by removing the asterisks. Not sure why this happens though, AFAICS it shouldn't

... and it doesn't on a Jolla 1 and an Xperia X. Can you please cut & paste (or upload) the output of ls -l /sys/class/power_supply/*battery* executed as root on your Gemini PDA.

That almost sounds as if your bash does not honor a set +f after a set -f!?!

POWER_SUPPLY_STATUS=Not charging

... is simply wrong (in this kernel): sysfs uevent files are made for sourcing and hence must not contain spaces. Other devices set this variable to either "Discharging" or "Charging".

Can you also provide the whole battery uevent file from your Gemini, please.

[nephros]

Here's some info as requested:

[nemo@pgemini:~]$ devel-su
Password:
[root@pgemini:nemo]# ls -l /sys/class/power_supply/*battery*
lrwxrwxrwx 1 root root 0 Jul 25  2010 /sys/class/power_supply/battery -> ../../devices/platform/battery/power_supply/battery

[root@pgemini:nemo]# cat /sys/class/power_supply/battery/uevent
POWER_SUPPLY_NAME=battery
POWER_SUPPLY_STATUS=Not charging
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CAPACITY=80
POWER_SUPPLY_BATT_VOL=4132
POWER_SUPPLY_BATT_TEMP=260
POWER_SUPPLY_TEMPERATURER=9383
POWER_SUPPLY_TEMPBATTVOLTAGE=787
POWER_SUPPLY_INSTATVOLT=4021
POWER_SUPPLY_BATTERYAVERAGECURRENT=0
POWER_SUPPLY_BATTERYSENSEVOLTAGE=4132
POWER_SUPPLY_ISENSEVOLTAGE=3965
POWER_SUPPLY_CHARGERVOLTAGE=0
POWER_SUPPLY_STATUS_SMB=3
POWER_SUPPLY_CAPACITY_SMB=50
POWER_SUPPLY_PRESENT_SMB=0
POWER_SUPPLY_ADJUST_POWER=-1

[root@pgemini:nemo]# grep POWER_SUPPLY_STATUS= /sys/class/power_supply/battery/uevent | od -bc
0000000 120 117 127 105 122 137 123 125 120 120 114 131 137 123 124 101
      P   O   W   E   R   _   S   U   P   P   L   Y   _   S   T   A
0000020 124 125 123 075 116 157 164 040 143 150 141 162 147 151 156 147
      T   U   S   =   N   o   t       c   h   a   r   g   i   n   g
0000040 012
     \n
0000041

[root@pgemini:nemo]# bash --version
GNU bash, version 3.2.57(1)-release (armv7l-unknown-linux-gnueabi)
Copyright (C) 2007 Free Software Foundation, Inc.

[root@pgemini:nemo]# uname -a
Linux Sailfish 3.18.41 #1 SMP PREEMPT Mon Jun 3 09:10:16 UTC 2019 aarch64 aarch64 aarch64 
GNU/Linux

[root@pgemini:nemo]# ssu re
Device release is currently: 3.2.0.12

[Olf0]

Thanks! I will think about it. Maybe just filtering out all " " (ASCII 040) per tr before sourcing. I am considering to avoid a temporary file.

But I may not come back to this issue until after Christmas.

[Olf0]

About the first issue: I can reproduce it! Shame on me, too little testing lately, because sfos-upgrade is basically done for me.

[nephros]

Thanks! I will think about it. Maybe just filtering out all " " (ASCII 040) per tr before sourcing. I am considering to avoid an temporary file.

How about

eval $( sed 's/=(.*)/="\1"/' < /sys/class/foo/uevent )

that would properly quote every value.

[Olf0]

The sub-expression's braces need "protection" for simple regexes (but IIRC not for Pearl extended regexes): { eval $(sed 's/=\(.*\)/="\1"/g' "${battery_uevents}/uevent") ; } > /dev/null 2>&1

[Olf0]

@nephros, can you please try the v3.5-7 pre-release: Simply download the noarch.rpm and install it as root per pkcon --install-local or as nemo per a native file manager (needs "untrusted sources" in the Settings on).

After a successful installation, you might start a "same-grade" per sfos-upgrade "$(version | grep -o '[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*' | sed -n '1p')" and answer the final question to actually perform the "same-grade" with "n". All checks (including the battery checks) are executed before that "final question".

And please report back, if sfos-upgrade 3.5-7 runs fine on your Gemini PDA.

[Olf0]

@nephros, as you seem to be an experienced shell programmer: What do you consider better, the eval statement used { eval $(sed 's/=\(.*\)/="\1"/g' "${battery_uevents}/uevent") ; } > /dev/null 2>&1 or the alternatively eval "$(sed 's/=\(.*\)/="\1"/g' "${battery_uevents}/uevent")" > /dev/null 2>&1 ?

See commit https://github.com/Olf0/sfos-upgrade/commit/b3c56148070301b72f8731f89f5b7cddc5cfbb3d for details and discussion.

[Olf0]

Answer: The original version is not correct, but the alternative variant works well. Hence please try v3.5-8.

[nephros]

@nephros, as you seem to be an experienced shell programmer: What do you consider better, the eval statement used { eval $(sed 's/=\(.*\)/="\1"/g' "${battery_uevents}/uevent") ; } > /dev/null 2>&1 or the alternatively eval "$(sed 's/=\(.*\)/="\1"/g' "${battery_uevents}/uevent")" > /dev/null 2>&1 ?

Shell yes, regex not so much ;) It's always quite the trial and error process with those!

See commit b3c5614 for details and discussion.

I shall test and report back. Thank you very much.

[nephros]

@nephros, can you please try the v3.5-7 pre-release: Simply download the noarch.rpm and install it as root per pkcon --install-local or as nemo per a native file manager (needs "untrusted sources" in the Settings on).

After a successful installation, you might start a "same-grade" per sfos-upgrade "$(version | grep -o '[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*' | sed -n '1p')" and answer the final question to actually perform the "same-grade" with "n". All checks (including the battery checks) are executed before that "final question".

And please report back, if sfos-upgrade 3.5-7 runs fine on your Gemini PDA.

That seems to work fine (v3.5-8):

[root@pgemini:apps]# sfos-upgrade "$(version | grep -o '[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*' | sed -n '1p')"
Notice: Mind that sfos-upgrade is best run on a freshly rebooted device.

Notice: Do you want to ensure this SailfishOS 3.2.0.12 installation to be complete and up to date? (Y/N)

[Olf0]

Documentation:

• The battery driver for the Gemini PDA creates a sysfs uevent file with an entry in the form FOO=bar baz (i.e., containing a space).
• According to the Kernel documentation (https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-uevent), "The KEY=VALUE pairs can contain alphanumeric characters only."!
• All officially defined entries for power supplies and batteries adhere to this statement: https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-power
• Although in practice (on other devices than the Gemini PDA) entries in sysfs uevent files have been observed using the following character sets:
  • SYNTH_ARG_FOO=bar with the key containing underscores. As the SYNTH_ARG_ prefix is prepended to all keys, presumably this is the reason why the original key definitions must be constructed from [[:alnum:]] only.
  • SYNTH_ARG_=foo with the key (consisting solely of SYNTH_ARG_) ending in an underscore.
  • SYNTH_ARG_FOO="bar baz" in a few real life examples, i.e. with a value containing space(s) being quoted in double quotes, in contrast to all aforementioned, officially documented values, of which none contains spaces, but some have spaces substituted by - or _ (e.g., Li-ion). I assume this to be a flaw in the drivers generating these entries, but they still retain the ability to source (.) an uevent file.
• But the battery driver for the Gemini PDA generates entries in the form SYNTH_ARG_FOO=bar baz, which breaks the sourceablility of its uevent file. No other device with SailfishOS installed has been yet reported to do so.

Hence ultimately switched from sourceing the battery's uevent file to transforming every statement, which may be translatable into a valid shell variable assignment, to enclose its value (by definition until the end of the line) in double quotes per sed s. For this to work safely, all " are substituted by ' per tr, first. Finally the resulting output is evaluated by the shell per eval as can be seen in the source code. Side note: This results in FOO="bar baz" being transformed to FOO="'bar baz'", thus the value 'bar baz' becoming assigned to the shell variable FOO (instead of bar baz, as originally intended). But as double quotes (") are not part of the defined character set and this still works fine when evaluated right, plus per above described technique almost all characters (combinations) in a value can be correctly assigned to a shell variable (and the ones which cannot would let later variable evaluations fail, anyway), this was the most generic workaround for Gemini's broken battery uevent file I could think of. Furthermore, by using "$(echo $FOO)" (instead of "$FOO") for evaluating a variable, its originally intended value can be obtained regardless of the format of the value (with or without spaces, double quotes etc.; only an odd number of single quotes will break this, which are also not defined as part of the valid character set for values).

[Olf0]

Ultimately I did some further research to scrutinize the root cause of this.

• Found these strings in the kernel code drops from PlanetCom:
  • In their original kernel for the Gemini
  • In their kernel for Android 8 on the Gemini
• But also found them (now knowing where to look) in recent mainline kernels.

So it is some the Linux kernel developers not adhering to to the rules once set by the Sysfs developers (who are also kernel developers)!

Anyway, the parsing of the battery uevent file should now handle all these deviations / variations fine.