Skip to content
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

[WIP][Blocked] enhancement of respondd wifi #188

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

genofire
Copy link
Contributor

@genofire genofire commented Jul 1, 2018

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

Please squash before merge and merge together with freifunk-gluon/gluon#1512

@genofire genofire force-pushed the respondd-module-wifisettings branch from 05e7aeb to c213f1f Compare July 1, 2018 15:03
@skorpy2009
Copy link
Member

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

@genofire
Copy link
Contributor Author

genofire commented Jul 1, 2018

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
Copy link
Contributor Author

genofire commented Jul 1, 2018

@skorpy2009 together with count of clients?

@christf
Copy link
Member

christf commented Jul 1, 2018

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

@genofire
Copy link
Contributor Author

genofire commented Jul 2, 2018

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

@christf
Copy link
Member

christf commented Jul 3, 2018

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

@genofire genofire changed the title add respondd-module-wifisettings merge every wifi data to one respondd-module-wifi module Aug 8, 2018
@genofire
Copy link
Contributor Author

genofire commented Aug 8, 2018

What are you thinking?

@genofire
Copy link
Contributor Author

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

@genofire genofire changed the title merge every wifi data to one respondd-module-wifi module [WIP] merge every wifi data to one respondd-module-wifi module Aug 17, 2018
@genofire genofire changed the title [WIP] merge every wifi data to one respondd-module-wifi module merge every wifi data to one respondd-module-wifi module Aug 18, 2018
@genofire
Copy link
Contributor Author

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

@rotanid rotanid requested a review from neocturne August 20, 2018 12:29
@neocturne
Copy link
Member

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...

@rotanid
Copy link
Member

rotanid commented Aug 25, 2018

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

@genofire
Copy link
Contributor Author

@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.

@neocturne
Copy link
Member

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...

@rotanid
Copy link
Member

rotanid commented Aug 29, 2018

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

wifi5 += clients_count;
}

//TODO wiphy only one radio added? (not necessary on gluon - only one ap-ssid at radio)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about private wifi?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are skipped by this line:
https://github.com/freifunk-gluon/packages/pull/188/files#diff-372361d4845238ed37120eb1554c176eR31
Thankyou for your review, i will correct this

@christf
Copy link
Member

christf commented Sep 25, 2018 via email

@neocturne
Copy link
Member

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
Copy link
Member

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.

@mweinelt
Copy link
Contributor

mweinelt commented Oct 9, 2018

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

@genofire
Copy link
Contributor Author

genofire commented Oct 13, 2018

@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
Copy link
Contributor Author

genofire commented Oct 13, 2018

@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?

@mweinelt
Copy link
Contributor

mweinelt commented Nov 5, 2018

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: #200

@mweinelt
Copy link
Contributor

mweinelt commented Nov 8, 2018

Regarding the output seen in #188 (comment), 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
Copy link
Contributor

mweinelt commented Nov 9, 2018

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

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

@christf
Copy link
Member

christf commented Nov 10, 2018

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

@mweinelt
Copy link
Contributor

mweinelt commented Nov 16, 2018

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 [WIP][Blocked] enhancement of respondd wifi #188 (comment))
    • 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.

@rotanid
Copy link
Member

rotanid commented Nov 25, 2018

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.

@neocturne
Copy link
Member

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).

@rotanid rotanid removed the request for review from neocturne February 16, 2019 15:18
@genofire genofire changed the title merge every wifi data to one respondd-module-wifi module WIP: merge every wifi data to one respondd-module-wifi module Apr 10, 2019
@genofire genofire changed the title WIP: merge every wifi data to one respondd-module-wifi module WIP: enhancement of respondd wifi Apr 10, 2019
@genofire
Copy link
Contributor Author

so moved duplicated code to here: #217

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

@genofire genofire changed the title WIP: enhancement of respondd wifi [WIP][Blocked] enhancement of respondd wifi Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants