phy: refactor push validator (review changes)

jonathan-srs · jonathan-srs · commit b55cb0e90e1b · 2024-10-11T09:32:03.000Z
diff --git a/include/srsran/phy/upper/channel_processors/pusch/pusch_processor.h b/include/srsran/phy/upper/channel_processors/pusch/pusch_processor.h
@@ -184,7 +184,7 @@ class pusch_pdu_validator
   virtual ~pusch_pdu_validator() = default;
 
   /// \brief Validates PUSCH processor configuration parameters.
-  /// \return True if the parameters contained in \c pdu are supported, false otherwise.
+  /// \return A success if the parameters contained in \c pdu are supported, an error message otherwise.
   virtual error_type<std::string> is_valid(const pusch_processor::pdu_t& pdu) const = 0;
 };
 
diff --git a/include/srsran/phy/upper/uplink_processor.h b/include/srsran/phy/upper/uplink_processor.h
@@ -161,7 +161,7 @@ class uplink_pdu_validator
   virtual error_type<std::string> is_valid(const pucch_processor::format4_configuration& config) const = 0;
 
   /// \brief Validates PUSCH configuration parameters.
-  /// \return True if the parameters contained in \c config are supported, false otherwise.
+  /// \return A success if the parameters contained in \c pdu are supported, an error message otherwise.
   virtual error_type<std::string> is_valid(const pusch_processor::pdu_t& config) const = 0;
 
   /// \brief Validates Sounding Reference Signals channel estimator configuration.
diff --git a/lib/phy/upper/channel_processors/pusch/factories.cpp b/lib/phy/upper/channel_processors/pusch/factories.cpp
@@ -238,7 +238,7 @@ class pusch_processor_factory_generic : public pusch_processor_factory
     config.dec_nof_iterations             = dec_nof_iterations;
     config.dec_enable_early_stop          = dec_enable_early_stop;
     config.csi_sinr_calc_method           = csi_sinr_calc_method;
-    return std::make_unique<pusch_processor_impl>(create_validator(), config);
+    return std::make_unique<pusch_processor_impl>(config);
   }
 
   std::unique_ptr<pusch_pdu_validator> create_validator() override
diff --git a/lib/phy/upper/channel_processors/pusch/pusch_processor_impl.cpp b/lib/phy/upper/channel_processors/pusch/pusch_processor_impl.cpp
@@ -11,6 +11,8 @@
 #include "pusch_processor_impl.h"
 #include "pusch_decoder_buffer_dummy.h"
 #include "pusch_processor_notifier_adaptor.h"
+#include "pusch_processor_validator_impl.h"
+
 #include "srsran/phy/upper/channel_processors/pusch/formatters.h"
 #include "srsran/phy/upper/channel_processors/pusch/pusch_codeword_buffer.h"
 #include "srsran/phy/upper/channel_processors/pusch/pusch_decoder_buffer.h"
@@ -101,8 +103,7 @@ class pusch_processor_csi_part1_feedback_impl : public pusch_processor_csi_part1
 // Dummy PUSCH decoder buffer. Used for PUSCH transmissions without SCH data.
 static pusch_decoder_buffer_dummy decoder_buffer_dummy;
 
-pusch_processor_impl::pusch_processor_impl(std::unique_ptr<pusch_pdu_validator> pdu_validator_, configuration& config) :
-  pdu_validator(std::move(pdu_validator_)),
+pusch_processor_impl::pusch_processor_impl(configuration& config) :
   thread_local_dependencies_pool(std::move(config.thread_local_dependencies_pool)),
   decoder(std::move(config.decoder)),
   dec_nof_iterations(config.dec_nof_iterations),
@@ -130,7 +131,8 @@ void pusch_processor_impl::process(span<uint8_t>                    data,
 
   // Assert PDU.
   [[maybe_unused]] std::string msg;
-  srsran_assert(handle_validation(msg, pdu_validator->is_valid(pdu)), "{}", msg);
+  srsran_assert(
+      handle_validation(msg, pusch_processor_validator_impl(ch_estimate.capacity()).is_valid(pdu)), "{}", msg);
 
   // Number of RB used by this transmission.
   unsigned nof_rb = pdu.freq_alloc.get_nof_rb();
diff --git a/lib/phy/upper/channel_processors/pusch/pusch_processor_impl.h b/lib/phy/upper/channel_processors/pusch/pusch_processor_impl.h
@@ -107,8 +107,8 @@ class pusch_processor_impl : public pusch_processor
   };
 
   /// \brief Constructs a generic software PUSCH processor.
-  /// \param[in] config PUSCH processor dependencies and configuration parameters.
-  pusch_processor_impl(std::unique_ptr<pusch_pdu_validator> pdu_validator_, configuration& config);
+  /// \param[in] config          PUSCH processor dependencies and configuration parameters.
+  pusch_processor_impl(configuration& config);
 
   // See interface for documentation.
   void process(span<uint8_t>                    data,
@@ -118,8 +118,6 @@ class pusch_processor_impl : public pusch_processor
                const pdu_t&                     pdu) override;
 
 private:
-  /// PUSCH PDU validator.
-  std::unique_ptr<pusch_pdu_validator> pdu_validator;
   /// Thread local dependencies pool.
   std::shared_ptr<concurrent_dependencies_pool_type> thread_local_dependencies_pool;
   /// UL-SCH transport block decoder.
diff --git a/lib/phy/upper/channel_processors/pusch/pusch_processor_validator_impl.cpp b/lib/phy/upper/channel_processors/pusch/pusch_processor_validator_impl.cpp
@@ -63,7 +63,11 @@ error_type<std::string> pusch_processor_validator_impl::is_valid(const pusch_pro
 
   // The frequency allocation is not compatible with the BWP parameters.
   if (!pdu.freq_alloc.is_bwp_valid(pdu.bwp_start_rb, pdu.bwp_size_rb)) {
-    return make_unexpected(fmt::format("Invalid BWP configuration {}:{}.", pdu.bwp_start_rb, pdu.bwp_size_rb));
+    return make_unexpected(
+        fmt::format("Invalid BWP configuration, i.e., [{}, {}) for the given RB allocation, i.e., {}.",
+                    pdu.bwp_start_rb,
+                    pdu.bwp_start_rb + pdu.bwp_size_rb,
+                    pdu.freq_alloc));
   }
 
   // Currently, none of the UCI field sizes can exceed 11 bit.
@@ -90,15 +94,15 @@ error_type<std::string> pusch_processor_validator_impl::is_valid(const pusch_pro
 
   // The number of OFDM symbols in the DM-RS mask must be equal to the number of OFDM symbols in a slot.
   if (pdu.dmrs_symbol_mask.size() != nof_symbols_slot) {
-    return make_unexpected(fmt::format("The DM-RS symbol mask size (i.e., {}) must be the same as the number of "
-                                       "symbols allocated to the transmission within the slot (i.e., {}).",
-                                       pdu.dmrs_symbol_mask.size(),
-                                       nof_symbols_slot));
+    return make_unexpected(
+        fmt::format("The DM-RS symbol mask size (i.e., {}) must be equal to the slot size (i.e., {}).",
+                    pdu.dmrs_symbol_mask.size(),
+                    nof_symbols_slot));
   }
 
   // The number of symbols carrying DM-RS must be greater than zero.
   if (pdu.dmrs_symbol_mask.none()) {
-    return make_unexpected("The number of OFDM symbols carrying DM-RS RE must be greater than zero.");
+    return make_unexpected("The number of OFDM symbols carrying DM-RS must be greater than zero.");
   }
 
   // The index of the first OFDM symbol carrying DM-RS shall be equal to or greater than the first symbol allocated to
@@ -123,7 +127,8 @@ error_type<std::string> pusch_processor_validator_impl::is_valid(const pusch_pro
 
   // None of the occupied symbols must exceed the slot size.
   if (nof_symbols_slot < (pdu.start_symbol_index + pdu.nof_symbols)) {
-    return make_unexpected(fmt::format("The occupied symbols (i.e., {}) exceed the slot size (i.e., {}).",
+    return make_unexpected(fmt::format("The symbol allocation (i.e., [{}, {})) exceeds the slot size (i.e., {}).",
+                                       pdu.start_symbol_index,
                                        pdu.start_symbol_index + pdu.nof_symbols,
                                        nof_symbols_slot));
   }
diff --git a/tests/integrationtests/phy/upper/channel_processors/pxsch_bler_test_factories.cpp b/tests/integrationtests/phy/upper/channel_processors/pxsch_bler_test_factories.cpp
@@ -275,7 +275,7 @@ std::shared_ptr<pusch_processor_factory> srsran::create_sw_pusch_processor_facto
   pusch_proc_factory_config.decoder_factory        = pusch_dec_factory;
   pusch_proc_factory_config.uci_dec_factory        = uci_dec_factory;
   pusch_proc_factory_config.ch_estimate_dimensions = {
-      MAX_NOF_PRBS, MAX_NSYMB_PER_SLOT, pusch_constants::MAX_NOF_RX_PORTS, pusch_constants::MAX_NOF_LAYERS};
+      MAX_NOF_PRBS, MAX_NSYMB_PER_SLOT, pusch_constants::MAX_NOF_RX_PORTS, 1};
   pusch_proc_factory_config.dec_nof_iterations         = nof_ldpc_iterations;
   pusch_proc_factory_config.dec_enable_early_stop      = dec_enable_early_stop;
   pusch_proc_factory_config.max_nof_concurrent_threads = max_nof_threads;
diff --git a/tests/unittests/phy/upper/channel_processors/pusch/pusch_processor_validator_test.cpp b/tests/unittests/phy/upper/channel_processors/pusch/pusch_processor_validator_test.cpp
@@ -85,8 +85,7 @@ const std::vector<test_case_t> pusch_processor_validator_test_data = {
        pdu.bwp_size_rb            = 1;
        return pdu;
      },
-     // TODO: since vrb_mask was private I took out the part that said "for a VRB mask of size 16"
-     R"(Invalid BWP configuration 0:1\.)"},
+     R"(Invalid BWP configuration, i\.e\., \[0, 1\) for the given RB allocation, i\.e\., \[15, 16\)\.)"},
     {[] {
        pusch_processor::pdu_t pdu                                      = base_pdu;
        pdu.uci.nof_csi_part1                                           = 0;
@@ -99,13 +98,14 @@ const std::vector<test_case_t> pusch_processor_validator_test_data = {
        pdu.dmrs_symbol_mask       = {true};
        return pdu;
      },
-     R"(The DM-RS symbol mask size \(i\.e\., 1\) must be the same as the number of symbols allocated to the transmission within the slot \(i\.e\., 14\)\.)"},
+     R"(The DM-RS symbol mask size \(i\.e\., 1\) must be equal to the slot size \(i\.e\., 14\)\.)"},
+
     {[] {
        pusch_processor::pdu_t pdu = base_pdu;
        pdu.dmrs_symbol_mask       = symbol_slot_mask(get_nsymb_per_slot(pdu.cp));
        return pdu;
      },
-     R"(The number of OFDM symbols carrying DM-RS RE must be greater than zero\.)"},
+     R"(The number of OFDM symbols carrying DM-RS must be greater than zero\.)"},
     {[] {
        pusch_processor::pdu_t pdu = base_pdu;
        pdu.dmrs_symbol_mask       = symbol_slot_mask(get_nsymb_per_slot(pdu.cp));
@@ -131,7 +131,7 @@ const std::vector<test_case_t> pusch_processor_validator_test_data = {
        pdu.nof_symbols            = 15;
        return pdu;
      },
-     R"(The occupied symbols \(i\.e\., 15\) exceed the slot size \(i\.e\., 14\)\.)"},
+     R"(The symbol allocation \(i\.e\., \[0, 15\)\) exceeds the slot size \(i\.e\., 14\)\.)"},
     {[] {
        pusch_processor::pdu_t pdu                                   = base_pdu;
        std::get<pusch_processor::dmrs_configuration>(pdu.dmrs).dmrs = dmrs_type::TYPE2;
@@ -178,7 +178,18 @@ const std::vector<test_case_t> pusch_processor_validator_test_data = {
        return pdu;
      },
      R"(Transform precoding is only possible with a valid number of PRB\.)"},
-
+    {[] {
+       pusch_processor::pdu_t pdu = base_pdu;
+       pdu.uci.nof_harq_ack       = 12;
+       return pdu;
+     },
+     R"(UCI field sizes in bits \(12, 1\), exceed the maximum bit size, i\.e\., 11\.)"},
+    {[] {
+       pusch_processor::pdu_t pdu = base_pdu;
+       pdu.uci.nof_csi_part1      = 12;
+       return pdu;
+     },
+     R"(UCI field sizes in bits \(0, 12\), exceed the maximum bit size, i\.e\., 11\.)"},
 };
 
 class PuschProcessorFixture : public ::testing::TestWithParam<test_case_t>

Original file line number	Diff line number	Diff line change
`@@ -238,7 +238,7 @@ class pusch_processor_factory_generic : public pusch_processor_factory`
`238`	`238`	`config.dec_nof_iterations = dec_nof_iterations;`
`239`	`239`	`config.dec_enable_early_stop = dec_enable_early_stop;`
`240`	`240`	`config.csi_sinr_calc_method = csi_sinr_calc_method;`
`241`		`- return std::make_unique<pusch_processor_impl>(create_validator(), config);`
	`241`	`+ return std::make_unique<pusch_processor_impl>(config);`
`242`	`242`	`}`
`243`	`243`
`244`	`244`	`std::unique_ptr<pusch_pdu_validator> create_validator() override`