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 <pma...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <lafo...@osmocom.org> Gerrit-CC: neels <nhofm...@sysmocom.de> Gerrit-Attention: neels <nhofm...@sysmocom.de> Gerrit-Attention: laforge <lafo...@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 <lafo...@osmocom.org> Gerrit-MessageType: comment
