[DCS Patch 2/5] port dynamic channel selection from rdkb infrastructure #643

itayx · 2020-01-06T16:37:56Z

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 second out of 5
start review from 9db41a9

High Level Infastructure (Controller & yaml): Support for Channel Scan & DCS:

Changes in this PR include the following:

beerocks defines: enums for Channel Scan & DCS
yaml: beerocks_message_common: Add channel scan structures
yaml: beerocks_message_control: Add channel scan & DCS CMDUs
controller: node: Add channel scan structures
controller: db: Add channel scan DB methods

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

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

common/beerocks/tlvf/yaml/beerocks/tlvf/beerocks_message_bml.yaml

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

tomereli · 2020-01-09T11:57:16Z

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

+                                                                          const bool single_scan)
+{
+    auto n = get_node(mac);
+    static std::list<sChannelScanResultsElement> dummy_return;


Oh this one is super ugly.
I prefer return std::list<sChannelScanResultsElement>(); instead everywhere.
Or you can return a shared_ptr instead of const reference and return nullptr instead of dummy_return.

Anything BUT dummy_return 👎

return {} can work as well

Tried that, when I do I get an error saying:

error: returning reference to temporary [-Werror=return-local-addr]

Or you can return a shared_ptr instead of const reference and return nullptr instead of dummy_return.

This will be the only thing that works. But it's super-annoying, because it means the caller has to check for nullptr everywhere while it could just work with the empty set without problem.

Or you can return a shared_ptr instead of const reference and return nullptr instead of dummy_return.

This will be the only thing that works. But it's super-annoying, because it means the caller has to check for nullptr everywhere while it could just work with the empty set without problem.

So you think the dummy_return should stay?

for now it stays, if we decide to fix this later on we'll decide in #665

tomereli · 2020-01-09T11:58:07Z

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

+    // Channel Scan
+    //
+    bool set_channel_scan_is_enabled(const std::string &mac, const bool enable);
+    bool get_channel_scan_is_enabled(const std::string &mac);


here and in any other method which doesn't change the state of the class, declare the function as const:

bool get_channel_scan_is_enabled(const std::string &mac) const;

Also - the name is really long and for no reason. I prefer channel_scan_enabled(const std::string &mac) (nitpick)

So it will be

channel_scan_enabled(const std::string &mac) const; //get
channel_scan_enabled(const std::string &mac, const bool enable); //set

?

There are far more important things to fix in this PR. So let's keep it for #665

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

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

common/beerocks/bwl/CMakeLists.txt

common/beerocks/tlvf/yaml/beerocks/tlvf/beerocks_message_action.yaml

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

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

vitalybu · 2020-01-09T14:06:57Z

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

+                                                                          const bool single_scan)
+{
+    auto n = get_node(mac);
+    static std::list<sChannelScanResultsElement> dummy_return;


return {} can work as well

common/beerocks/tlvf/yaml/beerocks/tlvf/beerocks_message_common.yaml

arnout · 2020-01-15T10:46:21Z

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

+    auto n = get_node(mac);
+    if (!n) {
+        LOG(ERROR) << "node not found.... ";
+        return *(std::make_shared<std::set<uint8_t>>());


Does this work? AFAICS this has the same problem as return std::set<uint8_t>(); only the compiler doesn't complain about it...

This is basically creating a shared_ptr, then instead of returning the shared_ptr itself which would increment the reference count preventing the memory from being released, we return the contents and when we return they are invalid.

Added a dummy_return, if we decide that using a dummy_return is incorrect we'll fix it in #665

tomereli

one small comment

kantera800 · 2020-01-15T19:59:27Z

ready for re-review:

fixed all code review issues that not require push -f .
clang format ran only from icsl.

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

tomereli

Please rebase-squash and I'll do the followup review.
@morantr please review after the rebase.

kantera800 · 2020-01-16T15:14:37Z

[DCS Patch 2/5]Port dynamic channel selection

tomereli

Small comments - but please fix them before merging. Especially everything related to the string to mac and mac to string conversions.

tomereli · 2020-01-19T10:10:11Z

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

@@ -443,6 +447,30 @@ enum eBssType {
    BSS_TYPE_INVALID
 };

+enum class eChannelScanErrCode : uint8_t {
+    CHANNEL_SCAN_SUCCESS = 0,


Its an enum class - no need for the CHANNEL_SCAN_ prefix. #665

tomereli · 2020-01-19T10:10:46Z

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

+};
+
+enum class eChannelScanOpErrCode : uint8_t {
+    CHANNEL_SCAN_OP_SUCCESS = 0,


Here as well

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

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

This is a preperative commit for 'Add channel scan structures'. Add enums to support the DCS feature. Add eChannelScanErrCode that describes the scan operation status. Add eDcsOpErrCode that describes the API's operation status. Signed-off-by: itay elenzweig <[email protected]>

This is a preperative commit for 'Add channel scan & DCS CMDUs'. Add DCS structures to support the DCS feature [yaml only]. Signed-off-by: itay elenzweig <[email protected]>

This is a preperative commit for 'Add channel scan & DCS CMDUs'. Add DCS structures to support the DCS feature [AutoGenerated]. Signed-off-by: itay elenzweig <[email protected]>

Add requests from the bml to the controller [YAML]. Add requests from the controller to the agent [YAML]. Add notifications from the agent to the controller [YAML]. Add requests from the agent to the monitor [YAML]. Add notifications from the monitor to the agent [YAML]. Signed-off-by: itay elenzweig <[email protected]>

Add requests from the bml to the controller [AutoGenerated]. Add requests from the controller to the agent [AutoGenerated]. Add notifications from the agent to the controller [AutoGenerated]. Add requests from the agent to the monitor [AutoGenerated]. Add notifications from the monitor to the agent [AutoGenerated]. Signed-off-by: itay elenzweig <[email protected]>

This is a preperative commit for 'Add channel scan DB methods'. Add the result class & container, which is used to store the DCS scan results returned from the monitor/agent. Add DCS configutation class for the DCS feature. Add DCS scan status class, which is used to store the latest DCS scan results status. Signed-off-by: itay elenzweig <[email protected]>

Add DB method get_hostap_by_mac Add DB methods to get/set scan params (configutation). Add DB methods to get/set scan status. Add DB methods to get/set scan results. Add DB methods to clear channel pool. Add DB methods to check if a channel is in the current channel scan's pool. Signed-off-by: itay elenzweig <[email protected]>

kantera800 mentioned this pull request Jan 6, 2020

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

Closed

8 tasks

itayx mentioned this pull request Jan 6, 2020

[DCS Patch 3/5] port dynamic channel selection from rdkb task #644

Merged

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-infastructure branch 2 times, most recently from fd4c6ed to 894b852 Compare January 7, 2020 13:58

itayx mentioned this pull request Jan 8, 2020

Feature/port dynamic channel selection from rdkb #619

Closed

itayx marked this pull request as ready for review January 8, 2020 13:54

itayx requested review from arnout, morantr and tomereli as code owners January 8, 2020 13:54

kantera800 changed the title ~~Feature/port dynamic channel selection from rdkb infastructure~~ [DCS Patch 2/5] port dynamic channel selection from rdkb infrastructure Jan 8, 2020

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-infastructure branch from 894b852 to 81c8bd4 Compare January 8, 2020 14:19

tomereli suggested changes Jan 9, 2020

View reviewed changes

vitalybu reviewed Jan 9, 2020

View reviewed changes

common/beerocks/bwl/CMakeLists.txt Outdated Show resolved Hide resolved

vitalybu reviewed Jan 9, 2020

View reviewed changes

common/beerocks/tlvf/yaml/beerocks/tlvf/beerocks_message_action.yaml Outdated Show resolved Hide resolved

vitalybu suggested changes Jan 9, 2020

View reviewed changes

arnout reviewed Jan 15, 2020

View reviewed changes

tomereli reviewed Jan 15, 2020

View reviewed changes

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-infastructure branch from 81c8bd4 to 7736ee5 Compare January 15, 2020 16:45

kantera800 requested review from vitalybu and tomereli January 15, 2020 19:56

tomereli reviewed Jan 15, 2020

View reviewed changes

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

tomereli reviewed Jan 15, 2020

View reviewed changes

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-infastructure branch 2 times, most recently from 5a2ee50 to 9202cba Compare January 16, 2020 12:51

kantera800 requested a review from tomereli January 16, 2020 12:54

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-infastructure branch 4 times, most recently from 1e47869 to c971631 Compare January 16, 2020 13:48

kantera800 force-pushed the feature/port-dynamic-channel-selection-from-rdkb-infastructure branch 3 times, most recently from b574c68 to 6a5042e Compare January 17, 2020 02:02

ghost assigned itayx Jan 17, 2020

tomereli approved these changes Jan 19, 2020

View reviewed changes

kantera800 self-requested a review January 19, 2020 12:23

vitalybu approved these changes Jan 19, 2020

View reviewed changes

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-infastructure branch 2 times, most recently from d5defe5 to 43c37fc Compare January 19, 2020 14:52

itayx added 7 commits January 19, 2020 22:25

TLVF: beerocks_message_common: Add channel scan structures

2211368

This is a preperative commit for 'Add channel scan & DCS CMDUs'. Add DCS structures to support the DCS feature [yaml only]. Signed-off-by: itay elenzweig <[email protected]>

TLVF: beerocks_message_common: channel scan structure autogenerated

fe1d5c9

This is a preperative commit for 'Add channel scan & DCS CMDUs'. Add DCS structures to support the DCS feature [AutoGenerated]. Signed-off-by: itay elenzweig <[email protected]>

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-infastructure branch from 43c37fc to cd6b49c Compare January 19, 2020 20:26

itayx mentioned this pull request Jan 19, 2020

Hotfix/channel scan trigger #689

Merged

kantera800 merged commit dde1672 into master Jan 20, 2020

kantera800 deleted the feature/port-dynamic-channel-selection-from-rdkb-infastructure branch January 20, 2020 10:00

tomereli mentioned this pull request Jan 26, 2020

[TASK] DCS cleanups #665

Closed

1 task

itayx mentioned this pull request Apr 28, 2020

DCS cleanups #1202

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DCS Patch 2/5] port dynamic channel selection from rdkb infrastructure #643

[DCS Patch 2/5] port dynamic channel selection from rdkb infrastructure #643

itayx commented Jan 6, 2020 •

edited by kantera800

Loading

tomereli Jan 9, 2020

vitalybu Jan 9, 2020

itayx Jan 15, 2020

arnout Jan 15, 2020

tomereli Jan 15, 2020

itayx Jan 16, 2020

tomereli Jan 9, 2020

tomereli Jan 9, 2020

itayx Jan 15, 2020

tomereli Jan 15, 2020

vitalybu Jan 9, 2020

arnout Jan 15, 2020

tomereli Jan 15, 2020

itayx Jan 16, 2020

tomereli left a comment

kantera800 commented Jan 15, 2020

tomereli left a comment

kantera800 commented Jan 16, 2020

tomereli left a comment

tomereli Jan 19, 2020

tomereli Jan 19, 2020

kantera800 Jan 19, 2020

[DCS Patch 2/5] port dynamic channel selection from rdkb infrastructure #643

[DCS Patch 2/5] port dynamic channel selection from rdkb infrastructure #643

Conversation

itayx commented Jan 6, 2020 • edited by kantera800 Loading

High Level Infastructure (Controller & yaml): Support for Channel Scan & DCS:

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

kantera800 commented Jan 15, 2020

tomereli left a comment

Choose a reason for hiding this comment

kantera800 commented Jan 16, 2020

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

itayx commented Jan 6, 2020 •

edited by kantera800

Loading