Skip to content
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

Fix the stability issues in controller and agent #221

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AswiniViswanathan
Copy link
Contributor

Fixed the following issues
1)CTRL DB BSSID table incorrect values [duplicate entries]
2)Freq band incorrect
3)Update to CTRL DB not woking for SSID and BSSID configs
4)freq band not updated to DB
5)Autoconfig renew based al mac is empty and added to check for al mac
7)Set channel fail in controller em even when agent is configured
8)Autoconfig renew fail when happens for both the radio

Copy link
Contributor

@amarnathhullur amarnathhullur left a comment

Choose a reason for hiding this comment

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

Lets have a discussion on these changes as some changes need more clarification.

@@ -287,9 +287,7 @@ void em_agent_t::handle_autoconfig_renew(em_bus_event_t *evt)
em_cmd_t *pcmd[EM_MAX_CMD] = {NULL};
unsigned int num;

if (m_orch->is_cmd_type_in_progress(evt->type) == true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this check removed? Is the check not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not allow autoconfig renew simultaneously for both 2.4 and 5G

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted since its not required

@@ -121,7 +121,7 @@ dm_orch_type_t dm_bss_list_t::get_dm_orch_type(db_client_t& db_client, const dm_
pbss = get_bss(key);

if (pbss != NULL) {
if (entry_exists_in_table(db_client, bss_mac_str) == false) {
if (entry_exists_in_table(db_client, key) == false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a significant change to have the key be different, Has this change been properly tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes amarnath. I did test it and it works fine. Issue is while adding an entry key is construction as combination of bss mac haultype but this check uses only mac.

@@ -196,7 +196,7 @@ int dm_radio_list_t::update_db(db_client_t& db_client, dm_orch_type_t op, void *
switch (op) {
case dm_orch_type_db_insert:
ret = insert_row(db_client, key, radio_mac_str, info->enabled,
info->media_data.media_type, info->media_data.band, info->media_data.center_freq_index_1, info->media_data.center_freq_index_1,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the band in media_data which was earlier used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

media_data is a new variable introduced recently, not exactly sure on the exact purpose. Band is the variable which is updated in webconfig translator and also on the agent side implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the webconfig translator and the agent needs to change to update media_data.band instead of using info.band?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will discuss with soumya and get the information on media_data struct and update it the right way, but for M2 we have to updated the band variable.

@@ -261,6 +261,7 @@ int dm_radio_list_t::sync_db(db_client_t& db_client, void *ctx)
info.enabled = db_client.get_number(ctx, 3);
info.media_data.media_type = db_client.get_number(ctx, 4);
info.media_data.band = db_client.get_number(ctx, 5);
info.band = (em_freq_band_t) db_client.get_number(ctx, 5);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the need for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

media_data is not updated because of which frequency band is also not updated. Modified to use band variable which is updated from M1.

@@ -1950,7 +1952,9 @@ void em_channel_t::process_msg(unsigned char *data, unsigned int len)
break;

case em_msg_type_channel_sel_rsp:
handle_channel_sel_rsp(data, len);
if (get_service_type() == em_service_type_ctrl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the understanding that this needs to be processed only in controller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes thats correct

if (memcmp(radio->get_radio_interface_mac(), get_radio_interface_mac(), sizeof(mac_address_t)) != 0) {
continue;
}
found++;
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation incorrect

}
else if (em->get_state() == em_state_ctrl_configured) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can add an || instead of if, elseif as you are returning same value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

return true;
} else if( em->get_state() == em_state_ctrl_channel_selected) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can add an || instead of if, elseif as you are returning same value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

queue_push(pcmd->m_em_candidates, em);
count++;
}
if (em->is_al_interface_em() == false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The for loop is removed, significant change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SSID change is for all EM except the AL EM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, how will this be applied for all EM? Is this function called for all EM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is.

}
if (em->is_al_interface_em() == false) {
dm_easy_mesh_t::macbytes_to_string(em->get_radio_interface_mac(), mac_str);
printf("%s:%d Set SSSID : %s push to queue \n", __func__, __LINE__,mac_str);
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in SSID

@@ -196,7 +196,7 @@ int dm_radio_list_t::update_db(db_client_t& db_client, dm_orch_type_t op, void *
switch (op) {
case dm_orch_type_db_insert:
ret = insert_row(db_client, key, radio_mac_str, info->enabled,
info->media_data.media_type, info->media_data.band, info->media_data.center_freq_index_1, info->media_data.center_freq_index_1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the webconfig translator and the agent needs to change to update media_data.band instead of using info.band?

queue_push(pcmd->m_em_candidates, em);
count++;
}
if (em->is_al_interface_em() == false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, how will this be applied for all EM? Is this function called for all EM?

@@ -455,6 +468,14 @@ unsigned int em_orch_ctrl_t::build_candidates(em_cmd_t *pcmd)
break;

case em_cmd_type_set_channel:
dm = pcmd->get_data_model();
Copy link
Contributor

Choose a reason for hiding this comment

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

How was set_channel working earlier without the below change?

Testing performed:
1)On board remote and local agent
2)Verified not duplicate entries in DB
3)Autoconfig renew success when controller /agent is restarting
for (i = 0; i < dm->m_num_radios; i++) {
radio = dm->get_radio(i);
if (memcmp(radio->get_radio_interface_mac(), get_radio_interface_mac(), sizeof(mac_address_t)) != 0) {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation incorrect, two tabs for continue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants