-
Notifications
You must be signed in to change notification settings - Fork 2k
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: initial import of a congestion control framework #15951
Conversation
Is there any whitelist/black list needed for the boards or features required? A quick test on the
|
Shouldn't be, which is why so far, I only tested on |
Where do I see the timeout in your output? |
( |
It doesn't show a prompt and locks or something compared to native. |
Mh... two problems, I did not check the length of the argument, before accessing the array, which I fixed. However, it seems like the prompt is not flushed when a command has no output in general:
Note the |
Seems to be an issue with
Will have a look if I find a fix. |
1a67d29
to
dd0b5c8
Compare
Rebased to include #15962. |
Great... now when hitting enter on an empty command, does not work anymore... |
Ah, RIOTCtrl needs to use |
Done |
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.
Tests work with make and pytest on both native and nucleo-f411re.
It seems like there are some kconfig issues:
make clean
weiss@mobiweiss:~/wd/RIOT/tests/congure_test$ TEST_KCONFIG=1 make all
=== [ATTENTION] Testing Kconfig dependency modelling ===
[genconfig.py]:ERROR-=> The parameter KCONFIG_USEMODULE_SHELL could not be set to y.
[genconfig.py]:ERROR- Check the following unmet dependencies: USEMODULE_SHELL (=n), (!TEST_KCONFIG) (=n)
[genconfig.py]:ERROR-=> The parameter SHELL_NO_ECHO could not be set to y.
[genconfig.py]:ERROR- Check the following unmet dependencies: ((KCONFIG_USEMODULE_SHELL && !TEST_KCONFIG) || (MODULE_SHELL && TEST_KCONFIG)) (=n)
make: *** No rule to make target '/home/weiss/wd/RIOT/tests/congure_test/bin/native/generated/out.config', needed by 'check-kconfig-errors'. Stop.
Maybe @leandrolanzieri can give some feedback.
same thing if I say |
Mhhh I thought I solved that already :-/ Will have another look, but I think it's because with |
@leandrolanzieri would it make sense that with |
Yepp, if I remove the $ TEST_KCONFIG=1 make -C tests/congure_test/ -j
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/congure_test'
=== [ATTENTION] Testing Kconfig dependency modelling ===
[genconfig.py]:ERROR-=> The parameter KCONFIG_USEMODULE_SHELL could not be set to y.
[genconfig.py]:ERROR- Check the following unmet dependencies: USEMODULE_SHELL (=n), (!TEST_KCONFIG) (=n)
make: *** No rule to make target '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/congure_test/bin/native/generated/out.config', needed by 'check-kconfig-errors'. Stop.
make: *** Waiting for unfinished jobs....
make: Leaving directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/congure_test'
$ rm tests/congure_test/app.config
$ TEST_KCONFIG=1 make -C tests/congure_test/ -j
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/congure_test'
=== [ATTENTION] Testing Kconfig dependency modelling ===
=== [ATTENTION] Testing Kconfig dependency modelling ===
Building application "tests_congure_test" for "native" with MCU "native".
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/boards/native
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/core
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/cpu/native
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/drivers
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/sys
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/boards/native/drivers
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/sys/auto_init
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/sys/congure
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/sys/fmt
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/cpu/native/periph
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/drivers/periph_common
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/cpu/native/stdio_native
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/sys/shell
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/sys/shell/commands
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/sys/test_utils/interactive_sync
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/sys/congure/mock
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/sys/congure/test
/usr/bin/ld: /home/mlenders/Repositories/RIOT-OS/RIOT/tests/congure_test/bin/native/cpu/tramp.o: warning: relocation against `_native_saved_eip' in read-only section `.text'
/usr/bin/ld: warning: creating DT_TEXTREL in a PIE
text data bss dec hex filename
47915 772 48004 96691 179b3 /home/mlenders/Repositories/RIOT-OS/RIOT/tests/congure_test/bin/native/tests_congure_test.elf
make: Leaving directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/congure_test' |
Just got there.. yup, it looks like the |
Could it have something to do with #16064? |
probably not... |
No, I don't think so. This is purely about Lines 58 to 65 in f3871c0
|
Oh, I did not even push the |
I wonder if we just drop trying to support the old way and only keep the post migration way... I guess it depends on if app.config would be able to be used with app.config.test (like it is now) or if it should be either or. |
This will make full migration a pain, I believe, since we then have to port all the |
Please rebase and I will retest with the app.config.test fix |
Rebased and squashed. Remember the clean step inbetween though ;-) |
Rebased and squashed (forgot to push) |
62585ab
to
90a1e4d
Compare
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 think things look good to me. Kconfig is checked, the idea is documented, test work for a board and native. ACK!
Thanks for the review and all the help with Kconfig migration and RIOTCtrl deployment! |
Contribution description
This provides CongURE (Congestion Control Utilizing Reusable Elements, pronounced like "conjure"), a framework to implement congestion control and use it in a reusable manner with a number of protocols.
The original motivation was to provide congestion control with 6LoWPAN selective fragment recovery and allow for the congestion control mechanism to be interchangeable. However, now that there is a framework with multiple well tested implementations of congestion control, it can also be used for other protocols, such as TCP or QUIC, as I kept the units for the congestion window up to the user (TCP and QUIC, e.g., uses bytes, SFR, e.g., uses number of fragments). It is also designed as such that multiple instances of the same implementation can run on the same image (e.g. for when TCP and SFR want to use the same CC). In that case, I can imagine a lot of ROM can be saved, since there is only one CC code for two layers.
A test framework for the CongURE framework is also provided in form of shell commands to trigger the API functions of CongURE, as well as tests for said framework (a framework test framework test if you want 😉). If preferred, I can move those to a separate PR.
As to why this is in
sys/
and notsys/net/
: There are no networking components to this framework, it just provides the functions to calculate the size of a congestion window.seq
e.g. is also insys/
, though sequence number arithmetic mostly shows up in networking. Not sure this could help with local queue congestion problems, but when thinking out of the box, one might think as such :-P.Testing procedure
A test framework including tests for it is provided.
riotctrl
(at least v0.2.2) is required for them to run. If installed,should succeed.
Issues/PRs references
Follow-up PRs for actual implementations of congestion control mechanisms will follow, as well as integration PRs for GNRC's 6LoWPAN SFR and maybe TCP as well :-). 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 ;-)).