update_vap_stats() fails for wlan0.0 in Gl.Inet B1300 #1563
Conversation
mariomaz
force-pushed
the
bugfix/PPM-311_update_vap_stats_fails_in_glinet
branch
3 times, most recently
from
3b4bcf9 to
4e9ac7f
Compare
mariomaz
changed the title
mariomaz
requested review from
adam1985d,
arnout,
kantera800,
morantr,
rmelotte,
tomereli and
vitalybu
as code owners
mariomaz
force-pushed
the
bugfix/PPM-311_update_vap_stats_fails_in_glinet
branch
from
722db9a to
e59e3f8
Compare
https://jira.prplfoundation.org/browse/PPM-311 Add a new field to VAPElement structure to hold the BSS (name) of the VAP. This field will later be used "as is", instead of the hardcoded value that is currently being computed from the VAP ID with method `beerocks::utils::get_iface_string_from_iface_vap_ids` Signed-off-by: Mario Maz <mmazzapater@gmail.com>
https://jira.prplfoundation.org/browse/PPM-311 Fill in the BSS field of the VAPElement structure while refreshing VAP info. Signed-off-by: Mario Maz <mmazzapater@gmail.com>
https://jira.prplfoundation.org/browse/PPM-311 Fill in the BSS field of the VAPElement structure while refreshing VAP info. Signed-off-by: Mario Maz <mmazzapater@gmail.com>
https://jira.prplfoundation.org/browse/PPM-311 Do not use `beerocks::utils::get_iface_string_from_iface_vap_ids()` to compute the VAP name from the VAP id. Instead of the hardcoded value, use the BSS field of the VAPElement structure which was read from device. Signed-off-by: Mario Maz <mmazzapater@gmail.com>
https://jira.prplfoundation.org/browse/PPM-311 Do not use `beerocks::utils::get_iface_string_from_iface_vap_ids()` to compute the VAP name from the VAP id. Instead of the hardcoded value, use the BSS field of the VAPElement structure which was read from device. Signed-off-by: Mario Maz <mmazzapater@gmail.com>
mariomaz
force-pushed
the
bugfix/PPM-311_update_vap_stats_fails_in_glinet
branch
from
c7c6408 to
a1fbd87
Compare
| /** | ||
| * Basic Service Set (i.e.: VAP name, e.g.: wlan0.0, wlan0.1, wlan0.2, ...). | ||
| */ | ||
| std::string bss; |
There was a problem hiding this comment.
bss is used in few meanings and formats along the code:
- Sometimes it is a
stringrepresenting mac address. - Sometimes it is a sMacAddr representing mac address.
- and here it is the name of the vap.
I think it would be easier for developers, newcomers, maintainers, etc. to have clear names along the code.
I suggest to replace to vap_name (or just vap) as you explained in the comment above this variable
(i.e.: VAP name, e.g.: wlan0.0, wlan0.1, wlan0.2, ...)
| std::string bss; | |
| std::string vap_name; |
There was a problem hiding this comment.
I agree with you that there's room for improvement regarding names and types in this project.
But in this case, I think the field that contains a MAC address (sometimes as a string and others as a sMacAddr) is the BSSID, not the BSS.
The reason to chose bss over any other name is simple: the information contained in the VAPElement struct is obtained using the hostapd command "STATUS", whose output in a RAX40 is like:
...
bss[0]=wlan0
bssid[0]=02:9a:96:fb:59:0f
ssid[0]=dummy_ssid_0
num_sta[0]=0
bss[1]=wlan0.0
bssid[1]=02:9a:96:fb:59:11
ssid[1]=prplMesh
num_sta[1]=0
bss[2]=wlan0.1
bssid[2]=02:9a:96:fb:59:12
ssid[2]=wave_11
num_sta[2]=0
bss[3]=wlan0.2
bssid[3]=02:9a:96:fb:59:13
ssid[3]=wave_12
num_sta[3]=0
bss[4]=wlan0.3
bssid[4]=02:9a:96:fb:59:14
ssid[4]=wave_13
num_sta[4]=0
Fields bssid and ssid where already present in the VAPElement struct and then I just added the new bss field.
What we might do is to rename mac to bssid in VAPElement, but that's not part of this PR.
| // New VAP Element | ||
| VAPElement vapElement; | ||
|
|
||
| vapElement.bss = ifname; |
There was a problem hiding this comment.
naming (see previous comment regarding this issue)
this is so misleading:
bss == ifname == mac (sometimes) == vap
| if (radio_vaps.find(vap_id) != radio_vaps.end()) { | ||
| auto iface_name = beerocks::utils::get_iface_string_from_iface_vap_ids( | ||
| mon_wlan_hal->get_radio_info().iface_name, vap_id); | ||
| auto iface_name = radio_vaps.at(vap_id).bss; |
There was a problem hiding this comment.
again, the naming issue... I think the idea is clear at this point, too many names for the same thing.
There was a problem hiding this comment.
As said, I agree with you. But if I rename this variable, then I'll also have to rename the parameter of the method where it's used. And then the monitor_vap_node class. And then ... And that's out of the scope of this PR.
4 participants
PPM-311
Method
beerocks::utils::get_iface_string_from_iface_vap_idscomputes VAP name from interface name and VAP id using a hardcoded formula that produces names like "wlan0.0", "wlan0.1" and so on. While this is working fine for devices like RAX40, it does not work with Gl.Inet B1300, where there's only one VAP and it has the same name as the interface, so the computed VAP name does not exist.To fix the problem, BSS (or name) of the VAP is now read from the device instead of computing it.