Attention is currently required from: neels, dexter.
laforge has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-pcu/+/31176 )
Change subject: support for Ericsson RBS E1 CCU
......................................................................
Patch Set 16:
(9 comments)
File src/ericsson-rbs/er_ccu_descr.h:
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/a5fc4f28_cb3a31c1
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 might be a good idea. Also, we have a trau_sync_fi above (but that's in the
i460 subslot group? so maybe clarify which kind of sync is meant where.
Also, I think it would be nice to have comments for each member of the struct (could be on
the same line). some are self-explanatory (trx_no, ts_, pdch_connected, ccu_synced,
ccu_connected). Some others don't have any meaning to me, like what is
"pseq" or "afn".
File src/ericsson-rbs/er_ccu_if.c:
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/ee9fb459_a5abb1da
PS16, Line 38: LOGP(DE1, level, "%s: E1-l
this probably also should use LOGPITS, see comment below?
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/083e780a_ea12a29a
PS16, Line 54: static void *tall_ctx = NULL;
might make sens to give this a less generic name. like ccu_tall_ctx?
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/c7e6b8a3_18a894e9
PS16, Line 100: static void
"e1_send" is very generic. I'd try to be more specific. It is sending one
frame/chunk for one timeslot. So both of that could be part of the function to make it
clear to the reader what it does. e1_ts_send_one_{frame,chunk} ?
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/3a7be8fa_afcfb32c
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. Please use in all locations where an e1inp_ts is available.
In general, every time you end up writing more than two log lines that print some context
like "(line=%u,ts=%u)" it is a very strong indication that you're not using
an existing macro for logging context information, or that you should write a new one if
it doesn't exist yet.
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/7186f198_8ed02cbd
PS16, Line 156: e1inp_rx_ts
might be worth mentioning that e1inp_rx_ts is the caller of this call-back. like
"our caller e1inp_rx_ts() ..."
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/24ef8a02_cc4b7c19
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.
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/9ef85535_b19266a0
PS16, Line 285: e1inp_ts_config_raw
why are we not using e1inp_ts_config_i460() here? It seems we're using I.460 but not
calling the related ts_config function.
https://gerrit.osmocom.org/c/osmo-pcu/+/31176/comment/661ae5dd_59b959d5
PS16, Line 367: return
don't we need to close the e1_timeslot in the error path, if add_i468_subslot fails?
--
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: 16
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: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 28 Feb 2023 17:59:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment