-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvements & Fixes #368
Improvements & Fixes #368
Conversation
- Made improvements in the code that builds the cron schedule string for the automatic script updates to make it more robust. - Made improvements in the cron schedule validation code & minor modifications to the error messages generated when validation fails. - Fixed bugs when trying to determine if we have a "386" build version in order to select the corresponding changelog URL. - Various code improvements & fine-tuning.
Whenever you get a chance, please review this PR. No major changes, but several minor changes and one moderate change were made, along with a couple of fixes. Your 2nd pair of eyes is always welcome to double-check :>). BTW, the initial report from the Linter tool found 3 new warnings - nothing major. After making the changes this evening, I re-ran the latest script and it came out "clean" as a whistle. :>) Have a good night, bud. |
Good to go! |
Well I just give everything a quick test and it seems all is working! |
I have nothing else to add for this release, so you can issue the production version at any time you're ready. BTW, earlier this afternoon I ordered from Amazon three new RT-AX86U-PRO models that were on sale for "Black Friday" ($179.99 USD). One for my parents, the other for my in-laws (currently both households have the RT-AC68U), and another one for my home. When I talked with both my parents & in-laws yesterday during our Thanksgiving celebration, they told me to look for a replacement <= $200, and the PRO model was just in the right price range so I decided to also get one for myself to give it a try. It looks like a really good replacement for the AC models we have and for the right price. We'll see how it goes... |
Fantastic, I actually have a bit of time this evening so I may have a quick look at your recommendation about the changelog logic. I know the fix we applied worked but I'm going to look through past PRs to see how it developed into the solution we currently have (many fixes over time) and get a sense of there's any reason we couldn't adjust the logic. If I find anything or decide it's not a quick thing ill just issue a production releases as is.
Fantastic choice! It officially is getting the 3006 updates, and is supported by Merlin, so with some luck, eventually Merlin will move it over to the 3006 codebase with the other "PRO" AX models! Looking at the specs, it's a decent router for the price! Especially with a black Friday sale! I've been itching to buy something, probably a new motherboard or a new wireless headset for work, before the tarrif war starts anyways 😆😉 |
OK, sounds good. As you said, the current solution works but if you have the time before the production release to make it better and more robust, go for it!! :>).
I think for the RT-AC68U models, this PRO model is a great replacement, especially given the use-case for my parents & in-laws, and it may even be a bit more than they really need for their home network. They don't have any WiFi7 devices, have no need for VLANs, and their total number of clients (wired & wireless) is about 20. In my case, I want to give it a try and check out the WiFi coverage, and I still have the option to add the RT-AC86U as an AP. We'll see... But I think I'll have some fun setting them up for my parents & in-laws first. |
I think you did the right call, it's probably a little over kill for a regular user like them, but for you it's going to be just right. Plus like you said you can experiment with setting up the old one as a node and see how that works for expanding your coverage. Obviously the AC won't reach the speeds the AX router will, but for testing purposes and extra coverage it's should do the job without a hickup. |
My primary concern is not so much speed but getting a strong & consistent signal all the way to the corners and far side of our patio, mostly for spring & summer afternoons/evenings when we're hanging out on the patio. It's not a critical requirement, but it would be nice too if it works well having the AC86U as an AP (not an AiMesh node). |
readonly CRON_DAYofWEEK_rexp2="${CRON_DAYofWEEK_NAME}[-]${CRON_DAYofWEEK_NAME}|[0-6][-][0-6]" | ||
readonly CRON_DAYofWEEK_rexp3="${CRON_DAYofWEEK_NAME}([,]${CRON_DAYofWEEK_NAME})+|[0-6]([,][0-6])+" | ||
readonly CRON_DAYofWEEK_RegEx="($CRON_DAYofWEEK_rexp1|$CRON_DAYofWEEK_rexp2|$CRON_DAYofWEEK_rexp3)" | ||
readonly CRON_DAYofWEEK_Names="([S|s]un|[M|m]on|[T|t]ue|[W|w]ed|[T|t]hu|[F|f]ri|[S|s]at)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized I left all my comments pending on this review again. Whoops my bad... Submitting review comments now!
Approved regex changes
I found the PRs... Just trying to get more information as to what prompted those changes from me. Especially 198... Little detail in the PR and you didn't even get to review it; makes me think it must of been a reported bug but where in the threads... Lol |
Okay; I now understand why we ended up with this solution.
Now the idea moving forwards would be to move that logic out of the top code and instead try to incorporate it into the Regex. |
OK, this is good. Making a list of requirements like that in such detail is an excellent way to analyze the problem and figure out a more robust & elegant solution. I see that you got the production release out in the field. Good job, bud. Hopefully, things will be calm (i.e. no major bugs) for the rest of the year so we get more rest & more free time during the holidays. LOL!!! Take care, bud, and have a great weekend. |
Agreed! That's my plan, over the next few weeks I may or may not be available as we get closer to the holidays, my plan is to focus on the upcoming Christmas and new year's plans and celebrations. I wanted to get everything out the door so we could take the next month to focus on other things (unless some unexpected bug comes up) but i know you've been busy as well. I think we did great work this release, lots of good stuff done.
You as well! Chat soon and enjoy the weekend :) |
Just an FYI that we did get a bug report here: I can't replicate the issue but I'm assuming based on his debug messages that it's coming from "dd" in this function:
Google tells me: 'dd typically also requires a block size (bs) to function predictably.' |
Yeah, I saw the post ~8 minutes ago, and the real issue is the current PATH set up by the user's environment. |
Made improvements in the code that builds the cron schedule string for the automatic script updates to make it more robust.
Made improvements in the cron schedule validation code & some minor wording changes in the error messages generated when validation fails.
Fixed bugs when trying to determine if we have a "386" build version in order to select the corresponding changelog URL.
Various code improvements & fine-tuning.