Patch Set 12: (3 comments)
https://gerrit.osmocom.org/#/c/5214/12/include/osmo-bts/gsm_data_shared.h File include/osmo-bts/gsm_data_shared.h: Line 322: struct osmo_ecu_fr_state ecu_fr_state; so this is for full-rate. What about we later add something about EFR or AMR? In this case we add another member, and we occupy the memory for both FR and those other codecs. I think what we should do here is add union { struct osmo_ecu_fr_state fr; } ecu_state; this way it's clean and extensible with future implementations, without then having to update all the FR code with different naming to access the state. https://gerrit.osmocom.org/#/c/5214/12/src/osmo-bts-trx/scheduler_trx.c File src/osmo-bts-trx/scheduler_trx.c: Line 1084: rc = osmo_ecu_fr_conceal(&lchan->ecu_fr_state, tch_data); why not right away create an osmo_ecu_conceal() function, which is called from above this switch(tch_mode) statement, and then dispatch to the FR specific code inside that function? This way it's easier to later add other codec implementations. Line 1118: if (!bfi_flag && tch_mode == GSM48_CMODE_SPEECH_V1) same here. I'd recommend to just check for "if (!bfi_flag)" here, call an osmo_ecu_reset(&lchan->ecu_state, tch_data) and let that function dispatch it to the codec-specific routine. -- To view, visit https://gerrit.osmocom.org/5214 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iae9e69a9578ae305bca42f834694af96a29084e6 Gerrit-PatchSet: 12 Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Vadim Yanitskiy <[email protected]> Gerrit-Reviewer: dexter <[email protected]> Gerrit-HasComments: Yes
