Added getedid script #968

DaVukovic · 2016-11-22T01:49:44Z

This is a newer version for the getedid script which contains the changes @MilhouseVH mentioned in the previous PR

awiouy

My first code review
Hope it is useful

awiouy · 2016-11-22T01:56:33Z

packages/sysutils/busybox/scripts/getedid

@@ -0,0 +1,254 @@
+#!/bin/sh


Add LE header

awiouy · 2016-11-22T01:59:23Z

packages/sysutils/busybox/scripts/getedid

+      rm -fr /storage/.config/firmware
+      sys_reboot
+    else
+      echo "You don't have a backup file for $file. You didn't used this script to create the custom EDID"


did not use

awiouy · 2016-11-22T01:59:54Z

packages/sysutils/busybox/scripts/getedid

+      sys_reboot
+    else
+      echo "You don't have a backup file for $file. You didn't used this script to create the custom EDID"
+      echo "Therefore we can't be sure the script working prooerly. Exiting"


is working properly

awiouy · 2016-11-22T02:06:00Z

packages/sysutils/busybox/scripts/getedid

+check_file() {
+  # check boot system
+  boot="$([ -d /sys/firmware/efi ] && echo UEFI || echo BIOS)"
+  if [ "$boot" = "BIOS" ]; then


would be clearer:
if -d... then
boot=... sys_path=...
else
boot=... sys_path=...
fi

awiouy · 2016-11-22T02:06:37Z

packages/sysutils/busybox/scripts/getedid

+
+  #check which file is available
+  if [ -f "$sys_path"/syslinux.cfg ] && [ -f "$sys_path"/extlinux.conf ];then
+    echo "Your system contains both. A /flash/syslinux.cfg and a /flash/extlinux.conf file"


awiouy · 2016-11-22T02:08:56Z

packages/sysutils/busybox/scripts/getedid

+check_content() {
+  # check if changes are already made to $file and exit if yes
+  if [ "$(grep "APPEND" $file)" = " APPEND boot=LABEL=System disk=LABEL=Storage ssh quiet" ]; then
+    echo "You alread did some changes to $file. Exiting."


i do not understand, how can you know that the user made exactly these changes?

AFAIK this is the standard content of the APPEND line after a fresh install

maybe != instead of = then, no?

awiouy · 2016-11-22T02:12:23Z

packages/sysutils/busybox/scripts/getedid

+      echo "You didn't created a custom EDID yet or you didn't use this script for this task."
+      echo "Therefore we can't ensure the script working properly."
+      echo "Exiting"
+      exit 3


you exit in rw mode, move mount_rw between if and else

awiouy · 2016-11-22T02:13:55Z

packages/sysutils/busybox/scripts/getedid

+      mv /storage/.config/xorg.conf /storage/.config/xorg.le.backup
+    else
+      echo "You don't have a xorg.conf in your .config folder. Exiting"
+      exit 7


here, too: you exit in rw mode

awiouy · 2016-11-22T02:20:09Z

packages/sysutils/busybox/scripts/getedid

+  sed -i 's/"ModeDebug" "true"/"ModeDebug" "false"/g' /storage/.config/xorg.conf
+
+  # set port and uncomment lines
+  sed -i "s/#    Option         \"ConnectedMonitor\" \"DFP-0\"/    Option         \"ConnectedMonitor\" \"$nv_port\"/g" /storage/.config/xorg.conf


Are the seded lines invariable (they exist and are never edited)?

The lines I change by using sed can ofc be edited by the user before. But therefore I check if a xorg.conf already exists and exit if yes. If the user already did some changes to the xorg.conf, he's also able to do that a 2nd time ;) and then won't need that script

MilhouseVH · 2016-11-22T09:40:50Z

packages/sysutils/busybox/scripts/getedid

+  fi
+
+  #check which file is available
+  if [ -f "$sys_path"/syslinux.cfg ] && [ -f "$sys_path"/extlinux.conf ];then


"$sys_path"/syslinux.cfg ->"$sys_path/syslinux.cfg"
"$sys_path"/extlinux.conf ->"$sys_path/extlinux.conf"

and the same wherever this has been repeated. Presumably it works as it is, but I find it a little odd.

ok, will do

DaVukovic · 2016-11-22T11:32:12Z

Did some changes ^^

MilhouseVH · 2016-11-23T09:06:49Z

Don't forget to install the script... https://github.com/LibreELEC/LibreELEC.tv/blob/master/packages/sysutils/busybox/package.mk#L158-L165

MilhouseVH · 2016-11-23T09:08:20Z

You should also chmod 775 the getedid file, like the other scripts in the same folder.

MilhouseVH · 2016-11-23T09:11:32Z

packages/sysutils/busybox/scripts/getedid

+help() {
+echo "This script generates a custom EDID depending on your GPU"
+echo ""
+echo "To check which GPU you are using, use: ./getedid gpu"


Assuming this script will eventually be on the users PATH, you shouldn't then instruct them to use ./getedid gpu because this won't work - it assumes the script is in the users current directory, when it won't be.

Change all uses of ./getedid to simply getedid.

MilhouseVH · 2016-11-23T09:13:18Z

packages/sysutils/busybox/scripts/getedid

+#
+#  You should have received a copy of the GNU General Public License
+#  along with LibreELEC.  If not, see <http://www.gnu.org/licenses/>.
+################################################################################


Two ++ here - cut & paste error? Please remove the "+" at the beginning of the line.

MilhouseVH · 2016-11-23T09:16:21Z

packages/sysutils/busybox/scripts/getedid

+echo ""
+echo "To check which GPU you are using, use: ./getedid gpu"
+echo ""
+echo "To create a custon EDID, just use this script like: ./getedid start"


getedid start is a little odd, what am I starting? Why not getedid create as this is more in keeping with the other delete and restore options.

MilhouseVH · 2016-11-23T09:20:48Z

packages/sysutils/busybox/scripts/getedid

+  #check which output is connnected:
+  for i in /sys/class/drm/*; do
+    if [ "$(cat "$i"/status 2>/dev/null)" = "connected" ]; then
+      hdmi="$(echo "$i" | cut -d / -f 5 | sed 's/card[0-9]-//g')"


Also, you need to keep the card0- prefix, at least on my Skylake, in which case you just need:

hdmi="$(basename "$i")"

MilhouseVH · 2016-11-23T09:22:34Z

packages/sysutils/busybox/scripts/getedid

+
+  #create edid
+  mkdir -p /storage/.config/firmware/edid
+  cat "/sys/class/drm/$hdmi/edid" > /storage/.config/firmware/edid/edid.bin


Binary data... dunno, maybe use cp instead?

MilhouseVH · 2016-11-23T09:36:28Z

packages/sysutils/busybox/scripts/getedid

+
+# mounting /flash to rw
+mount_rw() {
+  mount -o remout,rw /flash


MilhouseVH · 2016-11-23T09:39:55Z

packages/sysutils/busybox/scripts/getedid

+
+  #remount /flash to rw
+  mount_rw
+  mv /storage/edid.cpio "$sys_path"


sys_path isn't known at this point, you haven't called check_file yet...

MilhouseVH · 2016-11-23T09:44:31Z

packages/sysutils/busybox/scripts/getedid

+# check syslinux.cfg and/or extlinux.conf
+check_file() {
+  # check boot system
+  if [ -d /sys/firmware/efi]; then


Needs a space before ].

MilhouseVH · 2016-11-23T09:49:06Z

packages/sysutils/busybox/scripts/getedid

+  fi
+
+  #check which file is available
+  if [ -f "$sys_path/syslinux.cfg" ] && [ -f "$sys_path/extlinux.conf" ];then


space between ; and then

I'd also rewrite the condition thus:

if [ -f "$sys_path/syslinux.cfg" -a -f "$sys_path/extlinux.conf" ]; then

MilhouseVH · 2016-11-23T09:50:50Z

packages/sysutils/busybox/scripts/getedid

+  # check boot system
+  if [ -d /sys/firmware/efi]; then
+    boot="UEFI"
+    sys_path="flash/EFI/BOOT"


Missing / - should be:

sys_path="/flash/EFI/BOOT"

MilhouseVH · 2016-11-23T09:53:53Z

For intel, are both /storage/.config/firmware/edid/edid.bin and /storage/cpio/lib/firmware/edid/edid.bin required? Or is the cpio folder just being used temporarily to create the cpio file (in which case, it's probably not required at all)?

DaVukovic · 2016-11-26T13:01:54Z

changes done ^^

awiouy reviewed Nov 22, 2016

View reviewed changes

MilhouseVH reviewed Nov 22, 2016

View reviewed changes

CvH added the FEATURE label Nov 22, 2016

MilhouseVH reviewed Nov 23, 2016

View reviewed changes

packages/sysutils/busybox/scripts/getedid

# mounting /flash to rw

mount_rw() {

mount -o remout,rw /flash

Copy link

Contributor

MilhouseVH Nov 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remount

MilhouseVH reviewed Nov 23, 2016

View reviewed changes

new version of getedid and changed busybox/package.mk

5bc3ae5

chewitt approved these changes Dec 18, 2016

View reviewed changes

chewitt merged commit cce0158 into LibreELEC:master Dec 18, 2016

DaVukovic deleted the getedid branch January 3, 2017 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added getedid script #968

Added getedid script #968

DaVukovic commented Nov 22, 2016

awiouy left a comment

awiouy Nov 22, 2016

awiouy Nov 22, 2016

awiouy Nov 22, 2016

awiouy Nov 22, 2016

awiouy Nov 22, 2016

awiouy Nov 22, 2016

awiouy Nov 22, 2016

DaVukovic Nov 22, 2016

awiouy Nov 22, 2016

DaVukovic Nov 22, 2016

awiouy Nov 22, 2016

awiouy Nov 22, 2016

awiouy Nov 22, 2016

DaVukovic Nov 22, 2016

MilhouseVH Nov 22, 2016

DaVukovic Nov 22, 2016

DaVukovic commented Nov 22, 2016

MilhouseVH commented Nov 23, 2016

MilhouseVH commented Nov 23, 2016

MilhouseVH Nov 23, 2016

MilhouseVH Nov 23, 2016

MilhouseVH Nov 23, 2016 •

edited

Loading

MilhouseVH Nov 23, 2016

MilhouseVH Nov 23, 2016

MilhouseVH Nov 23, 2016

MilhouseVH Nov 23, 2016

MilhouseVH Nov 23, 2016

MilhouseVH Nov 23, 2016

MilhouseVH Nov 23, 2016

MilhouseVH Nov 23, 2016

MilhouseVH commented Nov 23, 2016

DaVukovic commented Nov 26, 2016

Added getedid script #968

Added getedid script #968

Conversation

DaVukovic commented Nov 22, 2016

awiouy 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

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

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

DaVukovic commented Nov 22, 2016

MilhouseVH commented Nov 23, 2016

MilhouseVH commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MilhouseVH Nov 23, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MilhouseVH commented Nov 23, 2016

DaVukovic commented Nov 26, 2016

MilhouseVH Nov 23, 2016 •

edited

Loading