-
Notifications
You must be signed in to change notification settings - Fork 73
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
base: main
Are you sure you want to change the base?
[WIP][Blocked] enhancement of respondd wifi #188
Conversation
05e7aeb
to
c213f1f
Compare
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 |
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) |
@skorpy2009 together with count of clients? |
@genofire only if it is obtained from nl80211 instead of the mesh protocol. |
Of course, to extract |
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: |
What are you thinking? |
f324d00
to
8ceea07
Compare
Okay, i found a 'Bug'. I still need to check type device - clients only at AP. |
@NeoRaider is |
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... |
@NeoRaider what do you suggest , what could be a better approach? allowing to switch the transmission on/off via uci / site.conf ? |
@rotanid i think he mean multi respondd packages - which would be cool. Slit in multiple packages without change of json
Rename json airtime in statistics respondd
provide wifi in nodeinfo
restruct from array to object
{
"radio0":{"frequency":5220,"txpower":2000,"channel_width":20, "clients":3}
}
{
"radio0":{"active":366649704,"busy":205221222,...}
} Very dirty in json + breaks current structur I hope you understand my discription of other possible structures. I think it is okay to have it all in one big package, which should be okay - it is just there to handle library |
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... |
@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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about private wifi?
There was a problem hiding this comment.
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
88421e7
to
f703626
Compare
+ 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
|
After thinking about this a bit more, I don't think this PR is a step in the right direction:
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. |
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. |
I'd appreciate some statistics towards DFS events and NOPs if possible. |
@NeoRaider so i split it to {
"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 and in package {
"statistics": {
"clients":{
"wifi24": 3,
"wifi5": 7
}
}
} Sorry, but before it change any code, i want now a finale format version. |
@mweinelt Great idea, could you design a json-format? (maybe in a extra issue). I could fetch it from code sample of
Do you have a idea how it could test it? |
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 |
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
I'll gladly take some time to work out the idea further with you. |
|
marking as "needs work" as per @NeoRaider's latest comments on the structure of the packages:
|
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
Please also consider the questions below. Open QuestionsChannel vs FrequencyNetlink 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
Center FrequenciesHigher 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.
https://elixir.bootlin.com/linux/v4.20-rc2/source/include/uapi/linux/nl80211.h#L1297 HT Mode vs. Channel WidthThe HT Mode contains more information from which others can be derived, like for example the channel width.
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. |
i'd prefer channels over frequencies. |
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). |
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