[PPM-275] Replace CLIENT_BEACON_11K_REQUEST/RESPONSE on the standard one #1585

VladyslavTupikin · 2020-08-05T17:00:16Z

As part of the issue for replacing vendor-specific messages on the standard one (PPM-273) need to replace next messages
on the standard one

Vendor-specific messages:

ACTION_CONTROL_CLIENT_BEACON_11K_REQUEST
ACTION_CONTROL_CLIENT_BEACON_11K_RESPONSE

Easy-mesh messages:

BEACON_METRICS_QUERY_MESSAGE 
BEACON_METRICS_RESPONSE_MESSAGE

According to the EMR2 specification (17.2.27 BeaconMetricsQuery) need to add h-field - Number of AP Channel Reports. Add new uint8_t variable between ssid and ap_channel_reports_list_length. Also add updated autogenerated files. https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

Need to create new method for sending BEACON_METRICS_QUERY_MESSAGE to the agent. Create new method with filling up fields according to the EMR2 specification. Also autoformatting removed whitespace from 1 commentary. https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

Need to replace code for creating vendor-specific message CLIENT_BEACON_11K_REQUEST on the new method for creating BEACON_METRICS_QUERY_MESSAGE. Replace code for case CHECK_11K_BEACON_MEASURE_CAP on the call send_beacon_metrics_query_msg(). Also autoformatting removed whitespace for commentary. https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

Need to replace CLIENT_BEACON_11K_REQUEST vendor-specific message on the same easy mesh message. Replace code for creating CLIENT_BEACON_11K_REQUEST message on the method send_beacon_metrics_query_msg(). Also autoformating removed whitespace from the commentary. https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

Need to replace creating vendor-specific message ACTION_CONTROL_CLIENT_BEACON_11K_REQUEST on the standard one. Replace creation of vendor-specific message on the forwarding standard BEACON_METRICS_QUERY_MESSAGE. https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

Need to create method for handling BEACON_METRICS_QUERY_MESSAGE. Create method handle_beacon_metrics_query(). https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

Need to add new method for creation BEACON_METRICS_RESPONSE_MESSAGE. Add create_beacon_metrics_response(). https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

Need to modify RRM_Beacon_Response event processing by replacing creation of vendor-specific message on the standard one. Replace creation vendor-specific message ACTION_CONTROL_CLIENT_BEACON_11K_RESPONSE on the call of the new method create_beacon_metrics_response(). https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

Nee to create handler for message BEACON_METRICS_RESPONSE_MESSAGE which coming from the monito. Create method handle_monitor_beacon_metrics_response() which receives data from the monitor and forwards it to the controller. https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

Need to modify ACTION_CLI_CLIENT_BEACON_11K_REQUEST by replacing creation vendor-specific message to the standard one. Replace ACTION_CONTROL_CLIENT_BEACON_11K_REQUEST to BEACON_METRICS_QUERY_MESSAGE. https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

adam1985d

the PR looks good and I liked how you split the commits - much easier to review.
I gave some comments, but wanted to add comments that are communal to all code/commits:

in commit messages:
replace x on y should be replace x with y
many comments don't start with capitals - same for log prints

Would have approved the PR - but I see the tests are failing - the change might also require adjustments in the tests-flows - so not approving until the tests are fixed

adam1985d · 2020-08-09T05:16:19Z

controller/src/beerocks/master/son_actions.cpp

@@ -505,3 +505,70 @@ bool son_actions::send_ap_config_renew_msg(ieee1905_1::CmduMessageTx &cmdu_tx, d

    return result;
 }
+


I think the commit title doesn't stand for itself - would be really hard to differentiate this commit from other based on the title.
would expect the title to mention what is added (not just "method").
e.g.:
controller: son_actions: add beacon_metrics_query method

adam1985d · 2020-08-09T05:16:44Z

controller/src/beerocks/master/son_actions.cpp

+
+    auto measurement_request =
+        cmdu_tx.create(0, ieee1905_1::eMessageType::BEACON_METRICS_QUERY_MESSAGE);
+


line-space can be removed
(nitpick)

adam1985d · 2020-08-09T05:16:49Z

controller/src/beerocks/master/son_actions.cpp

+    }
+
+    auto tlvBeaconMtericsQuery = cmdu_tx.addClass<wfa_map::tlvBeaconMetricsQuery>();
+


line-space can be removed
(nitpick)

adam1985d · 2020-08-09T05:18:13Z

controller/src/beerocks/master/son_actions.cpp

+    // According to the 10.3.3 from EMR1 specification
+    // reporting_detail_value should be 1 for minimize
+    // the disruption potentially caused to the ongoing
+    // traffic of the specified STA


Suggested change

// According to the 10.3.3 from EMR1 specification

// reporting_detail_value should be 1 for minimize

// the disruption potentially caused to the ongoing

// traffic of the specified STA

// According to the 10.3.3 from EMR1 specification reporting_detail_value should be 1 to

// minimize the disruption potentially caused to the ongoing traffic of the specified STA

adam1985d · 2020-08-09T05:18:33Z

controller/src/beerocks/master/son_actions.cpp

+    tlvBeaconMtericsQuery->ssid_length() = ssid.length();
+
+    if (!tlvBeaconMtericsQuery->alloc_ssid(ssid.length() + 1)) {
+        LOG(ERROR) << "Failed allocage buffer for SSID";


Suggested change

LOG(ERROR) << "Failed allocage buffer for SSID";

LOG(ERROR) << "Failed to allocage buffer for SSID";

and what about "allocage" -> "allocate"? 🙂

adam1985d · 2020-08-09T05:42:43Z

agent/src/beerocks/slave/son_slave_thread.cpp

+    }
+
+    auto beacon_metrics_response_tlv = cmdu_tx.addClass<wfa_map::tlvBeaconMetricsResponse>();
+


line-space can be removed
(nitpick)

adam1985d · 2020-08-09T05:46:37Z

agent/src/beerocks/slave/son_slave_thread.cpp

+        return false;
+    }
+
+    // TODO: Add filling up the correct data according to the specification.


it seems the monitor response and the forwarded message contain the exact same TLV: tlvBeaconMetricsResponse.
do we, and if not - shouldn't we, support a full copy of a TLV?
@morantr might be able to answer this.

adam1985d · 2020-08-09T05:48:33Z

controller/src/beerocks/master/son_management.cpp

+        }
+
+        auto beacon_metrics_request_tlv = cmdu_tx.addClass<wfa_map::tlvBeaconMetricsQuery>();
+


line-space can be removed
(nitpick)

adam1985d · 2020-08-09T05:48:54Z

controller/src/beerocks/master/son_management.cpp

+            break;
+        }
+        beacon_metrics_request_tlv->associated_sta_mac() = cli_request->client_mac();
+


line-space can be removed
(nitpick)

adam1985d · 2020-08-09T05:51:14Z

controller/src/beerocks/master/son_management.cpp

+        beacon_metrics_request_tlv->ssid_length() = beerocks::message::WIFI_SSID_MAX_LENGTH;
+
+        if (!beacon_metrics_request_tlv->alloc_ssid(beerocks::message::WIFI_SSID_MAX_LENGTH)) {
+            LOG(ERROR) << "Couldn't allocate memory for SSID.";
            isOK = false;
            break;
        }


Suggested change

beacon_metrics_request_tlv->ssid_length() = beerocks::message::WIFI_SSID_MAX_LENGTH;

if (!beacon_metrics_request_tlv->alloc_ssid(beerocks::message::WIFI_SSID_MAX_LENGTH)) {

LOG(ERROR) << "Couldn't allocate memory for SSID.";

isOK = false;

break;

}

beacon_metrics_request_tlv->ssid_length() = message::WIFI_SSID_MAX_LENGTH;

if (!beacon_metrics_request_tlv->alloc_ssid(message::WIFI_SSID_MAX_LENGTH)) {

LOG(ERROR) << "Couldn't allocate memory for SSID.";

isOK = false;

break;

}

the file contains using namespace beerocks so no need to explicitly specify it.

adam1985d

changed to request-changes due to the test-flows not passing

morantr

I reviewed only the first 4 commits and did not continue because it is clear to me that this PR breaks the functionality of the code.
I think you should close this PR and return to the planning phase. Some infrastructure needs to be added to the controller task infrastructure before even starting with replacing the beacon measurement message.

Also, I would not insert this kind of change without full steering test by QA.
@kantera800 @adam1985d

morantr · 2020-08-11T09:59:29Z

controller/src/beerocks/master/son_actions.cpp

+        return false;
+    }
+
+    auto tlvBeaconMtericsQuery = cmdu_tx.addClass<wfa_map::tlvBeaconMetricsQuery>();


Suggested change

auto tlvBeaconMtericsQuery = cmdu_tx.addClass<wfa_map::tlvBeaconMetricsQuery>();

auto tlvBeaconMetricsQuery = cmdu_tx.addClass<wfa_map::tlvBeaconMetricsQuery>();

morantr · 2020-08-11T10:05:56Z

controller/src/beerocks/master/son_actions.cpp

+    tlvBeaconMtericsQuery->ssid_length() = ssid.length();
+
+    if (!tlvBeaconMtericsQuery->alloc_ssid(ssid.length() + 1)) {
+        LOG(ERROR) << "Failed allocage buffer for SSID";


and what about "allocage" -> "allocate"? 🙂

morantr · 2020-08-11T10:15:29Z

controller/src/beerocks/master/son_actions.cpp

+
+    if (tlvBeaconMtericsQuery->channel_number() != 255) {
+
+        // If the value of Channel Number field is not set to 255,


I see that usually when you writing comments on the code, you start a new line although you still have a lot of space left, before exceeding the 100 characters line limit.
Please don't to it. Use the max characters you can before exceeding the limit.
It is like driving on 40 Km/h on a road with a limit of 110 Km/h.

Also, if you have "TODO" like on a few lines below, start the new line under the "TODO".

morantr · 2020-08-11T10:28:32Z

controller/src/beerocks/master/son_actions.cpp

+    tlvBeaconMtericsQuery->associated_sta_mac() = sta_mac;
+    tlvBeaconMtericsQuery->operating_class() =
+        database.get_hostap_operating_class(tlvf::mac_from_string(bssid));
+    tlvBeaconMtericsQuery->channel_number() = database.get_node_channel(bssid);
+    tlvBeaconMtericsQuery->bssid()          = tlvf::mac_from_string(bssid);


I don't understand why you filled it as like we will always request measurement on the bssid that the station is connected to?
Optimal path request beacon measurements on other bssids, in other channels/operating classes.

morantr · 2020-08-11T10:38:28Z

controller/src/beerocks/master/tasks/association_handling_task.cpp

-        measurement_request->params().measurement_mode =
-            beerocks::MEASURE_MODE_ACTIVE; // son::eMeasurementMode11K "passive"/"active"/"table"
-        measurement_request->params().channel = database.get_node_channel(bssid);
-        measurement_request->params().op_class =
-            database.get_hostap_operating_class(tlvf::mac_from_string(bssid));
-        measurement_request->params().rand_ival = beerocks::
-            BEACON_MEASURE_DEFAULT_RANDOMIZATION_INTERVAL; // random interval - specifies the upper bound of the random delay to be used prior to making the measurement, expressed in units of TUs [=1024usec]
-        measurement_request->params().duration = beerocks::
-            BEACON_MEASURE_DEFAULT_ACTIVE_DURATION; // measurement duration, expressed in units of TUs [=1024usec]
-        measurement_request->params().sta_mac = tlvf::mac_from_string(sta_mac);
-        measurement_request->params().bssid   = tlvf::mac_from_string(bssid);
-        //measurement_request.params.use_optional_ssid = true;
-        measurement_request->params().expected_reports_count = 1;


There are a lot of parameters that we use to send and now we don't. How do we make up for it?
Did you fixate these parameters to hard-coded value on the monitor side?
Please add a description of it on the commit message.

morantr · 2020-08-11T10:41:48Z

controller/src/beerocks/master/tasks/association_handling_task.cpp

-        add_pending_mac(radio_mac, beerocks_message::ACTION_CONTROL_CLIENT_BEACON_11K_RESPONSE);
-        TASK_LOG(DEBUG) << "requested beacon measurement request from sta: " << sta_mac
-                        << " on bssid: " << bssid;
-
-        son_actions::send_cmdu_to_agent(agent_mac, cmdu_tx, database, radio_mac);
-        set_responses_timeout(BEACON_MEASURE_REQ_TIME_SPAN);


You removed the add_pending_ma and the set_response_timeout. So what will happen now the state machine of the task will continue without making sure that responses were received?
If this is something you have a solution for on this PR, please add a description for it on the commit message.

morantr · 2020-08-11T10:46:55Z

controller/src/beerocks/master/tasks/optimal_path_task.cpp

                    }
-                    request->params() = measurement_request;
-


This does not make any sense. There is a state on the task that set the measurement parameters, and you replaced it with calling to a function that sends measurement request only on the bssid that the client is connected to.
This will break the optimal path task.

github-actions bot assigned VladyslavTupikin Aug 5, 2020

Vladyslav Tupikin added 10 commits August 7, 2020 16:38

agent: monitor_thread: Create handle_beacon_metrics_query()

d7ba835

Need to create method for handling BEACON_METRICS_QUERY_MESSAGE. Create method handle_beacon_metrics_query(). https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

agent: monitor_thread: Add new method

6cab1f8

Need to add new method for creation BEACON_METRICS_RESPONSE_MESSAGE. Add create_beacon_metrics_response(). https://jira.prplfoundation.org/browse/PPM-275 Signed-off-by: Vladyslav Tupikin <[email protected]>

VladyslavTupikin force-pushed the feature/ppm-275-replace-vs-11k-by-standard branch from 41dffae to a95d340 Compare August 7, 2020 17:30

VladyslavTupikin marked this pull request as ready for review August 7, 2020 17:30

VladyslavTupikin requested review from adam1985d, arnout, kantera800, mariomaz, morantr, rmelotte, tomereli and vitalybu as code owners August 7, 2020 17:30

adam1985d approved these changes Aug 9, 2020

View reviewed changes

adam1985d suggested changes Aug 9, 2020

View reviewed changes

morantr suggested changes Aug 11, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PPM-275] Replace CLIENT_BEACON_11K_REQUEST/RESPONSE on the standard one #1585

[PPM-275] Replace CLIENT_BEACON_11K_REQUEST/RESPONSE on the standard one #1585

VladyslavTupikin commented Aug 5, 2020

adam1985d left a comment •

edited

Loading

adam1985d Aug 9, 2020

adam1985d Aug 9, 2020

adam1985d Aug 9, 2020

adam1985d Aug 9, 2020

adam1985d Aug 9, 2020

morantr Aug 11, 2020

adam1985d Aug 9, 2020

adam1985d Aug 9, 2020

adam1985d Aug 9, 2020

adam1985d Aug 9, 2020

adam1985d Aug 9, 2020

adam1985d left a comment

morantr left a comment •

edited

Loading

morantr Aug 11, 2020

morantr Aug 11, 2020

morantr Aug 11, 2020

morantr Aug 11, 2020

morantr Aug 11, 2020

morantr Aug 11, 2020

morantr Aug 11, 2020

		@@ -505,3 +505,70 @@ bool son_actions::send_ap_config_renew_msg(ieee1905_1::CmduMessageTx &cmdu_tx, d

		return result;
		}


		auto measurement_request =
		cmdu_tx.create(0, ieee1905_1::eMessageType::BEACON_METRICS_QUERY_MESSAGE);

		}

		auto tlvBeaconMtericsQuery = cmdu_tx.addClass<wfa_map::tlvBeaconMetricsQuery>();

	LOG(ERROR) << "Failed allocage buffer for SSID";
	LOG(ERROR) << "Failed to allocage buffer for SSID";

		}

		auto beacon_metrics_response_tlv = cmdu_tx.addClass<wfa_map::tlvBeaconMetricsResponse>();

		}

		auto beacon_metrics_request_tlv = cmdu_tx.addClass<wfa_map::tlvBeaconMetricsQuery>();

	auto tlvBeaconMtericsQuery = cmdu_tx.addClass<wfa_map::tlvBeaconMetricsQuery>();
	auto tlvBeaconMetricsQuery = cmdu_tx.addClass<wfa_map::tlvBeaconMetricsQuery>();


		if (tlvBeaconMtericsQuery->channel_number() != 255) {

		// If the value of Channel Number field is not set to 255,

[PPM-275] Replace CLIENT_BEACON_11K_REQUEST/RESPONSE on the standard one #1585

Are you sure you want to change the base?

[PPM-275] Replace CLIENT_BEACON_11K_REQUEST/RESPONSE on the standard one #1585

Conversation

VladyslavTupikin commented Aug 5, 2020

adam1985d left a comment • edited Loading

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

Choose a reason for hiding this comment

adam1985d left a comment

Choose a reason for hiding this comment

morantr left a comment • edited Loading

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

adam1985d left a comment •

edited

Loading

morantr left a comment •

edited

Loading