Skip to content
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

implement support bandwidth autodetect. #42

Closed
rgnldo opened this issue Jun 26, 2020 · 32 comments
Closed

implement support bandwidth autodetect. #42

rgnldo opened this issue Jun 26, 2020 · 32 comments
Assignees
Labels
enhancement New feature or request

Comments

@rgnldo
Copy link

rgnldo commented Jun 26, 2020

Needs testing.

SHAPER PARAMETERS
autorate-ingress

[3] --> Queue Priority | [diffserv4]
[4] --> Extra Options | [ack-filter-aggressive]

@ttgapers
Copy link
Owner

How much effort to implement? Seems like a worthwhile issue/request to look at. I am open to your thoughts on this, but looks like it might address some of the users questions around speedtests etc.

@rgnldo
Copy link
Author

rgnldo commented Jun 26, 2020

With the autorate-ingress option, it is not necessary to inform DL / UP rates. It is a solution for environments with bandwidth fluctuation, prioritizing navigation, reducing latency and congestion. Cake is excellent for this situation.

@rgnldo
Copy link
Author

rgnldo commented Jun 26, 2020

The script needs to ignore the request to report DL / UP rate.
They can generate a queuing pattern.

autorate-ingress besteffort ack-filter
or
autorate-ingress diffserv4 ack-filter
or
autorate-ingress diffserv4 ack-filter-aggressive

@rgnldo
Copy link
Author

rgnldo commented Jun 26, 2020

Tests must be done on a LAN client with iperf and Wireshark. You should not use websites for testing

@Audionut
Copy link

Audionut commented Jun 26, 2020

The script needs to ignore the request to report DL / UP rate.

I don't think that is the ideal case.

https://man7.org/linux/man-pages/man8/tc-cake.8.html#SHAPER_PARAMETERS

Automatic capacity estimation based on traffic arriving at this
qdisc. This is most likely to be useful with cellular links, which
tend to change quality randomly. A bandwidth parameter can be used
in conjunction to specify an initial estimate. The shaper will
periodically be set to a bandwidth slightly below the estimated rate.
This estimator cannot estimate the bandwidth of links downstream of
itself.

It reads like the bandwidth parameter sets the maximum rate and autorate-ingress then shapes the bandwidth to a value lower then that rate.

edit: Thus the correct setting of the bandwidth parameter has no negative effect on the operation of autorate-ingress. /end edit

I also believe given that autorate-ingress "cannot estimate the bandwidth of links downstream of itself", is very good reason to maintain bandwidth parameters. The network in Australia has a very strict bandwidth policer, and a significantly better experience is obtained by applying rate limits within the home environment**.

**edit: rather then allowing the network policer to bandwidth limit the upload stream. /end edit.

If I read the documentation correctly, autorate-ingress would be unable to determine that the bandwidth of the upstream link is being limited by the network policer, in and of itself, and thus requires the assistance of bandwidth parameter.

@rgnldo
Copy link
Author

rgnldo commented Jun 26, 2020

It's not the case. Continuing the other options, with the definition of the bandwidth rate

@ttgapers
Copy link
Owner

It's not the case. Continuing the other options, with the definition of the bandwidth rate

Would it hurt to get both scenarios tested?

@maghuro
Copy link
Contributor

maghuro commented Jun 27, 2020

I have an LTE 40/10 connection.
My upload is consistent, but download oscillates between 5 and 30 most of the times, so an auto detect, in my particular case, would be awesome.

@maghuro
Copy link
Contributor

maghuro commented Jun 27, 2020

fd81ba3
Untested. Please test it well.

@Adamm00
Copy link
Collaborator

Adamm00 commented Jun 27, 2020

@maghuro Please test and debug changes locally before committing to the repo. I also suggest using a editing client that supports ShellCheck (atom/notepad++) to avoid basic coding mistakes (your past 5 commits have failed), not to mention commit names like "deeeeeeeerp" are very unprofessional in a group project. Using a web browser to write code is also a terrible idea.

@ttgapers
Copy link
Owner

ttgapers commented Jun 27, 2020

@maghuro Please test and debug changes locally before committing to the repo. I also suggest using a editing client that supports ShellCheck (atom/notepad++) to avoid basic coding mistakes (your past 5 commits have failed), not to mention commit names like "deeeeeeeerp" are very unprofessional in a group project. Using a web browser to write code is also a terrible idea.

I agree, we need to build more structure towards stable releases - I would love if @Adamm00 is the "Release Manager" so to speak for versioning/changelogs if you are up for it. If your preferred approach is for forks, local tests etc. I am also ok, but don't mind as formal develop branch if it makes sense.

Let's get consensus, however master branch stability should be first since we are past v1 and we have more users adopting the add-on thanks to everyone's work!

Thoughts from everyone appreciated. I am also learning a lot as this project unfolds - thanks to you all.

@kernol51
Copy link

@ttgapers - FULLY support your comment above ... it is CRUCIAL to control commits to the Master and/or Mainline Branch. Another add-on to Asus-Merlin routers descended into chaos before being rescued by one of the "Part of the Furniture" experts.

It would be a tragedy if Cake-QOS had to endure the same fate. NIP it in the bud NOW ... pretty please.

I'm no coder - nor do I have any Github config knowledge - but surely you can shutdown commits to the Master with a VERY select few who can commit there - with the rest putting in Pull Requests ... or whatever.

@maghuro
Copy link
Contributor

maghuro commented Jun 28, 2020

Hello guys, that's the reason I created a branch specifically for that implementation. I don't want to mess with master branch.

I'm no good at this, I'm not a coder either, I'm just a guy who have some ideas and want to try to help the best I can. But it is clear I'm no good doing it :)

So I'll close this issue, please delete the branch I've created, and I'm going off.

Thank you guys for letting me in, and thanks for your fantastic work! Really you're the best!!
A big thanks for Jack and then Adamm hoping in. You're the ones!

@maghuro maghuro closed this as completed Jun 28, 2020
@maghuro
Copy link
Contributor

maghuro commented Jun 28, 2020

Oh and if you guys want to implement this, let me say that autorate-ingress works pretty well and it detects pretty well your current bandwidth.

To implement it, my suggestion is that when user enters auto on download and upload, you have to hardcode autorate-ingress on extra options. And you should separate download and upload options as I did on this dork implementation. Because used may want to have a fixed bandwidth in download for example, and autorate-ingress on upload.

Regarding unlimited, user have to enter 0 and you should separate also both options.

@Audionut
Copy link

Audionut commented Jun 28, 2020

@maghuro Please test and debug changes locally before committing to the repo.

Might as well just have a webserver serving compiled binaries, yes?
Pushing completely untested (kludge) commits to the main branch that users rely upon on a day to day (nay minute to minute) basis to download, install and use this code, clearly should not be condoned at all.

Which IMO, is the entire point of branching. To collaborate with others regarding unpolished code. There is much to be learned through error. A closed source, paid for software this is not.

Seeing the above statement from people who've pushed more then one "hotfix" to the main branch these last days.......

I also suggest using a editing client that supports ShellCheck (atom/notepad++) to avoid basic coding mistakes (your past 5 commits have failed), not to mention commit names like "deeeeeeeerp" are very unprofessional in a group project. Using a web browser to write code is also a terrible idea.

All valid statements, and a wise man once said "exaggeratedly "friendly" (in that fashion) or useful, pick one". However.....

To directly point out the commit failings (your past 5 commits have failed) of others, and in the context of the rest of your post, not only is that not exaggeratedly friendly, it is not even useful. And I think we can all agree that if something is not useful, it may not have to be "exaggeratedly" friendly, but it should at least be respectful. Did I mention something already about hotfixes!

Cake works extremely well for me. Thankyou everyone for the development effort. Watching the collaboration so far, without a defined project manager and people being granted commit access relatively easily, has been interesting and refreshing. Thankyou again.

@rgnldo
Copy link
Author

rgnldo commented Jun 28, 2020

I tested the auto-bandwith branch. Functional. We still need to polish the code.
Well, each one contributes with what they have knowledge. Others with knowledge improve. We move forward and grow like that.
Excellent job.

@Adamm00
Copy link
Collaborator

Adamm00 commented Jun 28, 2020

Which IMO, is the entire point of branching. To collaborate with others regarding unpolished code. There is much to be learned through error. A closed source, paid for software this is not.

I treat every project as if I were being paid to do so, take pride in your work. I like to think users can be confident that any scripts I am involved with are of the highest quality. What isn't high quality is pushing 25+ one line commits in 24 hours and not even using a text editor or git client to review your work before committing the changes, let alone testing them locally. I'm sorry but its just lazy, and if I were a user monitoring this repo's commit log I would have 0 confidence running code from it.

I hate to be blunt and for anyone to feel like I am singling them out, but this is the exact reason open source projects have commit guidelines and reject low quality commits. If any person with commit access can't put that minimal effort into their content, they should not be committing.

@Adamm00
Copy link
Collaborator

Adamm00 commented Jun 28, 2020

Now as for the topic on hand. I read the manpage much like @Audionut in regards to the fact a "guideline" up/down value should also be specified with the autorate-ingress option

Automatic capacity estimation based on traffic arriving at this
qdisc. This is most likely to be useful with cellular links, which
tend to change quality randomly. A bandwidth parameter can be used
in conjunction to specify an initial estimate. The shaper will
periodically be set to a bandwidth slightly below the estimated rate.
This estimator cannot estimate the bandwidth of links downstream of
itself.

@Adamm00 Adamm00 reopened this Jun 28, 2020
@rgnldo
Copy link
Author

rgnldo commented Jun 28, 2020

There are several ways to express yourself. I work as a professor and university researcher. I take advantage of all knowledge and organize it in proportion to the level of knowledge. I admire @Adamm00 work a lot, like the others. I believe that it is necessary to curate decisions, with a hierarchy of knowledge. It is necessary to welcome everyone. The idea @maghuro of ​​creating separate branches is a good one. As Adamm has greater knowledge in FW and scripting, he will analyze and confirm if useful and feasible.

@BrummyGit
Copy link

Hi Everyone, I just posted in the forums but this may help:

I've been testing (as I remembered reading the information somewhere) and all that is needed to implement unlimited bandwidth is to change the validation for the bandwidth values so that they accept 0 (zero) - this is taken by cake as unlimited.

We can then add autorate-ingress in the extra options if required.

I manually edited my cake-qos.cfg to the following
Code:
##############################################

Generated By Cake - Do Not Manually Edit

Jun 27 11:47:36

##############################################

Installer

dlspeed="0"
upspeed="0"
queueprio="diffserv4"
extraoptions="bridged-ptm ack-filter"

##############################################
And the result is

CakeQOS-Merlin - v1.0.0: Running...
CakeQOS-Merlin - v1.0.0: > Download Status:
qdisc cake 8010: dev ifb9eth0 root refcnt 2 bandwidth unlimited diffserv4 triple-isolate nat nowash ingress ack-filter split-gso rtt 100.0ms ptm overhead 22 no-sce
CakeQOS-Merlin - v1.0.0: > Upload Status:
qdisc cake 800f: dev eth0 root refcnt 2 bandwidth unlimited diffserv4 triple-isolate nat nowash ack-filter split-gso rtt 100.0ms ptm overhead 22 no-sce

@Adamm00
Copy link
Collaborator

Adamm00 commented Jun 28, 2020

You can get the same result adding "unlimited" to your extra options field as it will take priority;

skynet@RT-AX88U-DC28:/jffs/addons/cake-qos# cake-qos status

-snip-

#########################################################

CakeQOS-Merlin: Running...
CakeQOS-Merlin: > Download Status:
qdisc cake 8010: dev ifb9eth0 root refcnt 2 bandwidth 92Mbit besteffort triple-isolate nat wash ingress ack-filter split-gso rtt 100.0ms noatm overhead 38 mpu 84 no-sce
CakeQOS-Merlin: > Upload Status:
qdisc cake 800f: dev eth0 root refcnt 2 bandwidth 37Mbit besteffort triple-isolate nat nowash ack-filter split-gso rtt 100.0ms noatm overhead 38 mpu 84 no-sce

#########################################################

skynet@RT-AX88U-DC28:/jffs/addons/cake-qos# cake-qos settings extraoptions "unlimited ethernet ack-filter"

-snip-

#########################################################

CakeQOS-Merlin: Stopping
Broadcom Packet Flow Cache HW acceleration
CPU Speed () ==> Enable
Changing power settings. Setting cpu frequency setting to adaptive.
Broadcom Packet Flow Cache learning via BLOG enabled.
CakeQOS-Merlin: Starting - ( 92Mbit | 37Mbit | besteffort | unlimited ethernet ack-filter )
Broadcom Packet Flow Cache HW acceleration
CPU Speed (
) ==> Disable
Changing power settings. Forcing cpu to max frequency.
Set cpuspeed to on(pwr config --cpuspeed on) to make it adaptive again
Broadcom Packet Flow Cache learning via BLOG disabled.
Broadcom Packet Flow Cache flushing the flows

#########################################################

skynet@RT-AX88U-DC28:/jffs/addons/cake-qos# cake-qos status

-snip-

#########################################################

CakeQOS-Merlin: Running...
CakeQOS-Merlin: > Download Status:
qdisc cake 8012: dev ifb9eth0 root refcnt 2 bandwidth unlimited besteffort triple-isolate nat wash ingress ack-filter split-gso rtt 100.0ms noatm overhead 38 mpu 84 no-sce
CakeQOS-Merlin: > Upload Status:
qdisc cake 8011: dev eth0 root refcnt 2 bandwidth unlimited besteffort triple-isolate nat nowash ack-filter split-gso rtt 100.0ms noatm overhead 38 mpu 84 no-sce

#########################################################

skynet@RT-AX88U-DC28:/jffs/addons/cake-qos#

@ttgapers
Copy link
Owner

Hello guys, that's the reason I created a branch specifically for that implementation. I don't want to mess with master branch.

I'm no good at this, I'm not a coder either, I'm just a guy who have some ideas and want to try to help the best I can. But it is clear I'm no good doing it :)

So I'll close this issue, please delete the branch I've created, and I'm going off.

Thank you guys for letting me in, and thanks for your fantastic work! Really you're the best!!
A big thanks for Jack and then Adamm hoping in. You're the ones!

@maghuro - I hope you do not just "go off", cause I am sure like others here do believe your contributions are super important. Let's just get the structure back - where the key is, we don't develop in master.

For the entire team - @Adamm00 would like/prefer local development/testing, and I am also ok if this means a formal develop branch for collaborative development/testing prior to pushing to master.

If that works let's formalize it and work in sync. I love what we have built here, but as the Dos Equis says we don't do...which is "When I do test I do it in Production" 👎

Trust we will see you @maghuro

@ttgapers
Copy link
Owner

There are several ways to express yourself. I work as a professor and university researcher. I take advantage of all knowledge and organize it in proportion to the level of knowledge. I admire @Adamm00 work a lot, like the others. I believe that it is necessary to curate decisions, with a hierarchy of knowledge. It is necessary to welcome everyone. The idea @maghuro of ​​creating separate branches is a good one. As Adamm has greater knowledge in FW and scripting, he will analyze and confirm if useful and feasible.

Exactly the goal of the "Release Manager" role assuming @Adamm00 would like to assume it, I am ok due to his rigor around quality control on what gets to Master....

@maghuro
Copy link
Contributor

maghuro commented Jun 28, 2020

Hello guys, that's the reason I created a branch specifically for that implementation. I don't want to mess with master branch.
I'm no good at this, I'm not a coder either, I'm just a guy who have some ideas and want to try to help the best I can. But it is clear I'm no good doing it :)
So I'll close this issue, please delete the branch I've created, and I'm going off.
Thank you guys for letting me in, and thanks for your fantastic work! Really you're the best!!
A big thanks for Jack and then Adamm hoping in. You're the ones!

@maghuro - I hope you do not just "go off", cause I am sure like others here do believe your contributions are super important. Let's just get the structure back - where the key is, we don't develop in master.

For the entire team - @Adamm00 would like/prefer local development/testing, and I am also ok if this means a formal develop branch for collaborative development/testing prior to pushing to master.

If that works let's formalize it and work in sync. I love what we have built here, but as the Dos Equis says we don't do...which is "When I do test I do it in Production"

Trust we will see you @maghuro

As I said... Adamm said me once, and absolutely right, to not "test" things in master.
In fact, I created a branch for this issue. The question is, how can you guys test it if we aren't phisically in the same place? I had to create a branch so you can test my code.

I made mistakes? Yes I made. My builds failed? Yes they failed. As I said I'm not a pro and most of the code I write is in a testing way. I'm learning shell by doing mistakes and correct them.

And as no one wanted to write this bandwithd auto detect, I tried by myself.

It wasn't by laziness that I haven't tested it. It is because I'm not phisically at home right now and I didn't want to test it because something could get wrong and I was unable to manually reboot my router.
As I said, I didn't commited anything to master, despite of the version bump with tested commits. I created a fully new branch to test this particular issue.

Sorry to do this way. From now one, Everytime I want to contribute, I'll only open a pull request for you to evaluate and correct my noobness mistakes.

I'm not mad, I've just lost the excitement I had with this :)

Sorry.

@RobotsAreCrazy
Copy link

@maghuro thanks for the work, effort and most importantly good intentions and passion :)

@Audionut
Copy link

Audionut commented Jun 28, 2020

I'm sorry but its just lazy

Agreed, but laziness borne from enthusiasm and excitement. It's not laziness in the sense of slob that lacks enthusiasm and excitement.

And while again, your statements are correct in the literal sense, one should be careful of over-damping enthusiasm through criticism. Open source projects need people willing to get their hands dirty with code. We live in a society where majority expect to be force feed on the effort of others.

I speak from the experience as someone who has dampened the enthusiasm of potential significant contributors in open source projects, and driven those contributors away. :(

but this is the exact reason open source projects have commit guidelines and reject low quality commits.

Yes, this exactly.

Perhaps, as encouragement, there could be a middle-ground. Branches are free range for the developer of said branch, but in order to commit to the main branch, the commits must follow standard, and thus need to be re-written to the appropriate standard before committed to the main branch.

Re-writing code gets old, quickly. Learn to do it right the first time :)

@Adamm00
Copy link
Collaborator

Adamm00 commented Jun 28, 2020

Hello guys, that's the reason I created a branch specifically for that implementation. I don't want to mess with master branch.
I'm no good at this, I'm not a coder either, I'm just a guy who have some ideas and want to try to help the best I can. But it is clear I'm no good doing it :)
So I'll close this issue, please delete the branch I've created, and I'm going off.
Thank you guys for letting me in, and thanks for your fantastic work! Really you're the best!!
A big thanks for Jack and then Adamm hoping in. You're the ones!

@maghuro - I hope you do not just "go off", cause I am sure like others here do believe your contributions are super important. Let's just get the structure back - where the key is, we don't develop in master.
For the entire team - @Adamm00 would like/prefer local development/testing, and I am also ok if this means a formal develop branch for collaborative development/testing prior to pushing to master.
If that works let's formalize it and work in sync. I love what we have built here, but as the Dos Equis says we don't do...which is "When I do test I do it in Production"
Trust we will see you @maghuro

As I said... Adamm said me once, and absolutely right, to not "test" things in master.
In fact, I created a branch for this issue. The question is, how can you guys test it if we aren't phisically in the same place? I had to create a branch so you can test my code.

I made mistakes? Yes I made. My builds failed? Yes they failed. As I said I'm not a pro and most of the code I write is in a testing way. I'm learning shell by doing mistakes and correct them.

And as no one wanted to write this bandwithd auto detect, I tried by myself.

It wasn't by laziness that I haven't tested it. It is because I'm not phisically at home right now and I didn't want to test it because something could get wrong and I was unable to manually reboot my router.
As I said, I didn't commited anything to master, despite of the version bump with tested commits. I created a fully new branch to test this particular issue.

Sorry to do this way. From now one, Everytime I want to contribute, I'll only open a pull request for you to evaluate and correct my noobness mistakes.

I'm not mad, I've just lost the excitement I had with this :)

Sorry.

Its not about being sorry, take it as constructive criticism and grow from it. Everyone has to start somewhere, my main point is everyone (myself included) should be doing things the right way, not taking shortcuts at the expense of QA.

Test locally first, everything on the repo including dev branches should be 100% personally tested at the time of writing and working to the authors knowledge. Measure twice, cut once. If you are not home to test the code yourself, then don't push it until you are home and its ready. There is no need to rush things, the script will still be here tomorrow and the next day to contribute to, there isn't a race on who can commit something first.

The next big thing is using a proper workflow. Download Notepad++/ Atom / VSCode and the corresponding shellcheck plugin, this plugin alone will alert you to 99% of easily avoidable mistakes and detail the reasons why you should avoid such mistakes.

Another major piece of software is your git client, there is no reason not to download Github Desktop (or similar software) to manage your changes. By doing this you also have the ability to diff your changes before pushing them so you can review and correct any obvious mistakes.

And there you have it, the right way to contribute to a group open source project, not the easy or fast way. Invest in yourself so users can be confident of your work.

@maghuro
Copy link
Contributor

maghuro commented Jun 28, 2020

I'm using Notepad++ and Github desktop.

Can you help me installing the proper shellcheck plugin? I've searched and didn't find it nor know how to install it.

Thanks.

@kernol51
Copy link

That's the spirit @maghuro - your enthusiasm and pushing forward your ideas have been most welcome ... the project is now on track to formalise the commit process - which is all that was needed so accuracy and reliability of the code can be maintained.

There is a great deal to learn from the experts here - so I'm pleased you are taking the messages to heart but not allowing yourself to be offended.

@Adamm00
Copy link
Collaborator

Adamm00 commented Jun 28, 2020

https://github.com/koalaman/shellcheck#in-your-editor

I guess notepad++ actually isn't supported, so Atom or VSCode would be your best bet.

Github desktop.

Then start using it from now on 😉 All your commits so far have been from the Github website from what I can tell.

@ttgapers
Copy link
Owner

ttgapers commented Jun 28, 2020

I am learning too and going to adapt to the "standards" that @Adamm00 just laid out. I was used to Notepad2 with Github Desktop, but moving to Atom/VS Code with the plugins....

So in summary:

  • All are welcome. Going to tighten up the release/commit strategy as @Adamm00 laid out
  • Use Atom/VSCode and the corresponding shellcheck plugin: https://github.com/koalaman/shellcheck#in-your-editor
  • Use a well supported git client. Use Github Desktop (or similar software) to manage your changes. By doing this you also have the ability to diff your changes before pushing them so you can review and correct any obvious mistakes.

And there you have it, the right way to contribute to a group open source project, not the easy or fast way. Invest in yourself so users can be confident of your work.

I'll be ensuring I do adhere and do correct me as I also learn the way of the Jedis here :)

Happy to see you @maghuro and @jkychn

Back on topic....sorry :p

@Adamm00
Copy link
Collaborator

Adamm00 commented Jul 1, 2020

Added in #52

@Adamm00 Adamm00 closed this as completed Jul 1, 2020
@ttgapers ttgapers added the enhancement New feature or request label Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

9 participants