From abbcbe6aa55af6e608a251b544954f5f8344d642 Mon Sep 17 00:00:00 2001 From: Melissa Elkadi Date: Tue, 22 Feb 2022 09:16:35 -0800 Subject: [PATCH] Adding fixes for MR from review These include: - Simplifying VNF code. - Adding comments to several areas of the code. - Using free_and_zero macros - Fixing spelling errors --- openair1/SCHED_NR_UE/fapi_nr_ue_l1.c | 4 + openair2/LAYER2/NR_MAC_UE/nr_ue_procedures.c | 7 +- openair2/NR_PHY_INTERFACE/NR_IF_Module.c | 105 ++++++++----------- openair2/RRC/LTE/MESSAGES/asn1_msg.c | 3 +- 4 files changed, 53 insertions(+), 66 deletions(-) diff --git a/openair1/SCHED_NR_UE/fapi_nr_ue_l1.c b/openair1/SCHED_NR_UE/fapi_nr_ue_l1.c index 5b13b2e9d0..b6fbf7452b 100644 --- a/openair1/SCHED_NR_UE/fapi_nr_ue_l1.c +++ b/openair1/SCHED_NR_UE/fapi_nr_ue_l1.c @@ -90,6 +90,9 @@ int8_t nr_ue_scheduled_response_stub(nr_scheduled_response_t *scheduled_response rx_ind->pdu_list[j].pdu = CALLOC(tx_req_body->pdu_length, sizeof(*rx_ind->pdu_list[j].pdu)); memcpy(rx_ind->pdu_list[j].pdu, tx_req_body->pdu, tx_req_body->pdu_length * sizeof(*rx_ind->pdu_list[j].pdu)); rx_ind->pdu_list[j].rnti = pusch_config_pdu->rnti; + /* TODO: Implement channel modeling to abstract TA and CQI. For now, + we hard code the values below since they are set in L1 and we are + abstracting L1. */ rx_ind->pdu_list[j].timing_advance = 31; rx_ind->pdu_list[j].ul_cqi = 255; char buffer[1024]; @@ -204,6 +207,7 @@ int8_t nr_ue_scheduled_response_stub(nr_scheduled_response_t *scheduled_response } } } + dl_config->number_pdus = 0; } } diff --git a/openair2/LAYER2/NR_MAC_UE/nr_ue_procedures.c b/openair2/LAYER2/NR_MAC_UE/nr_ue_procedures.c index c7d9d525a6..0cb3c4f7a6 100644 --- a/openair2/LAYER2/NR_MAC_UE/nr_ue_procedures.c +++ b/openair2/LAYER2/NR_MAC_UE/nr_ue_procedures.c @@ -3810,8 +3810,11 @@ int nr_ue_process_rar(nr_downlink_indication_t *dl_info, NR_UL_TIME_ALIGNMENT_t rar = (NR_MAC_RAR *) (dlsch_buffer + n_subheaders + (n_subPDUs - 1) * sizeof(NR_MAC_RAR)); ra->RA_RAPID_found = 1; if (get_softmodem_params()->emulate_l1) { - LOG_I(NR_MAC, "If we found the RAR, the other PDUs are for other UEs and we dont need to process. " - "Only save rx_indication_body[%d] as only PDU in RX_indication\n", pdu_id); + /* When we are emulating L1 with multiple UEs, the rx_indication will have + multiple RAR PDUs. The code would previously handle each of these PDUs, + but it should only be handling the single RAR that matches the current + UE. */ + LOG_I(NR_MAC, "RAR PDU found for our UE with PDU index %d\n", pdu_id); dl_info->rx_ind->number_pdus = 1; if (pdu_id != 0) { memcpy(&dl_info->rx_ind->rx_indication_body[0], diff --git a/openair2/NR_PHY_INTERFACE/NR_IF_Module.c b/openair2/NR_PHY_INTERFACE/NR_IF_Module.c index b94b22282c..4f50e0481c 100644 --- a/openair2/NR_PHY_INTERFACE/NR_IF_Module.c +++ b/openair2/NR_PHY_INTERFACE/NR_IF_Module.c @@ -225,13 +225,10 @@ static void free_unqueued_nfapi_indications(nfapi_nr_rach_indication_t *rach_ind { for(int i = 0; i < rach_ind->number_of_pdus; i++) { - free(rach_ind->pdu_list[i].preamble_list); - rach_ind->pdu_list[i].preamble_list = NULL; + free_and_zero(rach_ind->pdu_list[i].preamble_list); } - free(rach_ind->pdu_list); - rach_ind->pdu_list = NULL; - free(rach_ind); - rach_ind = NULL; + free_and_zero(rach_ind->pdu_list); + free_and_zero(rach_ind); } if (uci_ind && uci_ind->num_ucis > 0) { @@ -239,40 +236,31 @@ static void free_unqueued_nfapi_indications(nfapi_nr_rach_indication_t *rach_ind switch (uci_ind->uci_list[i].pdu_type) { case NFAPI_NR_UCI_FORMAT_0_1_PDU_TYPE: if (uci_ind->uci_list[i].pucch_pdu_format_0_1.harq) { - free(uci_ind->uci_list[i].pucch_pdu_format_0_1.harq->harq_list); - uci_ind->uci_list[i].pucch_pdu_format_0_1.harq->harq_list = NULL; + free_and_zero(uci_ind->uci_list[i].pucch_pdu_format_0_1.harq->harq_list); } - free(uci_ind->uci_list[i].pucch_pdu_format_0_1.harq); - uci_ind->uci_list[i].pucch_pdu_format_0_1.harq = NULL; - free(uci_ind->uci_list[i].pucch_pdu_format_0_1.sr); - uci_ind->uci_list[i].pucch_pdu_format_0_1.sr = NULL; + free_and_zero(uci_ind->uci_list[i].pucch_pdu_format_0_1.harq); + free_and_zero(uci_ind->uci_list[i].pucch_pdu_format_0_1.sr); break; case NFAPI_NR_UCI_FORMAT_2_3_4_PDU_TYPE: - free(uci_ind->uci_list[i].pucch_pdu_format_2_3_4.harq.harq_payload); - free(uci_ind->uci_list[i].pucch_pdu_format_2_3_4.csi_part1.csi_part1_payload); - free(uci_ind->uci_list[i].pucch_pdu_format_2_3_4.csi_part2.csi_part2_payload); + free_and_zero(uci_ind->uci_list[i].pucch_pdu_format_2_3_4.harq.harq_payload); + free_and_zero(uci_ind->uci_list[i].pucch_pdu_format_2_3_4.csi_part1.csi_part1_payload); + free_and_zero(uci_ind->uci_list[i].pucch_pdu_format_2_3_4.csi_part2.csi_part2_payload); break; } } - free(uci_ind->uci_list); - uci_ind->uci_list = NULL; - free(uci_ind); - uci_ind = NULL; + free_and_zero(uci_ind->uci_list); + free_and_zero(uci_ind); } if (rx_ind && rx_ind->number_of_pdus > 0) { - free(rx_ind->pdu_list); - rx_ind->pdu_list = NULL; - free(rx_ind); - rx_ind = NULL; + free_and_zero(rx_ind->pdu_list); + free_and_zero(rx_ind); } if (crc_ind && crc_ind->number_crcs > 0) { - free(crc_ind->crc_list); - crc_ind->crc_list = NULL; - free(crc_ind); - crc_ind = NULL; + free_and_zero(crc_ind->crc_list); + free_and_zero(crc_ind); } } @@ -321,73 +309,61 @@ static void match_crc_rx_pdu(nfapi_nr_rx_data_indication_t *rx_ind, nfapi_nr_crc "Invalid number of crc_ind->number_crcs %d or rx_ind->number_of_pdus %d\n", crc_ind->number_crcs, rx_ind->number_of_pdus); if (crc_ind->number_crcs > rx_ind->number_of_pdus) { + int num_unmatched_crcs = 0; nfapi_nr_crc_indication_t *crc_ind_unmatched = calloc(1, sizeof(*crc_ind_unmatched)); crc_ind_unmatched->header = crc_ind->header; crc_ind_unmatched->sfn = crc_ind->sfn; crc_ind_unmatched->slot = crc_ind->slot; - crc_ind_unmatched->number_crcs = 0; - crc_ind_unmatched->crc_list = NULL; + crc_ind_unmatched->number_crcs = crc_ind->number_crcs - rx_ind->number_of_pdus; + crc_ind_unmatched->crc_list = calloc(crc_ind_unmatched->number_crcs, sizeof(nfapi_nr_crc_t)); for (int i = 0; i < crc_ind->number_crcs; i++) { if (!rx_ind_has_rnti(rx_ind, crc_ind->crc_list[i].rnti)) { LOG_I(NR_MAC, "crc_ind->crc_list[%d].rnti %x does not match any rx_ind pdu rnti\n", i, crc_ind->crc_list[i].rnti); - crc_ind_unmatched->number_crcs++; - if (crc_ind_unmatched->crc_list == NULL) { - crc_ind_unmatched->crc_list = malloc(sizeof(nfapi_nr_crc_t)); - } - else { - crc_ind_unmatched->crc_list = realloc(crc_ind_unmatched->crc_list, - sizeof(nfapi_nr_crc_t) * crc_ind_unmatched->number_crcs); - } - crc_ind_unmatched->crc_list[crc_ind_unmatched->number_crcs - 1] = crc_ind->crc_list[i]; + crc_ind_unmatched->crc_list[num_unmatched_crcs] = crc_ind->crc_list[i]; + num_unmatched_crcs++; remove_crc_pdu(crc_ind, i); } if (crc_ind->number_crcs == rx_ind->number_of_pdus) { break; } } - AssertFatal(crc_ind_unmatched->number_crcs > 0, "The nubmer of unmatched crc_ind pdus %d <= 0\n", - crc_ind_unmatched->number_crcs); + AssertFatal(crc_ind_unmatched->number_crcs == num_unmatched_crcs, "crc_ind num_pdus %d doesnt match %d\n", + crc_ind_unmatched->number_crcs, num_unmatched_crcs); if (!requeue(&gnb_crc_ind_queue, crc_ind_unmatched)) { LOG_E(NR_PHY, "requeue failed for crc_ind_unmatched.\n"); - free(crc_ind_unmatched); - crc_ind_unmatched = NULL; + free_and_zero(crc_ind_unmatched->crc_list); + free_and_zero(crc_ind_unmatched); } } else if (crc_ind->number_crcs < rx_ind->number_of_pdus) { + int num_unmatched_rxs = 0; nfapi_nr_rx_data_indication_t *rx_ind_unmatched = calloc(1, sizeof(*rx_ind_unmatched)); rx_ind_unmatched->header = rx_ind->header; rx_ind_unmatched->sfn = rx_ind->sfn; rx_ind_unmatched->slot = rx_ind->slot; - rx_ind_unmatched->number_of_pdus = 0; - rx_ind_unmatched->pdu_list = NULL; + rx_ind_unmatched->number_of_pdus = rx_ind->number_of_pdus - crc_ind->number_crcs; + rx_ind_unmatched->pdu_list = calloc(rx_ind_unmatched->number_of_pdus, sizeof(nfapi_nr_pdu_t)); for (int i = 0; i < rx_ind->number_of_pdus; i++) { if (!crc_ind_has_rnti(crc_ind, rx_ind->pdu_list[i].rnti)) { LOG_I(NR_MAC, "rx_ind->pdu_list[%d].rnti %d does not match any crc_ind pdu rnti\n", i, rx_ind->pdu_list[i].rnti); - rx_ind_unmatched->number_of_pdus++; - if (rx_ind_unmatched->pdu_list == NULL) { - rx_ind_unmatched->pdu_list = malloc(sizeof(nfapi_nr_rx_data_pdu_t)); - } - else { - rx_ind_unmatched->pdu_list = realloc(rx_ind_unmatched->pdu_list, - sizeof(nfapi_nr_rx_data_pdu_t) * rx_ind_unmatched->number_of_pdus); - } - rx_ind_unmatched->pdu_list[rx_ind_unmatched->number_of_pdus - 1] = rx_ind->pdu_list[i]; + rx_ind_unmatched->pdu_list[num_unmatched_rxs] = rx_ind->pdu_list[i]; + num_unmatched_rxs++; remove_rx_pdu(rx_ind, i); } if (rx_ind->number_of_pdus == crc_ind->number_crcs) { break; } } - AssertFatal(rx_ind_unmatched->number_of_pdus > 0, "The nubmer of unmatched rx_ind pdus %d <= 0\n", - rx_ind_unmatched->number_of_pdus); + AssertFatal(rx_ind_unmatched->number_of_pdus == num_unmatched_rxs, "rx_ind num_pdus %d doesnt match %d\n", + rx_ind_unmatched->number_of_pdus, num_unmatched_rxs); if (!requeue(&gnb_rx_ind_queue, rx_ind_unmatched)) { LOG_E(NR_PHY, "requeue failed for rx_ind_unmatched.\n"); - free(rx_ind_unmatched); - rx_ind_unmatched = NULL; + free_and_zero(rx_ind_unmatched->pdu_list); + free_and_zero(rx_ind_unmatched); } } else { @@ -420,17 +396,19 @@ void NR_UL_indication(NR_UL_IND_t *UL_info) { if (get_softmodem_params()->emulate_l1) { if (gnb_rach_ind_queue.num_items > 0) { - LOG_I(NR_MAC, "gnb_rach_ind_queue size = %zu\n", gnb_rach_ind_queue.num_items); + LOG_D(NR_MAC, "gnb_rach_ind_queue size = %zu\n", gnb_rach_ind_queue.num_items); rach_ind = get_queue(&gnb_rach_ind_queue); + AssertFatal(rach_ind->number_of_pdus > 0, "Invalid number of PDUs\n"); UL_info->rach_ind = *rach_ind; } if (gnb_uci_ind_queue.num_items > 0) { - LOG_I(NR_MAC, "gnb_uci_ind_queue size = %zu\n", gnb_uci_ind_queue.num_items); + LOG_D(NR_MAC, "gnb_uci_ind_queue size = %zu\n", gnb_uci_ind_queue.num_items); uci_ind = get_queue(&gnb_uci_ind_queue); + AssertFatal(uci_ind->num_ucis > 0, "Invalid number of PDUs\n"); UL_info->uci_ind = *uci_ind; } if (gnb_rx_ind_queue.num_items > 0 && gnb_crc_ind_queue.num_items > 0) { - LOG_I(NR_MAC, "gnb_rx_ind_queue size = %zu and gnb_crc_ind_queue size = %zu\n", + LOG_D(NR_MAC, "gnb_rx_ind_queue size = %zu and gnb_crc_ind_queue size = %zu\n", gnb_rx_ind_queue.num_items, gnb_crc_ind_queue.num_items); rx_ind = get_queue(&gnb_rx_ind_queue); int sfn_slot = NFAPI_SFNSLOT2HEX(rx_ind->sfn, rx_ind->slot); @@ -443,11 +421,12 @@ void NR_UL_indication(NR_UL_IND_t *UL_info) { requeue(&gnb_rx_ind_queue, rx_ind); } else { + AssertFatal(rx_ind->number_of_pdus > 0, "Invalid number of PDUs\n"); + AssertFatal(crc_ind->number_crcs > 0, "Invalid number of PDUs\n"); + if (crc_ind->number_crcs != rx_ind->number_of_pdus) + match_crc_rx_pdu(rx_ind, crc_ind); UL_info->rx_ind = *rx_ind; UL_info->crc_ind = *crc_ind; - if (crc_ind->number_crcs != rx_ind->number_of_pdus) { - match_crc_rx_pdu(&UL_info->rx_ind, &UL_info->crc_ind); - } } } } diff --git a/openair2/RRC/LTE/MESSAGES/asn1_msg.c b/openair2/RRC/LTE/MESSAGES/asn1_msg.c index d271eecd5c..c3e41978e5 100644 --- a/openair2/RRC/LTE/MESSAGES/asn1_msg.c +++ b/openair2/RRC/LTE/MESSAGES/asn1_msg.c @@ -4294,7 +4294,8 @@ ssize_t do_nrMeasurementReport(uint8_t *buffer, asn1cSequenceAdd(measResultListEUTRA2->list, struct LTE_MeasResultEUTRA, measresulteutra_list); measresulteutra_list->physCellId = phy_id; /* TODO: This asn1cCalloc leaks memory but also masks a bug. - If we delete this asn1cCalloc statement, eNB will crash in NSA mode. */ + If we delete this asn1cCalloc statement, eNB will crash in NSA mode. + Please don't delete the following line unless the bug has been found. */ asn1cCalloc(measresulteutra_list->cgi_Info, measresult_cgi2); struct LTE_MeasResultEUTRA__measResult* measResult= &measresulteutra_list->measResult; asn1cCallocOne(measResult->rsrpResult, rsrp_tar);