Skip to content
This repository was archived by the owner on Sep 7, 2020. It is now read-only.

update_vap_stats() fails for wlan0.0 in Gl.Inet B1300 #1563

Open
mariomaz wants to merge 7 commits into
base: master
Choose a base branch
from
Open

update_vap_stats() fails for wlan0.0 in Gl.Inet B1300 #1563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
037603a
bwl: add bss field to VAPElement struct
mariomaz Jul 27, 2020
ab0dc74
bwl: nl80211: update refresh_vap_info to fill in bss field
mariomaz Jul 27, 2020
54f067d
bwl: dwpal: update refresh_vap_info to fill in bss field
mariomaz Jul 28, 2020
4803cda
fronthaul: monitor: use bss field of VAPElement struct
mariomaz Jul 27, 2020
a1fbd87
fronthaul: ap_manager: use bss field of VAPElement struct
mariomaz Jul 28, 2020
828137f
fixup! bwl: add bss field to VAPElement struct
mariomaz Aug 11, 2020
0ee69e4
fixup! bwl: nl80211: update refresh_vap_info to fill in bss field
mariomaz Aug 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions agent/src/beerocks/fronthaul_manager/ap_manager/ap_manager_thread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -948,9 +948,12 @@ bool ap_manager_thread::handle_cmdu(Socket *sd, ieee1905_1::CmduMessageRx &cmdu_
return element.second.mac == bssid;
});

auto vap_id = it->first;
auto vap_name = utils::get_iface_string_from_iface_vap_ids(
ap_wlan_hal->get_radio_info().iface_name, vap_id);
if (vap_unordered_map.end() == it) {
LOG(ERROR) << "BSSID " << bssid << " not found";
return false;
}

auto vap_name = it->second.bss;

if (!request->params().remove) {
if (!ap_wlan_hal->sta_softblock_add(
Expand Down
3 changes: 1 addition & 2 deletions agent/src/beerocks/fronthaul_manager/monitor/monitor_thread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2241,8 +2241,7 @@ void monitor_thread::update_vaps_in_db()

// if vap exist in HAL, update it in the local db.
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;
Copy link
Collaborator

@RanRegev RanRegev Aug 5, 2020

Choose a reason for hiding this comment

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

again, the naming issue... I think the idea is clear at this point, too many names for the same thing.

Copy link
Collaborator Author

@mariomaz mariomaz Aug 11, 2020

Choose a reason for hiding this comment

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

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.


auto curr_vap = radio_vaps.at(vap_id);

Expand Down
1 change: 1 addition & 0 deletions common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -969,6 +969,7 @@ bool base_wlan_hal_dwpal::refresh_vap_info(int vap_id)
// New VAP Element
VAPElement vapElement;

vapElement.bss = ifname;
Copy link
Collaborator

@RanRegev RanRegev Aug 5, 2020

Choose a reason for hiding this comment

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

naming (see previous comment regarding this issue)
this is so misleading:
bss == ifname == mac (sometimes) == vap

vapElement.mac = std::string(bssid);
vapElement.ssid = std::string(ssid);
if (!get_vap_type(ifname, vapElement.fronthaul, vapElement.backhaul)) {
Expand Down
13 changes: 7 additions & 6 deletions common/beerocks/bwl/include/bwl/base_wlan_hal_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,20 +46,21 @@ enum class AntMode { Invalid = 0, ANT_1X1, ANT_2X2, ANT_3X3, ANT_4X4 };
enum class WiFiChanBW { Invalid = 0, BW_20 = 20, BW_40 = 40, BW_80 = 80 };

struct VAPElement {
/**
* Basic Service Set (i.e.: VAP name, e.g.: wlan0.0, wlan0.1, wlan0.2, ...).
*/
std::string bss;
Copy link
Collaborator

@RanRegev RanRegev Aug 5, 2020

Choose a reason for hiding this comment

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

bss is used in few meanings and formats along the code:

  • Sometimes it is a string representing 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, ...)

Suggested change
std::string bss;
std::string vap_name;

Copy link
Collaborator Author

@mariomaz mariomaz Aug 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

std::string ssid;
std::string mac;
bool fronthaul;
bool backhaul;

virtual bool operator==(const VAPElement &other) const
bool operator==(const VAPElement &other) const
{
return (ssid == other.ssid && mac == other.mac);
return (bss == other.bss && ssid == other.ssid && mac == other.mac);
}

virtual bool operator!=(const VAPElement &other) const
{
return (ssid != other.ssid || mac != other.mac);
}
bool operator!=(const VAPElement &other) const { return !(*this == other); }
};

enum class ChanSwReason { Unknown = 0, Radar = 1, CoEx_20 = 2, CoEx_40 = 3 };
Expand Down
8 changes: 5 additions & 3 deletions common/beerocks/bwl/nl80211/base_wlan_hal_nl80211.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,7 +763,8 @@ bool base_wlan_hal_nl80211::refresh_vaps_info(int id)
vap_id++) {
VAPElement vap_element;

// Try reading the BSSID and SSID of the requested VAP
// Try reading values of the requested VAP
vap_element.bss = reply["bss[" + std::to_string(vap_id) + "]"];
vap_element.mac = reply["bssid[" + std::to_string(vap_id) + "]"];
vap_element.ssid = reply["ssid[" + std::to_string(vap_id) + "]"];
// TODO https://github.com/prplfoundation/prplMesh/issues/1175
Expand All @@ -780,15 +781,16 @@ bool base_wlan_hal_nl80211::refresh_vaps_info(int id)

// Store the VAP element
LOG(DEBUG) << "Detected VAP ID (" << vap_id << ") - MAC: " << vap_element.mac
<< ", SSID: " << vap_element.ssid;
<< ", SSID: " << vap_element.ssid << ", BSS: " << vap_element.bss;

m_radio_info.available_vaps[vap_id] = vap_element;
}
} else {

VAPElement vap_element;

// Try reading the BSSID and SSID of the requested VAP
// Try reading values of the requested VAP
vap_element.bss = reply["bss[" + std::to_string(id) + "]"];
vap_element.mac = reply["bssid[" + std::to_string(id) + "]"];
vap_element.ssid = reply["ssid[" + std::to_string(id) + "]"];
// TODO https://github.com/prplfoundation/prplMesh/issues/1175
Expand Down