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

adam1985d · 2020-01-21T06:52:51Z

This PR adds CLI BML commands to the beerocks_cli for the following DCS BML APIS:
bml_set_dcs_continuous_scan_enable
bml_get_dcs_continuous_scan_enable
bml_set_dcs_continuous_scan_params
bml_get_dcs_continuous_scan_params
bml_start_dcs_single_scan
bml_get_dcs_scan_results

The commands naming are aligned to the called APIs without the 'bml_' initial:
set_dcs_continuous_scan_enable
get_dcs_continuous_scan_enable
set_dcs_continuous_scan_params
get_dcs_continuous_scan_params
start_dcs_single_scan
get_dcs_scan_results

The command's description is available in the beerocks_cli help menu

adam1985d · 2020-01-21T07:32:45Z

add cli to all DCS BMLs

controller/src/beerocks/cli/beerocks_cli_bml.cpp

tomereli · 2020-01-21T09:44:42Z

controller/src/beerocks/cli/beerocks_cli_bml.h

@@ -190,6 +190,18 @@ class cli_bml : public cli {
    int steering_client_measure(uint32_t steeringGroupIndex, const std::string &str_bssid,
                                const std::string &str_client_mac);
 #endif
+    int set_dcs_continuous_scan_enable(const std::string &radio_mac, int8_t enable);


Please add doxygen to all new APIs either here or in the implementation file.
A rule of thumb - every new function comes with doxygen comments.

I added inside the CPP file (not to clatter the class header)

tomereli · 2020-01-21T09:46:16Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+    } else if (numOfArgs == 3) {
+        return set_dcs_continuous_scan_params(args.stringArgs[0], args.intArgs[1], args.intArgs[2]);
+    } else if (numOfArgs == 4) {
+        std::cout << "Invalid number of arguments, set of channel_pool must also provide "


Why don't we print such an error in other failure cases?

this is a special case where 2 params must be provided together or omitted together

the code was removed as part of refactoring due to @morantr comment

tomereli · 2020-01-21T09:51:10Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+        return -1;
+    }
+
+    struct BML_DCS_NEIGHBOR_AP *results = nullptr;


No need for struct keyword - its CPP.

removed the struct keyword

tomereli · 2020-01-21T10:00:22Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+
+    results =
+        (struct BML_DCS_NEIGHBOR_AP *)calloc(max_results_size, sizeof(struct BML_DCS_NEIGHBOR_AP));
+    if (results == nullptr) {


Did you run KW on this? please refrain from using if (ptr == nullptr) and instead use if (!ptr) to avoid KW issues.

removed the entirely - and will run KW

tomereli · 2020-01-21T10:01:42Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+            std::cout << __func__ << "ERROR: results_count(" << results_count << ")"
+                      << " > max_results_size(" << max_results_size << ")" << std::endl;
+        } else if (status == 0 && results_count > 0) {
+            //todo: replace the temporary debug print with actual prints of the new results structure


I'm not sure I follow - what is temporary with the debug print? Aren't the results already in the new structure?

no - since the PR that changes the structure is not available yet
(PRs 4/5):
#694

tomereli · 2020-01-21T10:06:21Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+    struct BML_DCS_NEIGHBOR_AP *results = nullptr;
+
+    results =
+        (struct BML_DCS_NEIGHBOR_AP *)calloc(max_results_size, sizeof(struct BML_DCS_NEIGHBOR_AP));


There is absolutely no good reason to use calloc or alloc in c++, and especially in this function where you simply print the returned results and free them right after.
Simply declare the results on the stack:

BML_DCS_NEIGHBOR_AP results[max_results_size];

Also - why is the max_results_size argument even needed? This can also be statically declared in the function.

the max_results_size is to simulate the max-results-size limit also used in in RDKB (currently set as 200)
when DCS supports ordering of the results by RSSI or another ranking, it is possible to the get only the X best results even if more are available

I replaced the with a stack declaration.

The best would be to change the DCS WLAN APIs as well - is it feasible?

it might be - you can create a JIRA to check this at a later time - shouldn't block this PR

controller/src/beerocks/cli/beerocks_cli_bml.cpp

controller/src/beerocks/cli/beerocks_cli_bml.h

morantr · 2020-01-21T09:10:41Z

controller/src/beerocks/cli/beerocks_cli_bml.h

+    int set_dcs_continuous_scan_params(const std::string &radio_mac,
+                                       int dwell_time                  = BML_DCS_INVALID_PARAM,
+                                       int interval_time               = BML_DCS_INVALID_PARAM,
+                                       const std::string &channel_pool = std::string(),


Why are you passing channel_pool as a string and not a vector or array? The caller function should parse it and convert it to vector/array.

none of the caller functions in the BML CLI perform any parsing. they are all a wrapper for getting the information from the CLI command to the cli_bml command.

I agree with Moran here - we should do the conversion in the BML CLI and get rid of the size of channel pool.

fixed - removed the channel pool size from user input params. calculated from channel_pool input.

morantr · 2020-01-21T09:11:17Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+}
+
+int cli_bml::get_dcs_scan_results(const std::string &radio_mac, unsigned int max_results_size,
+                                  int8_t is_single_scan)


why the is_single_scan is int8 and not bool?

was changed to bool

morantr · 2020-01-21T09:28:22Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+                       1, STRING_ARG);
+    insertCommandToMap(
+        "bml_set_dcs_continuous_scan_params",
+        "<mac> <dwell_time_msec> [<interval_time>] [<channel_pool>] [<channel_pool_size>]",


You can define it like this only if you set all the optional arguments altogether. This is not the case here, so what you need to do is to explicitly write to what argument do you mean:
<mac> <dwell_time_msec> [ interval_time=<interval_time>] [channel_pool=<channel_pool>]

on the caller function you will need to parse it, and here convert all the argument types to STRING_ARG.

ok - replaced the implementation to what you suggested (used the beerocks_cli_socket::client_beacon_11k_req as reference)

morantr · 2020-01-21T10:11:45Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+    int ret    = bml_get_dcs_continuous_scan_enable(ctx, radio_mac.c_str(), &enable);
+
+    if (ret == BML_RET_OK) {
+        std::cout << "dcs-continuous-scan-enable=" << ((enable == 1) ? "True" : "False")


string_utils::bool_str(enable);

wasn't familiar with it - very useful - fixed, thanks

morantr · 2020-01-21T10:18:24Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+    struct BML_DCS_NEIGHBOR_AP *results = nullptr;
+
+    results =
+        (struct BML_DCS_NEIGHBOR_AP *)calloc(max_results_size, sizeof(struct BML_DCS_NEIGHBOR_AP));


Why didn't you declare it on the stack???
Anyway, this is C allocation. The CPP way is with new/delete and the better option will be to use smart pointers.

@tomereli already mentioned it - I fixed it and am using a user-defined-sized array declared on the stack.

mariomaz · 2020-01-21T15:28:56Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+        "<mac> <dwell_time_msec> [<interval_time>] [<channel_pool>] [<channel_pool_size>]",
+        "Set the continuous scan params for the given AP mac: dwell_time - dwell time in "
+        "miliseconds, interval_time - interval time in seconds,"
+        " channel_pool - channels seperated by commas (set of channel_pool must also provide "


typo: should be "separated"

fixed - thanks

mariomaz · 2020-01-21T15:30:52Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+        "bml_set_dcs_continuous_scan_params",
+        "<mac> <dwell_time_msec> [<interval_time>] [<channel_pool>] [<channel_pool_size>]",
+        "Set the continuous scan params for the given AP mac: dwell_time - dwell time in "
+        "miliseconds, interval_time - interval time in seconds,"


typo: "milliseconds"
This typo is repeated in other commands

fixed - thanks

mariomaz · 2020-01-21T15:33:12Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+                       static_cast<pFunction>(&cli_bml::get_dcs_continuous_scan_params_caller), 1,
+                       1, STRING_ARG);
+    insertCommandToMap(
+        "bml_start_dcs_single_scan", "<mac> <dwell_time_msec> <channel_pool_size> <channel_pool>",


For the sake of consistency, channel_pool_size and channel_pool should be given in the same order in all commands.

I felt the same itch - but this was aligned to the signature of DCS BML APIS.
we decided to remove the channel_pool_size completely (and parse it from the string of channel_pool) - so will be fixed

update - removed the channel_pool_size from user input

mariomaz · 2020-01-21T15:51:29Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+
+int cli_bml::get_dcs_continuous_scan_params(const std::string &radio_mac)
+{
+    std::cout << "get_dcs_continuous_scan_params" << std::endl;


You should probably be using func instead of a literal string to avoid copy & paste errors. This way maintenance is easier (if method name changes, there is no need to change anything else).
This is something to be considered in all the methods in this file.

mariomaz · 2020-01-21T15:51:48Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+                                                 &interval_time, channel_pool, &channel_pool_size);
+
+    if (ret == BML_RET_OK) {
+        std::cout << "dcs-continuous-scan-params for mac:" << std::endl


MAC address is not being printed

mariomaz · 2020-01-21T15:52:54Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+    std::cout << "start_dcs_single_scan" << std::endl;
+
+    if (dwell_time <= 0) {
+        std::cout << __func__ << "invalid input: dwell_time(" << dwell_time << ") <= 0"


Method name is being printed together with next literal string

controller/src/beerocks/cli/beerocks_cli_bml.cpp

mariomaz · 2020-01-21T16:04:36Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+
+int cli_bml::get_dcs_continuous_scan_enable_caller(int numOfArgs)
+{
+    if (numOfArgs == 1) {


I've been using Yoda conditions for a long time now that these lines make me feel uncomfortable.
Some static code analyzers would also complain.

I am familiar with this approach - but this is not maintained in the prpl.

if @tomereli and @arnout want to add this requirement to prplmesh guidelines it is OK, but a short search of the following regex. <[a-z,A-Z] == [0-9]> produced 375 results in 93 files

will not be handled in this PR, but we can set a task for this change across the code if decided

Fix this, we will.

tomereli

I added some more comments, I think you should fix them but if this is urgent it can be done as part of #665 , so approving again.

tomereli · 2020-01-26T12:00:00Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

@@ -24,6 +24,17 @@ using namespace net;
 /////////////////////////// Local Module Functions ///////////////////////////
 //////////////////////////////////////////////////////////////////////////////

+static const std::string string_from_int_array(void *arr, size_t arr_max_size)


The name of this function implies it creates a string from an array of integers. So it should be

static const std::string string_from_int_array(int arr[], size_t len);

Also - please add a doxygen comment explaining this function.

tomereli · 2020-01-26T12:02:20Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+                          << "  channel=" << int(res.ap_Channel) << std::endl
+                          << "  signal_strength=" << int(res.ap_SignalStrength) << std::endl
+                          << "  security_mode_enabled="
+                          << string_from_int_array(res.ap_SecurityModeEnabled,


I'm think that security mode should be a single value - I'm dazzled why you chose array. Please make it a single value unless there's a very good reason why you need an array. Then we can get rid of this weird string_from_int_array function.

tomereli · 2020-01-26T12:02:52Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

+                                                   BML_CHANNEL_SCAN_ENUM_LIST_SIZE)
+                          << std::endl
+                          << "  encryption_mode="
+                          << string_from_int_array(res.ap_EncryptionMode,


Same here - there is a single encryption mode set at a given time. I don't understand why it is an array.

tomereli · 2020-01-26T12:07:20Z

controller/src/beerocks/cli/beerocks_cli_bml.cpp

@@ -2021,28 +2032,40 @@ int cli_bml::get_dcs_scan_results(const std::string &radio_mac, uint32_t max_res
            std::cout << __func__ << "ERROR: results_count(" << results_count << ")"
                      << " > max_results_size(" << max_results_size << ")" << std::endl;
        } else if ((status == 0) && (results_count > 0)) {
-            //todo: replace the temporary debug print with actual prints of the new results structure
            for (size_t i = 0; i < results_count; i++) {
                auto res = results[i];
                std::cout << "result[" << i << "]:" << std::endl


Instead of this whole big print, why not add inline std::ostream &operator<<(std::ostream &os, const BML_NEIGHBOR_AP &neigh); to struct BML_NEIGHBOR_AP in bml_defs.h then you can replace this with:

for (size_t i = 0; i < results_count; i++) { std::cout << "result[" << i << "]:" << std::endl << results[i] << std::endl; }

tomereli · 2020-01-26T21:29:38Z

Finally, please rebase and squash before merge.

added the following stubs of BML CLI commands of DCS BML APIs: set_dcs_continuous_scan_enable get_dcs_continuous_scan_enable set_dcs_continuous_scan_params get_dcs_continuous_scan_params start_dcs_single_scan get_dcs_scan_results Signed-off-by: Adam Dov <[email protected]> Signed-off-by: itay elenzweig <[email protected]>

Signed-off-by: Adam Dov <[email protected]> Signed-off-by: itay elenzweig <[email protected]>

The insertion of the BML commands is performed in the setFunctionsMapAndArray() using the insertCommandToMap command. After this change, the following commands are available in the BML CLI: set_dcs_continuous_scan_enable get_dcs_continuous_scan_enable set_dcs_continuous_scan_params get_dcs_continuous_scan_params start_dcs_single_scan get_dcs_scan_results Signed-off-by: Adam Dov <[email protected]> Signed-off-by: itay elenzweig <[email protected]>

The implementation of each command is aligned with the existing DCS wave functions implementation in "wave-api" in RDKB. A detailed description of each command use is available in the CLI help menu. Signed-off-by: Adam Dov <[email protected]> Signed-off-by: itay elenzweig <[email protected]>

adam1985d · 2020-01-27T12:08:38Z

fixed all code review comments and passed all tests

adam1985d requested review from arnout, morantr and tomereli as code owners January 21, 2020 06:52

adam1985d self-assigned this Jan 21, 2020

adam1985d requested a review from mariomaz January 21, 2020 09:15

tomereli suggested changes Jan 21, 2020

View reviewed changes

morantr suggested changes Jan 21, 2020

View reviewed changes

mariomaz reviewed Jan 21, 2020

View reviewed changes

controller/src/beerocks/cli/beerocks_cli_bml.cpp Outdated Show resolved Hide resolved

mariomaz reviewed Jan 21, 2020

View reviewed changes

mariomaz approved these changes Jan 21, 2020

View reviewed changes

tomereli requested review from tomereli and morantr January 22, 2020 16:39

tomereli approved these changes Jan 23, 2020

View reviewed changes

itayx force-pushed the feature/add-bml-cli-commands-to-dynamic-channel-selection-bml-apis branch from 3672fab to 612d86b Compare January 23, 2020 14:45

kantera800 mentioned this pull request Jan 23, 2020

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

Closed

8 tasks

itayx force-pushed the feature/add-bml-cli-commands-to-dynamic-channel-selection-bml-apis branch 2 times, most recently from 2357762 to ee5d3aa Compare January 23, 2020 18:08

adam1985d force-pushed the feature/add-bml-cli-commands-to-dynamic-channel-selection-bml-apis branch from ee5d3aa to 0ecb7a3 Compare January 23, 2020 18:49

kantera800 force-pushed the feature/add-bml-cli-commands-to-dynamic-channel-selection-bml-apis branch from 0ecb7a3 to 5da3456 Compare January 23, 2020 22:18

adam1985d force-pushed the feature/add-bml-cli-commands-to-dynamic-channel-selection-bml-apis branch from 5da3456 to 6836550 Compare January 26, 2020 06:45

adam1985d requested a review from rmelotte as a code owner January 26, 2020 06:45

adam1985d force-pushed the feature/add-bml-cli-commands-to-dynamic-channel-selection-bml-apis branch from 487e46f to dffb919 Compare January 26, 2020 06:57

adam1985d requested a review from tomereli January 26, 2020 07:23

adam1985d force-pushed the feature/add-bml-cli-commands-to-dynamic-channel-selection-bml-apis branch from dffb919 to d51494c Compare January 26, 2020 07:24

morantr approved these changes Jan 26, 2020

View reviewed changes

tomereli approved these changes Jan 26, 2020

View reviewed changes

adam1985d added 4 commits January 27, 2020 12:25

cli_bml:add caller functions to BML CLI commands

9275f70

Signed-off-by: Adam Dov <[email protected]> Signed-off-by: itay elenzweig <[email protected]>

adam1985d force-pushed the feature/add-bml-cli-commands-to-dynamic-channel-selection-bml-apis branch from d51494c to 4ed6526 Compare January 27, 2020 10:28

adam1985d merged commit e57acc3 into master Jan 27, 2020

adam1985d deleted the feature/add-bml-cli-commands-to-dynamic-channel-selection-bml-apis branch January 27, 2020 12:05

tomereli mentioned this pull request Feb 3, 2020

[TASK] prplmesh_utils.sh status enhancements #775

Closed

5 tasks

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

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

Conversation

adam1985d commented Jan 21, 2020

adam1985d commented Jan 21, 2020

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

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 Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam1985d Jan 26, 2020 • 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

adam1985d Jan 21, 2020 • 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

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 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

tomereli commented Jan 26, 2020

adam1985d commented Jan 27, 2020

adam1985d Jan 21, 2020 •

edited

Loading

adam1985d Jan 26, 2020 •

edited

Loading

adam1985d Jan 21, 2020 •

edited

Loading