Project

General

Profile

Support #3023

src/common/pcu_sock.c/pcu_sock_close(): The array 'ts->lchan' is being utilized as a pointer to single object, ts->lchan[0] would be clearer

Added by fixeria 8 months ago. Updated 16 days ago.

Status:
Resolved
Priority:
Low
Assignee:
Category:
-
Target version:
-
Start date:
03/01/2018
Due date:
% Done:

100%

Spec Reference:

Description

The gsm_bts_trx_ts struct has array of lchans:

struct gsm_lchan lchan[TS_MAX_LCHAN];

However, in the pcu_sock_close() it's utilized as a pointer to single object:

for (j = 0; j < 8; j++) {
    ts = &trx->ts[j];
    if (ts->mo.nm_state.operational == NM_OPSTATE_ENABLED
     && ts->pchan == GSM_PCHAN_PDCH) {
        ts->lchan->rel_act_kind = LCHAN_REL_ACT_PCU;
        l1sap_chan_rel(trx, gsm_lchan2chan_nr(ts->lchan));
    }
}

Is it intended?

History

#1 Updated by laforge 22 days ago

  • Assignee set to neels

#2 Updated by neels 20 days ago

  • Tracker changed from Bug to Support
  • Subject changed from src/common/pcu_sock.c/pcu_sock_close(): The array 'ts->lchan' is being utilized as a pointer to single object to src/common/pcu_sock.c/pcu_sock_close(): The array 'ts->lchan' is being utilized as a pointer to single object, ts->lchan[0] would be clearer
  • Status changed from New to Feedback
  • Assignee changed from neels to fixeria
  • Priority changed from Normal to Low

ts->lchan0.foo would be clearer, but yes, it is correct, since any timeslot used for PDCH has only one lchan anyway. Accessing ts->lchan->foo is accessing this first item.
fixeria, feel free to submit a comment or change it to lchan0 instead.

#3 Updated by fixeria 19 days ago

since any timeslot used for PDCH has only one lchan anyway

Hmm, but (according to scheduler.h and trx_chan_desc) there are two plchan types:
TRXC_PTCCH and TRXC_PDTCH. neels, are you sure we don't need to distinguish between them?

#4 Updated by laforge 19 days ago

Technically, the PDCH consists of various sub-channels such as PDTCH, PACCH, PTCCH.

I just don't think we are representing that difference to OsmoBTS in the gsm_lchan
structure, as it doesn't really make any difference (the messages are all encoded
the same). gsm_chan_t also doesn't have anything for PTCCH or PACCH specifically

Hmm, but (according to scheduler.h and trx_chan_desc) there are two plchan types:
TRXC_PTCCH and TRXC_PDTCH. neels, are you sure we don't need to distinguish between them?

This is "just" for osmo-bts-trx, as the PTCCH is scheduled at specific points
in time during downlink and uplink. The separation is made only by the following
macro:

#define L1SAP_IS_PTCCH(fn) (((fn % 52) 12) || ((fn % 52) 38))

Regards,
Harald

#5 Updated by fixeria 16 days ago

Hi,

I just checked GSM TS 05.02, and I am getting confused again...

Technically, the PDCH consists of various sub-channels such as PDTCH, PACCH, PTCCH.
[...]
I just don't think we are representing that difference to OsmoBTS in the gsm_lchan
structure, as it doesn't really make any difference (the messages are all encoded
the same). gsm_chan_t also doesn't have anything for PTCCH or PACCH specifically

Actually, we do distinguish between both PDTCH and PTCCH.

Please see the definition of PDCH multi-frame in 'scheduler_mframe.c':

https://git.osmocom.org/osmo-bts/tree/src/common/scheduler_mframe.c#n926

and definition of both logical channels in 'scheduler.c':

https://git.osmocom.org/osmo-bts/tree/src/common/scheduler.c#n156

They are actually using different convolutional encoding, thus different
logical channel handlers: 'tx_pdtch_fn' vs 'tx_data_fn'. So I still think
they should be activated and deactivated separately.

According to GSM 05.02, PTCCH stands for Packet Timing advance Control Channel,
so we should transmit Timing advance related messages there, not traffic.

What looks strange for me is that they both are using 0xc0 as channel number
so both logical channel handlers do dequeue and encode same messages from queue.
This looks odd to me, and I guess this is a mistake.

#6 Updated by laforge 16 days ago

fixeria wrote:

Hi,

I just checked GSM TS 05.02, and I am getting confused again...

Technically, the PDCH consists of various sub-channels such as PDTCH, PACCH, PTCCH.
[...]
I just don't think we are representing that difference to OsmoBTS in the gsm_lchan
structure, as it doesn't really make any difference (the messages are all encoded
the same). gsm_chan_t also doesn't have anything for PTCCH or PACCH specifically

Actually, we do distinguish between both PDTCH and PTCCH.

Please see the definition of PDCH multi-frame in 'scheduler_mframe.c':
https://git.osmocom.org/osmo-bts/tree/src/common/scheduler_mframe.c#n926

well, as I wrote, only osmo-bts-trx is doing that distinction :P
scheduler_common is only used by osmo-bts-{trx,virtual} and -virtual has no GPRS support

They are actually using different convolutional encoding, thus different
logical channel handlers: 'tx_pdtch_fn' vs 'tx_data_fn'. So I still think
they should be activated and deactivated separately.

the other PHYs like osmo-bts-sysmo,octphy, ... solve this by having multiple different "L1 SAPI" for those channels, and activating a given lchan will then activate all "L1 SAPI" for that lchan. The same applies for normal CS channels, e.g. a SDCCH lchan will activate L1 SAPIs for sdcch-downlink, sdcch-uplink, sacch-downlink, sacch-uplink, ...

According to GSM 05.02, PTCCH stands for Packet Timing advance Control Channel,
so we should transmit Timing advance related messages there, not traffic.

yes, that's correct.

#7 Updated by fixeria 16 days ago

Ok, it seems I was confused too much. Sorry.

Just checked how does the channels (de)activation work when one (dis)connects OsmoPCU:

<0009> pcu_sock.c:715 PCU socket has LOST connection
<0001> oml.c:76 Sending PCU version report to BSC: 
<0006> l1sap.c:1517 deactivating channel chan_nr=0xc5 trx=0
<0006> lchan.c:31 (bts=0,trx=0,ts=5,ss=0) state ACTIVE -> NONE
<0006> scheduler.c:624 Deactivating PDTCH on trx=0 ts=5
<0006> scheduler.c:624 Deactivating PTCCH on trx=0 ts=5
<0006> l1sap.c:647 deactivate confirm chan_nr=0xc5 trx=0
<0000> rsl.c:698 (bts=0,trx=0,ts=5,ss=0) PCU rel ack for unexpected lchan kind
<0000> rsl.c:714 (bts=0,trx=0,ts=5,ss=0) not sending REL ACK
<0006> l1sap.c:1517 deactivating channel chan_nr=0xc6 trx=0
<0006> lchan.c:31 (bts=0,trx=0,ts=6,ss=0) state ACTIVE -> NONE
<0006> scheduler.c:624 Deactivating PDTCH on trx=0 ts=6
<0006> scheduler.c:624 Deactivating PTCCH on trx=0 ts=6
<0006> l1sap.c:647 deactivate confirm chan_nr=0xc6 trx=0
<0000> rsl.c:698 (bts=0,trx=0,ts=6,ss=0) PCU rel ack for unexpected lchan kind
<0000> rsl.c:714 (bts=0,trx=0,ts=6,ss=0) not sending REL ACK
<0006> l1sap.c:1517 deactivating channel chan_nr=0xc7 trx=0
<0006> lchan.c:31 (bts=0,trx=0,ts=7,ss=0) state ACTIVE -> NONE
<0006> scheduler.c:624 Deactivating PDTCH on trx=0 ts=7
<0006> scheduler.c:624 Deactivating PTCCH on trx=0 ts=7
<0006> l1sap.c:647 deactivate confirm chan_nr=0xc7 trx=0
<0000> rsl.c:698 (bts=0,trx=0,ts=7,ss=0) PCU rel ack for unexpected lchan kind
<0000> rsl.c:714 (bts=0,trx=0,ts=7,ss=0) not sending REL ACK

Both PDTCH and PTCCH are being deactivated on PCU disconnect, so never mind.

#8 Updated by fixeria 16 days ago

  • Status changed from Feedback to Resolved
  • % Done changed from 0 to 100

The patch has been submitted: https://gerrit.osmocom.org/11258/. Closing now.

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)