[Click] [RFC] Update radiotap parser

Fri Apr 15 04:55:37 EDT 2011

Il 15/04/2011 09:44, 欧阳鑫 ha scritto:
>
> Hi, I have patched my click source files according to above. I want to
> implement click on 802.11n mode, but I find the above patch
>
> does not deal with MCS arguments such
> IEEE80211_RADIOTAP_MCS_HAVE_MCS，IEEE80211_RADIOTAP_MCS_BW_40，IEEE80211_RADIOTAP_MCS_SGI
>
>
well they are anyway ignored by an unpatched kernel
>
> and so on. So I modify radiotapencap.cc, radiotapdecap.cc,
> settxrate.cc, settxrate.hh, ettstat.cc and athdescdecap.cc. The patches
>
> are as follow:
> diff -urN click-1.8.0_bg_patch/elements/roofnet/sr/ettstat.cc
> click-1.8.0_patch_11n/elements/roofnet/sr/ettstat.cc
> --- click-1.8.0_bg_patch/elements/roofnet/sr/ettstat.cc 2011-04-01
> 15:09:40.000000000 +0800
> +++ click-1.8.0_patch_11n/elements/roofnet/sr/ettstat.cc 2011-04-14
> 15:59:10.252273517 +0800
> @@ -420,7 +420,15 @@
> struct click_wifi_extra *ceh = (struct click_wifi_extra *) p->user_anno();
> ceh->magic = WIFI_EXTRA_MAGIC;
> ceh->rate = rate;
> -ceh->max_tries = 7;
> +
> + ceh->flags |= WIFI_EXTRA_RX_FLAG_HT;
> + ceh->flags |= WIFI_EXTRA_RX_FLAG_40MHZ;
> + ceh->flags |= WIFI_EXTRA_RX_FLAG_SHORT_GI;
> + ceh->rateidx = _ads_rs_index;
> + ceh->max_tries = 1;
> +
> checked_output_push(0, p);
> }

this should probably be part of the element configuration rather that
hard coded.

> diff -urN click-1.8.0_bg_patch/elements/wifi/athdescdecap.cc
> click-1.8.0_patch_11n/elements/wifi/athdescdecap.cc
> --- click-1.8.0_bg_patch/elements/wifi/athdescdecap.cc 2011-04-01
> 15:09:40.000000000 +0800
> +++ click-1.8.0_patch_11n/elements/wifi/athdescdecap.cc 2011-04-14
> 10:36:26.468273490 +0800
> @@ -30,7 +30,7 @@
>
> AthdescDecap::AthdescDecap()
> {
> - static_assert(WIFI_EXTRA_ANNO_SIZE >= sizeof(click_wifi_extra));
> + //static_assert(WIFI_EXTRA_ANNO_SIZE >= sizeof(click_wifi_extra));
> }
>

is there a particular reason to remove this? anyway the atheros
descriptor is supported only by the madwifi
driver which does not support 11n cards.

> AthdescDecap::~AthdescDecap()
> diff -urN click-1.8.0_bg_patch/elements/wifi/radiotapdecap.cc
> click-1.8.0_patch_11n/elements/wifi/radiotapdecap.cc
> --- click-1.8.0_bg_patch/elements/wifi/radiotapdecap.cc 2011-04-12
> 10:33:15.000000000 +0800
> +++ click-1.8.0_patch_11n/elements/wifi/radiotapdecap.cc 2011-04-14
> 16:35:28.604273504 +0800
> @@ -26,6 +26,7 @@
> #include <clicknet/radiotap.h>
> #include <clicknet/llc.h>
>
> @@ -105,6 +106,26 @@
>
> CLICK_DECLS
>
> +#define IEEE80211_MCS_RATE_COUNT 16
> +int IEEE80211_RADIOTAP_MCS_RATE[IEEE80211_MCS_RATE_COUNT][2] = {
> + {0, 150},
> + {1, 300},
> + {2, 450},
> + {3, 600},
> + {4, 900},
> + {5, 1200},
> + {6, 1350},
> + {7, 1500},
> + {8, 300},
> + {9, 600},
> + {10, 900},
> + {11, 1200},
> + {12, 1800},
> + {13, 2400},
> + {14, 2700},
> + {15, 3000}
> +};
> +
I do not get this, the MCS index already embed the rate information. As
a matter of fact
the corresponding radiotap field shall containt the index NOT the
corresponding PHY
data rate:

http://www.radiotap.org/defined-fields/MCS

> RadiotapDecap::RadiotapDecap()
> {
> }
> @@ -221,9 +242,36 @@
> if (flags & IEEE80211_RADIOTAP_F_TX_FAIL)
> ceh->flags |= WIFI_EXTRA_TX_FAIL;
> break;
> + case IEEE80211_RADIOTAP_MCS:
> + uint8_t known, flags;
> + known = *(iter.this_arg);
> + flags = *(iter.this_arg + 1);
> +
> + // set Band width
> + if (known & IEEE80211_RADIOTAP_MCS_HAVE_BW) {
> + if (flags & IEEE80211_RADIOTAP_MCS_BW_40)
> + ceh->flags |= WIFI_EXTRA_RX_FLAG_HT;
> + }
where is this WIFI_EXTRA_RX_FLAG_HT defined?

> + if (known & IEEE80211_RADIOTAP_MCS_HAVE_MCS)
> + ceh->rateidx = *(iter.this_arg + 2);
> + else
> + // may not reasonable......
> + ceh->rateidx = 0;
> +
> + // set Guard interval
> + if (known & IEEE80211_RADIOTAP_MCS_HAVE_GI) {
> + if (flags & IEEE80211_RADIOTAP_MCS_SGI)
> + ceh->flags |= WIFI_EXTRA_RX_FLAG_SHORT_GI;
and this
> + }
> + // set FMT
> + if (known & IEEE80211_RADIOTAP_MCS_HAVE_FMT) {
> + if (flags & IEEE80211_RADIOTAP_MCS_FMT_GF)
> + ceh->flags |= WIFI_EXTRA_RX_FLAG_LDPC;
> + }
> +
same here
>
> + ceh->rate = IEEE80211_RADIOTAP_MCS_RATE[ceh->rateidx][1] / 5;
> + break;
> +
ok, i get you want to set rate to the actual phy rate. why an actual use
for it?
>
> - //if (flags & IEEE80211_RADIOTAP_F_FCS) {
> - // p->take(4);
so what happens if the frame actually include the FCS ?
> }
> }
>
> diff -urN click-1.8.0_bg_patch/elements/wifi/radiotapencap.cc
> click-1.8.0_patch_11n/elements/wifi/radiotapencap.cc
> --- click-1.8.0_bg_patch/elements/wifi/radiotapencap.cc 2011-04-12
> 10:51:03.000000000 +0800
> +++ click-1.8.0_patch_11n/elements/wifi/radiotapencap.cc 2011-04-14
> 16:21:15.760273037 +0800
> @@ -46,6 +46,14 @@
> (1 << IEEE80211_RADIOTAP_EXT) | \
> 0)
>
> +//566fix: mcs
> +#define CLICK_RADIOTAP_MCS_PRESENT ( \
> + (1 << IEEE80211_RADIOTAP_MCS) | \
> + 0)
> +
> +#define IEEE80211_MCS_RATE_COUNT 16
> +extern int IEEE80211_RADIOTAP_MCS_RATE[IEEE80211_MCS_RATE_COUNT][2];
> +
>
> struct click_radiotap_header {
> struct ieee80211_radiotap_header wt_ihdr;
> @@ -66,6 +74,12 @@
> u_int8_t wt_rts_retries;
> u_int8_t wt_data_retries;
>
> + u_int8_t wt_known;
> + u_int8_t wt_flags;
> + u_int8_t wt_mcs;
> +
> u_int8_t wt_rate1;
> u_int8_t wt_data_retries1;
>
> } __attribute__((__packed__));
>
>
> @@ -206,6 +219,29 @@
> }
> }
>
> +
> + if (ceh->flags & WIFI_EXTRA_RX_FLAG_HT) {
> + crh->wt_ihdr.it_present |= CLICK_RADIOTAP_MCS_PRESENT;
> + crh->wt_known = IEEE80211_RADIOTAP_MCS_HAVE_BW | \
> + IEEE80211_RADIOTAP_MCS_HAVE_MCS | \
> + IEEE80211_RADIOTAP_MCS_HAVE_GI | \
> + IEEE80211_RADIOTAP_MCS_HAVE_FMT | \
> + IEEE80211_RADIOTAP_MCS_HAVE_FEC;
> + crh->wt_mcs = ceh->rateidx;
> + if (ceh->flags & WIFI_EXTRA_RX_FLAG_40MHZ)
> + crh->wt_flags |= IEEE80211_RADIOTAP_MCS_BW_40;
> + if (ceh->flags & WIFI_EXTRA_RX_FLAG_SHORT_GI)
> + crh->wt_flags |= IEEE80211_RADIOTAP_MCS_SGI;
> + if (ceh->flags & WIFI_EXTRA_RX_FLAG_GF)
> + crh->wt_flags |= IEEE80211_RADIOTAP_MCS_FMT_GF;
> + crh->wt_rate = IEEE80211_RADIOTAP_MCS_RATE[ceh->rateidx][1] / 5;
> +
> + }
> +
>
Here you add the three new MCS fields (known, flags, mcs), but the
present bitmask is updated
only if the WIFI_EXTRA_RX_FLAG_HT is set. As a result if
WIFI_EXTRA_RX_FLAG_HT is not set
the three fields are still there resulting in an invalid radiotap header.

Finally, I would try to stabilize the radiotap parser and the
decap/encap elements before
adapting other elements such as ettstat, settxrate, etc.

R.