-
Notifications
You must be signed in to change notification settings - Fork 117
RDKBWIFI-36: WiFi7 Datamodel Implementation #561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
RDKBWIFI-36: WiFi7 Datamodel Implementation #561
Conversation
Reason for change: Adding in the datamodels related to WiFi7 for OneWiFi component Test Procedure: Verify build is successfull and check if DMs are updated properly on Device Risks: Medium Priority: P2 Signed-off-by: Siddharth Nair <[email protected]>
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
Internal ticket for tracking https://ccp.sys.comcast.net/browse/RDKCOM-5414 |
| <syntax>bool</syntax> | ||
| <writable>false</writable> | ||
| </parameter> | ||
| <parameter> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please align with the rest of "parameters".
| //Get the parameter values from HAL | ||
| wifi_hal_getApMld(&hal_apmld, &APMLD_Count); | ||
|
|
||
| if (APMLD_Count > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To check if it's not null instead of number of AP MLD, because later in the code the same check is being done. Also in case there is 0 AP MLD it will be written in g_wifi_mgr->apmld_countand it will return successfully after freeing hal_apmld.
| if (APMLD_Count > 0) | |
| if (hal_apmld) |
|
|
||
| while (idx < APMLD_Count) | ||
| { | ||
| memcpy(g_wifi_mgr->apmld_info[idx].common_info.mld_addr,hal_apmld->common_info.mld_addr,6); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we replace this 6 with some constant or definition?
| } | ||
| *pInsNumber = nIndex + 1; | ||
| g_radio_instance_num = nIndex + 1; | ||
| last_radio_change = AnscGetTickInSeconds(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some debug print, same as in Radio_GetEntry.
Please add debug prints to other newly added functions as well.
| ANSC_HANDLE hInsContext | ||
| ) | ||
| { | ||
| return TRUE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return TRUE; | |
| UNREFERENCED_PARAMETER(hInsContext); | |
| return TRUE; |
Also consider adding one extra bool variable to update it once update_dml_apmld_info is being called to return a valid result.
|
|
||
| if(AnscEqualString(ParamName,"MLDMACAddress", TRUE)) | ||
| { | ||
| char mac_str[18]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding new constant for MAC string size or use the one available in <pwd>/lib/osa/os_types.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the OS_MACSTR_SZ const
| *puLong = pcfg->ru_id; | ||
| return TRUE; | ||
| } | ||
| else if( AnscEqualString(ParamName,"DisabledSubChannels", TRUE)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it would be nice to have definitions for those string literals - consider adding those definitions.
| if (hal_apmld) { | ||
| free (hal_apmld); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this if check won't be needed after applying Pavlo's comment from above (free still will).
| memcpy(g_wifi_mgr->apmld_info[idx].common_info.mld_addr,hal_apmld->common_info.mld_addr,6); | ||
| g_wifi_mgr->apmld_info[idx].affiliated_ap_number_of_entries = hal_apmld[idx].affiliated_ap_number_of_entries; | ||
| g_wifi_mgr->apmld_info[idx].sta_mld_num_entries = hal_apmld[idx].sta_mld_num_entries; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please consider just copying pointers instead of manual deep copy,
It's less complicated (and - more important, faster!) and hal_apmd is already allocated in wifi_hal_getApMld,
| memcpy(g_wifi_mgr->apmld_info[idx].common_info.mld_addr,hal_apmld->common_info.mld_addr,6); | |
| g_wifi_mgr->apmld_info[idx].affiliated_ap_number_of_entries = hal_apmld[idx].affiliated_ap_number_of_entries; | |
| g_wifi_mgr->apmld_info[idx].sta_mld_num_entries = hal_apmld[idx].sta_mld_num_entries; | |
| g_wifi_mgr->apmld_info[idx] = hal_apmld[idx]; | |
| g_wifi_mgr->apmld_info[idx].common_info.mld_addr = hal_apmld->common_info.mld_addr; | |
and then remove
free (hal_apmld);
- this memory will be freed when g_wifi_mgr->apmld_info will not be needed.
| wifi_util_info_print(WIFI_DMCLI,"Exit %s:%d \n",__func__, __LINE__); | ||
| return TRUE; | ||
| } | ||
| UINT get_dml_wifi7_radio_modes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed? Can't get_wifidb_wifi7_radio_modes() be called directly? if it can, please call it directly.
| int update_dml_apmld_info() | ||
| { | ||
| if(get_wifidb_apmld_info() == 0) | ||
| { | ||
| return 0; | ||
| } | ||
| else | ||
| { | ||
| return -1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
If additional layer is needed please shorten it:
| int update_dml_apmld_info() | |
| { | |
| if(get_wifidb_apmld_info() == 0) | |
| { | |
| return 0; | |
| } | |
| else | |
| { | |
| return -1; | |
| } | |
| } | |
| int update_dml_apmld_info() | |
| { | |
| return get_wifidb_apmld_info(); | |
| } |
If not, please remove it and call get_wifidb_apmld_info() directly.
| UNREFERENCED_PARAMETER(hInsContext); | ||
| wifi_radio_operationParam_t *wifiRadioOperParam = NULL; | ||
|
|
||
| if ( nIndex < (UINT)get_num_radio_dml() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_num_radio_dml already returns UINT, no need for cast.
|
|
||
| if(AnscEqualString(ParamName,"MLDMACAddress", TRUE)) | ||
| { | ||
| char mac_str[18]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the OS_MACSTR_SZ const
| *pInsNumber=nIndex+1; | ||
| return (ANSC_HANDLE)(&pcfg->affiliated_ap[nIndex]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand your idea here. Were you trying to do this:
| *pInsNumber=nIndex+1; | |
| return (ANSC_HANDLE)(&pcfg->affiliated_ap[nIndex]); | |
| *pInsNumber=nIndex+1; | |
| return (ANSC_HANDLE)(&pcfg->affiliated_ap[*pInsNumber]); |
?
Reason for change: Adding in the datamodels related to WiFi7 for OneWiFi component
Test Procedure: Verify build is successfull and check if DMs are updated properly on Device
Risks: Medium
Priority: P2