phy: refactor pusch validator

Author: jonathan-srs

include/srsran/phy/upper/channel_processors/pusch/pusch_processor.h

@@ -10,6 +10,7 @@
 
 #pragma once
 
+#include "srsran/adt/expected.h"
 #include "srsran/adt/static_vector.h"
 #include "srsran/phy/support/re_pattern.h"
 #include "srsran/phy/upper/channel_coding/ldpc/ldpc.h"
@@ -184,7 +185,7 @@ class pusch_pdu_validator
 
   /// \brief Validates PUSCH processor configuration parameters.
   /// \return True if the parameters contained in \c pdu are supported, false otherwise.
-  virtual bool is_valid(const pusch_processor::pdu_t& pdu) const = 0;
+  virtual error_type<std::string> is_valid(const pusch_processor::pdu_t& pdu) const = 0;
 };
 
 } // namespace srsran

include/srsran/phy/upper/uplink_processor.h

@@ -162,7 +162,7 @@ class uplink_pdu_validator
 
   /// \brief Validates PUSCH configuration parameters.
   /// \return True if the parameters contained in \c config are supported, false otherwise.
-  virtual bool is_valid(const pusch_processor::pdu_t& config) const = 0;
+  virtual error_type<std::string> is_valid(const pusch_processor::pdu_t& config) const = 0;
 
   /// \brief Validates Sounding Reference Signals channel estimator configuration.
   /// \return True if the parameters contained in \c config are supported, false otherwise.

lib/fapi_adaptor/phy/fapi_to_phy_translator.cpp

@@ -439,9 +439,11 @@ static expected<uplink_pdus> translate_ul_tti_pdus_to_phy_pdus(const fapi::ul_tt
       case fapi::ul_pdu_type::PUSCH: {
         uplink_processor::pusch_pdu& ul_pdu = pdus.pusch.emplace_back();
         convert_pusch_fapi_to_phy(ul_pdu, pdu.pusch_pdu, msg.sfn, msg.slot, carrier_cfg.num_rx_ant, part2_repo);
-        if (!ul_pdu_validator.is_valid(ul_pdu.pdu)) {
-          logger.warning("Upper PHY flagged a PUSCH PDU as having an invalid configuration. Skipping UL_TTI.request");
-
+        error_type<std::string> phy_pusch_validator = ul_pdu_validator.is_valid(ul_pdu.pdu);
+        if (!phy_pusch_validator.has_value()) {
+          logger.warning(
+              "Skipping UL_TTI.request: PUSCH PDU flagged as invalid by the Upper PHY with the following error\n    {}",
+              phy_pusch_validator.error());
           return make_unexpected(default_error_t{});
         }
         break;

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>(config);
+    return std::make_unique<pusch_processor_impl>(create_validator(), config);
   }
 
   std::unique_ptr<pusch_pdu_validator> create_validator() override

lib/phy/upper/channel_processors/pusch/pusch_processor_impl.cpp

@@ -23,6 +23,17 @@
 
 using namespace srsran;
 
+/// Looks at the output of the validator and, if unsuccessful, fills msg with the error message.
+/// This is used to call the validator inside the process methods only if asserts are active.
+[[maybe_unused]] static bool handle_validation(std::string& msg, const error_type<std::string>& err)
+{
+  bool is_success = err.has_value();
+  if (!is_success) {
+    msg = err.error();
+  }
+  return is_success;
+}
+
 namespace {
 class pusch_processor_csi_part1_feedback_impl : public pusch_processor_csi_part1_feedback
 {
@@ -90,7 +101,8 @@ 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(configuration& config) :
+pusch_processor_impl::pusch_processor_impl(std::unique_ptr<pusch_pdu_validator> pdu_validator_, configuration& config) :
+  pdu_validator(std::move(pdu_validator_)),
   thread_local_dependencies_pool(std::move(config.thread_local_dependencies_pool)),
   decoder(std::move(config.decoder)),
   dec_nof_iterations(config.dec_nof_iterations),
@@ -117,7 +129,8 @@ void pusch_processor_impl::process(span<uint8_t>                    data,
   channel_estimate& ch_estimate = dependencies.get_channel_estimate();
 
   // Assert PDU.
-  assert_pdu(pdu, ch_estimate);
+  [[maybe_unused]] std::string msg;
+  srsran_assert(handle_validation(msg, pdu_validator->is_valid(pdu)), "{}", msg);
 
   // Number of RB used by this transmission.
   unsigned nof_rb = pdu.freq_alloc.get_nof_rb();
@@ -306,49 +319,3 @@ void pusch_processor_impl::process(span<uint8_t>                    data,
   dependencies.get_demodulator().demodulate(
       demodulator_buffer, notifier_adaptor.get_demodulator_notifier(), grid, ch_estimate, demod_config);
 }
-
-void pusch_processor_impl::assert_pdu(const pusch_processor::pdu_t& pdu, const channel_estimate& ch_estimate) const
-{
-  using namespace units::literals;
-  interval<unsigned, true> nof_tx_layers_range(1, ch_estimate.capacity().nof_tx_layers);
-
-  // Make sure the configuration is supported.
-  srsran_assert((pdu.bwp_start_rb + pdu.bwp_size_rb) <= ch_estimate.capacity().nof_prb,
-                "The sum of the BWP start (i.e., {}) and size (i.e., {}) exceeds the maximum grid size (i.e., {} PRB).",
-                pdu.bwp_start_rb,
-                pdu.bwp_size_rb,
-                ch_estimate.capacity().nof_prb);
-  if (std::holds_alternative<dmrs_configuration>(pdu.dmrs)) {
-    const auto& dmrs = std::get<dmrs_configuration>(pdu.dmrs);
-    srsran_assert(dmrs.dmrs == dmrs_type::TYPE1, "Only DM-RS Type 1 is currently supported.");
-    srsran_assert(dmrs.nof_cdm_groups_without_data == 2, "Only two CDM groups without data are currently supported.");
-  } else {
-    srsran_assert(pdu.nof_tx_layers == 1, "Transform precoding is only possible with one layer.");
-    srsran_assert(pdu.freq_alloc.is_contiguous(), "Transform precoding is only possible with contiguous allocations.");
-    srsran_assert(is_transform_precoding_nof_prb_valid(pdu.freq_alloc.get_nof_rb()),
-                  "Transform precoding is only possible with a valid number of PRB.");
-  }
-  srsran_assert(nof_tx_layers_range.contains(pdu.nof_tx_layers),
-                "The number of transmit layers (i.e., {}) is out of the range {}.",
-                pdu.nof_tx_layers,
-                nof_tx_layers_range);
-  srsran_assert(pdu.rx_ports.size() <= ch_estimate.capacity().nof_rx_ports,
-                "The number of receive ports (i.e., {}) exceeds the maximum number of receive ports (i.e., {}).",
-                pdu.rx_ports.size(),
-                ch_estimate.capacity().nof_rx_ports);
-
-  srsran_assert(pdu.uci.csi_part2_size.is_valid(pdu.uci.nof_csi_part1),
-                "CSI Part 1 UCI field length (i.e., {}) does not correspond with the CSI Part 2 (i.e., {})",
-                pdu.uci.nof_csi_part1,
-                pdu.uci.csi_part2_size);
-  srsran_assert(pdu.tbs_lbrm > 0_bytes, "Invalid LBRM size (0 bytes).");
-
-  // Check DC is whithin the CE.
-  if (pdu.dc_position.has_value()) {
-    interval<unsigned> dc_position_range(0, ch_estimate.size().nof_prb * NRE);
-    srsran_assert(dc_position_range.contains(pdu.dc_position.value()),
-                  "DC position (i.e., {}) is out of range {}.",
-                  pdu.dc_position.value(),
-                  dc_position_range);
-  }
-}

lib/phy/upper/channel_processors/pusch/pusch_processor_impl.h

@@ -108,7 +108,7 @@ 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(configuration& config);
+  pusch_processor_impl(std::unique_ptr<pusch_pdu_validator> pdu_validator_, configuration& config);
 
   // See interface for documentation.
   void process(span<uint8_t>                    data,
@@ -118,9 +118,8 @@ class pusch_processor_impl : public pusch_processor
                const pdu_t&                     pdu) override;
 
 private:
-  /// Asserts the PDU. It triggers an assertion upon an invalid value or combination of values.
-  void assert_pdu(const pusch_processor::pdu_t& pdu, const channel_estimate& ch_estimate) const;
-
+  /// 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.

lib/phy/upper/channel_processors/pusch/pusch_processor_validator_impl.cpp

@@ -11,6 +11,7 @@
 #include "pusch_processor_validator_impl.h"
 #include "pusch_processor_impl.h"
 #include "srsran/ran/transform_precoding/transform_precoding_helpers.h"
+#include "srsran/ran/uci/uci_formatters.h"
 
 using namespace srsran;
 
@@ -31,115 +32,137 @@ pusch_processor_validator_impl::pusch_processor_validator_impl(
                 nof_tx_layers_range);
 }
 
-bool pusch_processor_validator_impl::is_valid(const pusch_processor::pdu_t& pdu) const
+error_type<std::string> pusch_processor_validator_impl::is_valid(const pusch_processor::pdu_t& pdu) const
 {
   using namespace units::literals;
   unsigned nof_symbols_slot = get_nsymb_per_slot(pdu.cp);
 
   // The BWP size exceeds the grid size.
   if ((pdu.bwp_start_rb + pdu.bwp_size_rb) > ce_dims.nof_prb) {
-    return false;
+    return make_unexpected(fmt::format(
+        "The sum of the BWP start (i.e., {}) and size (i.e., {}) exceeds the maximum grid size (i.e., {} PRB).",
+        pdu.bwp_start_rb,
+        pdu.bwp_size_rb,
+        ce_dims.nof_prb));
   }
 
   // The implementation only works with a single transmit layer.
   if (pdu.nof_tx_layers > ce_dims.nof_tx_layers) {
-    return false;
+    return make_unexpected(fmt::format("The number of transmit layers (i.e., {}) is out of the range {}.",
+                                       pdu.nof_tx_layers,
+                                       interval<unsigned, true>(1, ce_dims.nof_tx_layers)));
   }
 
   // The number of receive ports cannot exceed the maximum dimensions.
   if (pdu.rx_ports.size() > ce_dims.nof_rx_ports) {
-    return false;
+    return make_unexpected(
+        fmt::format("The number of receive ports (i.e., {}) exceeds the maximum number of receive ports (i.e., {}).",
+                    pdu.rx_ports.size(),
+                    ce_dims.nof_rx_ports));
   }
 
   // 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 false;
+    return make_unexpected(fmt::format("Invalid BWP configuration {}:{}.", pdu.bwp_start_rb, pdu.bwp_size_rb));
   }
 
   // Currently, none of the UCI field sizes can exceed 11 bit.
   static constexpr unsigned max_uci_len = 11;
   if ((pdu.uci.nof_harq_ack > max_uci_len) || (pdu.uci.nof_csi_part1 > max_uci_len)) {
-    return false;
-  }
-
-  // CSI Part 2 must not be present if CSI Part 1 is not present.
-  if ((pdu.uci.nof_csi_part1 == 0) && !pdu.uci.csi_part2_size.entries.empty()) {
-    return false;
+    return make_unexpected(fmt::format("UCI field sizes in bits ({}, {}), exceed the maximum bit size, i.e., {}.",
+                                       pdu.uci.nof_harq_ack,
+                                       pdu.uci.nof_csi_part1,
+                                       max_uci_len));
   }
 
   // CSI Part 2 size parameters must be compatible with the CSI Part 1 number of bits.
   if (!pdu.uci.csi_part2_size.is_valid(pdu.uci.nof_csi_part1)) {
-    return false;
+    return make_unexpected(
+        fmt::format("CSI Part 1 UCI field length (i.e., {}) does not correspond with the CSI Part 2 (i.e., {}).",
+                    pdu.uci.nof_csi_part1,
+                    pdu.uci.csi_part2_size));
   }
 
   // The limited buffer for rate matching size must not be zero.
   if (pdu.tbs_lbrm == 0_bytes) {
-    return false;
+    return make_unexpected("Invalid LBRM size (0 bytes).");
   }
 
   // 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 false;
+    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));
   }
 
   // The number of symbols carrying DM-RS must be greater than zero.
   if (pdu.dmrs_symbol_mask.none()) {
-    return false;
+    return make_unexpected("The number of OFDM symbols carrying DM-RS RE 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
   // transmission.
   int first_dmrs_symbol_index = pdu.dmrs_symbol_mask.find_lowest(true);
   if (static_cast<unsigned>(first_dmrs_symbol_index) < pdu.start_symbol_index) {
-    return false;
+    return make_unexpected(fmt::format("The index of the first OFDM symbol carrying DM-RS (i.e., {}) must be equal to "
+                                       "or greater than the first symbol allocated to transmission (i.e., {}).",
+                                       first_dmrs_symbol_index,
+                                       pdu.start_symbol_index));
   }
 
   // The index of the last OFDM symbol carrying DM-RS shall not be larger than the last symbol allocated to
   // transmission.
   int last_dmrs_symbol_index = pdu.dmrs_symbol_mask.find_highest(true);
   if (static_cast<unsigned>(last_dmrs_symbol_index) >= (pdu.start_symbol_index + pdu.nof_symbols)) {
-    return false;
+    return make_unexpected(fmt::format("The index of the last OFDM symbol carrying DM-RS (i.e., {}) must be less than "
+                                       "or equal to the last symbol allocated to transmission (i.e., {}).",
+                                       last_dmrs_symbol_index,
+                                       (pdu.start_symbol_index + pdu.nof_symbols - 1)));
   }
 
   // None of the occupied symbols must exceed the slot size.
   if (nof_symbols_slot < (pdu.start_symbol_index + pdu.nof_symbols)) {
-    return false;
+    return make_unexpected(fmt::format("The occupied symbols (i.e., {}) exceed the slot size (i.e., {}).",
+                                       pdu.start_symbol_index + pdu.nof_symbols,
+                                       nof_symbols_slot));
   }
 
   // Check if transform precoding is enabled.
   if (std::holds_alternative<pusch_processor::dmrs_configuration>(pdu.dmrs)) {
-    const pusch_processor::dmrs_configuration& dmrs_config = std::get<pusch_processor::dmrs_configuration>(pdu.dmrs);
+    const auto& dmrs_config = std::get<pusch_processor::dmrs_configuration>(pdu.dmrs);
     // Only DM-RS Type 1 is supported.
     if (dmrs_config.dmrs != dmrs_type::TYPE1) {
-      return false;
+      return make_unexpected("Only DM-RS Type 1 is currently supported.");
     }
 
     // Only two CDM groups without data is supported.
     if (dmrs_config.nof_cdm_groups_without_data != 2) {
-      return false;
+      return make_unexpected("Only two CDM groups without data are currently supported.");
     }
   } else {
     // Number of layers must be one.
     if (pdu.nof_tx_layers != 1) {
-      return false;
+      return make_unexpected("Transform precoding is only possible with one layer.");
     }
 
     // Frequency allocation must be contiguous.
     if (!pdu.freq_alloc.is_contiguous()) {
-      return false;
+      return make_unexpected("Transform precoding is only possible with contiguous allocations.");
     }
 
     // Number of PRB must be valid.
     if (!is_transform_precoding_nof_prb_valid(pdu.freq_alloc.get_nof_rb())) {
-      return false;
+      return make_unexpected("Transform precoding is only possible with a valid number of PRB.");
     }
   }
 
   // DC position is outside the channel estimate dimensions.
   interval<unsigned> dc_position_range(0, ce_dims.nof_prb * NRE);
   if (pdu.dc_position.has_value() && !dc_position_range.contains(pdu.dc_position.value())) {
-    return false;
+    return make_unexpected(
+        fmt::format("DC position (i.e., {}) is out of range {}.", pdu.dc_position.value(), dc_position_range));
   }
 
-  return true;
+  return default_success_t();
 }

lib/phy/upper/channel_processors/pusch/pusch_processor_validator_impl.h

@@ -9,6 +9,7 @@
  */
 #pragma once
 
+#include "srsran/adt/expected.h"
 #include "srsran/phy/upper/channel_estimation.h"
 #include "srsran/phy/upper/channel_processors/pusch/pusch_processor.h"
 
@@ -27,7 +28,7 @@ class pusch_processor_validator_impl : public pusch_pdu_validator
   explicit pusch_processor_validator_impl(const channel_estimate::channel_estimate_dimensions& ce_dims_);
 
   // See interface for documentation.
-  bool is_valid(const pusch_processor::pdu_t& pdu) const override;
+  error_type<std::string> is_valid(const pusch_processor::pdu_t& pdu) const override;
 
 private:
   channel_estimate::channel_estimate_dimensions ce_dims;

lib/phy/upper/upper_phy_pdu_validators.h

@@ -55,7 +55,10 @@ class uplink_processor_validator_impl : public uplink_pdu_validator
   {
     return pucch->is_valid(config);
   }
-  bool is_valid(const pusch_processor::pdu_t& config) const override { return pusch->is_valid(config); }
+  error_type<std::string> is_valid(const pusch_processor::pdu_t& config) const override
+  {
+    return pusch->is_valid(config);
+  }
   bool is_valid(const srs_estimator_configuration& config) const override { return srs->is_valid(config); }
 
 private:

tests/unittests/fapi_adaptor/phy/fapi_to_phy_translator_test.cpp

@@ -79,8 +79,8 @@ class uplink_pdu_validator_dummy : public uplink_pdu_validator
   {
     return default_success_t();
   }
-  bool is_valid(const pusch_processor::pdu_t& pdu) const override { return true; }
-  bool is_valid(const srs_estimator_configuration& config) const override { return true; }
+  error_type<std::string> is_valid(const pusch_processor::pdu_t& pdu) const override { return default_success_t(); }
+  bool                    is_valid(const srs_estimator_configuration& config) const override { return true; }
 };
 
 class resource_grid_pool_dummy : public resource_grid_pool, private shared_resource_grid::pool_interface