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

Discrepency in PUSCH transmit slot lengths when generating multiple contiguous slots and frames #496

Open
naperman opened this issue Jun 21, 2024 · 13 comments

Comments

@naperman
Copy link

naperman commented Jun 21, 2024

I tried generating multiple slots in sequence using Sionna PUSCHTransmitter layer using the script shown below, where I'm setting the appropriate frame numbers and slot numbers(within each frame) within the loop. But, the slot lengths are different for slots 0, 14, 20, 34, ... compared to other slot numbers as show in the output text after the code segment.

When I plot the Resource Grid and Pilot Pattern for each slot(commented out at the end of the script), they appear identical for all slots. So why do the slot lengths differ and why for those specific slots numbers (0 and 14) within each frame?

import tensorflow as tf
import numpy as np
import matplotlib.pyplot as plt

import sionna
from sionna.nr import PUSCHConfig, PUSCHTransmitter

pusch_config = PUSCHConfig()
pusch_config.carrier.cyclic_prefix = "normal"
            
pusch_config.carrier.subcarrier_spacing = 30000/1000  # Expressed in units of KHz in Sionna
pusch_config.carrier.n_cell_id = 1                               # int, 1 (default) | [0,…,1007] 
pusch_config.carrier.n_size_grid = 106              # int, 4 (default) | [1,…,275]
pusch_config.carrier.n_start_grid = 0                            # int, 0 (default) | [0,…,2199]

pusch_config.mapping_type = "A" # string, “A” (default) | “B” ; 
pusch_config.n_rnti = 60        # int, 1 (default), [0,…,65535];
pusch_config.n_size_bwp = 104   # int, None (default), [1,…,275]; 
pusch_config.n_start_bwp = 1    # int, 0 (default) | [0,…,2199]
pusch_config.num_antenna_ports = 1
pusch_config.num_layers = 1
pusch_config.precoding = "non-codebook"   # str, “non-codebook” (default), “codebook”
pusch_config.tpmi = 0                     # int, 0 (default) | [0,…,27] ; Transmit precoding matrix indicator
pusch_config.transform_precoding = False  # bool, False (default); Use transform precoding

pusch_config.dmrs.dmrs_port_set = []   # list, [] (default) | [0,…,11];  If set to [], the port set will be equal to [0,…,num_layers-1]
pusch_config.dmrs.config_type = 1      # int, 1 (default) | 2; 
pusch_config.dmrs.length = 1           # int, 1 (default) | 2
pusch_config.dmrs.additional_position = 1  # int, 0 (default) | 1 | 2 | 3
pusch_config.dmrs.n_id = 0                 # 2-tuple, None (default), [[0,…,65535], [0,…,65535]]; If None, the property n_cell_id of the CarrierConfig is used. DM-RS scrambling identities (NID0 and NID1) - same as NIDNSCID (refer MATLAB help for nrPDSCHDMRSConfig/NIDNSCID).
pusch_config.dmrs.n_scid = 0               # int, 0 (default) | 1; DMRS scrambling initialization
pusch_config.dmrs.num_cdm_groups_without_data = 2  # int, 2 (default) | 1 | 3; 
pusch_config.dmrs.type_a_position = 2              # int, 2 (default) | 3 ; Defines the position of the first DMRS symbol within a slot. Applies only if the property mapping_type of PUSCHConfig is equal to “A”.

pusch_config.tb.channel_type = "PUSCH"  # 5G NR physical channel type. Valid choices are “PDSCH” and “PUSCH”.
pusch_config.tb.mcs_index = 16          # MCS index within selected mcs table
pusch_config.tb.mcs_table = 2           # MCS table number 
pusch_config.tb.n_id = None             # int, None (default), [0, 1023]; If None, the PUSCHConfig will automatically set to n_cell_id. Data scrambling initialization.

grid_list = []

for slot_num in range(0,36):
    num_slots_per_frame = pusch_config.carrier.num_slots_per_frame        # int, 0 (default), [0,…,num_slots_per_frame] | TODO: Find out how to extract slot number from scn
    pusch_config.carrier.frame_number = int(slot_num/num_slots_per_frame) # 0 (default), [0,…,1023]      
    pusch_config.carrier.slot_number = slot_num % (num_slots_per_frame)   # int, 0 (default), [0,…,num_slots_per_frame]; Slot number within a frame
    pusch_config.check_config()

    slotGeneratorPUSCH = PUSCHTransmitter(pusch_config, return_bits=True, output_domain='time', dtype=tf.complex64, verbose=False)
    
    batch_size = 1
    x,b = slotGeneratorPUSCH.call(batch_size)
    print(f"Slot#{slot_num} shape = {x.shape} [frame#{pusch_config.carrier.frame_number}, slotInFrame#{pusch_config.carrier.slot_number}]")
    
    #figRG = slotGeneratorPUSCH.resource_grid.show()
    #figRG.suptitle(f"Slot#{slot_num} Resource Grid")
    #figPP = slotGeneratorPUSCH.pilot_pattern.show()
    #figPP[-1].suptitle(f"Slot#{slot_num} Pilot Pattern") 

Slot#0 shape = (1, 1, 1, 18984) [frame#0, slotInFrame#0]
Slot#1 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#1]
Slot#2 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#2]
Slot#3 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#3]
Slot#4 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#4]
Slot#5 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#5]
Slot#6 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#6]
Slot#7 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#7]
Slot#8 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#8]
Slot#9 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#9]
Slot#10 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#10]
Slot#11 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#11]
Slot#12 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#12]
Slot#13 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#13]
Slot#14 shape = (1, 1, 1, 18984) [frame#0, slotInFrame#14]
Slot#15 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#15]
Slot#16 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#16]
Slot#17 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#17]
Slot#18 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#18]
Slot#19 shape = (1, 1, 1, 18704) [frame#0, slotInFrame#19]
Slot#20 shape = (1, 1, 1, 18984) [frame#1, slotInFrame#0]
Slot#21 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#1]
Slot#22 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#2]
Slot#23 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#3]
Slot#24 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#4]
Slot#25 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#5]
Slot#26 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#6]
Slot#27 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#7]
Slot#28 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#8]
Slot#29 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#9]
Slot#30 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#10]
Slot#31 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#11]
Slot#32 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#12]
Slot#33 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#13]
Slot#34 shape = (1, 1, 1, 18984) [frame#1, slotInFrame#14]
Slot#35 shape = (1, 1, 1, 18704) [frame#1, slotInFrame#15]

@naperman naperman changed the title Discrepency in PUSCH transmit slot lengths when generating multiple slots and frames Discrepency in PUSCH transmit slot lengths when generating multiple contiguous slots and frames Jun 21, 2024
@jhoydis
Copy link
Collaborator

jhoydis commented Jun 21, 2024

Hi @naperman,

The length of the cyclic prefix depends on the slot number. Please have a look at the CarrierConfig class. The implementation of the property cyclic_prefix_length is what you are looking for. I paste the relevant code snippet below:

@property
def cyclic_prefix_length(self):
    if self.cyclic_prefix=="extended":
        cp =  512*self.kappa*2**(-self.mu)
    else:
        cp = 144*self.kappa*2**(-self.mu)
        if self.slot_number in [0, 7*2**self.mu]:
            cp += 16*self.kappa
    return cp*self.t_c

Apparently, something in this implementation is incorrect according to #465. Please have a look as well.

@naperman
Copy link
Author

naperman commented Jul 1, 2024

@jhoydis I understand from your explanation, and after reading through #465 and #425 that in the sionna OFDMModulator implementation, the cyclic_prefix_length values are kept same across all symbols within a slot, to be be able to do the CP concatenation efficiently using tensor broadcast, whereas unequal CP lengths(first symbol vs rest) as required by the 3GPP spec would be significantly slower. I don't understand why the cyclic_prefix_length is made to change based on slot number though, as this is not a requirement in the relevant 3GPP specifications. Another observation is fft_size in sionna OFDMModulator is set based on number of occupied sub-carriers, but the 3GPP spec lists fixed fft_size independent of RB allocation, based only on subcarrier spacing. With this sionna CP and IFFT implementation, when i passed the sionna generated PUSCH slots into an external 3GPP compliant receiver, i saw incorrect constellation at the output of the OFDM demodulator.

Given these observations and requirements for my use case, I have the following questions:

  1. If the slot_number logic within the cyclic_prefix_length is a bug and not a required assumption within the sionna framework, is there a plan to fix this in the upcoming release to make the PUSCH slots 3GPP compliant?
  2. Same as previous question for changing cyclic_prefix_length for first OFDM symbol relative to other symbols.
  3. Same as above for fft_size in OFDM mod/demod.
  4. Same as above for generating slots by changing slot_numbers on the fly.

@jhoydis
Copy link
Collaborator

jhoydis commented Jul 1, 2024

Hi @naperman,

These are all good concerns and we plan to address them in the next release.
Some comments about the individual items:

  1. This is a bug.
  2. We might implement a more general OFDMModulator/Demodulator that can handle a different cyclic prefix for every symbol to address this issue. We can most likely add this feature in a way that does not break the API.
  3. Concerning 3, is there a particular reason why the fft_size should be independent of the resource allocation? If one would care only about the correct placement of the signal in the frequency domain, it could also be achieved by different means. But maybe there is a different reason that I am not aware of.
  4. This one is slightly more complex and requires probably breaking changes in the API. I currently do not see any alternative to precomputing DMRS for all slots and then gathering them depending on the provided slot number for every example in a batch. It is definitely a useful feature though.

@naperman
Copy link
Author

naperman commented Jul 1, 2024

Hi @naperman,

These are all good concerns and we plan to address them in the next release. Some comments about the individual items:

  1. This is a bug.
  2. We might implement a more general OFDMModulator/Demodulator that can handle a different cyclic prefix for every symbol to address this issue. We can most likely add this feature in a way that does not break the API.
  3. Concerning 3, is there a particular reason why the fft_size should be independent of the resource allocation? If one would care only about the correct placement of the signal in the frequency domain, it could also be achieved by different means. But maybe there is a different reason that I am not aware of.
  4. This one is slightly more complex and requires probably breaking changes in the API. I currently do not see any alternative to precomputing DMRS for all slots and then gathering them depending on the provided slot number for every example in a batch. It is definitely a useful feature though.

@jhoydis Thanks for the clarifications. Regarding fft_size I'm referring to the following table in 3GPP TS 38.104

Screenshot from 2024-07-01 11-32-31

I'm not sure, but it looks like these fixed FFT sizes from the spec is not implemented in sionna because the sionna.signal.utils.ifft function is limited by the underlying call to the tf.signal.ifft which does not support an fft_size input.

Ideally, it would be great to have an optional fft_size input parameter in PUSCHTransmitter which gets passed down to OFDMModulator and then to the ifft function. In my fork, i have rewritten sionna.signal.utils.ifft to take in fft_size and do a conversion of the input tensor to a cupy ndarray to apply cp.fft.ifft() (numpy and cupy ifft function do support an fft_size input) and then back to a tensor, using tf.experimental.dlpack.to_dlpack to avoid deep copies. I'm not sure how efficient this is compared to native tensorflow only computations - being able to do this without these conversions would be better.

@danielschaeufele
Copy link

Hi @naperman,

I have created a fork here, which fixes these issue and makes the code compatible to the 3GPP standard. Please let me know if you have any issues or questions.

The approach I took for the FFT size is a bit different from yours: I added the parameter sample_rate to PUSCHConfig and the FFT size is derived from that. You can set sample_rate="standard" and the FFT size will be chosen according to the table you cite above. For the implementation I used ResourceGrid.num_guard_carriers, which will insert the appropriate number of zero carriers, so that no modification to OFDMModulator is necessary.

@naperman
Copy link
Author

naperman commented Jul 11, 2024

Hi @danielschaeufele Adding the num_guard_carriers parameter to ResourceGrid initialization in PUSCHTransmitter did the trick for setting a different fft_size than num_subcarriers. Thanks for pointing that out. I have made changes similar to those in your fork to fix the first symbol cyclic_prefix_length problem. Btw, were you able to get these changes working with both TimeChannel and OFDMChannel in a link level simulation? Are there any additional considerations for TimeChannel pertaining to these changes?

@danielschaeufele
Copy link

Hi @naperman,

I didn't do any explicit link level simulations, but the test in test/unit/nr/test_pusch_receiver.py performs simulations with both TimeChannel and OFDMChannel and both work fine. There is one small caveat: Because the code for OFDMChannel doesn't support non-uniform cyclic prefix length, the channel will be sampled at a slightly wrong time.

@naperman
Copy link
Author

naperman commented Jul 12, 2024

Hi @danielschaeufele,

Thanks for bringing the channel sampling issue to attention. So far, I have only tested on typical TDL scenarios with high SNR and doppler upto to 100Hz and didn't see any degradations in BER compared to a reference simulator. I'll have to do wider sweeps over different channel parameters to check where the performance starts to degrade relative to the reference simulator.

Also, regarding contiguous multi-slot generation, were you able to arrive at a strategy to generate all slots with graph mode? I tried initializing a list of PUSCHTransmitter objects with the required slot numbers and then accessing them by invoking the callables of the objects in the list using a for loop to construct a tensor with slots concatenated along the batch dimension. The problem is, I'm not able to run the function containing this for loop in graph mode because of the use of standard python list under the @tf.function which expects everything to be tensors under it's scope.

@danielschaeufele
Copy link

Hi @naperman,

As far as I know there currently is no way to generate multiple slots with a single PUSCHTransmitter object. So far I was lucky enough to use transform precoding with 30KHz SCS, where the slot number does not affect the generated signal, so I could just reuse the same object 😅

@jhoydis
Copy link
Collaborator

jhoydis commented Jul 15, 2024

Hi @danielschaeufele and @naperman,

The issue is not forgotten from our side. The way that we will probably make the PUSCHTransmitter/Receiver work with arbitrary slot indices, is to precompute the DMRS signals for all slots and then gather them depending on the slots which are supposed to be simulated. Ideally one could provide for every example in a batch the desired slot number as input.
This approach should work on XLA, too.

@danielschaeufele
Copy link

Hi @jhoydis,

I think another issue will be that not every slot has the same number of time domain samples, because for SCS > 30KHz, not every slot has the longer cyclic prefix. So maybe this can be solved with a ragged tensor.

@naperman
Copy link
Author

naperman commented Oct 9, 2024

Is this issue still being tracked?

@danielschaeufele
Copy link

There is #465, which will hopefully be merged soon. This will at least generate the correct cyclic prefix length. However, this won't help with generating multiple slots at once. I don't know if there are any plans to provide a good solution for this...

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

No branches or pull requests

3 participants