Attention is currently required from: pespin, dexter.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31618 )
Change subject: pcu_sock: handle multiple BTSs with one BSC co-located PCU (in theory)
......................................................................
Patch Set 2:
(1 comment)
File src/osmo-bsc/pcu_sock.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/7a34cfa6_dac40965
PS1, Line 981: LOGP(DPCU, LOGL_ERROR, "This version of OsmoBSC can only handle one (Ericsson RBS) BTS with BSC co-located PCU\n");
> So if the limitation is in osmo-pcu, I see no reason to have this warning here. […]
ack
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31618
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I0b42c2c130106f6ffca2dd08d079e1a7bda41f0b
Gerrit-Change-Number: 31618
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 02 Mar 2023 14:14:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: neels, fixeria, dexter.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31497 )
Change subject: pcu_sock: activate/deactivate PDCH on pcu reconnect
......................................................................
Patch Set 7:
(1 comment)
File src/osmo-bsc/pcu_sock.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/985c08f8_9499f442
PS2, Line 941: bts = llist_entry(state->net->bts_list.next, struct gsm_bts, list);
> The PCU still has a limitation that it can only handle one BTS at a time. […]
I agree adding multiple BTS support here is out of scope.
But whatever we merge, it must be safe, and the user should be protected from running invalid configurations that are easy to detect.
I don't see how this addresses my concern "we should make sure that the user can operate this only in a safe configuration. In the worst case, we could check if thre are more than one (ericsson) BTS configured in the BSC, and refuse to operate the PCU socket with an associated error message."
So at least add that kind of check, IMHO.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31497
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I9ea0c53a5e68a51c781ef43bae71f947cdb95678
Gerrit-Change-Number: 31497
Gerrit-PatchSet: 7
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 02 Mar 2023 14:12:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: neels, laforge.
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-pcu/+/31176
to look at the new patch set (#17).
Change subject: support for Ericsson RBS E1 CCU
......................................................................
support for Ericsson RBS E1 CCU
Ericsson RBS series BTSs do not have a built in PCU. Rather than having
the PCU on board the PCU is co-located to the BSC in those cases. Just
like the MGW the PCU would connect to the CCU (Channel Coding Unit) via
E1 line. The PCU is connected via an unix domain socket (pcu_sock) to
the BSC to receive RACH requests and to control Paging and TBF assignment.
This patch adds all the required functionality to run an Ercisson RBS in
GPRS/EGPRS mode. It supports 16k I.460 E1 subslots and full 64k E1
timeslots (recommended)
Change-Id: I5c0a76667339ca984a12cbd2052f5d9e5b0f9c4d
Related: OS#5198
---
M configure.ac
M src/Makefile.am
A src/ericsson-rbs/er_ccu_descr.h
A src/ericsson-rbs/er_ccu_if.c
A src/ericsson-rbs/er_ccu_if.h
A src/ericsson-rbs/er_ccu_l1_if.c
A src/ericsson-rbs/er_ccu_l1_if.h
M src/gprs_debug.c
M src/gprs_debug.h
M src/pcu_l1_if.cpp
M src/pcu_l1_if.h
11 files changed, 1,149 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/76/31176/17
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/31176
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I5c0a76667339ca984a12cbd2052f5d9e5b0f9c4d
Gerrit-Change-Number: 31176
Gerrit-PatchSet: 17
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: neels, laforge.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/31176 )
Change subject: support for Ericsson RBS E1 CCU
......................................................................
Patch Set 17:
(9 comments)
File src/ericsson-rbs/er_ccu_descr.h:
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/811a9b31_f36a4d48
PS16, Line 27: uint32_t pseq_ccu;
: uint32_t pseq_pcu;
: uint32_t last_afn_ul;
: uint32_t last_afn_dl;
: bool ccu_synced;
: enum time_adj_val tav;
: bool ul_frame_err;
> you could potentally group those things into sub-structs? If all of this is sync state, then it migh […]
Done
File src/ericsson-rbs/er_ccu_if.c:
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/30f3d460_9bb898fb
PS16, Line 38: LOGP(DE1, level, "%s: E1-l
> this probably also should use LOGPITS, see comment below?
I am not convinced that this makes sense to use LOGPITS here. LOGPITS requires a pointer to struct e1inp_line, which we only have in a few locations. We can use LOGPITS in those locations of course but here we would have to add an extra pointer to the ccu_descr. I have now modified the macro a bit so that it looks similar to LOGPITS.
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/79be8f23_6e448087
PS16, Line 54: static void *tall_ctx = NULL;
> might make sens to give this a less generic name. […]
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/d616917b_bdeb4e19
PS16, Line 100: static void
> "e1_send" is very generic. I'd try to be more specific. […]
I think e1_send_ts_frame is good. Than it is clear that it is about one timeslot frame.
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/6e33e3ee_85dc7cd2
PS16, Line 121: LOGP(DE1, LOGL_DEBUG, "E1-TX: (line=%u,t
> we do have a LOGPITS() macro for logging the e1inp_line + timeslot in a standardized manner. […]
Here LOGPITS() fits nicely.
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/cc0e913a_65927c22
PS16, Line 156: e1inp_rx_ts
> might be worth mentioning that e1inp_rx_ts is the caller of this call-back. […]
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/e0af2c80_82ff8c99
PS16, Line 233: ccu_descr->trau_sync_fi = osmo_trau_sync_alloc(NULL, "trau-sync", sync_frame_out_cb, sync_pattern, ccu_descr);
> do we really have no better talloc-parent? Attaching things to the NULL Context is a last resort.
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/ea3a64b6_a78fe02a
PS16, Line 285: e1inp_ts_config_raw
> why are we not using e1inp_ts_config_i460() here? It seems we're using I. […]
I am not using the built in I.460 functionality of e1_input.c. There is no specific reason for that. I have re used the experience and some code from osmo-mgw here.
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/e621e303_8ff770d3
PS16, Line 367: return
> don't we need to close the e1_timeslot in the error path, if add_i468_subslot fails?
yes, we should do that.
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/31176
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I5c0a76667339ca984a12cbd2052f5d9e5b0f9c4d
Gerrit-Change-Number: 31176
Gerrit-PatchSet: 17
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Thu, 02 Mar 2023 14:11:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: comment
Attention is currently required from: neels, pespin, fixeria.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/31145 )
Change subject: bts: add IMMEDIATE ASSIGNMENT via PCH transmission
......................................................................
Patch Set 19:
(6 comments)
Patchset:
PS19:
I see there is a lot of confusion on the backward compatibility topic. I will now look at the BTS patches.
I have fixed stuff in other places, so I will push the patch set again. I will get to the remaining comments later.
File include/osmocom/pcu/pcuif_proto.h:
https://gerrit.osmocom.org/c/osmo-pcu/+/31145/comment/2451598b_6ea9afa7
PS19, Line 279: uint8_t pgroup;
> > And AFAIR, yes, it can be bigger than 255. […]
I have checked the spec for RSL it is indeed only one octet, see 3GPP TS 08.58 section 9.3.14
File include/osmocom/pcu/pcuif_proto.h:
https://gerrit.osmocom.org/c/osmo-pcu/+/31145/comment/d93824b0_9a7f957d
PS18, Line 43: PCU_IF_SAPI_PCH_DT
> The patch for `osmo-bts.git` is already in Gerrit (currently WIP to avoid merging before this one). […]
I have seen your BTS patch. Thanks. I will test this today. I didn't look at the tests yet.
File src/bts.cpp:
https://gerrit.osmocom.org/c/osmo-pcu/+/31145/comment/2010506c_ba95065d
PS17, Line 1134: pcu_l1if_tx_pch(bts, immediate_assignment, plen, ms_paging_group(tbf_ms(tbf)));
> If we go this way (backwards compatibility), we would have to keep both `PCU_IF_MSG_DATA_CNF` and `P […]
Ack
File src/pcu_l1_if.cpp:
https://gerrit.osmocom.org/c/osmo-pcu/+/31145/comment/1f63b010_b9763c10
PS19, Line 298: pgroup
> I think this is where the confusion comes from. […]
Thats wired.
https://gerrit.osmocom.org/c/osmo-pcu/+/31145/comment/4cbb623c_55f60c47
PS19, Line 771: } else {
> According to above block (like 752), only PCU_IF_VERSION and 0x0a are allows to reach here. […]
Thats true. I thought that I had remove it though...
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/31145
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I2a78651593323e8b9627c39918d949a33497b70f
Gerrit-Change-Number: 31145
Gerrit-PatchSet: 19
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 02 Mar 2023 14:11:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: comment