Added CPU temperature and Device name for X86 #267

FauFra · 2022-10-19T15:54:38Z

What does this PR aim to accomplish?:

Updated cpu and sys_model variables with more precise files when the script is executed on a X86 machine.

By submitting this pull request, I confirm the following:

I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
I have commented my proposed changes within the code and I have tested my changes.
I am willing to help maintain this change if there are issues with it later.
It is compatible with the EUPL 1.2 license
I have squashed any insignificant commits. (git rebase)

I have read the above and my PR is ready for review. Check this box to confirm

yubiuser

The tests failing for stickler at L290

-f doesn't work with globs. Use a for loop

yubiuser · 2022-10-19T18:07:45Z

padd.sh

@@ -267,7 +269,9 @@ GetSystemInformation() {
  fi

  # Device model
-  if [ -f /sys/firmware/devicetree/base/model ]; then
+  if [ -f /sys/devices/virtual/dmi/id/product_name ]; then


On my system product_family would be the better choice.

chrko@ThinkPad-X230:~$ cat /sys/devices/virtual/dmi/id/product_name 2325CN3 cat /sys/devices/virtual/dmi/id/product_family ThinkPad X230

I used the same file used by applications like neofetch to show the host name. I tried with an MSI and a Mac Mini and product_name and the results are these:

cat /sys/devices/virtual/dmi/id/product_family PS cat /sys/devices/virtual/dmi/id/product_name PS63 Modern 8SC cat /sys/devices/virtual/dmi/id/product_family Macmini cat /sys/devices/virtual/dmi/id/product_name Macmini6,2

While on an Asus laptop the situation is as your:

cat /sys/devices/virtual/dmi/id/product_family VivoBook cat /sys/devices/virtual/dmi/id/product_name X510UQR

So, I thought that a possible solution could be if product_family is a substring of product_name then only the latter is shown, otherwise both the strings are displayed.

What do you think?

Yeah, try it.

Let me know if it works also in your system

It works.

chrko@ThinkPad-X230:~$ ./test.sh ThinkPad X230 2325CN3

yubiuser

Please address the Stickler complains.

yubiuser · 2022-10-21T09:38:58Z

padd.sh

@@ -267,7 +269,9 @@ GetSystemInformation() {
  fi

  # Device model
-  if [ -f /sys/firmware/devicetree/base/model ]; then
+  if [ -f /sys/devices/virtual/dmi/id/product_name ]; then


It works.

chrko@ThinkPad-X230:~$ ./test.sh ThinkPad X230 2325CN3

yubiuser · 2022-10-21T09:39:14Z

padd.sh

+      sys_model="${product_family} ${product_name}"
+    fi
+
+    # sys_model=$(tr -d '\0' < /sys/devices/virtual/dmi/id/product_name)


Suggested change

# sys_model=$(tr -d '\0' < /sys/devices/virtual/dmi/id/product_name)

DL6ER · 2022-10-21T11:56:30Z

I can furthermore provide on a small NUC:

d@nuc1:~$ cat /sys/firmware/devicetree/base/model
cat: /sys/firmware/devicetree/base/model: No such file or directory
d@nuc1:~$ cat /sys/devices/virtual/dmi/id/product_name
To Be Filled By O.E.M.
d@nuc1:~$ cat /sys/devices/virtual/dmi/id/product_family
To Be Filled By O.E.M.

On my HP microserver:

d@ubuntu-server:~$ cat /sys/firmware/devicetree/base/model
cat: /sys/firmware/devicetree/base/model: No such file or directory
d@ubuntu-server:~$ cat /sys/devices/virtual/dmi/id/product_name
ProLiant MicroServer
d@ubuntu-server:~$ cat /sys/devices/virtual/dmi/id/product_family

On a new server:

d@ubuntu-server2:~$ cat /sys/firmware/devicetree/base/model
cat: /sys/firmware/devicetree/base/model: No such file or directory
d@ubuntu-server2:~$ cat /sys/devices/virtual/dmi/id/product_name
KRPA-U16 Series
d@ubuntu-server2:~$ cat /sys/devices/virtual/dmi/id/product_family
Server

FauFra · 2022-10-21T13:55:55Z

Thank you very much, this is helpful! But what happens if you execute a program like neofetch on your NUC server? What is shown near host label?

yubiuser · 2022-10-21T14:06:27Z

Why don't we use the same logic as neofetch

https://github.com/dylanaraps/neofetch/blob/ccd5d9f52609bbdcd5d8fa78c4fdb0f12954125f/neofetch#L1235-L1257

Including the cleaning: https://github.com/dylanaraps/neofetch/blob/ccd5d9f52609bbdcd5d8fa78c4fdb0f12954125f/neofetch#L1361-L1374

yubiuser · 2022-10-21T14:18:25Z

That's interesting. Currently the ~~latest code (v7.1.0)~~ master looks like

            if [[ -d /system/app/ && -d /system/priv-app ]]; then
                model="$(getprop ro.product.brand) $(getprop ro.product.model)"

            elif [[ -f /sys/devices/virtual/dmi/id/board_vendor ||
                    -f /sys/devices/virtual/dmi/id/board_name ]]; then
                model=$(< /sys/devices/virtual/dmi/id/board_vendor)
                model+=" $(< /sys/devices/virtual/dmi/id/board_name)"

            elif [[ -f /sys/devices/virtual/dmi/id/product_name ||
                    -f /sys/devices/virtual/dmi/id/product_version ]]; then
                model=$(< /sys/devices/virtual/dmi/id/product_name)
                model+=" $(< /sys/devices/virtual/dmi/id/product_version)"

            elif [[ -f /sys/firmware/devicetree/base/model ]]; then
                model=$(< /sys/firmware/devicetree/base/model)

            elif [[ -f /tmp/sysinfo/model ]]; then
                model=$(< /tmp/sysinfo/model)
            fi

Producing LENOVO 2325CN3

Using the locally installed version (also v7.1.0) 2325CN3 ThinkPad X230

And the code looks like

            if [[ -d /system/app/ && -d /system/priv-app ]]; then
                model="$(getprop ro.product.brand) $(getprop ro.product.model)"

            elif [[ -f /sys/devices/virtual/dmi/id/product_name ||
                    -f /sys/devices/virtual/dmi/id/product_version ]]; then
                model=$(< /sys/devices/virtual/dmi/id/product_name)
                model+=" $(< /sys/devices/virtual/dmi/id/product_version)"

            elif [[ -f /sys/firmware/devicetree/base/model ]]; then
                model=$(< /sys/firmware/devicetree/base/model)

            elif [[ -f /tmp/sysinfo/model ]]; then
                model=$(< /tmp/sysinfo/model)
            fi

They added it here: dylanaraps/neofetch#1943

The old was more useful I think. We could simply change the order of if cases

FauFra · 2022-10-21T14:23:32Z

I noticed the same thing. If you download the last release code, it is different from the one in the repo.

However the same logic can be implemented without any problem, but that cleaning string way is not supported in sh. Is there any reason why sh is used instead of bash?

yubiuser · 2022-10-21T14:31:26Z

Is there any reason why sh is used instead of bash?

To increase POSIX compliance to make it more portable.

${name/foo/bar} -- you can use $(printf '%s\n' "$name" | sed 's/foo/bar/'), after changing shell patterns to regular expressions. This originated in ksh93 and is also present in mksh, and zsh, but variation in behaviour and glob pattern syntax between the three.

https://mywiki.wooledge.org/Bashism

FauFra · 2022-10-21T14:34:21Z

Okay, thanks. I will try to implement the same logic and string cleaning then.

yubiuser · 2022-10-21T16:19:49Z

Works good for me. Maybe @DL6ER could test on his devices.

rdwebdesign · 2022-10-21T18:01:16Z

padd.sh

+  fi
+
+  # Cleaning device model from useless OEM information
+  sys_model=$(echo "$sys_model" | sed -e 's:To be filled by O.E.M.::g')


Stickler test (actually Shellcheck) is complaining here. It is asking to use this variable substituion ${variable//search/replace}, but I'm almost sure this is not POSIX.

You can try to use shell "pattern filtering":

Suggested change

sys_model=$(echo "$sys_model" | sed -e 's:To be filled by O.E.M.::g')

sys_model="${sys_model#"To be filled by O.E.M."}"

Thanks! This is a more elegant way to clean the string.

This only works if the searched string is on the beginning of $sys_model. If you need to remove from the end, use ${sys_model%"To be filled by O.E.M."}

Okay, so for each string there must be two filters, one for the beginning and one for the end since there might be the possibility that the value of sys_model is To be filled by O.E.M. To be filled by O.E.M., as shown by @DL6ER

rdwebdesign · 2022-10-21T19:27:00Z

padd.sh

@@ -279,7 +279,6 @@ GetSystemInformation() {
    if [ -z "$sys_model" ]; then
      sys_model="${product_family} ${product_name}"
    fi    


There is one more test failing (if you click on "Details" on the failing test you will be able to see it).

There are trailing spaces here.

Oh, thanks. Now it should be fixed.

yubiuser · 2022-10-23T07:48:19Z

Tests pass now. Can you please squash down your commits into a single one?

Signed-off-by: FauFra <[email protected]>

FauFra · 2022-10-23T08:16:04Z

Is it okay now?

yubiuser requested changes Oct 19, 2022

View reviewed changes

yubiuser reviewed Oct 21, 2022

View reviewed changes

rdwebdesign requested changes Oct 21, 2022

View reviewed changes

Added CPU temperature and Device name for X86

8546070

Signed-off-by: FauFra <[email protected]>

FauFra force-pushed the development branch from 5e5df95 to 8546070 Compare October 23, 2022 08:14

yubiuser requested review from rdwebdesign and yubiuser October 23, 2022 21:55

yubiuser approved these changes Oct 24, 2022

View reviewed changes

yubiuser merged commit 6c2429a into pi-hole:development Oct 24, 2022

yubiuser mentioned this pull request Oct 24, 2022

PADD v3.9.0 #269

Merged

rdwebdesign mentioned this pull request Dec 15, 2022

Issue with product_family in docker on a Synology nas #293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added CPU temperature and Device name for X86 #267

Added CPU temperature and Device name for X86 #267

FauFra commented Oct 19, 2022

yubiuser left a comment

yubiuser Oct 19, 2022

FauFra Oct 20, 2022

yubiuser Oct 20, 2022

FauFra Oct 21, 2022

yubiuser Oct 21, 2022

yubiuser left a comment

yubiuser Oct 21, 2022

yubiuser Oct 21, 2022

DL6ER commented Oct 21, 2022

FauFra commented Oct 21, 2022

yubiuser commented Oct 21, 2022

yubiuser commented Oct 21, 2022 •

edited

Loading

FauFra commented Oct 21, 2022

yubiuser commented Oct 21, 2022

FauFra commented Oct 21, 2022

yubiuser commented Oct 21, 2022

rdwebdesign Oct 21, 2022

FauFra Oct 21, 2022

rdwebdesign Oct 21, 2022

FauFra Oct 21, 2022

rdwebdesign Oct 21, 2022

FauFra Oct 23, 2022

yubiuser commented Oct 23, 2022

FauFra commented Oct 23, 2022

	# sys_model=$(tr -d '\0' < /sys/devices/virtual/dmi/id/product_name)

	sys_model=$(echo "$sys_model" \| sed -e 's:To be filled by O.E.M.::g')
	sys_model="${sys_model#"To be filled by O.E.M."}"

Added CPU temperature and Device name for X86 #267

Added CPU temperature and Device name for X86 #267

Conversation

FauFra commented Oct 19, 2022

yubiuser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yubiuser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DL6ER commented Oct 21, 2022

FauFra commented Oct 21, 2022

yubiuser commented Oct 21, 2022

yubiuser commented Oct 21, 2022 • edited Loading

FauFra commented Oct 21, 2022

yubiuser commented Oct 21, 2022

FauFra commented Oct 21, 2022

yubiuser commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yubiuser commented Oct 23, 2022

FauFra commented Oct 23, 2022

yubiuser commented Oct 21, 2022 •

edited

Loading