packages icon indicating copy to clipboard operation
packages copied to clipboard

[WIP][Blocked] enhancement of respondd wifi

Open genofire opened this issue 6 years ago • 28 comments

A module which should take all information of wifi independent of mesh protocoll.

Please squash before merge and merge together with https://github.com/freifunk-gluon/gluon/pull/1512

genofire avatar Jul 01 '18 14:07 genofire

I kind of feel like this could be combined with the https://github.com/freifunk-gluon/packages/tree/master/net/respondd-module-airtime because they already have similar fields and structure

skorpy2009 avatar Jul 01 '18 15:07 skorpy2009

Greate, i will not fix it now. (It is just code which we use some places since 2 years - i lust wanted to make it public)


Till i have time (in 2-3 month) - there is a idea of json structur (and where to fetch the data)

genofire avatar Jul 01 '18 15:07 genofire

@skorpy2009 together with count of clients?

genofire avatar Jul 01 '18 15:07 genofire

@genofire only if it is obtained from nl80211 instead of the mesh protocol.

christf avatar Jul 01 '18 22:07 christf

Of course, to extract wifi24 and wifi5 and count_stations from such packages:

https://github.com/freifunk-gluon/gluon/blob/baebf9b85233b893dfc65548953df26367ddcc24/package/gluon-mesh-batman-adv/src/respondd.c#L544-L550

genofire avatar Jul 02 '18 07:07 genofire

These values are implemented specifically for batman and I would discourage an implementation that is based on gluon-mesh-batman as we have been trying hard to make gluon mesh-protocol-agnostic.

When I implemented the corresponding module for babel I was only able to find a solution that is independent of batman for wifi24 and wifi5, however for clients attached via cable I am still querying l3roamd. That means all l3 roaming protocols are supported: https://github.com/freifunk-gluon/gluon/pull/934/files#diff-81bb0be87b3d14fc48cc78fab16675bdR582

christf avatar Jul 03 '18 07:07 christf

What are you thinking?

genofire avatar Aug 08 '18 04:08 genofire

Okay, i found a 'Bug'. I still need to check type device - clients only at AP. To merge https://github.com/freifunk-gluon/gluon/pull/1512 is still a detailed "client" view as neighbours necessary (by type IBSS and Ad-hoc).

genofire avatar Aug 17 '18 08:08 genofire

@NeoRaider is noise in neighbours needed? - it is a radio specific value, not station ...

genofire avatar Aug 18 '18 02:08 genofire

Hmm... the primary problem I have with the approach of this PR is that it forces you to include the airtime information in your respondd data even when you only want the client counts, significantly increasing the respondd reply packet size. Reducing duplicated code is good, reducing modularity isn't...

neocturne avatar Aug 25 '18 00:08 neocturne

@NeoRaider what do you suggest , what could be a better approach? allowing to switch the transmission on/off via uci / site.conf ?

rotanid avatar Aug 25 '18 11:08 rotanid

@rotanid i think he mean multi respondd packages - which would be cool. But it is not easy.

Slit in multiple packages without change of json

respondd-module-wifi and respondd-module-airtime it is not possible to extends json-array on statistics.wireless

Rename json airtime in statistics respondd

respondd-module-wifi provide statistics.wireless respondd-module-airtime provide statistics.airtime breaks current structur

provide wifi in nodeinfo

respondd-module-wifi provide nodeinfo.wireless respondd-module-airtime provide statistics.wireless nodeinfo is not really correct, txpower is a live value (could change on auto settings) also channel could be configurated and not applied yet.

restruct from array to object

respondd-module-wifi provide nodeinfo.wireless

{
"radio0":{"frequency":5220,"txpower":2000,"channel_width":20, "clients":3}
}

respondd-module-airtime provide nodeinfo.wireless

{
"radio0":{"active":366649704,"busy":205221222,...}
}

Very dirty in json + breaks current structur


I hope you understand my discription of other possible structures. Maybe i have forgot one, but more or less they are all not beautiful.

I think it is okay to have it all in one big package, which should be okay - it is just there to handle library nl80211. We could create multi build options like there are already (one for gluon and one for openwrt) e.g. respondd-module-gluonwifi-mini, respondd-module-gluonwifi-without-airtime or respondd-module-gluonwifi-all so this package could be defined explixit in site.mk.

genofire avatar Aug 29 '18 13:08 genofire

I'm torn on this point. Splitting the data into multiple small modules that each provide a few of the fields would be optimal from the perspective of modularity; unfortunately, loading many shared objects also leads to higher memory usage, as each library is mapped separately, with at least text and data being separate mapping. So at least until we have found a good solution for our memory pressure issues, I'm reluctant to propose splitting respondd modules too far...

neocturne avatar Aug 29 '18 17:08 neocturne

@NeoRaider in short or in more concrete words: after a review we merge this without splitting?

rotanid avatar Aug 29 '18 22:08 rotanid

  • wireless = json_object_new_array();
  • if (!wireless) {
  •  //TODO why needed? : json_object_put(result);
    

Why is there no regular json_free() function? It should not be so hacky with a json_object_put(), Because that is how the api works. The lib controls ownership of the json objects.

-- () ascii ribbon campaign - against html e-mail /\ against proprietary attachments

christf avatar Sep 25 '18 20:09 christf

After thinking about this a bit more, I don't think this PR is a step in the right direction:

  • I don't think general and airtime information should be combined in a single module
  • I don't want any Gluon-specific code (not even conditional) in the packages repo

Moving the wifi information out of the mesh-batman-adv respondd provider is good, but I consider the base provider in the gluon-respondd package as a better place to put this.

neocturne avatar Sep 29 '18 18:09 neocturne

I'm not sure where to put the wireless settings provider though... they seem very separate from both the client stats and the airtime surveys.

I could live with creating a respondd-module-wireless module from the settings and airtime surveying, but client count reporting should be kept separate.

neocturne avatar Sep 29 '18 18:09 neocturne

I'd appreciate some statistics towards DFS events and NOPs if possible.

mweinelt avatar Oct 09 '18 19:10 mweinelt

@NeoRaider so i split it to respondd-module-wireless with:

{
  "statistics": {
    "wireless": [
      {
        "frequency": 5220,
        "channel_width": 40,
        "txpower": 1700,
        "active": 366561161,
        "busy": 46496566,
        "rx": 808415,
        "tx": 41711344,
        "noise": 162,
        "clients": 5
      },
      {
        "frequency": 5220,
        "channel_width": 40,
        "txpower": 1700,
        "active": 366561161,
        "busy": 46496566,
        "rx": 808415,
        "tx": 41711344,
        "noise": 162,
        "clients": 2
      },
      {
        "frequency": 2437,
        "channel_width": 20,
        "txpower": 2000,
        "active": 366649704,
        "busy": 205221222,
        "rx": 108121446,
        "tx": 85453679,
        "noise": 161,
        "clients": 3
      }
    ]
  },
  "neighbours": {
    "wifi":{
      "00:11:22:33:44:55:66":{
        "frequency": 5220,
        "neighbours":{
          "33:22:33:11:22:44":{
            "signal": 191,
            "inactive": 50
          }
        }
      }
    }
  }
}

keep statistics.wireless.[].clients, rename to statistics.wireless.[].stations or skip?


and in package gluon-respondd in gluon repository with:

{
  "statistics": {
    "clients":{
      "wifi24": 3,
      "wifi5": 7
    }
  }
}

Sorry, but before it change any code, i want now a finale format version.

genofire avatar Oct 13 '18 10:10 genofire

@mweinelt Great idea, could you design a json-format? (maybe in a extra issue). I thing be could to many sub discussion here.

I could fetch it from code sample of iw phy phy0 channels command: https://git.kernel.org/pub/scm/linux/kernel/git/jberg/iw.git/tree/phy.c#n131

if (!tb_freq[NL80211_FREQUENCY_ATTR_DISABLED] && tb_freq[NL80211_FREQUENCY_ATTR_DFS_STATE]) {
  enum nl80211_dfs_state state = nla_get_u32(tb_freq[NL80211_FREQUENCY_ATTR_DFS_STATE]);
  unsigned long time;

  printf("\t  DFS state: %s", dfs_state_name(state));
  if (tb_freq[NL80211_FREQUENCY_ATTR_DFS_TIME]) {
    time = nla_get_u32(tb_freq[NL80211_FREQUENCY_ATTR_DFS_TIME]);
    printf(" (for %lu sec)", time / 1000);
  }
  printf("\n");
  if (tb_freq[NL80211_FREQUENCY_ATTR_DFS_CAC_TIME])
    printf("\t  DFS CAC time: %u ms\n",
           nla_get_u32(tb_freq[NL80211_FREQUENCY_ATTR_DFS_CAC_TIME]));
}

Do you have a idea how it could test it?

genofire avatar Oct 13 '18 10:10 genofire

Testing the different DFS implementations is what we want to do with this data, so not sure what you mean with "how" you could test it.

The issue with the design idea is here: https://github.com/freifunk-gluon/packages/issues/200

mweinelt avatar Nov 05 '18 21:11 mweinelt

Regarding the output seen in https://github.com/freifunk-gluon/packages/pull/188#issuecomment-429528271, referencing the radio where a neighbour has been seen on by its radio frequency might not be ideal, as multiple radios could be tuned onto the same frequency. To me that basically means that wireless should somehow contain the name of the phy, in which case it could become a dictionary again.

~~Regarding clients vs stations, I prefer the correct technical term, which is stations.~~

I'll gladly take some time to work out the idea further with you.

mweinelt avatar Nov 08 '18 19:11 mweinelt

~~Stealing this discussion away into a hackmd, so we can more flexibly iterate on the structure.~~

~~https://md.darmstadt.ccc.de/gluon-respondd-wireless~~

mweinelt avatar Nov 09 '18 23:11 mweinelt

marking as "needs work" as per @NeoRaider's latest comments on the structure of the packages:

  • keep lcient count separate
  • move remaining duplicate code from gluon-mesh-batman-adv and babel into gluon-respondd
  • create a new package respondd-module-wireless which lives outside of gluon in the freifunk-gluon/packages repo

christf avatar Nov 10 '18 22:11 christf

So I came up with a proposal of the structure I would favor and a bunch of open questions because some information can be conveyed in multiple ways, I've inlined some of the questions into the proposal json object.

Proposal

{
    "statistics": {
        "wireless": {
            "radios": {
                // wiphy name
                "phy0": {
                    // frequency or channel?
                    "frequency": 5240,
                    "channel": 100,
                    // channel width or htmode?
                    "channel_width": 40,
                    "htmode": "VHT80",
                    // txpower in dBm
                    "txpower": 20,
                    // survey in separate object
                    "survey": {
                        "active": 366561161,
                        "busy": 46496566,
                        "rx": 808415,
                        "tx": 41711344,
                        "noise": 162
                    }
                },
                "phy1": {
                    // frequency or channel?
                    "frequency": 2432,
                    "channel": 5,
                    // channel width or htmode?
                    "channel_width": 20,
                    "htmode": "HT40+",
                    // txpower in dBm
                    "txpower": 20,
                    // survey in separate object
                    "survey": {
                        "active": 366561161,
                        "busy": 46496566,
                        "rx": 808415,
                        "tx": 41711344,
                        "noise": 162
                    }
                }
            },
            "dfs": {
                // a more compact form for dfs states
                "usable": [52, 56, 60, 64, 100, 104, 108, 112, 116, 132, 136, 140],
                "available": [100],
                "unavailable": [120, 124, 128]
            }
        }
    }
}

Design decisions

  • Adding the physical radio name as a key to its radio object
  • Move airtime data into survey object
  • Dropping station information
    • privacy concerns when mentioning MAC addresses
    • needs to be a non-optional part of respondd statistics (as per https://github.com/freifunk-gluon/packages/pull/188#issuecomment-425667887)
    • stations are per VAP information, not per PHY and we don't consider VAPs here
    • not sure there is anything to gain from signal and inactive
  • Have txpower in dBm, not mbm. Divide by 100, let it be float if it needs to be
  • Adding DFS states in a more compact form, dropping the time how long we've been in a state, possibly superfluous

Please also consider the questions below.

Open Questions

Channel vs Frequency

Netlink delivers frequency, converting to channel is easy, but what do we favor? Both are well defined as far as I know.

NetJSON uses the channel: http://netjson.org/rfc.html#radios1

int ieee80211_frequency_to_channel(int freq)
{
        /* see 802.11-2007 17.3.8.3.2 and Annex J */
        if (freq == 2484)
                return 14;
        else if (freq < 2484)
                return (freq - 2407) / 5;
        else if (freq >= 4910 && freq <= 4980)
                return (freq - 4000) / 5;
        else if (freq <= 45000) /* DMG band lower limit */
                return (freq - 5000) / 5;
        else if (freq >= 58320 && freq <= 64800)
                return (freq - 56160) / 2160;
        else
                return 0;
}

Source: https://git.kernel.org/pub/scm/linux/kernel/git/jberg/iw.git/tree/util.c?id=02bc7751d7ac7db1b4e2ca354bd369a3c7f27cde#n321

Center Frequencies

Higher HT Modes use one or two additional center frequencies to specify the frequency range used. Are they necessary or can they be derived?

Surely possible for HT40+/- to calculate the 20 MHz secondary channel above or below.

 * @NL80211_ATTR_CENTER_FREQ1: Center frequency of the first part of the
 *	channel, used for anything but 20 MHz bandwidth
 * @NL80211_ATTR_CENTER_FREQ2: Center frequency of the second part of the
 *	channel, used only for 80+80 MHz bandwidth

https://elixir.bootlin.com/linux/v4.20-rc2/source/include/uapi/linux/nl80211.h#L1297

HT Mode vs. Channel Width

The HT Mode contains more information from which others can be derived, like for example the channel width.

  • HT20 / VHT20: Per stream throughput
  • HT40+ / HT40-: Secondary channel up or down?
  • VHT160-80PLUS80: Width is 160 MHz, but setting is actually 80PLUS80

They might seem far away, but with outdoor modes other HT modes are possible and the same is true for devices with multiple radios on the same band, where only one radio needs to be available to the mesh, while the client radio can have different settings.

DFS: State per radio or shared?

In case multiple radios are present is the DFS state shared among them? I don't think it's worth it to have the DFS state published per radio and even if multiple radios provided different state the channels in the Non Occupancy Period (30 minutes) should simply be aggregated.

DFS: Only NOPs?

Even devices that don't use outdoor channels have states for DFS channels, and usually they are all marked as usable, which means: I have not seen a radar yet and I need to perform CAC - which is basically worthless if the radio is not tuned into any of these channels, so it wouldn't see the DFS pattern anyway.

mweinelt avatar Nov 16 '18 00:11 mweinelt

i'd prefer channels over frequencies. i'd agree on shared DFS state. on the other stuff i either have not enough knowledge or i don't see a question.

rotanid avatar Nov 25 '18 22:11 rotanid

I was wondering if someone is already working on moving duplicate code from gluon-mesh-batman-adv and -babel into gluon-respondd, which I see as the first step for this whole cleanup (and which I'd like to review and get merged before we start adding new features to the WLAN-related respondd providers).

neocturne avatar Dec 01 '18 12:12 neocturne

so moved duplicated code to here: https://github.com/freifunk-gluon/packages/pull/217

After that is merged, i will try to implements @mweinelt as another module respondd-wifiradios in this PR.

genofire avatar Apr 10 '19 08:04 genofire