Implement dynamic MCS selection based on signal strength in vWIFI driver #80
Conversation
There was a problem hiding this comment.
It seems that you are adjusting the MCS based on the current signal strength. However, what if a user wants to set the MCS manually using a command like iw dev wlan0 set bitrates ht-mcs-2.4 <mcs_index>?
To support this, you could consider implementing the set_bitrate_mask() callback in your cfg80211_ops and storing the specified MCS in the corresponding vwifi interface structure.
Thanks for you suggestion ,I can try to included the MCS for maunally adjust ! |
dingsen-Greenhorn
force-pushed
the
dynamic_MCS
branch
from
79a5101 to
4e81b72
Compare
vwifi.c
Outdated
| @@ -88,6 +88,8 @@ struct vwifi_vif { | |||
| struct wireless_dev wdev; | |||
| struct net_device *ndev; | |||
| struct net_device_stats stats; | |||
| int manual_mcs; /* ADDED: Store user-specified MCS */ | |||
There was a problem hiding this comment.
Remove "ADDED:" prefix from the comment
vwifi.c
Outdated
| @@ -88,6 +88,8 @@ struct vwifi_vif { | |||
| struct wireless_dev wdev; | |||
| struct net_device *ndev; | |||
| struct net_device_stats stats; | |||
| int manual_mcs; /* ADDED: Store user-specified MCS */ | |||
| bool manual_mcs_set; /* ADDED: Flag to indicate manual MCS override */ | |||
There was a problem hiding this comment.
Remove "ADDED:" prefix from the comment
vwifi.c
Outdated
| @@ -1728,13 +1743,8 @@ static int vwifi_start_ap(struct wiphy *wiphy, | |||
|
|
|||
| /* Initialize hrtimer of beacon */ | |||
| pr_info("vwifi: init beacon_timer.\n"); | |||
| #if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 15, 0) | |||
There was a problem hiding this comment.
Don't modify unrelated code segment!
You should align with the latest version of vwifi.
First,the code handles MCS selection and modulation assignment, either manually (based on vif->manual_mcs_set and vif->manual_mcs) or automatically (based on signal strength). Second,The vWIFI driver constrains MCS indices to 7, 15, 23, and 31 to simplify rate calculation, testing, and implementation. This avoids the need for a comprehensive calculation of data-rates to handle all 32 MCS indices, varying streams, and channel widths. The chosen indices represent a range of modulations (BPSK to 64-QAM) for 4 streams, supporting both manual (iw commands) and automatic (signal-based) modes. The code’s design prioritizes simplicity for a virtual driver, ensuring functionality (as shown in your test output) while limiting complexity. And thanks for you thoughtful question,it might be the next PR to optimize it ! |
|
Nit: It might be better to squash all the “Fix coding style” commits. |
scripts/hostapd.conf
Outdated
| @@ -2,7 +2,7 @@ interface=vw0 | |||
| driver=nl80211 | |||
| debug=1 | |||
| ctrl_interface=/var/run/hostapd | |||
| ctrl_interface_group=0 | |||
| ctrl_interface_group=root | |||
There was a problem hiding this comment.
The changes to the MCS configuration logic and the hostapd.conf control group setting serve different purposes and should be separated into distinct PRs.
The MCS logic affects runtime behavior of the vwifi driver, while the ctrl_interface_group=root change only impacts permission control for the hostapd socket.
Splitting them would improve clarity, ease future debugging, and allow better tracking of changes.
vwifi.c
Outdated
| const char *modulation; | ||
| if (vif->manual_mcs_set) { | ||
| mcs_index = vif->manual_mcs; | ||
| switch (mcs_index) { |
There was a problem hiding this comment.
According to IEEE 802.11n, MCS indices 7, 15, 23, and 31 all use 64-QAM modulation.
You should revise the descriptions associated with each MCS index accordingly, or consider modifying your design if your intention was to differentiate based on modulation type.
ref:
mcsindex
|
|
vwifi.c
Outdated
| { \ | ||
| .bitrate = (_rate), \ | ||
| .hw_value = (_hw_value), \ | ||
| for file in ${SOURCES} |
There was a problem hiding this comment.
Why is there a shell script embedded in the kernel module???
Use command
$ clang-format -i *.[ch]
to maintain a consistent coding style in your implementation!
dingsen-Greenhorn
force-pushed
the
dynamic_MCS
branch
13 times, most recently
from
c556347 to
c96febb
Compare
vwifi.c
Outdated
| /* Checks vif->manual_mcs_set to use vif->manual_mcs if set; | ||
| * Assigns modulation string for manual MCS ; else auto change based |
There was a problem hiding this comment.
Nit: Use imperative mood
Edit: solved
vwifi.c
Outdated
| @@ -1403,8 +1403,19 @@ static int vwifi_get_station(struct wiphy *wiphy, | |||
| sinfo->tx_failed = vif->stats.tx_dropped; | |||
| sinfo->tx_bytes = vif->stats.tx_bytes; | |||
| sinfo->rx_bytes = vif->stats.rx_bytes; | |||
|
|
|||
|
|
|||
vwifi.c
Outdated
| sinfo->tx_bytes, sinfo->rx_bytes); | ||
|
|
||
| /* Dynamic modulation based on signal strength */ | ||
| /* Checks vif->manual_mcs_set to use vif->manual_mcs if set; |
above picture demonstrate the random signal strength mapping to different MCS .(code from MCS 24-31) |
|
Also, I realized a serious issue that in auto selection MCS, for the guard intervals (GI), which have multiple functions, such as prefix, that eliminate ICI and ISI in OFDM based system, now we only have signal strength, this should also consider many other physical parameters, such as delay spread for estimating the GI .The above issue also indicates that manual set guard intervals is a little bit weird in modern wireless system. |
dingsen-Greenhorn
force-pushed
the
dynamic_MCS
branch
from
c142feb to
fa402a1
Compare
Integrated and conduct result in same behavior of <test_vwifi_bitrates.sh>. |
As we mention in meeting can i break this branch into multiple ? |
dingsen-Greenhorn
force-pushed
the
dynamic_MCS
branch
from
fa402a1 to
591910d
Compare
Update the vwifi_set_bitrate_mask callback to support the full range of MCS indices 24 through 31, aligning with the 4-spatial-stream configuration in nf_band_2ghz. This change enabling support for manual MCS from 24-31 coding schemes (1/2, 3/4, 2/3, 5/6) and modulations (BPSK, QPSK,16-QAM, 64-QAM) as defined by the IEEE 802.11n specification. The callback now rejects indices outside 24–31, ensuring compliance with 4-stream capabilities and allowing comprehensive rate testing (26–260 Mbps). The auto MCS selection, due to this function is based on only signal strength, while the MCS should construct with modulation and coding scheme, which the coding scheme is related with BER (bite error rate), previous vWiFI only implement random signal strength, meaning that bit error rate should also based on the channel state info.
- Adjust MCS indices 24 to 31 to match spec table's modulation (BPSK to 64-QAM) and coding rates (1/2 to 5/6). - Revise signal strength thresholds (-45 to -75 dBm) to reflect realistic SNR requirements for each modulation scheme. - Preserve random signal generation logic using rand_int_smooth. - Ensure consistency with the reference table's structure and parameters.
dingsen-Greenhorn
force-pushed
the
dynamic_MCS
branch
from
591910d to
c800205
Compare
solved ! thanks. |
Check https://github.com/sysprog21/lab0-c/blob/master/CONTRIBUTING.md for Git commit style. |
vwifi.c
Outdated
| /* Set GI based on mask->control[NL80211_BAND_2GHZ].gi and GI string for | ||
| * logging */ |
There was a problem hiding this comment.
/*
* Set GI based on mask->control[NL80211_BAND_2GHZ].gi and GI string for
* logging
*/…CS 0-31 This commit enhances the vwifi driver and test script to fully support HT mode with MCS 0-31, dynamic guard interval (GI) selection (0.8 µs and 0.4 µs), and robust testing. Also, for dump station simplify sta_vif validation and remove ambiguous sta_vif == ap_vif check. Key changes: - Adde `gi_mode` module parameter to vwifi.c to enable dynamic switching between long GI (0.8 µs) and short GI (0.4 µs), replacing hardcoded `vif->gi = VWIFI_TXRATE_GI_800NS` in `vwifi_set_bitrate_mask`. - Update `vwifi_set_bitrate_mask` to use `gi_mode` and added logging for GI and MCS settings. - Enhance `vwifi_get_station` to report GI correctly in `sinfo->txrate` and `sinfo->rxrate` with `RATE_INFO_FLAGS_SHORT_GI`. - Update `test_vwifi_bitrates.sh` to: - Test MCS 0-31 with both long and short GI on `vw1` and `vw2`. - Validate bitrates against `ht_mcs_table` (`rate_800ns`, `rate_400ns`). - Add error handling for `gi_mode` writes and connection checks. - Include debug output for `wpa_supplicant` and `iw dev link`. - Suggested debugfs fallback for per-interface GI control if `gi_mode` fails. - Replaced the sta_vif == ap_vif check with a direct null check on sta_vif. The previous comparison between station and access point virtual interfaces (sta_vif == ap_vif) was logically unsound, as these represent distinct roles in Wi-Fi operation and should not be equated. Returning -ENONET when sta_vif equals ap_vif was unclear and potentially masked deeper issues. The new logic simplifies the flow: if no valid sta_vif is found after iterating, return -ENONET; otherwise, continue normally. Also removed an unused assignment to `ret = 0`. These changes ensure compliance with reviewer requirements for HT mode, MCS 0-31, GI selection, and dynamic MCS selection, with robust testing and validation.
dingsen-Greenhorn
force-pushed
the
dynamic_MCS
branch
3 times, most recently
from
5745d03 to
5206e45
Compare
vwifi.c
Outdated
| pr_info( | ||
| "vwifi: Station %pM tx_bytes %llu, rx_bytes %llu, tx_packets %u, " | ||
| "rx_packets %u, tx_failed %u\n", | ||
| mac, sinfo->tx_bytes, sinfo->rx_bytes, sinfo->tx_packets, | ||
| sinfo->rx_packets, sinfo->tx_failed); |
There was a problem hiding this comment.
Too long. Split into two lines.
vwifi.c
Outdated
| * https://semfionetworks.com/blog/mcs-table-updated-with-80211ax-data-rates/ | ||
| * IEEE 802.11n : https://zh.wikipedia.org/zh-tw/IEEE_802.11n | ||
| * - https://zh.wikipedia.org/zh-tw/IEEE_802.11n |
There was a problem hiding this comment.
Use English entry of Wikipedia instead.
There was a problem hiding this comment.
When building on kernel version 6.8.0-60-generic, several warning messages appeared.
It seems there are multiple format string mismatches in pr_info() calls.
/home/elian/test/MCS/vwifi.c: In function ‘vwifi_get_station’:
./include/linux/kern_levels.h:5:25: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘const char *’ [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
./include/linux/printk.h:429:25: note: in definition of macro ‘printk_index_wrap’
429 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
./include/linux/printk.h:530:9: note: in expansion of macro ‘printk’
530 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
./include/linux/kern_levels.h:14:25: note: in expansion of macro ‘KERN_SOH’
14 | #define KERN_INFO KERN_SOH "6" /* informational */
| ^~~~~~~~
./include/linux/printk.h:530:16: note: in expansion of macro ‘KERN_INFO’
530 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~
/home/elian/test/MCS/vwifi.c:1640:5: note: in expansion of macro ‘pr_info’
1640 | pr_info("vwifi: Station %pM signal %d dBm, MCS %d (%s, %s), GI %s, SS %s\n",
| ^~~~~~~
./include/linux/kern_levels.h:5:25: warning: format ‘%s’ expects argument of type ‘char *’, but argument 7 has type ‘int’ [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
./include/linux/printk.h:429:25: note: in definition of macro ‘printk_index_wrap’
429 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
./include/linux/printk.h:530:9: note: in expansion of macro ‘printk’
530 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
./include/linux/kern_levels.h:14:25: note: in expansion of macro ‘KERN_SOH’
14 | #define KERN_INFO KERN_SOH "6" /* informational */
| ^~~~~~~~
./include/linux/printk.h:530:16: note: in expansion of macro ‘KERN_INFO’
530 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~
/home/elian/test/MCS/vwifi.c:1640:5: note: in expansion of macro ‘pr_info’
1640 | pr_info("vwifi: Station %pM signal %d dBm, MCS %d (%s, %s), GI %s, SS %s\n",
| ^~~~~~~
./include/linux/kern_levels.h:5:25: warning: format ‘%s’ expects a matching ‘char *’ argument [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
./include/linux/printk.h:429:25: note: in definition of macro ‘printk_index_wrap’
429 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
./include/linux/printk.h:530:9: note: in expansion of macro ‘printk’
530 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
./include/linux/kern_levels.h:14:25: note: in expansion of macro ‘KERN_SOH’
14 | #define KERN_INFO KERN_SOH "6" /* informational */
| ^~~~~~~~
./include/linux/printk.h:530:16: note: in expansion of macro ‘KERN_INFO’
530 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~
/home/elian/test/MCS/vwifi.c:1640:5: note: in expansion of macro ‘pr_info’
1640 | pr_info("vwifi: Station %pM signal %d dBm, MCS %d (%s, %s), GI %s, SS %s\n",
| ^~~~~~~
./include/linux/kern_levels.h:5:25: warning: format ‘%s’ expects argument of type ‘char *’, but argument 5 has type ‘int’ [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
./include/linux/printk.h:429:25: note: in definition of macro ‘printk_index_wrap’
429 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
./include/linux/printk.h:530:9: note: in expansion of macro ‘printk’
530 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
./include/linux/kern_levels.h:14:25: note: in expansion of macro ‘KERN_SOH’
14 | #define KERN_INFO KERN_SOH "6" /* informational */
| ^~~~~~~~
./include/linux/printk.h:530:16: note: in expansion of macro ‘KERN_INFO’
530 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~
/home/elian/test/MCS/vwifi.c:1663:5: note: in expansion of macro ‘pr_info’
1663 | pr_info("vwifi: Station %pM txrate MCS %d, rxrate MCS %d, SS %s\n", mac,
| ^~~~~~~
/home/elian/test/MCS/vwifi.c: At top level:
/home/elian/test/MCS/vwifi.c:2589:17: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
2589 | .channels = vwifi_supported_channels_2ghz,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/elian/test/MCS/vwifi.c:2591:17: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
2591 | .bitrates = vwifi_supported_rates,
| ^~~~~~~~~~~~~~~~~~~~~
/home/elian/test/MCS/vwifi.c:2574:14: warning: ‘vwifi_get_mcs_rate’ defined but not used [-Wunused-function]
2574 | static float vwifi_get_mcs_rate(u8 mcs_index, bool short_gi)
| ^~~~~~~~~~~~~~~~~~
vwifi.c
Outdated
|
|
||
| static struct ieee80211_supported_band nf_band_2ghz = { | ||
| .band = NL80211_BAND_2GHZ, | ||
| .channels = vwifi_supported_channels_2ghz, |
There was a problem hiding this comment.
Use an explicit cast when assigning to the .channels and .bitrates fields.
| @@ -21,7 +29,7 @@ if [ $? -ne 0 ]; then | |||
| fi | |||
|
|
|||
| if [ $final_ret -eq 0 ]; then | |||
| # to avoid device or resource busy error | |||
There was a problem hiding this comment.
Do not modify unrelated code.
If you believe a word or sentence needs refinement, submit it in a separate PR.
dingsen-Greenhorn
force-pushed
the
dynamic_MCS
branch
3 times, most recently
from
ca99f9f to
6208224
Compare
There is some warning indeed ! Thanks ,but I am not sure is some of the warning comes from different version? |
…egrate test script
This commit refactors the vwifi driver to store Guard Interval (GI)
information solely in cfg80211_bitrate_mask, eliminating the redundant short_gi variable,
and introduces a spatial stream index for future optimization.
It also adds a comprehensive comment block for manual_mcs and updates the test script to
validate all MCS indices (0–31) with sgi-2.4 and lgi-2.4.
changes:
- vwifi_vif:
- Remove short_gi, storing GI in vif->bitrate_mask.control[NL80211_BAND_2GHZ].gi.
- Add spatial_streams field (int, default 1) for future multi-stream support.
- Add manual_mcs field with detailed comment block explaining its role in storing
the first enabled MCS for consistent bitrate reporting.
- vwifi_set_bitrate_mask:
- Store GI in vif->bitrate_mask.control[NL80211_BAND_2GHZ].gi, removing short_gi usage.
- vwifi_get_station:
- GI logic using vif->bitrate_mask.gi for short (0.4µs), long (0.8µs), or default GI.
- Correct pr_info format string for modulation and coding_rate alignment.
- Configure sinfo->rxrate/txrate with vif->bitrate_mask.gi and vif->manual_mcs.
Test script (test_vwifi_bitrates.sh):
-Updated to test MCS 0–31 with sgi-2.4 and lgi-2.4:
- Test header (Testing MCS <mcs> with <gi> on vw1).
- GI status (Set GI to long/short).
- iw dev vw1 link output (MAC, SSID, freq, RX/TX, signal, bitrates).
- Success/failure message with actual vs. expected bitrate.
- Add expected bitrate arrays for lgi-2.4 and sgi-2.4.
- Enhance stability with 2s retry sleep, 1s sleep after iw set bitrates, and 2s setup delay.
- Ensure cleanup resets bitrate.
Testing:
- Verified GI (0.4µs/0.8µs) and MCS (0–31) with iw dev vw1 set bitrates and iw dev vw1 link.
dingsen-Greenhorn
force-pushed
the
dynamic_MCS
branch
from
6208224 to
fd3a0db
Compare
This commit enhances the vWIFI driver by implementing dynamic Modulation and Coding Scheme (MCS) selection in the
vwifi_get_stationfunction, adjusting the MCS index based on signal strengthAfter implement dynamic MCS can avoid TX power waste for a bad channel quality