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?:

enable/disable can be moved to the _gui function

I wanted to have these available from the RPMenu item, without having to wade through RPSetup > configuration/tools to get there.

I, also, do want them available in RPSetup, though, if that's where we happen to be at, but I don't want to have to go in there as the only way to do it.

Play controls don't need to be in RPSetup, but they also don't need to not be in there, either, I think.

Was your initial suggestion, about using a separate menu script for play/pause, to mean "don't have playback controls in the GUI function," or "don't call the GUI function for RPMenu"? In either case, can you explain why? I have resolved the delay on enable/disable and also no longer need the sudo su sudo parts, by completely divorcing the GUI_ function and RPMenu items and just writing everything twice, but I'm still not sure what the benefit of it all is, and I'm about ready to just say "hang it" and go back to using the one GUI for everything, unless there's good reason not to.

and you don't need to have separate functions for your global variables.

I'm not sure what you mean here, could you give an example? As I understand it, I can't just put:

function _global_vars_bgm123() {
    local autostart="$configdir/all/autostart.sh"
    local bashrc="$home/.bashrc"
   (etc.)
}

...because the "local" values won't propagate to the parent function that calls them, right? Or do I misunderstand the whole point of local?

Can I just make them global vars, (without local, but still inside a function), and then call that function once in every other function that needs them? Or, that's going to pollute the whole "shell space" if I do, and that's why we use local in the first place?

use moveConfigFile to copy the configuration files, this preserves the user configuration and sets the correct permissions on the file. Don't overwrite existing config files.

I've already considered/begun to implement this for a different config file I am considering (...or, wait. moveConfigFile makes a symlink, just like moveConfigDir. Did you mean copyDefaultConfig which makes the .rp-dist version when the target file already exists? That's the one I meant to use for this other (actual "user config") file I'm considering), that might allow for customized settings in the future. But this config file (you're referring to the $configfile for the menu script right?) is not a "user config", it only has the variables' definitions in it, and if they have changed since last it was written, they need to be updated/overwritten with current values, or the menu that relies on them won't work.

If that's not what you mean, can you explain? I'm not seeing any other files that are problematically overwritten. Fade and menu scripts are copied to the install directory, which has either just been wiped clean if we have installed before, or has just been created anew if not. The menu icon does overwrite, but I wouldn't think that's an issue? The initial backup is skipped if the $file.old.$md_id versions exist already, and the immediate backups are meant to be overwritten (only the most-recent is preserved.) What did I miss?

use configure_ to install/remove the configuration, not install_, install_ should be used mainly for providing the installed files.

Yeah, I could see a lot of this going in configure.

don't re-enable the script on each configure, if the user disables the script, it will be re-enabled on an update.

I've implemented a test so that it's enabled automatically on first install, and only then. I create the user config file with installed="0"; then at configure_, it's enabled only if it's still 0, then sets the value to 1 so it won't be re-enabled on future updates. Is that a good way to handle this? I do want it enabled somewhere, automatically after install, so they don't have to do it manually the first time, but if configure is called on updates then you're right, we wouldn't want it to re-enable every time if they have already manually disabled it.

Here's where I am at after this morning. It can still be improved, I am sure, but I think everything "works." You can even choose the fade profile by setting "mapped_volume" or not in the config (no GUI option to set it, yet, you still have to edit the file manually at this time) to use either a linear (mapped) scale in 20 steps, or curved (natural) scale with step-size based on current position. Here is the module script:

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

...and the supplementary files are in:

https://github.com/s1eve-mcdichae1/RetroPie-Extra/tree/48ffd1f3209aecfdacd38f415d0ef6fb90c138f6/scriptmodules/supplementary/bgm123/

mitu

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

I, also, do want them available in RPSetup, though, if that's where we happen to be at, but I don't want to have to go in there as the only way to do it.

The thing is, if you defined your gui_ function then you have both options working automatically. If you create a module.rp file in retropiemenu, RetroPie will call the gui_module function for that menu entry.

.. I'm not sure what you mean here, could you give an example? ..

I meant that you're only using the "global" vars in your "enable/disable" functions, so if you have just one function for enable/disable then you can do

local globalish_var
...
# show a select dialog for enable/disable
cmd=(dialog --backtitle "$__backtitle" --cancel-label "Exit" --default-item "$default" --menu "Choose an option." 22 86 16)
        options=()
# add options and show choice
local choice=$("${cmd[@]}" "${options[@]}" 2>&1 >/dev/tty)
        [[ -z "$choice" ]] && break
        default="$choice"
        case "$choice" in
            1)
                # run disable code, using $globalish_var
                ;;
            2)
               # run enable code, using $globalish_var
                ;;

moveConfigFile makes a symlink, just like moveConfigDir. Did you mean copyDefaultConfig ..

Yes, I meant copyDefaultConfig.

I've implemented a test so that it's enabled automatically on first install, and only then. I create the user config file with installed="0"; then at configure_, it's enabled only if it's still 0, then sets the value to 1 so it won't be re-enabled on future updates. Is that a good way to handle this? I do want it enabled somewhere, automatically after install, so they don't have to do it manually the first time, but if configure is called on updates then you're right, we wouldn't want it to re-enable every time if they have already manually disabled it.

When you create the default config, it should be created with the enabled settings. Subsequent configure will generate the same default config, but don't overwrite the user config file (this is where copyDefaultConfig comes into play). This way, if the user has disabled the functionality, updating will not change their choice.

sleve_mcdichael

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

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

I, also, do want them available in RPSetup, though, if that's where we happen to be at, but I don't want to have to go in there as the only way to do it.

The thing is, if you defined your gui_ function then you have both options working automatically. If you create a module.rp file in retropiemenu, RetroPie will call the gui_module function for that menu entry.

That's what I'd done initially. That's what I thought you were suggesting I change.

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

Btw, I wouldn't use the _gui function for regular operation process (Play/Next), they should have a separate UI that doesn't involve calling RetroPie-Setup.

So if I use an .rp file in the menu, to call the gui function, but play/pause aren't in the gui function, then where are they, and how do I get there? Do you mean you'd have both an .rp (to run the gui function with configuration options) and an .sh with a separate UI for the play controls? I don't really want two separate menu options, though. So how else? An option in the gui function that calls the external playback UI? I don't see a benefit in that either.

Sorry for being dense. I'm really not trying to be combative, I'm just struggling to understand what you are actually suggesting, and why.

.. I'm not sure what you mean here, could you give an example? ..

I meant that you're only using the "global" vars in your "enable/disable" functions,

Well, they're also in configure (or install or wherever we end up making that first initial backup. Right now that's in configure.)

so if you have just one function for enable/disable then you can do

local globalish_var
...
# show a select dialog for enable/disable
cmd=(dialog --backtitle "$__backtitle" --cancel-label "Exit" --default-item "$default" --menu "Choose an option." 22 86 16)
        options=()
# add options and show choice
local choice=$("${cmd[@]}" "${options[@]}" 2>&1 >/dev/tty)
        [[ -z "$choice" ]] && break
        default="$choice"
        case "$choice" in
            1)
                # run disable code, using $globalish_var
                ;;
            2)
               # run enable code, using $globalish_var
                ;;

...then there's no enable function that can be called to enable the music on first install. They'll need to manually enable it after installing, or else we'll need to duplicate the "enable" code somewhere else (in configure, probably) to do that.

This way, if the user has disabled the functionality, updating will not change their choice.

How I have now does that, too:

1: initial config is created, with "installed" (as in, "has it been installed already once before?") set to 0 ("false.")
2: configure checks the config, sees that it has not been installed before, runs enable. Sets "installed" to 1 ("true.")
3: user disables music.
4: user runs update or reinstall. Config already exists, default with installed 0 is created at .rp-dist; working file with installed 1 is retained; configure checks the working file, sees that it has been installed before, does not re-enable.
∴: if the user has disabled the functionality, updating will not change this.

mitu

So if I use an .rp file in the menu, to call the gui function, but play/pause aren't in the gui function, then where are they, and how do I get there? Do you mean you'd have both an .rp (to run the gui function with configuration options) and an .sh with a separate UI for the play controls? I don't really want two separate menu options, though. So how else? An option in the gui function that calls the external playback UI? I don't see a benefit in that either.

Then leave the configuration (gui_) just in RetroPie-Setup and in the RetroPie menu just have a .sh script for normal operations (play/next/pause/stop/shuffle/etc.) that doesn't need any of the RetroPie-Setup functions.

...then there's no enable function that can be called to enable the music on first install.

Installation is a non-interactive operation, so there's no user input at this stage. If the user installs the package, then just assume they want it working and enable it - why require an extra step after installation ?

1: initial config is created, with "installed" (as in, "has it been installed already once before?") set to 0 ("false.")
2: configure checks the config, sees that it has not been installed before, runs enable. Sets "installed" to 1 ("true.")
3: user disables music.
4: user runs update or reinstall. Config already exists, default with installed 0 is created at .rp-dist; working file with installed 1 is retained; configure checks the working file, sees that it has been installed before, does not re-enable.
∴: if the user has disabled the functionality, updating will not change this.

I don't see why any checks would be necessary if you don't overwrite the user's configuration file. Configure just installs a default configuration file and that's it - just call copyDefaultConfig and no checks are necessary.

sleve_mcdichael

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

Then leave the configuration (gui_) just in RetroPie-Setup and in the RetroPie menu just have a .sh script for normal operations (play/next/pause/stop/shuffle/etc.) that doesn't need any of the RetroPie-Setup functions.

So, we're back at where we started. I want the play/pause/next functions and the setup options (enable/disable, there's already a mapped volume setting, I hope to make others) all available from the same place. You don't want to go in one menu (gui_ using RPSetup) to select your playlist, and then have to back out of that and enter the BGM UI menu to play it.

At least, I don't. I want to go in one menu, from the ES RPMenu (as its own menu item, with the icon, without going into RPSetup first) and do it all in one place. Enable the music, start the music, stop the music, change the settings.

But, if I happen to be at a terminal, instead of a gamepad, when I decide to change my settings, I do still want the option to go into RPSetup if I want to and do it all from there, as well. I don't want to have to use RPSetup, but I want to be able to.

So, I'm either doing the same thing twice by duplicating it all in RPSetup and RPMenu, or I'm just using the same gui_ for everything.

I'm leaning heavily towards "everything."

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.

Installation is a non-interactive operation, so there's no user input at this stage. If the user installs the package, then just assume they want it working and enable it - why require an extra step after installation ?

(The "extra step" is performed automatically, after all. You're not under the impression that the users need take any action besides install it, drop some songs in the network share, and restart, are you?)

So that I can use the same piece of code two times, to enable it both automatically on initial install, and then again later when the user enables it manually after previously disabling.

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.

I suppose, if I wrack my brain for an upside, I guess if we put the "enable" code once in ~~configure~~* to do it automatically, and then again in gui_ for manual operation, it would only need two blocks of the "local autostart, autostart=, local bashrc, bashrc=, etc." instead of three (configure needs it, already has it; enable and disable both need it, or it could be done once in gui, if they were both rolled into there). But that's just trading one duplicate (the enable code itself) for another (the global vars.) The enable code is half as many lines as the variables block, but it's more than twice the bytes, so I'm not sure if that's a win or not...

*(actually it would be in install, since we can't do it in configure, in case it's not the first run , and you don't want to check the .cfg to find out. So now we need the global vars in there again too...if we did the backup and the enable in the install function, then the vars could come back out of configure and we're back at three uses of them.)

I don't see why any checks would be necessary if you don't overwrite the user's configuration file. Configure just installs a default configuration file and that's it - just call copyDefaultConfig and no checks are necessary.

I do not overwrite the configuration.

pi@retropie:/opt/retropie/configs/all $ cat bgm123.cfg
# Configuration file for bgm123

installed="1"
mapped_volume="1"
music_player="mpg123"
pi@retropie:/opt/retropie/configs/all $ cat bgm123.cfg.rp-dist 
# Configuration file for bgm123

installed="0"
music_player="mpg123"
mapped_volume="1"

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.