[XL] Change in osmo-pcu[master]: support for Ericsson RBS E1 CCU - gerrit-log

2 Mar 2023

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 &lt;pmaier(a)sysmocom.de&gt;
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-CC: neels &lt;nhofmeyr(a)sysmocom.de&gt;
Gerrit-Attention: neels &lt;nhofmeyr(a)sysmocom.de&gt;
Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-Comment-Date: Thu, 02 Mar 2023 14:11:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-MessageType: comment