This repository has been archived by the owner on Sep 7, 2020. It is now read-only.
Hotfix/fix dcs bwl issues #777
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kantera800
requested review from
arnout,
morantr,
rmelotte and
tomereli
as code owners
vitalybu
suggested changes
Feb 2, 2020
tomereli
suggested changes
Feb 2, 2020
ghost
added this to the
kantera800
force-pushed
the
hotfix/fix-dcs-bwl-issues
branch
4 times, most recently
from
adf6f4e to
ae93ee3
Compare
tomereli
approved these changes
Feb 5, 2020
There was a problem hiding this comment.
Mainly nitpicks about the commit messages so approving.
But fix the commit messages before merging please!
Use proper English - no more "Before this change" - "After this change" formats.
Commit message should be like a story - you explain to the reader what was the issue and how it is solved by this commit. Examples inline.
| @@ -1112,7 +1112,7 @@ bool mon_wlan_hal_dwpal::process_dwpal_nl_event(struct nl_msg *msg) | |||
| return false; | |||
| } | |||
| // Initialize the message | |||
| results_buff = {}; | |||
| memset(results_buff.get(), 0, sizeof(sCHANNEL_SCAN_RESULTS_NOTIFICATION)); | |||
There was a problem hiding this comment.
Now this is a good commit message!
tomereli
reviewed
Feb 5, 2020
vitalybu
suggested changes
Feb 5, 2020
itayx
force-pushed
the
hotfix/fix-dcs-bwl-issues
branch
from
ae93ee3 to
9cd271a
Compare
kantera800
force-pushed
the
hotfix/fix-dcs-bwl-issues
branch
from
9cd271a to
7bf2360
Compare
vitalybu
suggested changes
Feb 9, 2020
Comment on lines
598
to
601
| // Coping vendor_data to a local non-const variable in order | ||
| // to avoid passing a non-const reference in all upper layers. | ||
| unsigned char vendor_data_copy[vendor_data_size]; | ||
| std::copy_n(vendor_data, vendor_data_size, vendor_data_copy); |
There was a problem hiding this comment.
Why? Just to avoid a cast?
Doesn't make sense.
Also, why not change the vendor_data to void*?
There was a problem hiding this comment.
vendor_data is used in upper layer and scan functionality is depended on it .
so yes , i dont want to allow it to be changed in lower levels.
kantera800
force-pushed
the
hotfix/fix-dcs-bwl-issues
branch
from
df4b130 to
6727741
Compare
vitalybu
approved these changes
Feb 10, 2020
kantera800
force-pushed
the
hotfix/fix-dcs-bwl-issues
branch
3 times, most recently
from
4f2297c to
a46f626
Compare
kantera800
force-pushed
the
hotfix/fix-dcs-bwl-issues
branch
from
a46f626 to
1c46e50
Compare
kantera800
force-pushed
the
hotfix/fix-dcs-bwl-issues
branch
from
1c46e50 to
aa2ac7a
Compare
vitalybu
approved these changes
Feb 12, 2020
| @@ -393,19 +393,19 @@ enum eChannelScanResultChannelBandwidth : uint8_t { | |||
|
|
|||
| typedef struct sChannelScanResults { | |||
| //The current service set identifier in use by the neighboring WiFi SSID. The value MAY be empty for hidden SSIDs. | |||
| char ssid[beerocks::message::WIFI_SSID_MAX_LENGTH]; | |||
| char ssid[beerocks::message::WIFI_SSID_MAX_LENGTH] = {'\0'}; | |||
There was a problem hiding this comment.
Suggested change
| char ssid[beerocks::message::WIFI_SSID_MAX_LENGTH] = {'\0'}; | |
| char ssid[beerocks::message::WIFI_SSID_MAX_LENGTH] = {0}; |
There was a problem hiding this comment.
I disagree. For me at least, '\0' tells me it's the zero-terminator of a C string, while 0 is just a zero that doesn't tell me much.
Print dwpal_nl_cmd_get returned value, containing the received result size if the latter does not match the expected result size. Signed-off-by: Adam Dov <[email protected]>
Before this change, if an event did not contain an interface name, the function returned an error. This return must be removed since there are NL events that do not contain interface name (e.g. "Channel_Scan_Finished" event). After this change, failure to get the interface name from the NL event in process_dwpal_nl_event will not return an error. Additionally, changed log print from error to warning. Signed-off-by: Adam Dov <[email protected]>
When an attach event is received, the dwpal_driver_nl_fd_get is not performed due to wrong condition check of the m_dwpal_nl_ctx pointer. Fix the wrong condition check so netlink attach can be performed. Signed-off-by: Adam Dov <[email protected]>
When a triggered scan completes, an empty 'Channel_Scan_Dump_Result' event sent to signal the scan is completed and that results are ready. At this point, if dump-results is requested, it will result with a series of 'Channel_Scan_Dump_Result' events with the results. Fix current logic by treating the first 'Channel_Scan_Dump_Result' as results-ready and trigger the dump-result sequence. Signed-off-by: Adam Dov <[email protected]>
Default initialization of the "smart" results_buffer is wrong in this scenario. results_buffer is actually a shared pointer to a uint8_t buffer. so, calling a shared_ptr default constructor is actually writing zeros to a buffer of an unknown size. Since results structure also contains non basic types (std::vector) we need to create shared pointer with instance of the actual results structure (not using smart buffer anymore). Finally the initialization will be done in the definition of the structure. Signed-off-by: alex kanter <[email protected]>
arnout
force-pushed
the
hotfix/fix-dcs-bwl-issues
branch
from
69f0bad to
4e96afa
Compare
added 3 commits
February 12, 2020 14:40
Since we're expecting a Solicited (asynchronous) event from the driver, and it's impossible to know the type of the received message without processing it (using a callback function), we continue processing events until data_size != 0. also, added 1 sec timeout is added so we dont get stuck here forever. In practice, the response usually arrives much faster. Signed-off-by: alex kanter <[email protected]>
Fixing wrong usage of std::copy_n. syntax of copy_n ( InputIt first, Size count, OutputIt result ) InputIt and OutputIt were switched and as the result bssid was wrongly copied to results buffer. Signed-off-by: alex kanter <[email protected]>
Wrong error condition validation used for dwpal_nl_cmd_set(). Also, dwpal_nl_cmd_set() proxy "vendor data" without modifying it with unnecessary C style casting. Fix error condition. Change dwpal_nl_cmd_set signature to accept (const void *) and cast vendor data (cpp style) inside before passing it to dwpal lib. Signed-off-by: Alex Kanter <[email protected]>
arnout
force-pushed
the
hotfix/fix-dcs-bwl-issues
branch
from
4e96afa to
567bfc1
Compare
arnout
approved these changes
Feb 12, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
all DCS code has been ported #562
DCS Validation has been started on both RDKB/UGW platforms.
This PR is used to fix all bwl related issues that has been spotted during the validation process.