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

congure_reno: initial import of TCP Reno congestion control #15953

Merged
merged 2 commits into from
Apr 9, 2022

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Feb 8, 2021

Contribution description

This provides an implementation for CongURE of the congestion control mechanism of classic TCP congestion control (TCP Reno) as specified in RFC 5681.

Testing procedure

Tests are provided for which riotctrl (at least v0.2.2) is required for them to run. If installed,

make -j -C tests/congure_reno flash test

should succeed.

Issues/PRs references

Requires #15951 (merged), #16111, and #16119 (merged). When merged, I will adapt the test framework to use the turo framework introduced in #15950, which was the original idea to begin with (which is why it is already printing JSON strings).

PR dependency graph

@miri64 miri64 added Area: sys Area: System State: waiting for other PR State: The PR requires another PR to be merged first Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Feb 8, 2021
@miri64 miri64 changed the title Congure/feat/congure reno congure_reno: initial import of TCP Reno congestion control Feb 8, 2021
@miri64
Copy link
Member Author

miri64 commented Feb 26, 2021

Same as with #15952: Rebased to current master, needs some adaptions to the changes done in #15951, so I am marking this WIP temporarily.

@miri64 miri64 added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Feb 26, 2021
@miri64
Copy link
Member Author

miri64 commented Feb 26, 2021

Adopted tests, however on real boards, there seems to be a problem with the shell: even though I increased the input buffer for stdio_uart to 512, the shell only reads up to 64 characters and then garbage after that :-/. Anyone has any idea why? In tests/emcute (where there is also large input) this does not seem to be a problem.

@kaspar030
Copy link
Contributor

This is probably caused by the uart usb converter that is provided by the on-board debugger.

@miri64
Copy link
Member Author

miri64 commented Feb 26, 2021

This is probably caused by the uart usb converter that is provided by the on-board debugger.

But why does it work with tests/emcute then?

@miri64
Copy link
Member Author

miri64 commented Feb 26, 2021

This is probably caused by the uart usb converter that is provided by the on-board debugger.

Ok, there might be something to it though. On IoT-LAB M3 it succeeds.

@miri64 miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Feb 26, 2021
@miri64
Copy link
Member Author

miri64 commented Feb 26, 2021

Latest round of commits finishes adopting to the changes from #15951. If the error I am facing really stems from the hardware I might need to rethink my test frame work.

@miri64
Copy link
Member Author

miri64 commented Feb 26, 2021

(mostly for the report_msgs_timeout method, but still)

@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 1, 2021
@miri64 miri64 force-pushed the congure/feat/congure_reno branch from 780c451 to 9551e9d Compare March 2, 2021 11:31
@miri64
Copy link
Member Author

miri64 commented Mar 2, 2021

Same treatment as in #15952: Rebased to current master and dependencies and added #16119 as new dependency.

@miri64
Copy link
Member Author

miri64 commented Sep 9, 2021

Might be the good old line-off-by-one error I encountered in the past with RIOTCtrl :-/ We might need to fix that in the backend.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 14, 2021
@benpicco
Copy link
Contributor

The code looks fine though, so what do we do about the test?

@miri64
Copy link
Member Author

miri64 commented Sep 16, 2021

The code looks fine though, so what do we do about the test?

Fix the framework, once I have some time to look into it :-) (or maybe @fjmolinas can have a look as well). Maybe its also just a matter of usage. I remember that for congure_test I needed to do some adaptations after the fact as well, maybe these are required here as well and I just did not integrate them here yet (and in #15952)

@miri64
Copy link
Member Author

miri64 commented Sep 21, 2021

Fixed the same issue here.

@github-actions github-actions bot added Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework labels Sep 21, 2021
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the only remaining failure are due to missing Makefile.ci (maybe #16109 could come in handy here 😉 But then again the list is small enough to be created manually)

Please squash!

@miri64
Copy link
Member Author

miri64 commented Sep 21, 2021

From what I observed locally, there will be similar errors in the tests as in #15952 ;-) 95% they are test errors, not module errors, but still need an hour or two of spare time to check it for sure.

@OlegHahm
Copy link
Member

OlegHahm commented Apr 8, 2022

@miri64 ping :)

@miri64 miri64 force-pushed the congure/feat/congure_reno branch from 80b6e31 to 7d6543b Compare April 8, 2022 12:37
@miri64
Copy link
Member Author

miri64 commented Apr 8, 2022

Squashed

@miri64 miri64 force-pushed the congure/feat/congure_reno branch from 7d6543b to 56cf940 Compare April 8, 2022 12:37
@miri64
Copy link
Member Author

miri64 commented Apr 8, 2022

And rebased (in the hopes that something in the current master will fix the murdock issues :P)

@miri64
Copy link
Member Author

miri64 commented Apr 8, 2022

@OlegHahm If Murdock fails, don't bother to push for it to get into the release. Sadly, I don't have time for bug hunting today :(.

@miri64
Copy link
Member Author

miri64 commented Apr 8, 2022

@miri64 miri64 removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Apr 8, 2022
@miri64 miri64 force-pushed the congure/feat/congure_reno branch from 56cf940 to 3c05f72 Compare April 8, 2022 15:59
@miri64
Copy link
Member Author

miri64 commented Apr 9, 2022

@benpicco can this still be merged?

@benpicco benpicco merged commit c89f6bf into RIOT-OS:master Apr 9, 2022
@benpicco
Copy link
Contributor

benpicco commented Apr 9, 2022

Sure!

@miri64 miri64 deleted the congure/feat/congure_reno branch April 10, 2022 18:42
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants