Update MerlinAU.sh

[Martinski4GitHub]

Decided to stop toggling the LEDs during the F/W flash to avoid modifying NVRAM during the actual flash process.

[Martinski4GitHub]

@ExtremeFiretop,
OK, take a look at the changes. The LEDs are now reset & stopped before the F/W flash is about to start and added a "sleep counter" function to provide some feedback to the user during interactive sessions.

[ExtremeFiretop]

Reviewing now. Would be nice to get this out before Wednesday

[ExtremeFiretop]

How does your sleep function deal with an interactive session that is closed?

Does it quit itself? And instantly go to the reboot step?

Not sure of the value since when I flashed I didn't even get to see the count down it instantly kicked me before it even started as seen below:

Also now I can't accept my jffs upon reboot lol:

[ExtremeFiretop]

Yeah I can't access any internal memory, woot, I'd say this is worse than the last one so far :P

[ExtremeFiretop]

Je suis stuck just trying to browse to it:

[ExtremeFiretop]

Woot, factory reset time!

[ExtremeFiretop]

This issue above might be related to the unmount function because this is my first flash test since we added it.

[ExtremeFiretop]

I'm back, I'll test once more with a regular sleep and the unmount still there to narrow it down further.

[ExtremeFiretop]

Just had a successful upgrade using the regular sleep 180. Zero issues identified with option 3.

So may actually be unrelated to the unmount and more related to this new function, will add it back once more and try again.

[ExtremeFiretop]

Second in a row successful with the regular sleep 180 so unlikely related to the unmount.

[ExtremeFiretop]

Okay added the timer back and had another successful upgrade. Shrug

Maybe we drop all the fancy functions after the flash, stick with a regular sleep, and maybe even the unmount should be changed for the back-pocket solution?

Not sure.

[ExtremeFiretop]

Confirmed your unmount USB function isn't working correctly for me.

[ExtremeFiretop]

Just ran the function and checked in the WebUI and my USB is still mounted.

[Martinski4GitHub]

How does your sleep function deal with an interactive session that is closed?

Does it quit itself? And instantly go to the reboot step?

If the router reboots itself as the last step of the built-in flash process (i.e. during the curl command), all processes are terminated as usual, including the script and any child threads, the same as if it were a regular sleep call.

Not sure of the value since when I flashed I didn't even get to see the count down it instantly kicked me before it even started as seen below:

The sleep countdown would be visible only if the router did not reboot itself during the flash process. IOW, if the curl command returns and, for some reason, the router has not rebooted itself (perhaps because the flash was unsuccessful) the 180-sec. countdown begins and then the script forces a reboot. This is exactly the same as before, the only difference is that now the LEDs are not flashing and instead there's the sleep countdown feedback if/when the session is interactive.

[ExtremeFiretop]

And in comparison just ran: ejusb -1 0 -u 1 and it's gone.

[ExtremeFiretop]

1. Fix the unmount USB function.
2. Return the post flash sleep to a regular sleep

Then this can be merged, function can be kept for the WebUI, etc.
But user gets no feedback from the function interactively for the flash and it seems to add a lot of code/complication (and risk of error) for a simple sleep 180.

[ExtremeFiretop]

Didn't mean to close it, meant to edit a comment lol!

[Martinski4GitHub]

Okay added the timer back and had another successful upgrade. Shrug

Maybe we drop all the fancy functions after the flash, stick with a regular sleep, and maybe even the unmount should be changed for the back-pocket solution?

The "sleep countdown" function uses regular sleep calls except in increments of 1 second per call to allow for the feedback message. It's a pretty simple loop actually.

WRT the "unmount" function, it may fail to unmount to USB drive(s) if some process is still accessing the drive(s) at the time, even if just temporarily.

[Martinski4GitHub]

Confirmed your unmount USB function isn't working correctly for me.

The function worked when I tested it with my router (outside the script), and then when I tested it once it was integrated. But yes, there's no guarantee that it will work 100% of the time. Only the OS itself has the power to terminate all processes that might be accessing the USB drives and unmount them.

[ExtremeFiretop]

The "sleep countdown" function uses regular sleep calls except in increments of 1 second per call to allow for the feedback message. It's a pretty simple loop actually.

Interactive Check: The script first checks if it's running in an interactive mode with the variable $isInteractive. If the script is not in interactive mode, it simply performs a sleep for the specified duration and then returns.

Closing an Interactive Session: If the script function is running in an interactive mode (e.g., a terminal window) and the session is closed (like closing the terminal or disconnecting from an SSH session), the behavior will depend on how the shell handles background processes and the SIGHUP signal. Typically, when a terminal session is closed, the SIGHUP (hang-up) signal is sent to the foreground process group of the terminal. This often results in the termination of the script unless it is specifically designed to handle or ignore this signal.

Handling of SIGHUP by the Parent Script: If the larger script has a signal handler for SIGHUP or is set to ignore this signal, then the behavior upon closing the interactive session could be different. The script may continue running in the background, but as mentioned earlier, without a terminal, it won't display the timer.

Background Execution of the Parent Script: If the entire script is run in the background (using tools like nohup, screen, or tmux), closing the terminal might not affect it. However, the functionality of SleepCounter that depends on terminal output will still be moot as the standard output would not be visible.

Function Continuation Post-Interruption: If the function needs to continue functioning after an interruption (like closing an interactive session), it would be beneficial to include error handling and state management. This ensures that the script can resume or safely terminate its operations.

Confirmed your unmount USB function isn't working correctly for me.

The function worked when I tested it with my router (outside the script), and then when I tested it once it was integrated. But yes, there's no guarantee that it will work 100% of the time. Only the OS itself has the power to terminate all processes that might be accessing the USB drives and unmount them.

I've tried twice now and both times it did not function, and I feel the need to mention I have nothing on this USB, it's cleanly formatted, I doubt anything is keeping it busy, the function did not work for me but the commands did. Let alone when I have swap files, etc. Considering the command did we either fix the function or replace it with something stronger like the command mentioned.

I think we need to focus on safety and keep things simple where possible around the curl as mentioned in the code here:

" # IMPORTANT: Due to the nature of 'nohup' and the specific behavior of this 'curl' request,
# the following 'curl' command MUST always be the last step in this block.
# Do NOT insert any operations after it! (unless you understand the implications).
"

The simpler the better, this function can stay for the WebUI, but lets simplify stuff around the curl for safety

[Martinski4GitHub]

1. Fix the unmount USB function.

2. Return the post flash sleep to a regular sleep

I can guarantee that the "sleep countdown" function had no bearing on the issues you experienced. Your screenshot shows that it didn't even run because the router rebooted itself. You're blaming the wrong piece of code.

Then this can be merged, function can be kept for the WebUI, etc. But user gets no feedback from the function interactively for the flash and it seems to add a lot of code/complication (and risk of error) for a simple sleep 180.

We'll have to agree to disagree on this. While it seems to be a lot of code (mostly because of the input validation), the core is pretty simple actually. And again, you seem to be putting the blame on the wrong function for the issues you had. Don't confuse correlation with causation.

[ExtremeFiretop]

Okay but your disagreeing to fight to keep something that has no value.

Sleep 180 does the job the same as your function does.

In all 4 of my flashs not once did your countdown show, even when it was successful, so whether or not it was the cause, it should and can be simplified. And that's the route we should be sticking too, else safety is not the obvious concern.

[Martinski4GitHub]

Okay but your disagreeing to fight to keep something that has no value.

Ah, don't misunderstand my objection. I'm not fighting to keep it, I'm trying to clarify two things:

1. The function has no bearing on the problems you had. Guaranteed.

2. The function may appear "complex" but it's actually not. And if/when it gets called, as a child thread, the function terminates when the script is terminated. This is the normal termination of parent & child processes; the same that happens for a regular sleep call.

Sleep 180 does the job the same as your function does.

In all 4 of my flashs not once did your countdown show, even when it was successful,

In such situations, it's not supposed to and that's by design, as explained earlier. The function would run only if the router did not reboot itself (like when in normal WebGUI F/W updates, the router sometimes explicitly prompts the user to do a reboot).

so whether or not it was the cause, it should and can be simplified. And that's the route we should be sticking too, else safety is not the obvious concern.

I don't see it as a safety issue because the function does not even run UNLESS the curl command has returned and no reboot has taken place. IOW, by the time the function has a chance to run, the curl command has already completed execution and there's no automatic reboot for some reason, in which case we decided to wait 3 minutes and then force a reboot.

[ExtremeFiretop]

Okay but your disagreeing to fight to keep something that has no value.

Ah, don't misunderstand my objection. I'm not fighting to keep it, I'm trying to clarify two things:

1. The function has no bearing on the problems you had. Guaranteed.
2. The function may appear "complex" but it's actually not. And if/when it gets called, as a child thread, the function terminates when the script is terminated. This is the normal termination of parent & child processes; the same that happens for a regular sleep call.

Sleep 180 does the job the same as your function does.
In all 4 of my flashs not once did your countdown show, even when it was successful,

In such situations, it's not supposed to and that's by design, as explained earlier. The function would run only if the router did not reboot itself (like when in normal WebGUI F/W updates, the router sometimes explicitly prompts the user to do a reboot).

so whether or not it was the cause, it should and can be simplified. And that's the route we should be sticking too, else safety is not the obvious concern.

I don't see it as a safety issue because the function does not even run UNLESS the curl command has returned and no reboot has taken place. IOW, by the time the function has a chance to run, the curl command has already completed execution and there's no automatic reboot for some reason, in which case we decided to wait 3 minutes and then force a reboot.

The router curl returns instantly, the putty season closes, and the reboot happens sometime later, which means by design it will never show, even if the curl fails, and you hit your 3 minute sleep count down your putty season has already ended either way.

[ExtremeFiretop]

Okay but your disagreeing to fight to keep something that has no value.

Ah, don't misunderstand my objection. I'm not fighting to keep it, I'm trying to clarify two things:

1. The function has no bearing on the problems you had. Guaranteed.
2. The function may appear "complex" but it's actually not. And if/when it gets called, as a child thread, the function terminates when the script is terminated. This is the normal termination of parent & child processes; the same that happens for a regular sleep call.

Sleep 180 does the job the same as your function does.
In all 4 of my flashs not once did your countdown show, even when it was successful,

In such situations, it's not supposed to and that's by design, as explained earlier. The function would run only if the router did not reboot itself (like when in normal WebGUI F/W updates, the router sometimes explicitly prompts the user to do a reboot).

so whether or not it was the cause, it should and can be simplified. And that's the route we should be sticking too, else safety is not the obvious concern.

I don't see it as a safety issue because the function does not even run UNLESS the curl command has returned and no reboot has taken place. IOW, by the time the function has a chance to run, the curl command has already completed execution and there's no automatic reboot for some reason, in which case we decided to wait 3 minutes and then force a reboot.

The router curl returns instantly, the putty season closes, and the reboot happens sometime later, which means by design it will never show, even if the curl fails, and you hit your 3 minute sleep count down your putty season has already ended either way.

So, let's stick with the simpler version. Wether it was the cause or not

[Martinski4GitHub]

I don't see it as a safety issue because the function does not even run UNLESS the curl command has returned and no reboot has taken place. IOW, by the time the function has a chance to run, the curl command has already completed execution and there's no automatic reboot for some reason, in which case we decided to wait 3 minutes and then force a reboot.

The router curl returns instantly, the putty season closes, and the reboot happens sometime later, which means by design it will never show, even if the curl fails, and you hit your 3 minute sleep count down your putty season has already ended either way.

Most of the time the "sleep 180" or the function (whichever is in place) would not run at all when the flash is successful AND the reboot occurs automatically. That's the expected behavior. However, when doing a normal webGUI update, there are times (and I don't know exactly the reasons for it) when the router prompts the user to manually reboot & waits for it. That's the scenario that we're trying to handle, so the rest of the code (after the curl command) was meant to wait 3 minutes and force the reboot. Again, this does not happen often. IIRC, I have seen this prompt for manual reboot about 6-8 times (certainly less than 10) in the 12+ years I've used ASUS routers. So yeah, the probability of that happening is small, but it's not zero.

[ExtremeFiretop]

I don't see it as a safety issue because the function does not even run UNLESS the curl command has returned and no reboot has taken place. IOW, by the time the function has a chance to run, the curl command has already completed execution and there's no automatic reboot for some reason, in which case we decided to wait 3 minutes and then force a reboot.

The router curl returns instantly, the putty season closes, and the reboot happens sometime later, which means by design it will never show, even if the curl fails, and you hit your 3 minute sleep count down your putty season has already ended either way.

Most of the time the "sleep 180" or the function (whichever is in place) would not run at all when the flash is successful AND the reboot occurs automatically. That's the expected behavior. However, when doing a normal webGUI update, there are times (and I don't know exactly the reasons for it) when the router prompts the user to manually reboot & waits for it. That's the scenario that we're trying to handle, so the rest of the code (after the curl command) was meant to wait 3 minutes and force the reboot. Again, this does not happen often. IIRC, I have seen this prompt for manual reboot about 6-8 times (certainly less than 10) in the 12+ years I've used ASUS routers. So yeah, the probability of that happening is small, but it's not zero.

I understand all that, I'm saying in those scenarios you just described where your waiting for the manual reboot, if you did the flash interactively with the scripts your putty season has closed anyways once the curl had returned. Which means no matter if your waiting the sleep or not because it didn't auto-reboot, the sleep function you wrote will -never- be displayed. Even in the small chance cenarios you describe.

Due to that, I requested formally as a design choice you return the original sleep method. Call it simplier for outsides to understand and review. Call it simpler for the script itself. Call it safer, because it's all true.

Think of a car, the simpler cars have less moving parts and simpler parts and last longer.

I rather less moving parts after the curl, it's a design choice. It's simpler, it's easier to read and and understand for others reviewing the code, it's cleaner less bulky, etc.

The question is not about how simple the function is, you can convince me it's a simple function, I can read it. But it's not really up to debate if "sleep 180"

Is simpler than:

##-------------------------------------##
## Added by Martinski W. [2024-Jan-30] ##
##-------------------------------------##
_SleepCounter_()
{
   if [ $# -lt 1 ] || [ -z "$1" ] || \
      ! echo "$1" | grep -qE "^[1-9][0-9]*[sm]?$"
   then return 1 ; fi

   if ! "$isInteractive"
   then sleep $1 ; return 0 ; fi

   local counter  numStr="$1"
   local maxCounterSecs  numCounterSecs

   if [ $# -lt 2 ] || [ -z "$2" ] || \
      ! echo "$2" | grep -qE "^(up|down)$"
   then counter=up
   else counter="$2"
   fi

   case "$numStr" in
       [1-9]s|[1-9][0-9]*s)
           numStr="${numStr%%s*}"
           ;;
       [1-9]m|[1-9][0-9]*m)
           numStr="${numStr%%m*}"
           numStr="$((numStr * 60))"
           ;;
   esac

   case "$counter" in
         up) numCounterSecs=0 ; maxCounterSecs="$numStr" ;;
       down) maxCounterSecs=0 ; numCounterSecs="$numStr" ;;
   esac

   while true
   do
       numLen="${#numCounterSecs}"
       printf "\r\033[0K[%*d sec. ]" "$((numLen + 1))" "$numCounterSecs"
       if [ "$counter" = "down" ]
       then [ "$((numCounterSecs--))" -le 0 ] && break
       else [ "$((numCounterSecs++))" -ge "$maxCounterSecs" ] && break
       fi
       sleep 1
   done
   echo
}

More moving parts, let's go with less moving parts.

[ExtremeFiretop]

I want my car to last long long time 😉

[Martinski4GitHub]

The router curl returns instantly, the putty season closes, and the reboot happens sometime later, which means by design it will never show, even if the curl fails, and you hit your 3 minute sleep count down your putty season has already ended either way.

So, let's stick with the simpler version. Wether it was the cause or not

Ultimately, I'll leave it up to you.

[ExtremeFiretop]

The router curl returns instantly, the putty season closes, and the reboot happens sometime later, which means by design it will never show, even if the curl fails, and you hit your 3 minute sleep count down your putty season has already ended either way.

So, let's stick with the simpler version. Wether it was the cause or not

Ultimately, I'll leave it up to you.

I don't often ask for changes in your side, you know i think you pump out good work, its why im not asking you remove the full function, but I'm this case I'm pretty firm I rather the simpler solution. (After the curl)

Asides from that it looks good 😊👍

[Martinski4GitHub]

More moving parts, let's go with less moving parts.

I understand the analogy you're trying to make, but that applies mostly to mechanical & electrical/electronic moving parts. In S/W we have a different situation. For example, we have what is called "cyclomatic complexity" which is how we measure the complexity (i.e "the moving parts") of any piece of code, and it's not based at all on the number of lines (that's the superficial, 30-thousand-feet view). I won't go into further details as in beyond the scope here, but you can google the concept if you're interested to know more. The point is that we're looking at the same situation from 2 different perspectives and vastly different experiences, which is why I said: "We'll have to agree to disagree on this." It was not meant to blow you off and ignore your opinion. It was an acknowledgment that we're coming at this with distinct points of view and different frames of reference, which will invariably influence the technical decisions we make.

[ExtremeFiretop]

More moving parts, let's go with less moving parts.

I understand the analogy you're trying to make, but that applies mostly to mechanical & electrical/electronic moving parts. In S/W we have a different situation. For example, we have what is called "cyclomatic complexity" which is how we measure the complexity (i.e "the moving parts") of any piece of code, and it's not based at all on the number of lines (that's the superficial, 30-thousand-feet view). I won't go into further details as in beyond the scope here, but you can google the concept if you're interested to know more. The point is that we're looking at the same situation from 2 different perspectives and vastly different experiences, which is why I said: "We'll have to agree to disagree on this." It was not meant to blow you off and ignore your opinion. It was an acknowledgment that we're coming at this with distinct points of view and different frames of reference, which will invariably influence the technical decisions we make.

At the end of the day, we put a sleep in a wrapper, it's a nice wrapper that is handy elsewhere in the script! But it doesn't come in handy behind the curl, so my technically opinion is the wrapper behind the curl is unnecessary.

I understand your not trying to blow off my opinion, but in this instance my opinion is that if it's unnecessary at such a critical step, let's remove it.

Think about all the routers out there that will run this, do you want your wrapper with your name on it to be the last step? Or just a sleep?

This if anything this is a move to protect you. I hope you see it that way.

[ExtremeFiretop]

Phone is about to die. Hope you didn't take anything I said the wrong way or personally, I'll check this again once my phone gets a charge.

Thanks again @Martinski4GitHub for the discussion. Always appreciated :)

[Martinski4GitHub]

More moving parts, let's go with less moving parts.

I understand the analogy you're trying to make, but that applies mostly to mechanical & electrical/electronic moving parts. In S/W we have a different situation. For example, we have what is called "cyclomatic complexity" which is how we measure the complexity (i.e "the moving parts") of any piece of code, and it's not based at all on the number of lines (that's the superficial, 30-thousand-feet view). I won't go into further details as in beyond the scope here, but you can google the concept if you're interested to know more. The point is that we're looking at the same situation from 2 different perspectives and vastly different experiences, which is why I said: "We'll have to agree to disagree on this." It was not meant to blow you off and ignore your opinion. It was an acknowledgment that we're coming at this with distinct points of view and different frames of reference, which will invariably influence the technical decisions we make.

At the end of the day, we put a sleep in a wrapper, it's a nice wrapper that is handy elsewhere in the script! But it doesn't come in handy behind the curl, so my technically opinion is the wrapper behind the curl is unnecessary.

I understand your not trying to blow off my opinion, but in this instance my opinion is that if it's unnecessary at such a critical step, let's remove it.

Oh yeah, that's fine. Again, I'm not fighting to keep it; don't misunderstand me on this. I can actually remove it without a second thought. It's not like I'm married to it :>). I'm just trying to clarify what the function is and what it's not in technical terms, and how I view its apparent "complexity" based on my frame of reference.

Think about all the routers out there that will run this, do you want your wrapper with your name on it to be the last step? Or just a sleep?

This if anything this is a move to protect you. I hope you see it that way.

I'm not worried about that, but thanks for the thought.

[Martinski4GitHub]

@ExtremeFiretop,
To clear any lingering doubts, I've removed the function so there's no confusion at all about me fighting to keep it. It was never about that. So let's move on and forward.

[ExtremeFiretop]

@ExtremeFiretop, To clear any lingering doubts, I've removed the function so there's no confusion at all about me fighting to keep it. It was never about that. So let's move on and forward.

Agreed.

I wasn't trying to be hard headed . Just wanted a the bare minimum line of code in that specific spot, that's all :)
There was no need to remove the entire function, but I can understand why you did.

(Considering it was probably build mostly for that spot in the first place)
Let's move forwards and deal with bigger things, a sleep is really not worth 40 pages of discussion and our valuable time ;)

[ExtremeFiretop]

Also thank you for addressing the LED reset in this! :)
It's what I was looking forwards too in this PR!