[DCS Patch 4/5] port dynamic channel selection from rdkb flow #694

itayx · 2020-01-20T16:06:03Z

The Dynamic Channel Selection as a feature can provide information on the supported channels which can be utilized by the management system to provide better availability and user experience.
#562

The Dynamic Channel Selection feature is divided into several pull request, this is the forth out of 5

Dynamic channel selection message flow & actions

Changes in this PR include the following:

agent: monitor_thread: proccess nl events
agent: monitor_thread: handle channel scan events
agent: monitor_thread: handle channel scan trigger CMDU
controller & agent: master & slave: DCS channel scan
controller: son_managment: DCS channel scan stubs
controller: son_managment: DCS channel scan
controller: bml: DCS channel scan setup
controller: bml: DCS channel scan, set/get enable
controller: bml: DCS channel scan, set/get params
controller: bml: DCS channel scan, start single scan
controller: bml: DCS channel scan, get results

Before merging this PR please merge hotfix #695

vitalybu

Reviewed till: controller: bml: DCS channel scan setup

agent/src/beerocks/monitor/monitor_thread.cpp

controller/src/beerocks/master/son_master_thread.cpp

agent/src/beerocks/monitor/monitor_thread.cpp

controller/src/beerocks/master/son_management.cpp

itayx · 2020-01-20T19:13:31Z

This PR is dependent on hotfix #695

arnout · 2020-01-21T09:06:46Z

[DCS Patch 4/5]Port dynamic channel selection

rmelotte

Did a quick code review, lot of the comments are the same. Note that I only blindly reviewed the code itself, not the overall changes, because I don't know enough about it to review it. For this reason I'm only adding this as a comment, not approving nor requesting changes (please take the comments into account though).

Most were:

There were many uses of ptr == nullptr to compare pointers, you should use ! ptr directly. There were a lot of these, I didn't add a comment for all of them.
Please also remove all C style-casts and use a more appropriate one (probably static_cast in all cases).
Also don't use std::string for mac addresses, use sMacAddr instead.

controller/src/beerocks/master/son_master_thread.cpp

agent/src/beerocks/monitor/monitor_thread.cpp

rmelotte · 2020-01-21T10:33:46Z

agent/src/beerocks/monitor/monitor_thread.cpp

@@ -101,6 +102,13 @@ void monitor_thread::stop_monitor_thread()
            mon_hal_int_events = nullptr;
        }

+        if (mon_hal_nl_events) {
+            remove_socket(mon_hal_nl_events);
+            delete mon_hal_nl_events;


By looking at the rest of the file I wonder why we have to resort to explicitly calling delete here, though that's not related to this PR.

@kantera800
I'm not sure myself if this is needed

from looking at the code remove_socket just isn't strong enough

controller/src/beerocks/bml/internal/bml_internal.cpp

controller/src/beerocks/bml/internal/bml_internal.h

agent/src/beerocks/monitor/monitor_thread.cpp

agent/src/beerocks/slave/son_slave_thread.cpp

controller/src/beerocks/master/son_master_thread.cpp

controller/src/beerocks/bml/internal/bml_internal.cpp

tomereli · 2020-01-21T10:58:29Z

controller/src/beerocks/bml/internal/bml_internal.cpp

+                                       const unsigned int max_results_size,
+                                       uint8_t *output_result_status, bool is_single_scan)
+{
+    if (m_sockPlatform == nullptr && !connect_to_platform()) {


Also, please separate to 2 separate if conditions.

controller/src/beerocks/bml/internal/bml_internal.cpp

tomereli · 2020-01-21T11:02:43Z

controller/src/beerocks/bml/internal/bml_internal.cpp

+    //output_results_size will be set to the number of actual returning results
+    *output_results_size = 0;
+    for (auto res : scan_results) {
+        auto out = &((*output_results)[*output_results_size]);


I meant to pass the output result as a reference.

Change the declaration of translate_channel_scan_results to accept BML_NEIGHBOR_AP &res_out instead of BML_NEIGHBOR_AP *res_out and set auto &out = (*output_results)[*output_results_size];

Oh.. my bad
Fixed and added a comment explaining the whole (*output_results)[*output_results_size] deal so it would be more understandable

controller/src/beerocks/bml/internal/bml_internal.h

tomereli · 2020-01-21T19:56:42Z

Channel selection test fails with this pr

tomereli · 2020-01-21T20:07:39Z

Please fix the channel selection test, then rebase squash and we'll do the followup review.

agent/src/beerocks/monitor/monitor_thread.cpp

vitalybu · 2020-01-22T08:27:47Z

controller/src/beerocks/bml/internal/bml_internal.h

@@ -152,6 +152,9 @@ class bml_internal : public beerocks::socket_thread {
    int bml_set_vap_list_credentials(const BML_VAP_INFO *vaps, const uint8_t vaps_num);
    int bml_get_vap_list_credentials(BML_VAP_INFO *vaps, uint8_t &vaps_num);

+    //set and get channel scan enable flag
+    int set_dcs_continuous_scan_enable(const std::string &mac, int enable);
+    int get_dcs_continuous_scan_enable(const std::string &mac, int *output_enable);


Why not reference?

controller/src/beerocks/bml/internal/bml_internal.cpp

Superseded

tomereli

Only small comments only but since the commit messages where not changed yet I'm not approving.
Please fix the new comments, update commit messages appropriately, rebase + squash and ask for a final re-review.

tomereli · 2020-01-22T13:24:51Z

controller/src/beerocks/bml/bml.h

@@ -601,7 +601,7 @@ int bml_get_dcs_continuous_scan_params(BML_CTX ctx, const char *radio_mac, int *
 * @return BML_RET_OK on success.
 */
 int bml_get_dcs_scan_results(BML_CTX ctx, const char *radio_mac,
-                             struct BML_DCS_NEIGHBOR_AP **output_results,
+                             struct BML_NEIGHBOR_AP **output_results,


tomereli · 2020-01-22T13:25:53Z

controller/src/beerocks/bml/bml.cpp

-    return BML_RET_OP_FAILED;
+    auto pBML = static_cast<bml_internal *>(ctx);
+    return pBML->set_dcs_continuous_scan_enable(
+        network_utils::mac_from_string(std::string(radio_mac)), enable);
 }

 int bml_get_dcs_continuous_scan_enable(BML_CTX ctx, const char *radio_mac, int *output_enable)


Change to int bml_get_dcs_continuous_scan_enable(BML_CTX ctx, const char *radio_mac, int *enable)

Or even better as Vitaly suggested - why not use reference?

In the BML I can't use a reference, since I'm not sure how well it will be imported in C
I can change the bml_internal to use a reference instead

tomereli · 2020-01-22T13:26:30Z

controller/src/beerocks/bml/internal/bml_internal.h

@@ -152,6 +152,9 @@ class bml_internal : public beerocks::socket_thread {
    int bml_set_vap_list_credentials(const BML_VAP_INFO *vaps, const uint8_t vaps_num);
    int bml_get_vap_list_credentials(BML_VAP_INFO *vaps, uint8_t &vaps_num);

+    //set and get channel scan enable flag
+    int set_dcs_continuous_scan_enable(const sMacAddr &mac, int enable);
+    int get_dcs_continuous_scan_enable(const sMacAddr &mac, int *enable);


I prefer int bml_get_dcs_continuous_scan_enable(BML_CTX ctx, const char *radio_mac, int &enable)

the BML cannot use a reference since it's used as a C API
but the bml_internal uses reference

controller/src/beerocks/bml/internal/bml_internal.cpp

tomereli · 2020-01-22T13:39:52Z

controller/src/beerocks/bml/internal/bml_internal.cpp

+    //output_results_size will be set to the number of actual returning results
+    *output_results_size = 0;
+    for (auto res : scan_results) {
+        auto out = &((*output_results)[*output_results_size]);


I meant to pass the output result as a reference.

Change the declaration of translate_channel_scan_results to accept BML_NEIGHBOR_AP &res_out instead of BML_NEIGHBOR_AP *res_out and set auto &out = (*output_results)[*output_results_size];

tomereli · 2020-01-22T13:41:05Z

controller/src/beerocks/bml/internal/bml_internal.cpp

@@ -67,6 +67,39 @@ static void config_logger(const std::string log_file = std::string())

 #endif // BEEROCKS_DEBUG

+static void translate_channel_scan_results(const beerocks_message::sChannelScanResults &res_in,


Change to static void translate_channel_scan_results(const beerocks_message::sChannelScanResults &res_in, BML_NEIGHBOR_AP &res_out)

Please always use reference whenever possible.

was clearing config::channel_pool instead scan_results Signed-off-by: itay elenzweig <[email protected]>

Remove the "current state" and "next state" logs Move the log and compare functions to the cpp file Add log on fsm_move_state Signed-off-by: itay elenzweig <[email protected]>

The dynamic channel selection feature uses the NL and dwpal apis to trigger a neighboring scan and uses the results received for analytical data. Open the NL sockets and start listening for events Process the NL events and add handling stubs Signed-off-by: itay elenzweig <[email protected]>

The monitor thread receives channel scan events sent from the wlan hal dwpal and handles them accordingly, by propagating them to the agent Add handling for Channel_Scan_Triggered Add handling for Channel_Scan_New_Results_Ready Add handling for Channel_Scan_Dump_Result Add handling for Channel_Scan_Finished Add handling for Channel_Scan_Abort Signed-off-by: itay elenzweig <[email protected]>

The agent can request the monitor thread to trigger a channel scan Receive the channel scan trigger CMDU Extract parameters and print descriptive log Trigger the channel scan with parameters Signed-off-by: itay elenzweig <[email protected]>

Requests, responses and notifications are propagated from the agent to the controller and to the dynamic channel selection task with no alteration to the data within Create the task and add it to the task list on slave join Handle channel scan trigger request and response Handle channel scan triggered notification Handle channel scan results notification Handle channel scan finish notification Handle channel scan abort notification Signed-off-by: itay elenzweig <[email protected]>

This is a preparative commit for 'son_management: DCS channel scan'. Add DCS task to includes Add channel scan BML CMDUs to cases in "handle_bml_message" Signed-off-by: itay elenzweig <[email protected]>

The son_management's role is to handle incoming bml requests, which means storing values in the DB, launching an event to trigger the DCS scan in the task Handle set/get enable request/response Handle set/get params request/response Handle get results request/response Handle start single scan request/response Signed-off-by: itay elenzweig <[email protected]>

The DCS channel scan feature is used via the BML APIs. Ths is a preparative commit for all 'bml: DCS channel scan' commits. Update the neighboring ap structure to fit new architecture Add delayed response timeout to be used for results Add cases to handle BML CMDUs responses Add doxygen to the bml_internal APIs Signed-off-by: itay elenzweig <[email protected]>

Get/Set the enable flag for the continuous DCS channel scan Implement BML functionality Add bml_internal API Implement response CMDU handling Signed-off-by: itay elenzweig <[email protected]>

Get/Set the scan parameters for the continuous DCS channel scan Implement BML functionality Add bml_internal API Add response promise handling Implement response CMDU handling Signed-off-by: itay elenzweig <[email protected]>

Start the single (one time) channel scan with parameters Implement BML functionality Add bml_internal API Implement response CMDU handling Signed-off-by: itay elenzweig <[email protected]>

Get the scan results for both the single and continuous DCS channel scan Implement BML functionality Add bml_internal API Add response promise handling Implement response CMDU handling Add translation function from BML struct to CMDU struct Signed-off-by: itay elenzweig <[email protected]>

itayx requested review from arnout, morantr and tomereli as code owners January 20, 2020 16:06

kantera800 mentioned this pull request Jan 20, 2020

[TASK] port Dynamic channel selection (DCS) feature from RDKB #562

Closed

8 tasks

kantera800 requested a review from vitalybu January 20, 2020 16:56

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-flow branch from 34ce4b5 to d30c335 Compare January 20, 2020 17:32

vitalybu previously requested changes Jan 20, 2020

View reviewed changes

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-flow branch from d30c335 to bf6b727 Compare January 20, 2020 19:05

rmelotte reviewed Jan 21, 2020

View reviewed changes

tomereli previously requested changes Jan 21, 2020

View reviewed changes

adam1985d mentioned this pull request Jan 21, 2020

Feature/add bml cli commands to dynamic channel selection bml apis #699

Merged

kantera800 reviewed Jan 22, 2020

View reviewed changes

agent/src/beerocks/monitor/monitor_thread.cpp Outdated Show resolved Hide resolved

vitalybu reviewed Jan 22, 2020

View reviewed changes

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-flow branch from 41b6a12 to 22a9b51 Compare January 22, 2020 12:59

tomereli suggested changes Jan 22, 2020

View reviewed changes

ghost assigned itayx Jan 22, 2020

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-flow branch 3 times, most recently from 90d6b81 to 9b0a787 Compare January 22, 2020 17:42

tomereli self-requested a review January 22, 2020 17:45

tomereli approved these changes Jan 22, 2020

View reviewed changes

vitalybu self-requested a review January 22, 2020 17:57

vitalybu approved these changes Jan 22, 2020

View reviewed changes

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-flow branch from 9b0a787 to 8036cbe Compare January 22, 2020 20:18

kantera800 force-pushed the feature/port-dynamic-channel-selection-from-rdkb-flow branch from 8036cbe to 2f6ede3 Compare January 23, 2020 07:52

itayx added 13 commits January 23, 2020 11:38

hotfix: fix clear_channel_scan_results()

1e51e07

was clearing config::channel_pool instead scan_results Signed-off-by: itay elenzweig <[email protected]>

hotfix: spamming logs removed and fixed incorrect logs

240a575

Remove the "current state" and "next state" logs Move the log and compare functions to the cpp file Add log on fsm_move_state Signed-off-by: itay elenzweig <[email protected]>

controller: son_management: DCS channel scan stubs

0e9ac58

This is a preparative commit for 'son_management: DCS channel scan'. Add DCS task to includes Add channel scan BML CMDUs to cases in "handle_bml_message" Signed-off-by: itay elenzweig <[email protected]>

controller: bml: DCS channel scan, set/get enable

08dccb7

Get/Set the enable flag for the continuous DCS channel scan Implement BML functionality Add bml_internal API Implement response CMDU handling Signed-off-by: itay elenzweig <[email protected]>

controller: bml: DCS channel scan, set/get params

b48fbcd

Get/Set the scan parameters for the continuous DCS channel scan Implement BML functionality Add bml_internal API Add response promise handling Implement response CMDU handling Signed-off-by: itay elenzweig <[email protected]>

controller: bml: DCS channel scan, start single scan

99f62e6

Start the single (one time) channel scan with parameters Implement BML functionality Add bml_internal API Implement response CMDU handling Signed-off-by: itay elenzweig <[email protected]>

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-flow branch from 2f6ede3 to c51544b Compare January 23, 2020 09:39

kantera800 merged commit db81eec into master Jan 23, 2020

kantera800 deleted the feature/port-dynamic-channel-selection-from-rdkb-flow branch January 23, 2020 11:27

tomereli mentioned this pull request Feb 3, 2020

[TASK] prplmesh_utils.sh status enhancements #775

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DCS Patch 4/5] port dynamic channel selection from rdkb flow #694

[DCS Patch 4/5] port dynamic channel selection from rdkb flow #694

itayx commented Jan 20, 2020 •

edited

Loading

vitalybu left a comment

itayx commented Jan 20, 2020

arnout commented Jan 21, 2020

rmelotte left a comment

rmelotte Jan 21, 2020

itayx Jan 22, 2020

kantera800 Jan 22, 2020

tomereli Jan 21, 2020

itayx Jan 22, 2020

tomereli Jan 21, 2020

itayx Jan 21, 2020

tomereli Jan 22, 2020

itayx Jan 22, 2020

tomereli commented Jan 21, 2020

tomereli commented Jan 21, 2020

vitalybu Jan 22, 2020

tomereli left a comment

tomereli Jan 22, 2020

tomereli Jan 22, 2020

itayx Jan 22, 2020

tomereli Jan 22, 2020

itayx Jan 22, 2020

tomereli Jan 22, 2020

tomereli Jan 22, 2020

itayx Jan 22, 2020

		@@ -67,6 +67,39 @@ static void config_logger(const std::string log_file = std::string())

		#endif // BEEROCKS_DEBUG

		static void translate_channel_scan_results(const beerocks_message::sChannelScanResults &res_in,

[DCS Patch 4/5] port dynamic channel selection from rdkb flow #694

[DCS Patch 4/5] port dynamic channel selection from rdkb flow #694

Conversation

itayx commented Jan 20, 2020 • edited Loading

Dynamic channel selection message flow & actions

vitalybu left a comment

Choose a reason for hiding this comment

itayx commented Jan 20, 2020

arnout commented Jan 21, 2020

rmelotte 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

tomereli commented Jan 21, 2020

tomereli commented Jan 21, 2020

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

itayx commented Jan 20, 2020 •

edited

Loading