1
\$\begingroup\$

I want to start omxplayer, check if there is the process is up, check if there is internet connection start with play a streaming, if there is not play a local loop, meanwhile try to check if the internet connection is up, if there is internet connection stop local file and play streaming video. I write this... any suggestion? Thanks

#!/bin/bash
killall -9 omxplayer.bin
SERVICE='omxplayer'
while true; do
if ps ax | grep -v grep | grep $SERVICE > /dev/null
then
echo "Running" # sleep 5
else
sleep 5
log=log_file_pette.txt
# create log file or overrite if already present
printf "Log File Streaming- " >> $log

# append date to log file
date >> $log

# send email with date restart
echo `date` | mailx -s "Subject: Start streaming Ad" [email protected]
# if there is no internet and no ping
if ping -q -c 1 -W 1 google.com >/dev/null
then
killall -9 omxplayer.bin
$SERVICE -o alsa https://xxxxxxxxxxxx/playlist.m3u8 &
else
# create log file or overrite if already present
printf "Log File Local- " >> $log

# append date to log file
date >> $log
killall -9 omxplayer.bin
$SERVICE -o alsa looptv.mp4 
sleep 5
fi
fi
done
```
\$\endgroup\$
1
  • \$\begingroup\$ If [email protected] is a real email address, then I suggest to redact it. \$\endgroup\$
    – janos
    Nov 10, 2021 at 21:43

1 Answer 1

2
\$\begingroup\$

Use indentation to make the flow of logic easier to see

With indentation added to the original code, it's a lot easier to understand the branches of the logic:

killall -9 omxplayer.bin
SERVICE='omxplayer'
while true; do
    if ps ax | grep -v grep | grep $SERVICE > /dev/null
    then
        echo "Running" # sleep 5
    else
        sleep 5
        log=log_file_pette.txt
        # create log file or overrite if already present
        printf "Log File Streaming- " >> $log

        # append date to log file
        date >> $log

        # send email with date restart
        echo `date` | mailx -s "Subject: Start streaming Ad" [email protected]
        # if there is no internet and no ping
        if ping -q -c 1 -W 1 google.com >/dev/null
        then
            killall -9 omxplayer.bin
            $SERVICE -o alsa https://xxxxxxxxxxxx/playlist.m3u8 &
        else
            # create log file or overrite if already present
            printf "Log File Local- " >> $log

            # append date to log file
            date >> $log
            killall -9 omxplayer.bin
            $SERVICE -o alsa looptv.mp4 
            sleep 5
        fi
    fi
done

Use functions to avoid duplicated logic

This snippet appears twice, with one word replaced:

# create log file or overrite if already present
printf "Log File Streaming- " >> $log

# append date to log file
date >> $log

It would be good to create a helper function for it, with the changing part a variable:

log_with_label() {
    local label=$1
    echo "Log File $label- $(date)" >> "$log"
}

Then you can call this with log_with_label "Streaming" and log_with_label "Local".

Even the simple killall -9 omxplayer.bin would be good in a helper function:

kill_player() {
    killall -9 omxplayer.bin
}

The comments... are lying...

Some of the comments are confusing, because they don't tell the truth.

No sleeping happens here:

echo "Running" # sleep 5

The file will not be overwritten here. This will append to the file if exists:

# create log file or overrite if already present
printf "Log File Streaming- " >> $log

Use double-quotes around variables in shell commands

Instead of these:

if ps ax | grep -v grep | grep $SERVICE > /dev/null
printf "Log File Streaming- " >> $log
$SERVICE -o alsa https://xxxxxxxxxxxx/playlist.m3u8 &

write like this:

if ps ax | grep -v grep | grep "$SERVICE" > /dev/null
printf "Log File Streaming- " >> "$log"
"$SERVICE" -o alsa https://xxxxxxxxxxxx/playlist.m3u8 &

Avoid deeply nested code

Looking at this code:

while true; do
    if ps ax | grep -v grep | grep $SERVICE > /dev/null
    then
        echo "Running" # sleep 5
    else

        # ... many, many lines here....

    fi
done

By the time the I reach the fi, I don't remember what the base condition was. It would be good to reduce the nesting with a continue in the if branch. (I'm wondering if you wanted to put a sleep 5 in there, in code, instead of a comment...)

while true; do
    if ps ax | grep -v grep | grep $SERVICE > /dev/null
    then
        echo "Running" # sleep 5
        continue
    fi

    # ... many, many lines here....

done

Use shellcheck.net

shellcheck.net is a that finds bugs in your shell scripts. It's good to fix all the violations raised by it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.