bwl: update basic radio capabilities #1065

itayx · 2020-04-01T09:21:04Z

The supported_channels are needed to created the Radio Basic Capabilities TLV
To do so they need to be sent to the son_slave_thread, where they will be stored locally
Then the son_slave_thread needs to sent the stored hardware_supported_channels to the son_master_thread as part of the Radio Basic Capabilities TLV

tomereli · 2020-04-01T17:05:57Z

I don't like where this is going - reading supported channels should be done as part of refresh_radio_info. Please make sure to follow this direction.

common/beerocks/tlvf/yaml/beerocks/tlvf/beerocks_message_apmanager.yaml

agent/src/beerocks/slave/ap_manager_thread.cpp

arnout

In addition to the comments I made: I would expect supported_channels to be removed from sNodeHostap as well. Is there any reason to keep it there?

Not marking as changes requested because I don't know if I'll still do the re-review.

agent/src/beerocks/slave/ap_manager_thread.cpp

common/beerocks/tlvf/yaml/beerocks/tlvf/beerocks_message_apmanager.yaml

agent/src/beerocks/slave/ap_manager_thread.cpp

agent/src/beerocks/slave/son_slave_thread.h

kantera800

I saw couple valuable comments in this PR that need to be addressed.
IMO this is what is still missing:

in handle_hostapd_attached() - when there is no "acs report" need to update_preference_channels_from_supoorted_channels() instead of read_supported_channels()
need to add controller commit that removes set_hostap_supported_channels() from handle_intel_slave_join() since only add_hostap_supported_operating_class() will handle this from now on.
verify changes in 2 scenarios :
a. Radio configured as Auto channel
b. Radio configured as Static channel
print "supported_channels" after autoconfig_wsc_parse_radio_caps().
Check that in both scenarios prints are the same.
add the result to PR description.

itayx · 2020-04-13T08:46:21Z

This is the result of the print in master_thread::autoconfig_wsc_parse_radio_caps after the changes made in this PR

I checked the print two times, one with the radio channel set to auto
and one with the radio channel set to a static value (5G: 36, 2.4G: 5)

While on auto-channel:

operating_class=115
maximum_transmit_power_dbm=30
channel list={ 36 40 44 48 }
statically_non_operable_channel_list={ }
operating_class=116
maximum_transmit_power_dbm=30
channel list={ 36 44 }
statically_non_operable_channel_list={ }
operating_class=117
maximum_transmit_power_dbm=30
channel list={ 40 48 }
statically_non_operable_channel_list={ }
operating_class=124
maximum_transmit_power_dbm=30
channel list={ 149 153 157 161 }
statically_non_operable_channel_list={ }
operating_class=125
maximum_transmit_power_dbm=30
channel list={ 149 153 157 161 165 169 }
statically_non_operable_channel_list={ 169 }
operating_class=126
maximum_transmit_power_dbm=30
channel list={ 149 157 }
statically_non_operable_channel_list={ }
operating_class=127
maximum_transmit_power_dbm=30
channel list={ 153 161 }
statically_non_operable_channel_list={ }
operating_class=81
maximum_transmit_power_dbm=30
channel list={ 1 2 3 4 5 6 7 8 9 10 11 12 13 }
statically_non_operable_channel_list={ 12 13 }
operating_class=83
maximum_transmit_power_dbm=30
channel list={ 1 2 3 4 5 6 7 8 9 }
statically_non_operable_channel_list={ }

while on static channel:

operating_class=81
maximum_transmit_power_dbm=30
channel list={ 1 2 3 4 5 6 7 8 9 10 11 12 13 }
statically_non_operable_channel_list={ 12 13 }
operating_class=83
maximum_transmit_power_dbm=30
channel list={ 1 2 3 4 5 6 7 8 9 }
statically_non_operable_channel_list={ }
operating_class=115
maximum_transmit_power_dbm=30
channel list={ 36 40 44 48 }
statically_non_operable_channel_list={ }
operating_class=116
maximum_transmit_power_dbm=30
channel list={ 36 44 }
statically_non_operable_channel_list={ }
operating_class=117
maximum_transmit_power_dbm=30
channel list={ 40 48 }
statically_non_operable_channel_list={ }
operating_class=124
maximum_transmit_power_dbm=30
channel list={ 149 153 157 161 }
statically_non_operable_channel_list={ }
operating_class=125
maximum_transmit_power_dbm=30
channel list={ 149 153 157 161 165 169 }
statically_non_operable_channel_list={ 169 }
operating_class=126
maximum_transmit_power_dbm=30
channel list={ 149 157 }
statically_non_operable_channel_list={ }
operating_class=127
maximum_transmit_power_dbm=30
channel list={ 153 161 }

controller/src/beerocks/master/son_master_thread.cpp

agent/src/beerocks/slave/son_slave_thread.cpp

common/beerocks/bwl/include/bwl/ap_wlan_hal.h

common/beerocks/bwl/nl80211/ap_wlan_hal_nl80211.h

agent/src/beerocks/slave/ap_manager_thread.cpp

tomereli

This PR is wrong and confusing as hell. You should build a new one which does this with the following commits:

rename supported_channels -> prefered_channels
change prefered_channels to use std::array
add supported_channels using nl

tomereli · 2020-04-20T10:41:15Z

common/beerocks/bwl/common/base_wlan_hal.cpp

@@ -31,6 +31,7 @@ base_wlan_hal::base_wlan_hal(HALType type, std::string iface_name, IfaceType ifa
    }

    // Initialize complex containers of the radio_info structure
+    m_radio_info.preferred_channels.resize(128 /* TODO: Get real value */);


TODO should be accompanied with an issue URL which should fix it.

Also, the commit message is really short.

Suggestion:

Preparative commit.
Currently, the supported channels used in prplMesh per radio are the channels read from ACS (Automatic channel scan) report. This is wrong, the ACS report holds the prefered channels, not the supported channels.
The supported channels should be read from the driver via NL, and a new preferred channels vector should be added to hold the ACS results. This will be done in followup commits, for now, just add empty preferred_channels to the radio info structure.

common/beerocks/bwl/common/base_wlan_hal.cpp

common/beerocks/tlvf/AutoGenerated/include/beerocks/tlvf/beerocks_message_common.h

agent/src/beerocks/slave/son_slave_thread.cpp

…LENGTH The SUPPORTED_CHANNELS_LENGTH is used throughout the code-base. It is used to set the maximum number of channels in the preferred and supported channel list. Since the SUPPORTED_CHANNELS_LENGTH isn't used only for the supported_channels but is used for all radio channel the current name is incorrect. Rename the SUPPORTED_CHANNELS_LENGTH to RADIO_CHANNELS_LENGTH. Increase the value to 128 to allow for the full size of the available channels. Signed-off-by: Itay Elenzweig <[email protected]>

In ap_manager_thread there are two helper functions, copy_radio_supported_channels and get_radio_supported_channels_string They only work on the ap_wlan_hal's supported_channels. Change the functions to take the supported_channels vector to make them usable for other radio channel lists. Signed-off-by: Itay Elenzweig <[email protected]>

Currently the preferred channels are used in the code-base under the wrong name "supported_channels". Change supported_channels to preferred_channels Change the container type to std::array Signed-off-by: Itay Elenzweig <[email protected]>

Currently only the preferred_channels are used throughout the codebase. They is sent as part of the ACTION_APMANAGER_JOINED_NOTIFICATION message. To support the AP Radio Basic Capabilities the supported_channels gathered from the NL80211 need to be used instead. Add supported_channels to the RadioInfo in the bwl Add supported_channels to the ACTION_APMANAGER_JOINED_NOTIFICATION message. Signed-off-by: Itay Elenzweig <[email protected]>

In handle_hostapd_attached the ACS report is used to fill the preferred_channels array. If the ACS is not enabled the supported_channels are used instead. Create a function that copies the supported_channels to the preferred_channels. Use the function when the ACS is not enabled. Read the supported channels regardless of the ACS. Signed-off-by: Itay Elenzweig <[email protected]>

The supported_channels are sent to the son_slave but are not used. The supported_channels need to be used in the AP radio capabilities during the JOIN_MASTER state. Store the supported_channels from ACTION_APMANAGER_JOINED_NOTIFICATION. Use the supported_channels for add_ap_radio_basic_capabilities. Signed-off-by: Itay Elenzweig <[email protected]>

The supported channels are sent to the son_slave from the AP manager. Then they are sent from the son_slave to the son_master as part of the. AP Radio Basic Capabilities message that is then processed in autoconfig_wsc_parse_radio_caps. Add a print in the son_master to see the AP Radio Basic Capabilities. Signed-off-by: Itay Elenzweig <[email protected]>

tomereli

Better, but still a lot of comments, some major, so asking for re-review.

tomereli · 2020-04-21T12:57:36Z

common/beerocks/bcl/include/bcl/beerocks_defines.h

@@ -43,7 +43,7 @@ enum eStructsConsts {
    VERSION_LENGTH                = 16,
    NODE_NAME_LENGTH              = 32,
    IFACE_NAME_LENGTH             = 32 + 4, //need extra 1 byte for null termination + alignment
-    SUPPORTED_CHANNELS_LENGTH     = 64,     //support upto # channels, every channel item is 32-bit
+    RADIO_CHANNELS_LENGTH         = 128,    //support upto # channels, every channel item is 32-bit


Why change to 128 by default?

why change at all? it should be changed to supported channels length and when you add the preferred channels length we will use a new enum.

The name is bad - NUM_SUPPORTED_CHANNELS and NUM_PREFERRED_CHANNELS is better.

tomereli · 2020-04-21T13:03:11Z

agent/src/beerocks/slave/ap_manager_thread.cpp

@@ -33,30 +33,28 @@ using namespace beerocks::net;
 /////////////////////////// Local Module Functions ///////////////////////////
 //////////////////////////////////////////////////////////////////////////////

-static void copy_radio_supported_channels(std::shared_ptr<bwl::ap_wlan_hal> &ap_wlan_hal,
-                                          beerocks::message::sWifiChannel supported_channels[])
+static void copy_radio_channels(std::vector<bwl::WiFiChannel> radio_channels,


I think a better name now should be just copy_channels cause thats all it does:

static void copy_channels(std::vector<bwl::WiFiChannel> &from, beerocks::message::sWiFiChannel to[]);

(nitpick)

tomereli · 2020-04-21T13:04:38Z

agent/src/beerocks/slave/ap_manager_thread.cpp

@@ -1569,6 +1569,9 @@ void ap_manager_thread::handle_hostapd_attached()
                beerocks::message::VHT_MCS_SET_SIZE, notification->params().vht_mcs_set);

    // Copy the channels supported by the AP


Suggested change

// Copy the channels supported by the AP

tomereli · 2020-04-21T13:09:15Z

common/beerocks/bwl/dwpal/ap_wlan_hal_dwpal.cpp

@@ -1551,8 +1551,8 @@ bool ap_wlan_hal_dwpal::read_supported_channels()
        }
    }

-    m_radio_info.preferred_channels.insert(m_radio_info.preferred_channels.begin(),
-                                           supported_channels.begin(), supported_channels.end());
+    m_radio_info.supported_channels.insert(m_radio_info.supported_channels.begin(),


This looks like a bug - isn't it?

Suggested change

m_radio_info.supported_channels.insert(m_radio_info.supported_channels.begin(),

m_radio_info.supported_channels.insert(m_radio_info.supported_channels.begin(),

supported_channels.begin(), supported_channels.end());

Now that I look at it again - this is part of a functino which shouldn't exist in this commit - bool ap_wlan_hal_dwpal::read_supported_channels() - I'm confused, didn't the previous commit replace all occurrences of supported channels with preferred channels?

tomereli · 2020-04-21T13:12:01Z

common/beerocks/bwl/nl80211/ap_wlan_hal_nl80211.cpp

@@ -469,7 +469,7 @@ bool ap_wlan_hal_nl80211::read_supported_channels()
        }
    }

-    m_radio_info.preferred_channels.insert(m_radio_info.preferred_channels.begin(),
+    m_radio_info.supported_channels.insert(m_radio_info.supported_channels.begin(),


Here as well - the read_supported_channels() should not exist in this commit.

tomereli · 2020-04-21T13:15:25Z

common/beerocks/tlvf/yaml/beerocks/tlvf/beerocks_message_apmanager.yaml

@@ -25,6 +25,9 @@ cACTION_APMANAGER_4ADDR_STA_JOINED:

 cACTION_APMANAGER_JOINED_NOTIFICATION:
  _type: class
+  supported_channels:


This should be in a separate commit with the autogenerated files changes (nitpick)

tomereli · 2020-04-21T13:16:38Z

agent/src/beerocks/slave/ap_manager_thread.cpp

@@ -1512,6 +1512,7 @@ void ap_manager_thread::handle_hostapd_attached()
 {
    LOG(DEBUG) << "handling enabled hostapd";

+    ap_wlan_hal->read_supported_channels();


Why do we read the supported channels here? we anyway read it from NL so why every time we attach to hostapd? This should be done only once.

tomereli · 2020-04-21T13:18:29Z

agent/src/beerocks/slave/ap_manager_thread.cpp

@@ -1527,7 +1528,7 @@ void ap_manager_thread::handle_hostapd_attached()
            usleep(ACS_READ_SLEEP_USC);
        }
    } else {
-        ap_wlan_hal->read_supported_channels();
+        ap_wlan_hal->update_preference_channels_from_supported_channels();


No - the logic of how we choose the channel preference should be abstracted by the BWL.
So instead we should have a read_preferred_channels() API in BWL which internally will use the radio supported channels we read at boot if ACS is disabled.
So - this whole commit should be removed, and replaced with a simple clear API to read preferred channels exactly like the API to read supported channels.

tomereli · 2020-04-21T13:19:47Z

common/beerocks/bwl/nl80211/ap_wlan_hal_nl80211.cpp

@@ -474,6 +474,12 @@ bool ap_wlan_hal_nl80211::read_supported_channels()
    return true;
 }

+bool ap_wlan_hal_nl80211::update_preference_channels_from_supported_channels()
+{
+    m_radio_info.preferred_channels = m_radio_info.supported_channels;


So you already do this here - so the API is very simple.. Just use m_radio_info.supported_channels if ACS is disabled.

github-actions bot assigned itayx Apr 1, 2020

itayx force-pushed the feature/update-basic-radio-capabilities branch from 8965e83 to b0b8a9e Compare April 1, 2020 09:32

itayx linked an issue Apr 1, 2020 that may be closed by this pull request

[TASK] Update Radio Basic Capabilities TLV using the new supported_channels #1064

Closed

itayx force-pushed the feature/update-basic-radio-capabilities branch 2 times, most recently from 95f0cc2 to bd8d2c9 Compare April 1, 2020 11:05

itayx changed the title ~~bwl: refactor read_supported_channels to use get_radio_info~~ bwl: update basic radio capabilities Apr 1, 2020

itayx force-pushed the feature/update-basic-radio-capabilities branch 3 times, most recently from 5df65ff to 6b4002f Compare April 6, 2020 12:18

itayx marked this pull request as ready for review April 6, 2020 12:19

itayx requested review from arnout, morantr, rmelotte and tomereli as code owners April 6, 2020 12:19

itayx force-pushed the feature/update-basic-radio-capabilities branch from 6b4002f to 547a603 Compare April 6, 2020 14:49

kantera800 mentioned this pull request Apr 7, 2020

[TASK][Channel scan] use AP Radio Basic Capabilities TLV to validate channel pool #686

Closed

4 tasks

itayx force-pushed the feature/update-basic-radio-capabilities branch from 547a603 to cb65b88 Compare April 7, 2020 09:02

tomereli suggested changes Apr 7, 2020

View reviewed changes

arnout reviewed Apr 7, 2020

View reviewed changes

kantera800 self-requested a review April 8, 2020 11:45

kantera800 suggested changes Apr 8, 2020

View reviewed changes

itayx force-pushed the feature/update-basic-radio-capabilities branch from cb65b88 to 2c2f23c Compare April 12, 2020 16:10

itayx requested a review from vitalybu as a code owner April 12, 2020 16:10

itayx force-pushed the feature/update-basic-radio-capabilities branch 3 times, most recently from 83f8546 to bfc3d86 Compare April 15, 2020 12:22

itayx force-pushed the feature/update-basic-radio-capabilities branch from bfc3d86 to 7264120 Compare April 16, 2020 09:22

kantera800 reviewed Apr 19, 2020

View reviewed changes

controller/src/beerocks/master/son_master_thread.cpp Show resolved Hide resolved

itayx force-pushed the feature/update-basic-radio-capabilities branch from 7264120 to 3b8bd4b Compare April 19, 2020 18:43

itayx requested review from kantera800 and tomereli April 19, 2020 18:43

kantera800 reviewed Apr 20, 2020

View reviewed changes

agent/src/beerocks/slave/son_slave_thread.cpp Outdated Show resolved Hide resolved

kantera800 reviewed Apr 20, 2020

View reviewed changes

agent/src/beerocks/slave/son_slave_thread.cpp Outdated Show resolved Hide resolved

kantera800 reviewed Apr 20, 2020

View reviewed changes

common/beerocks/bwl/include/bwl/ap_wlan_hal.h Outdated Show resolved Hide resolved

kantera800 reviewed Apr 20, 2020

View reviewed changes

common/beerocks/bwl/nl80211/ap_wlan_hal_nl80211.h Outdated Show resolved Hide resolved

kantera800 reviewed Apr 20, 2020

View reviewed changes

agent/src/beerocks/slave/ap_manager_thread.cpp Outdated Show resolved Hide resolved

kantera800 reviewed Apr 20, 2020

View reviewed changes

agent/src/beerocks/slave/ap_manager_thread.cpp Outdated Show resolved Hide resolved

tomereli suggested changes Apr 20, 2020

View reviewed changes

itayx force-pushed the feature/update-basic-radio-capabilities branch from 3b8bd4b to 14facdf Compare April 20, 2020 21:21

itayx added 7 commits April 21, 2020 07:35

itayx force-pushed the feature/update-basic-radio-capabilities branch from 14facdf to ea6d3f5 Compare April 21, 2020 07:37

tomereli suggested changes Apr 21, 2020

View reviewed changes

itayx closed this May 20, 2020

itayx deleted the feature/update-basic-radio-capabilities branch May 20, 2020 07:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bwl: update basic radio capabilities #1065

bwl: update basic radio capabilities #1065

itayx commented Apr 1, 2020

tomereli commented Apr 1, 2020

arnout left a comment

kantera800 left a comment

itayx commented Apr 13, 2020

tomereli left a comment

tomereli Apr 20, 2020

tomereli Apr 20, 2020

tomereli left a comment

tomereli Apr 21, 2020

tomereli Apr 21, 2020

tomereli Apr 21, 2020

tomereli Apr 21, 2020

tomereli Apr 21, 2020

tomereli Apr 21, 2020

tomereli Apr 21, 2020

tomereli Apr 21, 2020

tomereli Apr 21, 2020

tomereli Apr 21, 2020

		@@ -1569,6 +1569,9 @@ void ap_manager_thread::handle_hostapd_attached()
		beerocks::message::VHT_MCS_SET_SIZE, notification->params().vht_mcs_set);

		// Copy the channels supported by the AP

	m_radio_info.supported_channels.insert(m_radio_info.supported_channels.begin(),
	m_radio_info.supported_channels.insert(m_radio_info.supported_channels.begin(),
	supported_channels.begin(), supported_channels.end());

bwl: update basic radio capabilities #1065

bwl: update basic radio capabilities #1065

Conversation

itayx commented Apr 1, 2020

tomereli commented Apr 1, 2020

arnout left a comment

Choose a reason for hiding this comment

kantera800 left a comment

Choose a reason for hiding this comment

itayx commented Apr 13, 2020

While on auto-channel:

while on static channel:

tomereli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomereli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment