More script-dev questions. How to log errors in external scripts?

sleve_mcdichael

@mitu said in More script-dev questions. How to log errors in external scripts?:

On upgrades though, why do you need to re-read the configuration and invoke enable/disable ?

On upgrades, we don't need to enable or disable, just leave it alone. That's why we need to check (one way or another): 'is this an "upgrade" configure, or an "install" configure.'

...is there an [[ "$md_mode" == "upgrade" ]]?

Yes, you're right - I forgot that only the ports run bash only. Maybe something that could be improved here.

See my edit.

mitu

@sleve_mcdichael said in More script-dev questions. How to log errors in external scripts?:

On upgrades, we don't need to enable. That's why we need to check: is this an "upgrade" configure, or an "install" configure.
...is there an [[ "$md_mode" == "upgrade" ]]?

No, but you can always check for the config file existence to determine if it's a new install and use a toggle_bgm "on" before copyDefaultConfig (toggle_bgm would be the function that enables/disables the module, with the on/off param).

sleve_mcdichael

@mitu said in More script-dev questions. How to log errors in external scripts?:

you can always check for the config file existence to determine if it's a new install and use a toggle_bgm "on" before copyDefaultConfig

Yeah that's how I've done it. I guess I hadn't published this version yet:

https://github.com/s1eve-mcdichae1/RetroPie-Extra/blob/e0b2a0707a4f09b0ec505578b8059f61b16c9441/scriptmodules/supplementary/bgm123.sh

Do you think the sed and/or touch should get moved from install to configure?

mitu

@sleve_mcdichael said in More script-dev questions. How to log errors in external scripts?:

Do you think the sed and/or touch should get moved from install to configure?

No, they can stay there.

sleve_mcdichael

@mitu here is where I am at after last night:

https://github.com/s1eve-mcdichae1/RetroPie-Extra/blob/9344ee0c3941689c84c89f9d32db6aa05aa3fd0a/scriptmodules/supplementary/bgm123.sh

I made the assignment of the "global-ish" vars look "prettier." I put them all in one function, with the $1 param dictating what should be returned. Then I put declaration and assignment on one line (still as two statements, with a && in between. Makes them wider and less tall, less "towering.") This seems to be working, does it break any of the rules? Definition and asignment

...

Re: onstart/onend. In case the user already has other actions here, do you think the fade-out/in should be the first thing that happens on game start and the last thing that happens on return, or some other combination?

...

I said:

So then we are tracking every change of the enabled status what, just to check it in configure once in a while. (...) does the enabled status even matter for configure?

Turns out, maybe. remove calls disable first, for obvious reasons. So, if you remove then reinstall, it currently doesn't get re-enabled, because it 1) has been installed before, and 2) has subsequently been disabled. So maybe recording this status in user config and using that to determine what to do at configure would be better, after all. I'll write the status change in the gui action so it's not recorded when uninstall calls the disable function directly. Then reinstall will still see the "enabled" config and know to take the necessary action.

...

I said:

And sometimes the track doesn't start up the first time you hit "play" after re-enabling it.

I discovered this in the stand-alone menu script but it appears is present also in the current GUI-only iteration. I've at least identified how and when this happens, if not why. If I PAUSE the music, then DISABLE the music, then ENABLE the music, then PLAY the music, it doesn't start, and I have to hit PLAY/PAUSE a second time.

Experimenting, I found that if I DISABLE while the music is playing, then SSH in and pgrep mpg123 I get nothing. But if I PAUSE it first and then DISABLE, I still get a PID.

If I ps -ostate= -C mpg123 I can see that it is:

S (interruptible sleep) while playing, and
T (stopped) while paused.

If I pkill while it's "sleeping" (playing) it works fine, but if I pkill while it's "stopped," nothing. No error message, it's just still there after. sudo pkill didn't work either but using SIGKILL (pkill -kill mpg123), did.

In the stop script I am using pkill (SIGTERM). I have read that SIGKILL should only be used as a last resort when nothing else works, so I am seeking alternate solutions.

Also, I'm still not sure why this causes it to do nothing when I hit "play" the first time. Since the process is still there and stopped, shouldn't the fade script still just restart it with -CONT, just like after it's paused? What does the (failed) attempt at killing the process do, that prevents it from restarting the first time but works fine the next time?

mitu

@sleve_mcdichael said in More script-dev questions. How to log errors in external scripts?:

I made the assignment of the "global-ish" vars look "prettier." I put them all in one function, with the $1 param dictating what should be returned. Then I put declaration and assignment on one line (still as two statements, with a && in between. Makes them wider and less tall, less "towering.") This seems to be working, does it break any of the rules? Definition and asignment

You can use direct assignment as-well ( local var=<value> ) and it's even shorter. Doesn't break any rules AFAIK.

If I pkill while it's "sleeping" (playing) it works fine, but if I pkill while it's "stopped," nothing. No error message, it's just still there after. sudo pkill didn't work either but using SIGKILL (pkill -kill mpg123), did.

For mpg123 related stuff, I don't have any experience with it. Maybe using a FIFO named pipe for commands (similar to how RetroArch accepts commands via the network interface) is more robust, since you're not relying on OS signals to control the instance, but send full commands instead. Look up the --fifo parameter and see if it works better.

sleve_mcdichael

@mitu said in More script-dev questions. How to log errors in external scripts?:

You can use direct assignment as-well ( local var=<value> ) and it's even shorter. Doesn't break any rules AFAIK.

local autostart="$(_get_vars_bgm123 autostart)"

...won't get me in trouble with the Style Police because it's a command substitution?

Declaration and assignment must be separate statements when the assignment value is provided by a command substitution, as the local builtin does not propagate the exit code from the command substitution.

    # DO NOT do this: $? contains the exit code of 'local', not my_func
    local my_var="$(my_func)"
    [[ $? -eq 0 ]] || return

Of course I'm not making any use of the command's exit codes so if that's the only reason to avoid this, it may not apply here. But that's the only reason I didn't do this in the first place; if people who know more than me think this is safe I would go ahead and use it.

Edit: I see this is actually used repeatedly in local choice=$("${cmd[@]}" "${options[@]}" 2>&1 >/dev/tty) in gui functions (including the one I copy/pasted from another module to build this one off of) and a handful of other uses including some that look an awful lot like what I'm doing so I'ma go ahead and go with it.

mitu

@sleve_mcdichael said in More script-dev questions. How to log errors in external scripts?:

...won't get me in trouble with the Style Police because it's a command substitution?

Arguably, since you're using "$(my_func)" instead of a hashmap (hint!). In any case, if you'd like to separate declaration from assignment, then the && doesn't make much sense, sine local is not a command that may fail

local var;  var = "$(my_func)"

sleve_mcdichael

@mitu said in More script-dev questions. How to log errors in external scripts?:

Arguably, since you're using "$(my_func)" instead of a hashmap (hint!).

Is that the same as an "associative array"?

function _get_vars_bgm123() {
    declare -A gvar=(
        [autostart]="$configdir/all/autostart.sh"
        [bashrc]="$home/.bashrc"
        [onstart]="$configdir/all/runcommand-onstart.sh"
        [onend]="$configdir/all/runcommand-onend.sh"
        [autoconf]="$configdir/all/$md_id.cfg"
        [menudir]="$datadir/retropiemenu"
        [init]="$md_inst/bgm_start.sh"
        [killscript]="$md_inst/bgm_stop.sh"
        [fadescript]="$md_inst/bgm_fade.sh"
    )
    echo "${gvar[$1]}"
}

I've just done this with no other changes and it didn't break anything. (Well it broke a lot, because I made some typos, but after I cleaned up that mess and fixed the code, it all works fine again.)

I'm still just echo-ing the value, and assigning and using them in functions with "$(my_func)" exactly as before. I experimented with just calling the "get_vars" function once and then using ${gvar[foo]} directly but this seemed to work much like local vars, in that the array doesn't seem to exist outside the function:

pi@retropie:~/temp/foo $ cat test.sh 
#!/bin/bash

function get_vars() {
    declare -A global_var=(
        [key1]="value1"
        [key2]="value2"
    )
    echo "${global_var[key1]}"
}

get_vars
echo "${global_var[key2]}"
pi@retropie:~/temp/foo $ bash test.sh 
value1

pi@retropie:~/temp/foo $

EDIT: This looks promising:

pi@retropie:~/temp/foo $ cat test.sh 
#!/bin/bash

function get_vars() {
    declare -A global_var=(
        [key1]="value1"
        [key2]="value2"
    )

    for f in ${!global_var[@]}; do
        echo "local $f=${global_var[$f]}"
    done
}

function test() {
    $(get_vars)
    echo "$key1"
    echo "$key2"
}

test
pi@retropie:~/temp/foo $ bash test.sh 
value1
value2
pi@retropie:~/temp/foo $

EDIT: https://github.com/s1eve-mcdichae1/RetroPie-Extra/blob/42cc99b75d4412ba120f860b94a202a0bd3840ff/scriptmodules/supplementary/bgm123.sh

mitu

@sleve_mcdichael Yes, this looks ok.
You may want to remove the file backup stanza in configure, since you're already doing it inside the toggle_ function when you call it later.

sleve_mcdichael

More lines again but I feel like it's cleaner, code-wise:

https://github.com/s1eve-mcdichae1/RetroPie-Extra/blob/0518184f40965a1f02d558aa548301ad987ce3ee/scriptmodules/supplementary/bgm123.sh

@mitu said in More script-dev questions. How to log errors in external scripts?:

@sleve_mcdichael Yes, this looks ok.
You may want to remove the file backup stanza in configure, since you're already doing it inside the toggle_ function when you call it later.

The idea here was to have a permanent "archival" backup of the original files, and also an immediate backup of the previous state from before it was toggled.

Mostly I just wanted to make sure I had a way out if I goofed something up. Since the toggle function does seem to reliably be adding and removing only the selected lines, though, maybe that backup isn't really necessary and I should only keep the first one in configure. That way, whatever happens to the files, one can always go back to how they were before it was ever installed. And the "toggle" backup doesn't really help much, it's exactly what you'll get if you just toggle it again anyway.