Conversation
Updated version.
Code changes WRT the blinking LEDs. Rather than stopping the blinking LEDs while waiting for a reboot, we restart them with a slower blinking rate to show users some activity is still going on but not the same as before when the curl cmd was being executed.
Added last LED reset *before* the reboot call.
| #----------------------------------------------------------# | ||
| _Reset_LEDs_ ; sleep 180 | ||
| _Reset_LEDs_ ; Toggle_LEDs 3 & Toggle_LEDs_PID=$! | ||
| sleep 180 ; _Reset_LEDs_ |
There was a problem hiding this comment.
Theres a chance we never reach the last Reset_LEDs with this, if the router reboots itself automatically as expected before we tell it too at the end of our 180 second timer. Which I think for most models is the case.
There was a problem hiding this comment.
Before this PR, the last thing we did to the LEDs was before the firmware upgrade started, which was a was reset to original settings, now it's at the end of the 180 wait time, and we may never reach there.
There was a problem hiding this comment.
We could utilize this idea but instead, make it blink slow in the wait cycle while it's postponed and found and update.
So lets say, default is 7 days wait, so for those 7 days, it does a slow cycle as a way to get the users attention an update is available or upcoming. And then when the actual update starts , we cycle the LEDS quickly until we reset at the end like previously so post update the LEDs are back to how they were.
There was a problem hiding this comment.
I guess this means we would be executing a child process to run for 7 days, right now it just checks once a day to see if your past the 7 days. This adds overhead and complication I guess.
There was a problem hiding this comment.
That being said I just reviewed and noticed that we already start the LEDs as a background child process, which makes sense so the script can continue and focus on other things.
So is there really that much of an impact to have it just run in the background for that longer period of time?
There was a problem hiding this comment.
We could utilize this idea but instead, make it blink slow in the wait cycle while it's postponed and found and update.
So lets say, default is 7 days wait, so for those 7 days, it does a slow cycle as a way to get the users attention an update is available or upcoming. And then when the actual update starts , we cycle the LEDS quickly until we reset at the end like previously so post update the LEDs are back to how they were.I don't like the idea of starting a background task to blink the LEDs for days. First, we would need an easy way for people to stop the blinking LEDs, and logging into the router via SSH to call the script to stop is not simple for non-technical folks (like my in-laws), especially if the schedule checks for new F/W updates at 2am and then "suddenly" the LEDs would start blinking and they would think something is wrong with the router.
No worries, it was just an idea to throw out there. I rather just be sure the LEDs reset post reboot, that was the main concern. here.
There was a problem hiding this comment.
Wouldn't the variable for DoCleanUp usually be refreshed after the reboot and firmware update?
Lets say we rebooted after the firmware update, but that reboot happened while the LEDs were on instead of my regular off setting..
Well, post reboot the script would now take a value of the LEDs being "On" since that was the last set before the reboot, at the next boot, at this line: readonly LED_InitState="$(nvram get led_disable)" it's now 1 instead of 0 like it was.
Meaning the DoCleanUp and Reset_LEDs will now reset the LEDs to 1 instead of 0, unless we save it into the file, since every load of the script is when we take the readonly LED_InitState="$(nvram get led_disable)
The "Reset_LEDs" function sets the LEDs back to the initial state found when the script was started so if the function is called when the router reboots itself (due to the trap statement), the LEDs are back to the initial state after the reboot. But again, this assumes that the router terminates all processes with a TERM signal; if this is not what happens during F/W Updates then all bets are off.
There was a problem hiding this comment.
@Martinski4GitHub
Wouldn't the variable for DoCleanUp usually be refreshed after the reboot and firmware update?
Lets say we rebooted after the firmware update, but that reboot happened while the LEDs were on instead of my regular off setting..
Well, post reboot the script would now take a value of the LEDs being "On" since that was the last set before the reboot, at the next boot, at this line: readonly LED_InitState="$(nvram get led_disable)" it's now 1 instead of 0 like it was.
Meaning the DoCleanUp and Reset_LEDs will now reset the LEDs to 1 instead of 0, unless we save it into the file, since every load of the script is when we take the readonly LED_InitState="$(nvram get led_disable)The "Reset_LEDs" function sets the LEDs back to the initial state found when the script was started so if the function is called when the router reboots itself (due to the trap statement), the LEDs are back to the initial state after the reboot. But again, this assumes that the router terminates all processes with a TERM signal; if this is not what happens during F/W Updates then all bets are off.
I'm just not sure I understand this.
I'll need more testing and research. For now I'm holding off on merging my change until I understand better.
If the variable isn't saved and the memory is flushed between reboots I just don't understand how a variable could be passed on from one run of the script to another run of the script
There was a problem hiding this comment.
@Martinski4GitHub
Wouldn't the variable for DoCleanUp usually be refreshed after the reboot and firmware update?
Lets say we rebooted after the firmware update, but that reboot happened while the LEDs were on instead of my regular off setting..
Well, post reboot the script would now take a value of the LEDs being "On" since that was the last set before the reboot, at the next boot, at this line: readonly LED_InitState="$(nvram get led_disable)" it's now 1 instead of 0 like it was.
Meaning the DoCleanUp and Reset_LEDs will now reset the LEDs to 1 instead of 0, unless we save it into the file, since every load of the script is when we take the readonly LED_InitState="$(nvram get led_disable)The "Reset_LEDs" function sets the LEDs back to the initial state found when the script was started so if the function is called when the router reboots itself (due to the trap statement), the LEDs are back to the initial state after the reboot. But again, this assumes that the router terminates all processes with a TERM signal; if this is not what happens during F/W Updates then all bets are off.
I'm just not sure I understand this.
I'll need more testing and research. For now I'm holding off on merging my change until I understand better.
If the variable isn't saved and the memory is flushed between reboots I just don't understand how a variable could be passed on from one run of the script to another run of the script
See my comment about the LEDs state always being saved/stored in NVRAM. IOW, runtime variables are used only during execution. Between reboots and separate executions, NVRAM variables are used to read & write values that need to persist across reboots (i.e. non-volatile).
There was a problem hiding this comment.
@Martinski4GitHub
Wouldn't the variable for DoCleanUp usually be refreshed after the reboot and firmware update?
Lets say we rebooted after the firmware update, but that reboot happened while the LEDs were on instead of my regular off setting..
Well, post reboot the script would now take a value of the LEDs being "On" since that was the last set before the reboot, at the next boot, at this line: readonly LED_InitState="$(nvram get led_disable)" it's now 1 instead of 0 like it was.
Meaning the DoCleanUp and Reset_LEDs will now reset the LEDs to 1 instead of 0, unless we save it into the file, since every load of the script is when we take the readonly LED_InitState="$(nvram get led_disable)The "Reset_LEDs" function sets the LEDs back to the initial state found when the script was started so if the function is called when the router reboots itself (due to the trap statement), the LEDs are back to the initial state after the reboot. But again, this assumes that the router terminates all processes with a TERM signal; if this is not what happens during F/W Updates then all bets are off.
I'm just not sure I understand this.
I'll need more testing and research. For now I'm holding off on merging my change until I understand better.
If the variable isn't saved and the memory is flushed between reboots I just don't understand how a variable could be passed on from one run of the script to another run of the scriptSee my comment about the LEDs state always being saved/stored in NVRAM. IOW, runtime variables are used only during execution. Between reboots and separate executions, NVRAM variables are used to read & write values that need to persist across reboots (i.e. non-volatile).
I understand better, this was an older comment from an hour ago.
I think I brought the conversation here:
| return 1 | ||
| fi | ||
|
|
||
| if [ $# -eq 0 ] || [ -z "$1" ] || \ |
|
Merged in the changes, thanks for catching the trap for the cleanup, smart to have it outside the versioning check. As for the LED blink rate idea, I think it's really cool! However; there's a chance we never reach the last: Reset_LEDs just due to the fact that most of the time once the firmware upgrade is complete it reboots itself, and usually before our 180 timer is done. Means I'd have that behavior's again when it's kinda luck/random after the upgrade if your LEDs are on or off. Before, the last thing we did to the LEDs before the firmware upgrade started was reset, now it's at the end of the 180 wait time, and we may never reach there. |
See my comment above regarding the trap statement in the script calling "DoCleanUp" function which calls "Reset_LEDs" function. Having said that, I'm not against saving the initial LED state in the configuration file to make sure the LEDs are set properly when the "post-reboot" is called. |
Check this comment: does this make sense or not really?: #50 (comment) |
The trap statement working is predicated on the assumption that the reboot terminates all processes with the TERM signal which by definition gives each process a chance to exit "gracefully" by doing whatever cleanup is needed (which includes any trap statements). Now, I know this works under "normal" reboots called via "/sbin/service reboot" command; however, if this is not the same mechanism used during the F/W Updates then yes, there would be a problem with the initial LEDs state being potentially different after each reboot initiated by the router right after F/W updates. |
Is the trap a background process? If I'm understanding correctly. What your saying is we launch this trap, the script moves on, this trap is still active though, and it's waiting for a TERM signal from the router, and if it receives one than it does the cleanup and resets the LEDS before it completely shuts down? I think that's the part that confuses me. I didn't understand the trap as a background process that remained active as the script moves on. If it wasn't a background process, then it's too high up in the flash function to actually catch the TERM signal initiated below firmware update/curl. |
The trap statement is not a background function. It's essentially a mechanism that sets "signal traps" in the intertask communications interface so that, for example, when you type "Ctrl-C" on your keyboard while a process is running in the foreground, the SIGINT signal is sent and caught by the trap routine to do whatever "cleanup" is supposed to do before exiting. If no "signal traps" are set by the running process, the signal is still detected but not acted upon and the process just terminates. IOW, the trap cmd does not actually execute the arguments within the statement when called in the script, it simply sets up the "signal traps" to react & catch them when any of the signals specified in the statement is caught via the intertask communication interface. |
2 participants
Code changes WRT the blinking LEDs. Instead of stopping the LEDs while waiting for a reboot, we restart the LEDs with a slower blinking rate to let users know that some activity is still going on but not the same as before when executing the curl command.