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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <[email protected]> Gerrit-CC: neels <[email protected]> Gerrit-Attention: neels <[email protected]> Gerrit-Attention: dexter <[email protected]> Gerrit-Comment-Date: Tue, 28 Feb 2023 17:59:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
