Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

Feature/channel pool validation #1093

Merged
merged 1 commit into from
Apr 30, 2020
Merged

Conversation

itayx
Copy link
Collaborator

@itayx itayx commented Apr 5, 2020

Currently the DCS's set_channel_scan_pool uses a static list of supported channels for verifications to see if the channels provided in the channel pool are supported.

The DB has a list known as hostap_supported_channels, this is filled with channels provided by the basic radio capabilities

The DCS's set_channel_scan_pool need to validate it's channel pool against this supported channels list

@itayx itayx force-pushed the feature/channel-pool-validation branch 3 times, most recently from 6e79c90 to e584bbb Compare April 7, 2020 08:59
@itayx itayx force-pushed the feature/channel-pool-validation branch 3 times, most recently from 474ee96 to 233395e Compare April 13, 2020 16:22
controller/src/beerocks/master/db/db.cpp Outdated Show resolved Hide resolved
@itayx itayx force-pushed the feature/channel-pool-validation branch from 233395e to 891bff6 Compare April 14, 2020 16:11
@itayx itayx requested a review from kantera800 April 14, 2020 16:17
@itayx itayx marked this pull request as ready for review April 14, 2020 16:17
Copy link
Collaborator

@rmelotte rmelotte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nitpicks 👍

return supported_channel.channel == channel;
});
if (found_channel == supported_channels.end()) {
LOG(ERROR) << "channel #" << int(channel) << " is not supported";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it's a bit weird that a function called is_channel_scan_pool_supported logs something (especially an error) if a channel is not supported. Maybe it would be best to leave that to the caller (or to change the level to INFO or DEBUG).

Aren't there any case where you would want to call is_channel_scan_pool_supported without logging an error?

(nitpick)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely understand
And I think I'll change it to INFO
But I still want the log, that way there is a log of which sent channel isn't supported
Since the function doesn't return the unsupported channel, the only way to know which is via the log

controller/src/beerocks/master/db/db.h Show resolved Hide resolved
@kantera800
Copy link
Collaborator

approved , but don't merge it yet.
Wait for the NO_IR issue to be solved.

@itayx itayx added the don't merge This PR is not ready for merge or review label Apr 16, 2020
controller/src/beerocks/master/db/db.cpp Outdated Show resolved Hide resolved
@itayx itayx force-pushed the feature/channel-pool-validation branch from 891bff6 to 46f87b6 Compare April 30, 2020 07:18
In the set_channel_scan_pool function to validate the channel pool, we
run a check to see if the channel is supported. Currently the supported
channel list is a static list.

Add is_channel_scan_pool_supported function to the DB.
Use the is_supported function to validate channel pool.

Signed-off-by: itay elenzweig <[email protected]>
@itayx itayx force-pushed the feature/channel-pool-validation branch from 46f87b6 to 127f98b Compare April 30, 2020 07:23
@itayx itayx added ready for merge PR is ready to be merged (automatically) and removed don't merge This PR is not ready for merge or review labels Apr 30, 2020
@mergify mergify bot merged commit 358798e into master Apr 30, 2020
@mergify mergify bot deleted the feature/channel-pool-validation branch April 30, 2020 10:51
@mergify mergify bot removed the ready for merge PR is ready to be merged (automatically) label Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants