On 01.05.2018 12:20, Stefan Sperling wrote:
On Mon, Apr 30, 2018 at 08:57:23AM +0200, Peter Hessler wrote:
On 2018 Apr 29 (Sun) at 11:51:26 +0200 (+0200), Stefan Sperling wrote:
:This diff tries to avoid situations where background scans play
:ping-pong between different APs with nearly equal RSSI, as
:observed by phessler.
:
:Not all drivers represent RSSI values in dBm or percentage, so the
:diff includes the possibility for drivers to override the new RSSI
:comparison function. However, since the threshold is rather low
:applying this to all drivers for now should not do any harm, unless
:there is a driver where the RSSI value range is ridiculously small.
:I'm not aware of any such driver at present.
:
I'm concerned about two things.
The usage is confusing, because you pass two incompatible things to be
compared. I would prefer ieee80211_node_cmprssi(ic, rssi_one, rssi_two).
Agreed. Updated diff below. I've chosen to make it compare two nodes
since the function lives in the nodes namespace. It makes the function
a bit less flexible as it cannot be used to compare naked rssi values.
But that's not really needed for the problem at hand; after all, we
want to compare APs.
Also, in the case where ni->ni_rssi has a very weak signal, the second
comparison can underflow.
Indeed. Thanks for catching this!
The way you are using this function (and for any usage that I can think
of) you probably want a setter instead of a tester: here are two
networks, return the one I should connect to.
Another minor nit is variable naming in rssicmp. Instead of ni1 and ni2
you could probably use source and candidate, or something that better
suggests their intention since it is not exactly a strcmp-like function.
By the way, I like the way you handled overflow :)
If you decide this is the best way to go (tester instead of setter), OK.
Index: ieee80211_node.c
===================================================================
RCS file: /cvs/src/sys/net80211/ieee80211_node.c,v
retrieving revision 1.129
diff -u -p -r1.129 ieee80211_node.c
--- ieee80211_node.c 28 Apr 2018 14:49:07 -0000 1.129
+++ ieee80211_node.c 30 Apr 2018 07:30:43 -0000
@@ -67,6 +67,8 @@ u_int8_t ieee80211_node_getrssi(struct i
const struct ieee80211_node *);
int ieee80211_node_checkrssi(struct ieee80211com *,
const struct ieee80211_node *);
+int ieee80211_node_cmprssi(struct ieee80211com *,
+ const struct ieee80211_node *, const struct ieee80211_node *);
void ieee80211_setup_node(struct ieee80211com *, struct ieee80211_node *,
const u_int8_t *);
void ieee80211_free_node(struct ieee80211com *, struct ieee80211_node *);
@@ -133,6 +135,7 @@ ieee80211_node_attach(struct ifnet *ifp)
ic->ic_node_copy = ieee80211_node_copy;
ic->ic_node_getrssi = ieee80211_node_getrssi;
ic->ic_node_checkrssi = ieee80211_node_checkrssi;
+ ic->ic_node_cmprssi = ieee80211_node_cmprssi;
ic->ic_scangen = 1;
ic->ic_max_nnodes = ieee80211_cache_size;
@@ -761,12 +764,15 @@ ieee80211_end_scan(struct ifnet *ifp)
if (ic->ic_caps & IEEE80211_C_SCANALLBAND) {
if (IEEE80211_IS_CHAN_2GHZ(ni->ni_chan) &&
- (selbs2 == NULL || ni->ni_rssi > selbs2->ni_rssi))
+ (selbs2 == NULL ||
+ ic->ic_node_cmprssi(ic, ni, selbs2) > 0))
selbs2 = ni;
else if (IEEE80211_IS_CHAN_5GHZ(ni->ni_chan) &&
- (selbs5 == NULL || ni->ni_rssi > selbs5->ni_rssi))
+ (selbs5 == NULL ||
+ ic->ic_node_cmprssi(ic, ni, selbs5) > 0))
selbs5 = ni;
- } else if (selbs == NULL || ni->ni_rssi > selbs->ni_rssi)
+ } else if (selbs == NULL ||
+ ic->ic_node_cmprssi(ic, ni, selbs) > 0)
selbs = ni;
}
@@ -782,9 +788,12 @@ ieee80211_end_scan(struct ifnet *ifp)
*/
if (selbs5 && selbs5->ni_rssi > min_5ghz_rssi)
selbs = selbs5;
- else if (selbs5 && selbs2)
- selbs = (selbs5->ni_rssi >= selbs2->ni_rssi ? selbs5 : selbs2);
- else if (selbs2)
+ else if (selbs5 && selbs2) {
+ if (ic->ic_node_cmprssi(ic, selbs5, selbs2) >= 0)
+ selbs = selbs5;
+ else
+ selbs = selbs2;
+ } else if (selbs2)
selbs = selbs2;
else if (selbs5)
selbs = selbs5;
@@ -989,6 +998,38 @@ ieee80211_node_checkrssi(struct ieee8021
IEEE80211_RSSI_THRES_2GHZ :
IEEE80211_RSSI_THRES_5GHZ;
return (ni->ni_rssi >= (u_int8_t)thres);
+}
+
+/*
+ * Determine if RSSI values of two nodes are significantly different.
+ * This function assumes RSSI values are represented either in dBm or
+ * as a percentage of ic_max_rssi. Drivers should override this function
+ * in case their RSSI values use a different representation.
+ */
+int
+ieee80211_node_cmprssi(struct ieee80211com *ic,
+ const struct ieee80211_node *ni1, const struct ieee80211_node *ni2)
+{
+ uint8_t thres;
+
+ if (ic->ic_max_rssi)
+ thres = IEEE80211_RSSI_CMP_THRES_RATIO;
+ else
+ thres = IEEE80211_RSSI_CMP_THRES_DBM;
+
+ /* cap threshold to prevent overflow in calculations below */
+ while (thres > 0) {
+ if (ni1->ni_rssi + thres >= ni1->ni_rssi &&
+ ni1->ni_rssi - thres <= ni1->ni_rssi)
+ break;
+ thres--;
+ }
+
+ if (ni1->ni_rssi + thres < ni2->ni_rssi)
+ return -1;
+ if (ni1->ni_rssi - thres > ni2->ni_rssi)
+ return 1;
+ return 0;
}
void
Index: ieee80211_var.h
===================================================================
RCS file: /cvs/src/sys/net80211/ieee80211_var.h,v
retrieving revision 1.85
diff -u -p -r1.85 ieee80211_var.h
--- ieee80211_var.h 26 Apr 2018 12:50:07 -0000 1.85
+++ ieee80211_var.h 30 Apr 2018 07:30:56 -0000
@@ -62,6 +62,9 @@
#define IEEE80211_RSSI_THRES_RATIO_2GHZ 60 /* in percent */
#define IEEE80211_RSSI_THRES_RATIO_5GHZ 50 /* in percent */
+#define IEEE80211_RSSI_CMP_THRES_DBM 8 /* in dBm */
+#define IEEE80211_RSSI_CMP_THRES_RATIO 5 /* in percent */
+
#define IEEE80211_BGSCAN_FAIL_MAX 360 /* units of 500 msec */
enum ieee80211_phytype {
@@ -269,6 +272,9 @@ struct ieee80211com {
u_int8_t (*ic_node_getrssi)(struct ieee80211com *,
const struct ieee80211_node *);
int (*ic_node_checkrssi)(struct ieee80211com *,
+ const struct ieee80211_node *);
+ int (*ic_node_cmprssi)(struct ieee80211com *,
+ const struct ieee80211_node *,
const struct ieee80211_node *);
u_int8_t ic_max_rssi;
struct ieee80211_tree ic_tree;
