Attention is currently required from: neels, laforge.

View Change

9 comments:

• File src/ericsson-rbs/er_ccu_descr.h:

  • Patch Set #16, 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:

  • Patch Set #16, 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.

  • Patch Set #16, Line 54: static void *tall_ctx = NULL;

    might make sens to give this a less generic name. […]

    Done

  • Patch Set #16, 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.

  • Patch Set #16, 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.

  • Patch Set #16, Line 156: e1inp_rx_ts

    might be worth mentioning that e1inp_rx_ts is the caller of this call-back. […]

    Done

  • Patch Set #16, 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

  • Patch Set #16, 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.

  • Patch Set #16, 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 change 31176. To unsubscribe, or for help writing mail filters, visit settings.