Change in osmo-bts[master]: l1sap: add repeated downlink FACCH

historical

dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/21014 )

Change subject: l1sap: add repeated downlink FACCH
......................................................................

Patch Set 3:

(10 comments)

(force gerrit to post all comments!)

https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/include/osmo-bts/gsm_data.h 
File include/osmo-bts/gsm_data.h:

https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/include/osmo-bts/gsm_data.h@159 
PS2, Line 159: gsm_rep_facch
> I guess you could just store fn within msgb->cb?
Its probably more clear if I keep it the way it is.

https://gerrit.osmocom.org/c/osmo-bts/+/21014/1/include/osmo-bts/gsm_data.h 
File include/osmo-bts/gsm_data.h:

https://gerrit.osmocom.org/c/osmo-bts/+/21014/1/include/osmo-bts/gsm_data.h@261 
PS1, Line 261: /* SLOT #1 */
             : 			struct msgb *msg_1;
             : 			uint32_t fn_1;
             : 
             : 			/* SLOT #2 */
             : 			struct msgb *msg_2;
             : 			uint32_t fn_2;
> why not have an array[2] of a structure?
Done

https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/src/common/bts.c 
File src/common/bts.c:

https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/src/common/bts.c@458 
PS2, Line 458: 	if (lchan->repeated_acch_capability && (lchan->type == GSM_LCHAN_TCH_F || lchan->type == GSM_LCHAN_TCH_H))
> Not sure if we would ever have other scale values, so I would move this condition after the next blo […]
Done

https://gerrit.osmocom.org/c/osmo-bts/+/21014/1/src/common/l1sap.c 
File src/common/l1sap.c:

https://gerrit.osmocom.org/c/osmo-bts/+/21014/1/src/common/l1sap.c@925 
PS1, Line 925: <= fn
> why <=8 ?  The spec says "M+8 or M+9 (in case of SACCH or IDLE)". […]
The stored frame number + 8 is compared against the current fn. The repetition is triggered as soon as at least 8 frames are passed. That should be correct. If not at the 8th frame, then it triggers at the 9th.

We could have lookup tables that derive if it is exactly 8 or 9 frames, but I don't think that this is necessary since all this needs to make sure is that the repeated FACCH does not slip into the next but in the next+1 slot. For HR it is even the next slot anyway.

https://gerrit.osmocom.org/c/osmo-bts/+/21014/1/src/common/l1sap.c@947 
PS1, Line 947: for TCH/H only one
> what do you mean by 'instances' here?  Which part in Osmo-BTS is ensuring this never happens?  If th […]
I think "blocks" fits better. The channel mapping and the interleaving limits the amount of FACCH transmissions. I think 3gpp wanted to make sure that the repeated version of the FACCH does not overlap with the original version when it is transmitted. That is why they leave one FACCH block inbetween. For HR the blocks overlap again, but only by two frames. Skipping one block here would probably increase the latency too much.

https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/src/common/l1sap.c 
File src/common/l1sap.c:

https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/src/common/l1sap.c@141 
PS2, Line 141: 		LOGPLCHAN(lchan, DRTP, LOGL_ERROR, "RTP clock out of sync with lower layer:"
> Why?
Done

https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/src/common/l1sap.c@909 
PS2, Line 909: }
> missing new line
Done

https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/src/common/l1sap.c@925 
PS2, Line 925: fn + 8 <= fn
> Are you sure this is safe? Don't we need GSM_TDMA_FN_SUM() / GSM_TDMA_FN_DIFF() here? […]
The spec says: 3GPP TS 44.006, section 10.2: A repeated FACCH block shall be sent in such a way that, if the first burst of the downlink FACCH block containing the first instance of a LAPDm frame is sent in TDMA frame M, the first burst of the downlink FACCH block containing the repeated instance of the LAPDm frame is sent in TDMA frame M+8 or M+9 (the latter corresponding to the case where the two FACCH blocks are separated by either a SACCH frame or an idle frame, see 3GPP TS 45.002).

What we doe here is simply wait until the fn is exactly 8, 9 or more bursts later before we trigger the repeated version of the FACCH. The exact timing is done by the scheduler. If there is a SACCH burst in between this code path is simply not executed but then we come along here one burst later (+9) - so I think it is safe. I also have checked it multiple times against my tables.

https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/src/common/l1sap.c@929 
PS2, Line 929: fn + 8 <= fn
> Same here.
Done

https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/src/common/vty.c 
File src/common/vty.c:

https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/src/common/vty.c@817 
PS2, Line 817: 	   "disable downlink FACCH repetition\n",
> missing NO_STR
Done

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/21014
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I72f0cf7eaaef9f80fc35e752c90ae0e2d24d0c75
Gerrit-Change-Number: 21014
Gerrit-PatchSet: 3
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria <vyanitskiy at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Tue, 10 Nov 2020 20:27:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Comment-In-Reply-To: fixeria <vyanitskiy at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201110/81862443/attachment.htm>