Feature/ppm 5 handle smart steering bml requests and fix code review comments by adam1985d · Pull Request #1569 · prplfoundation/prplMesh

adam1985d · 2020-07-28T09:19:03Z

This PR handles comments raised in the previous controller DB PR - related to the runtime DB in the controller.
As part of this PR - I have also added handling for the following BML requests which complete the BML<->controller integration for the smart-steering feature.
ACTION_BML_CLIENT_GET_CLIENT_LIST_REQUEST
ACTION_BML_CLIENT_SET_CLIENT_REQUEST
ACTION_BML_CLIENT_GET_CLIENT_REQUEST

The above-mentioned Fixes are limited to the BML, BML-CLI, and controller.

The controller<->BPL integration is handled separately by the following PR: #1502

itayx

Looks great

controller/src/beerocks/master/db/db.h

controller/src/beerocks/cli/beerocks_cli_bml.cpp

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

controller/src/beerocks/master/db/node.h

controller/src/beerocks/master/db/db.cpp

controller/src/beerocks/master/son_management.cpp

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

The type of the mac address parameter is incorrect. Changed sta_mac from char[6] to uint8_t[6]. PPM-5. Signed-off-by: Itay Elenzweig <itayx.elenzweig@intel.com> Signed-off-by: Adam Dov <adamx.dov@intel.com>

- In client_set_client changed incoming type from INT_ARG to STRING_ARG. To use parameters as optional parameters, we need to retrieve them from the CLI as strings and convert them to integers. - Added int conversion in the client_set_client LOG print. To print the incoming values in client_set_client, need to convert them to type int, otherwise, they will be printed as Char. - Added tlvf::mac_to_string in the client_get_client LOG print. The received mac address needs to be converted to a readable format to be printed. PPM-5. Signed-off-by: Itay Elenzweig <itayx.elenzweig@intel.com Signed-off-by: Adam Dov <adamx.dov@intel.com>

Changed returning error values in the code to a negative values as expected by the BML. PPM-5. Signed-off-by: Itay Elenzweig <itayx.elenzweig@intel.com> Signed-off-by: Adam Dov <adamx.dov@intel.com>

Removed excess spaces from selected-bands debug print. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

The introduction of the stay_on_selected_device configuration, its DB support, and utilization delayed to the next phase of the smart-steering feature. Removed the stay_on_selected_device from the BML interface - from set_client and get_client APIs related structs. Removed the stay_on_selected_device from the BML CLI interface - both from set_client and get_client CLI functions. Note that the stay_on_selected_device infrastructure in the BML message to/from the controller is not removed, but not used. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

Added verification of "is someone waiting for the promise" before handling the response. Implement the function with error-handling first according to the agreed coding guidelines. Fixed broken return-on-error in the "get_client" implementation. PPM-5. Signed-off-by: Itay Elenzweig <itayx.elenzweig@intel.com> Signed-off-by: Adam Dov <adamx.dov@intel.com>

The BML APIs provide an optional-parameters interface, meaning the user can use the API with some of the parameters set to "NOT_CONFIGURED" value, and these parameters not configured in the DB. However, using the API with all the parameters set to "NOT_CONFIGURED" value is not a valid case and should be handled as an error. Added verification that at least one of the given parameters not set to "NOT_CONFIGURED" value. PPM-5. Signed-off-by: Itay Elenzweig <itayx.elenzweig@intel.com> Signed-off-by: Adam Dov <adamx.dov@intel.com>

The current condition of comparing the client's passed-timelife-delay against max-timelife-delay is correct but doesn't reflect the actual logic behind it which is "is remaining timelife over". This change enables a more coherent comparison of current-client-remaining-timelife-delay against a candidate-client-for-removal-timelife-delay (if the DB is full). Changed the aging condition to "client-remaining-time < 0". PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

The current client_db_entry_to_mac receives a const& to the mac address and internally makes a copy to perform an in-place replacement of characters. Changed the function to receive a copy instead of a const& and changed the implementation accordingly. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

Added `using ValuesMap = std::unordered_map<std::string, std::string>` and replaced DB APIs and private function to use the new `ValuesMap` instead of `std::unordered_map<std::string, std::string>`. The above change is a fix to comment raised in a previous PR, and it results in much cleaner code. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

The current implementation of selected bands mistakenly used the existing beerocks::eFreqType as the storage type. This is not extensible and doesn't define the full possible configurations. Moreover, by using the newly introduced eClientSelectedBands instead, which is used in the BML<->controller messaging, we can simplify the set and get operations and any future extension of supported values. Changed in node class the storage type of the selected bands and its description. Changed the DB APIs to use the int8_t instead of beerocks::eFreqType. Changed the selected bands` default value from beerocks::eFreqType::FREQ_UNKNOWN to beerocks::PARAMETER_NOT_CONFIGURED. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

As part of the load_persistent_db_clients() a corner-case where the user added to the persistent DB more than the allowed max-clients-db-size must be handled. Added a check of the number-of-added-clients against the max-clients-db-size - if the condition is met - the clients' DB is full. If clients DB is full, find a candidate client for removal and compare it against the client read from persistent DB and we are trying to add. If the new client's remaining timelife is greater than the timelife of the candidate_for_removal, then the candidate is removed from the runtime DB and the persistent DB and the new client is added instead to the runtime DB. If the new client's remaining timelife is smaller or equal to the timelife of the candidate_for_removal, then the new client is removed from the persistent DB and we continue to the next client. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

Preparative commit. Added API to get all the clients in the runtime DB that have persistent information configured. The assumption is that a client with persistent data configured must have a timestamp configured (which differs from the default value that the not-configured clients have). The API returns a dqueu of the MAC addresses of the above-mentioned clients. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

Changed the ACTION_BML_CLIENT_GET_CLIENT_LIST_REQUEST handling to read the clients from persistent DB using the get_clients_with_persistent_data_configured() DB API. Removed the related cpp-check issue from cppcheck_existing_issues.txt. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

Change the ACTION_BML_CLIENT_GET_CLIENT_REQUEST handling to read the client information from the DB using the DB APIs. The handling also takes care of the following scenarios: "client does not exist in DB" - handled as an error. "client timestamp not exist" - handled as an error. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

Change the ACTION_BML_CLIENT_SET_CLIENT_REQUEST handling to configure the client information to the runtime DB using the DB APIs. If persistent DB is enabled - the update_client_persistent_db() API called to update the persistent DB with the client information (to prevent multiple persistent DB access overhead). Note that if the client is not yet part of the runtime DB, a node for it added before-hand. Moreover, managing the persistent DB max size limit is handled internally and is transparent to the user. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

When configuring the stay_on_initial_radio the initial_radio needs to be set as well. If stay_on_initial_radio is disabled - clear the initial_radio configuration. If stay_on_initial_radio is enabled and the client is connected - set the initial_radio to the parent radio (not bssid) mac. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

The stay-on-initial-radio can be configured for disconnected clients as well as clients which never connected yet. This means, that when the client connects, we need to check if the stay-on-initial-radio is enabled and if so set the initial-radio. Note that the initial-radio is set only if it wasn't set before - to prevent overriding previous configuration. The value set to the initial-radio is the parent radio (not bssid) mac. Added the above-described functionality to the association_handling_task::finalize_new_connection() which is called on client connection. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

As part of testing the full flows of the persistent Db we encountered an issue where a successful removal of a client entry from the persistent DB resulted in a false-positive error. The cause of this error is that remove_client_entry_and_update_counter() in the db class returned false both for success and for failure. Changed the successful return value of the above-mentioned function from false to true. Signed-off-by: Adam Dov <adamx.dov@intel.com>

adam1985d added this to the M3 - Workable product milestone Jul 28, 2020

adam1985d requested review from itayx, kantera800 and morantr July 28, 2020 09:19

adam1985d requested review from arnout, mariomaz, rmelotte, tomereli and vitalybu as code owners July 28, 2020 09:19

adam1985d self-assigned this Jul 28, 2020

adam1985d force-pushed the feature/PPM-5-handle-smart-steering-BML-requests-and-fix-code-review-comments branch from 15bd0f7 to 3bfd26a Compare July 28, 2020 10:48

adam1985d marked this pull request as draft July 28, 2020 10:49

itayx approved these changes Jul 28, 2020

View reviewed changes

adam1985d force-pushed the feature/PPM-5-handle-smart-steering-BML-requests-and-fix-code-review-comments branch 2 times, most recently from a8c06d6 to c7b9267 Compare July 29, 2020 15:20

kantera800 reviewed Jul 30, 2020

View reviewed changes

controller/src/beerocks/master/db/db.h Show resolved Hide resolved

adam1985d force-pushed the feature/PPM-5-handle-smart-steering-BML-requests-and-fix-code-review-comments branch 2 times, most recently from 35e63f8 to 07544ab Compare July 30, 2020 06:25

adam1985d marked this pull request as ready for review July 30, 2020 06:27

adam1985d force-pushed the feature/PPM-5-handle-smart-steering-BML-requests-and-fix-code-review-comments branch from 07544ab to 79d6ee7 Compare July 30, 2020 15:22

morantr approved these changes Jul 30, 2020

View reviewed changes

adam1985d force-pushed the feature/PPM-5-handle-smart-steering-BML-requests-and-fix-code-review-comments branch from 79d6ee7 to 88b33b5 Compare August 2, 2020 12:50

adam1985d added 8 commits August 2, 2020 12:50

bml: BML_CLIENT: change sta_mac type from char to uint8_t

7603643

The type of the mac address parameter is incorrect. Changed sta_mac from char[6] to uint8_t[6]. PPM-5. Signed-off-by: Itay Elenzweig <itayx.elenzweig@intel.com> Signed-off-by: Adam Dov <adamx.dov@intel.com>

controller: bml: fix return values

b1f3d97

Changed returning error values in the code to a negative values as expected by the BML. PPM-5. Signed-off-by: Itay Elenzweig <itayx.elenzweig@intel.com> Signed-off-by: Adam Dov <adamx.dov@intel.com>

bml: cli: fix selected_bands debug prints

f24b176

Removed excess spaces from selected-bands debug print. PPM-5. Signed-off-by: Adam Dov <adamx.dov@intel.com>

adam1985d added 11 commits August 2, 2020 12:50

adam1985d force-pushed the feature/PPM-5-handle-smart-steering-BML-requests-and-fix-code-review-comments branch from 88b33b5 to 9a6a759 Compare August 2, 2020 12:51

adam1985d merged commit 842bc5b into master Aug 2, 2020

adam1985d deleted the feature/PPM-5-handle-smart-steering-BML-requests-and-fix-code-review-comments branch August 2, 2020 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ppm 5 handle smart steering bml requests and fix code review comments#1569

Feature/ppm 5 handle smart steering bml requests and fix code review comments#1569
adam1985d merged 19 commits intomasterfrom
feature/PPM-5-handle-smart-steering-BML-requests-and-fix-code-review-comments

adam1985d commented Jul 28, 2020 •

edited

Loading

Uh oh!

itayx left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

adam1985d commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itayx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adam1985d commented Jul 28, 2020 •

edited

Loading