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

[mitu]

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

You keep saying not to put play/pause in the gui_ and I keep asking why not, and you keep not saying why not, and I did it anyway and it's working fine, as far as I can tell.

I think you already said why - because it's faster and it shouldn't need to load RP's existing helpers.

Otherwise, I'm writing it out twice; first I write one piece of code to do it non-interactively at install, and then another piece of, near-identical code, to do it interactively in the gui. Why? Why not just call the same function to do it at both times? Like, literally, why not? That's not rhetorical I'm asking the actual reason.

Sure, just use the same function (so vars are only needed once) and have the 1st param of the function on or off.

I do not overwrite the configuration.

That was not about overwriting the config, but about the extra checks done during configure - if there is already a config file, why do all the other checks in steps 2 / 4 ?

[sleve_mcdichael]

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

Sure, just use the same function (so vars are only needed once) and have the 1st param of the function on or off.

Okay, that's something I can work with. Enable/disable are (will be) one function, with something like if $1 == "on" then [enable code] else [disable code], and then I'll call the function with enable_bgm123 on to enable and enable_bgm123 [anything else] to disable. That saves some repetition and still allows non-interactive operation when needed.

###

if there is already a config file, why do all the other checks in steps 2 / 4 ?

I've been thinking more about this, and perhaps our disconnect stems at least partly from the fact that the "enabled" status is not actually read from or written to the user config. If this is new information to you, then I think we're up to speed; if you already understood this, then I'm unfortunately still in the dark:

To check if it's enabled, I just grep autostart for the actual code that plays the music on startup (well, I check for the #bgm123 tag that I paste on the end of that code.) If that code is there, then the music will play and if it's not, then it won't.

And then to perform the enable/disable, I just add or remove those lines. Never does the config actually come into it. So that is why I can't just write the config with enabled or disabled status and then leave that status alone on updates if the user changes it. The only way the module knows whether it is enabled or not, is if the #bgm123 code is there or not, and if it isn't there, it needs another way to determine whether that is because 1) it has never been installed before and should be added now, or 2) the user has disabled it, and it should not be added.

###

I think you already said why - because it's faster

That was only an issue briefly when gui_ just said "bash $menuscript" and menu script then called back to retropie_packages.sh to call the enable and disable functions from the module. Only then, there was a long delay while _packages.sh loaded to run the enable or disable function. I abandoned that method pretty quickly, because of it.

Both this version (menu script is completely divorced from gui_ and everything is written twice) and the current one (everything done in gui_, called with .rp file and does not use a menu script -- does all the same things but I only have to write them once) do not have that issue. Both take about the same 5-6 Mississippi's to load the GUI from ES, and both respond promptly to all choices once they do load.

If it's any faster to get in with the .sh than it is with the .rp, it's not by much. I didn't break out a stopwatch to really test it down to the second, but I didn't notice any difference. Except the first one, 93940be. That was bad. I did notice that. And I'm not doing that anymore, because.

Actually I did break out the stopwatch just now. In 48ffd1f with the .sh file it took, from button to blue screen, 6.9, 6.6, and 6.9 seconds on three trials.

In 19f7d67 with the .rp file it took 6.9, 13.5, and 7.0. That middle one threw me so I did three more and got 6.9, 6.8, 6.9. Three more: 6.9, 7.8, 7.3. Again: 8.8, 7.7, 6.8. Okay last time: 6.8, 6.8, 7.0.

So it seems like that "13.5" looks like an anomaly; maybe I selected it again too soon after exiting from the previous time or something. But ignoring that one outlier, it takes "about seven or eight seconds, " regardless (I guess my Mississippi's are a little slow since I estimated "five or six"), with maybe a slight edge to the .sh version, at the expense of duplicating all of the code for any function I want in both menus (which doesn't have to include play/pause but does have to include enable/disable and any -- or at least probably most -- additional configurations that are added in the future.)

and it shouldn't need to load RP's existing helpers.

Doesn't that already happen when ES launches the item from retropiemenu with:

<command>sudo /home/pi/RetroPie-Setup/retropie_packages.sh retropiemenu launch %ROM% &lt;/dev/tty &gt;/dev/tty</command>

...? And then function launch_retropiemenu looks at it and decides whether to rp_callModule gui or bash the item depending on whether it's been given an .rp or an .sh file to work with. But it's already in _packages.sh so if it chooses to use rp_callModule, it just has to run the helper, it's all already been loaded in that first ~7 second pause. Right?

[mitu]

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

I've been thinking more about this, and perhaps our disconnect stems at least partly from the fact that the "enabled" status is not actually read from or written to the user config. If this is new information to you, then I think we're up to speed; if you already understood this, then I'm unfortunately still in the dark:

OK, so if you're not storing the status in the config, then the external checks are needed. Wouldn't it be easier to read it from the config though ? - the enable/disable operations will go through your menu/gui, so you can easily control this.

If it's any faster to get in with the .sh than it is with the .rp, it's not by much. I didn't break out a stopwatch to really test it down to the second, but I didn't notice any difference.
...

I was thinking that for normal operation loading the RetroPie packages functions wouldn't be needed, this involves additional function executions (i.e. evaluate the system platform, read packages, find the gui_ function, etc.) that shouldn't be needed just for running Next/Prev/etc. But if you think it's not a big difference (as benchmarked), you can have them in the gui_ function. Just have in mind that on other systems (rpi0/rpi3, slower sdcard) this time may increase.

[sleve_mcdichael]

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

OK, so if you're not storing the status in the config, then the external checks are needed. Wouldn't it be easier to read it from the config though ? - the enable/disable operations will go through your menu/gui, so you can easily control this.

I could write "enabled" to the default config and then check that to determine whether or not to enable at time of configure. For the gui though, wouldn't it still make sense to read the actual status from autostart and not just whatever is recorded in the user config? So then we are tracking every change of the enabled status what, just to check it in configure once in a while. But that's not all...

...does the enabled status even matter for configure?

If it's brand new:

• it is not enabled,
• It should be enabled though,
• So we need to enable it.

If it's old, and
...the user has not disabled it:

• it should be enabled,
• It already is enabled,
• ...no action is necessary.

If it's old, and
...the user has disabled it:

• it should not be enabled,
• It already is not enabled,
• No action is necessary.

It looks to me like the only time action is needed is to enable it once in the very first install and for that, we don't actually need to read any particular setting from the user config; as long as we check for it before we place it there ourselves, it should be enough just to know if the file exists at all (no action) or not (enable first time).

I was thinking that for normal operation loading the RetroPie packages functions wouldn't be needed, this involves additional function executions (i.e. evaluate the system platform, read packages, find the gui_ function, etc.) that shouldn't be needed just for running Next/Prev/etc.

But the es_systems command for all of retropiemenu is just retropie_packages.sh launch %ROM%. Even with an .sh file I think I can't not use it, unless like I put the .sh script in ports instead (where the command is simply bash %ROM%).*

Just have in mind that on other systems (rpi0/rpi3, slower sdcard) this time may increase.

Well, that is something to consider I suppose, but also it's not meant to be a full-fledged media player (just install Kodi or something). The core function is meant to be the automatic playing, pausing, resuming, and killing of the music player in the background. The manual pause and skip track options are just "bonus content," if you will.

EDIT: *Oh...! Maybe it could be it's own whole system. Have individual .sh "roms" for every play function, that launch with bash, then just have one that calls retropie_packages.sh gui for the "edit configuration" item.

...but that's a project for a future day, I think.

[mitu]

...
It looks to me like the only time action is needed is to enable it once in the very first install and for that, we don't actually need to read any particular setting from the user config; as long as we check for it before we place it there ourselves, it should be enough just to know if the file exists at all (no action) or not (enable first time).

Yes, that's correct. On upgrades though, why do you need to re-read the configuration and invoke enable/disable ?

But the es_systems command for all of retropiemenu is just retropie_packages.sh launch %ROM%. Even with an .sh file I think I can't not use it, unless like I put the .sh script in ports instead (where the command is simply bash %ROM%).

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

[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.