Project

General

Profile

Feature #2977

OsmoBTS measurment processing at L1SAP too complex / pass measurements along with data

Added by laforge about 1 year ago. Updated 6 days ago.

Status:
Stalled
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
02/21/2018
Due date:
% Done:

20%

Spec Reference:

Description

The entire dualism of PH_DATA.ind / PH_TCH.ind containg (unsued) measurement data, but then having a separate PRIM_INFO_MEAS is odd to begin with. The measurements should always accompany the PH-DATA.ind / PH-TCH.ind and PRIM_INFO_MEAS should be abandoned.

notes.txt notes.txt 5.18 KB dexter, 01/21/2019 03:25 PM
notes.txt notes.txt 7.12 KB dexter, 01/23/2019 12:51 PM

Related issues

Related to OsmoBTS - Bug #3032: (Wrong?) measurement of both RSSI and ToA on TCH channelsNew2018-03-05

Related to OsmoBTS - Bug #2975: OsmoBTS doesn't generate measurement indications in absence of uplink burstsStalled2018-02-21

Related to OsmoBTS - Feature #3530: merge PRIM_INFO_MEAS into PRIM_PH_DATA and PRIM_TCHNew2018-09-06

Related to OsmoBTS - Bug #3428: Too many contiguous elapsed fn, dropping...Stalled2018-07-28

Related to OsmoPCU - Bug #3395: Uplink CS/MCS control is broken osmo-pcu is used with osmo-bts-trx/osmo-trxFeedback2018-07-14

Related to OsmoPCU - Feature #1526: Acquire/update timing advance (TA)Stalled2016-02-22

Related to OsmoBTS - Bug #3803: fix frame number calculation in scheduler_trxNew2019-02-15

History

#1 Updated by laforge 12 months ago

  • Related to Bug #3032: (Wrong?) measurement of both RSSI and ToA on TCH channels added

#2 Updated by laforge 12 months ago

Ideally, we should attach all measurement information in the best resolution (ber10k, toa256, rssi) to the PH-DATA.ind and remove the somwhat strange INFO-IND that we use separately at the moment. After all, we have the following cases:

  • measurements only, no valid data: Use PH-DATA.ind with zero-length payload (bad frame)
  • measurements + data: Use fully filled-in PH-DATA.ind
  • data without measurements: doesn't exist, doesn't make sense to support (but theoretically possible now).

#3 Updated by fixeria 12 months ago

  • Assignee set to fixeria

#4 Updated by fixeria 5 months ago

  • Tracker changed from Bug to Feature
  • Assignee deleted (fixeria)

#5 Updated by fixeria 5 months ago

  • Related to Bug #2975: OsmoBTS doesn't generate measurement indications in absence of uplink bursts added

#6 Updated by fixeria 5 months ago

  • Related to Feature #3530: merge PRIM_INFO_MEAS into PRIM_PH_DATA and PRIM_TCH added

#7 Updated by laforge 5 months ago

  • Subject changed from OsmoBTS measurment processing at L1SAP too complex to OsmoBTS measurment processing at L1SAP too complex / pass measurements along with data
  • Assignee set to dexter

#8 Updated by dexter 5 months ago

This looks like a duplicate of #3530, probably we should reject #3530 then.

#9 Updated by pespin 4 months ago

  • Related to Bug #3428: Too many contiguous elapsed fn, dropping... added

#10 Updated by dexter about 1 month ago

  • File notes.txt added

#11 Updated by dexter about 1 month ago

  • File deleted (notes.txt)

#12 Updated by dexter about 1 month ago

I had another look into the code to pinpoint the locations where a change is needed. We will have to touch l1sap.h in libosmocore but I think when we just append struct members to struct ph_data_param and struct ph_tch_param we should be ok.

Attached one finds my notes. To me the process of changing the flow of the measurement results seems to be straight forward, but in some cases the old code did send a measurement indication up, but skipped the related data part because it has checked the received data bad. When we put both together we can either choose to skip the measurement as well or not to skip it but hand up empty data/tch into the common code. The latter one looks more correct to me. Its fine if we get empty tch/data, if required we can skip it anyway.

#13 Updated by dexter 29 days ago

I now further inspected the code. Its possible to merge meas_ind and data/tch, but before we can do that we need to resolve a few open questions. Especially the fn and the toa256 values are computed differently for meas_ind and tch/data. This is a bit confusing. I am not sure if it is the right way to add separate meas_ members to the tch/data struct. I would prefer not to add redundancy to the the tch/data strucht.

I have attached the current state of my notes and I have marked all open questions with a (!).

#14 Updated by laforge 26 days ago

adding tnt as a watcher here as he's working on #1851 which is somewhat related.

#15 Updated by laforge 26 days ago

Responding / quoting here would have been much easier if the "notes.txt" was simply copy+pasted. Please do that in the future, thanks!

scheduler_trx.c:tx_data_fn()
-> l1_if.c->l1if_process_meas_res()->l1sap_up() (sends meas_ind)
common/scheduler.c:_sched_compose_ph_data_ind()->l1sap_up() (sends data)
(directly one after another in the middle)

(!) Why do we use hardcoded values here?

Please see the comment "handle loss detection of SACCH". So basically we're sending substitute measurements in case there were lost frames. I'm not sure if this should still be done. I guess you are the person understanding the requirements of the higher-level code best in terms of whether and where we must substitute something. The hardcoded values indicate 100% BER and -110 dBm receive level, which are the "worst case" which we must assume in case a block was lost.

(!) Why do we use hardcoded frame number for data (0) and the real fn for meas?

I don't know.

scheduler_trx.c:rx_data_fn()
-> l1_if.c->l1if_process_meas_res()->l1sap_up() (sends meas_ind)
common/scheduler.c:_sched_compose_ph_data_ind()->l1sap_up() (sends data)
(directly one after another at the end)

(!) 4 * (*toa256_sum) / *toa_num != *toa256_sum / *toa_num -- Why that?

it's computing the toa256 average and then multiplying it to four, which may be a remainder of having previously used quarter-bits in the ph_data_ind. This seems wrong, as the function _sched_compose_ph_data_ind() has the argument "ta_offs_256bits". The factor-of-four is wrong here, IMHO.

Commit acefd0586e5d463b2e7a6a039131994bc12573fc introduced the "toa256" resolution change. Before the change, _sched_compose_ph_data_ind() used quarter-bits as units, now it does 256th bits. l1if_fill_meas_res() was changed, but somehow the other callers weren't changed. This means that the measurement reports are all wrong now in terms of TOA :(

(!) Cosmetic: link_qual_cb should be NULL instead of 0

ACK.

scheduler_trx.c:rx_pdtch_fn()
-> l1_if.c->l1if_process_meas_res()->l1sap_up() (sends meas_ind)
(may return 0 when PDTCH is bad)
common/scheduler.c:_sched_compose_ph_data_ind()->l1sap_up() (sends data)
(in case of bad PDTCH we will not get any measurement info anymore
or we allow to hand up bad pdtch data --> problem?)

(!) 4 * (*toa256_sum) / *toa_num != *toa256_sum / *toa_num -- Why that?

Same bug as above.

(!) fn != (fn + GSM_HYPERFRAME - 3) % GSM_HYPERFRAME

I guess this is some poor man's approach at converting a "last burst of block" frame number to "first burst of block" ? Might be a bug or some really odd difference how frame numbers are treated on he PDTCH/PCU side? But as it's only done inside osmo-bts-trx and not osmo-bts-{sysmo,...} I'd guess it's probably rather a bug? Maybe it doesn't matter as the frame number isn't used for anythign important? Needs more investingation!

scheduler_trx.c:rx_tchf_fn()
-> l1_if.c->l1if_process_meas_res()->l1sap_up() (sends meas_ind)
(complex decision logic in between, decides between TCH and FACCH)
common/scheduler.c:_sched_compose_ph_data_ind()->l1sap_up() (sends facch)
common/scheduler.c:_sched_compose_tch_ind()->l1sap_up() (sends tch, located at the end)

(!) FACCH: tn != (fn + GSM_HYPERFRAME - 7) % GSM_HYPERFRAME ?
(!) TCH: fn != (fn + GSM_HYPERFRAME - 7) % GSM_HYPERFRAME

It's probably again converting the frame number of the last burst of the block to the frame number of the first burst of the block. As TCH/F are spread over 8 bursts, it seems correct?

(!) FACCH: toa256 != 4 * toa256

Same bug as above.

scheduler_trx.c:rx_tchh_fn()

(!) FACCH: *first_fn != fn + GSM_HYPERFRAME - 10 - ((fn % 26) >= 19)) % GSM_HYPERFRAME?
(!) TCH: *first_fn != fn + GSM_HYPERFRAME - 10 - ((fn%26)==19) - ((fn%26)==20)) % GSM_HYPERFRAME

Again, likely the conversion from last-fn to first-fn, which is a bit more complicated in case of TCH/H

(!) FACCH: toa256 != toa256/64

Same bug as above.

#16 Updated by laforge 26 days ago

This is measurement related members are already present in data or tch indication:
  • int8_t rssi; (same as inv_rssi?)
  • uint32_t fn; /*!< GSM Frame Number */
  • uint16_t ber10k; /*!< BER in units of 0.01% */

inv_rssi is the inverse of RSSI. It's the same value just expressed differently.

(!) How do we handle the is_sub field? octphy does not set it.
==> who sets the flag? l1sap.c seems to read it only, but nobody seems
to set it!

The "is_sub" flag expresses if a given measurement/block is part of the RX*SUB (true), or only part of the RX*FULL measurements. This is important in the context of DTX. For HR/FR/EFR it's rather easy. I think it was even you who implemented the ts45008_83_is_sub() function which computes this flag. It is set in common/measurement.c:

        /* We expect the lower layers to mark AMR SID_UPDATE frames already as such.
         * In this function, we only deal with the comon logic as per the TS 45.008 tables */
        if (!ulm->is_sub)
                ulm->is_sub = ts45008_83_is_sub(lchan, fn, false);

So basically for the AMR SID UPDATE blocks the lower layer is responsible to mark them, for all other blocks the common layer takes care of it.

#17 Updated by laforge 26 days ago

  • Status changed from New to In Progress
  • % Done changed from 0 to 10

#18 Updated by laforge 26 days ago

I just pushed https://gerrit.osmocom.org/#/c/osmo-bts/+/12699 as an attempt to fix the "toa *4" error in osmo-bts-trx. The patch is not tested yet.

#19 Updated by fixeria 26 days ago

Just my two cents:

PDTCH: (!) fn != (fn + GSM_HYPERFRAME - 3) % GSM_HYPERFRAME
FACCH/F: (!) fn != (fn + GSM_HYPERFRAME - 7) % GSM_HYPERFRAME
TCH/F: fn != (fn + GSM_HYPERFRAME - 7) % GSM_HYPERFRAME

This is not only poor, but also a wrong approach. There is no warranty that
(fn + GSM_HYPERFRAME - X) % GSM_HYPERFRAME would always point to the first
burst (bid == 0) of a block. Almost every multiframe layout has IDLE frames,
TCH multiframes also do contain SACCH frames, while TCH/H is a pure nightmare.

A proper approach to calculate the first frame number using the last is
to introduce a few functions, which would rely on the existing multiframe
layouts. This is how it's done in trxcon for TCH/H.

#20 Updated by msuraev 25 days ago

  • Related to Bug #3395: Uplink CS/MCS control is broken osmo-pcu is used with osmo-bts-trx/osmo-trx added

#21 Updated by msuraev 25 days ago

  • Related to Feature #1526: Acquire/update timing advance (TA) added

#22 Updated by dexter 24 days ago

#23 Updated by dexter 20 days ago

  • % Done changed from 10 to 20

I have had a closer look to those wired fn calculations ((fn + GSM_HYPERFRAME - x) % GSM_HYPERFRAME). To me it sounds logical that the intend behind those formulas is to calculate the fn that belongs to the beginning of the block. The measurement reports are also connected to the beginning of the block. However, the measurement indications use a pointer *first_fn, which always stores the fn of the beginning of the block but the value there is not calculated, it is just stored when the first burst of the block arrives. This is much simpler and I have exchanged the formulas with the *first_fn pointers. Then it is also coherent with the measurement indications.

A patch can be found here:
https://gerrit.osmocom.org/#/c/osmo-bts/+/12779 scheduler_trx: use stored block fn instead of calculating it.

To make sure this change does not mess up anything I have checked through all possible PDCH locations and all possible TCH/F and TCH/H locations. I noticed no malfunctions or any other suspicious behavior. I also did a testrun with ttcn3 and docker. This also showed no problems.

====

Also for the sysmo-bts I have the same experiment running as I already have for osmo-bts-trx. The measurement indication is no attached to tch and data indications. However, this still needs some cleanups. However, once the problems with toa256 and the fn are resolved things should fall in place.

#24 Updated by dexter 6 days ago

  • Related to Bug #3803: fix frame number calculation in scheduler_trx added

#25 Updated by dexter 6 days ago

I think before we can move on here we need to fix the formulas in scheduler_trx.c. This is a dependency, before tch indication and measurement indication do not use coherent frame numbers we can not merge the primitives. See also #3803

#26 Updated by dexter 6 days ago

  • Status changed from In Progress to Stalled

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)