Attention is currently required from: falconia.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32714 )
Change subject: TCH DL, FR & EFR: reshape SIDs per 06.31/06.81 section 5.1.2 ......................................................................
Patch Set 4: Code-Review+1
(5 comments)
Patchset:
PS4: This looks very clean to me. The elaborated comments are also very helpful to understand whats going on.
File src/common/l1sap.c:
https://gerrit.osmocom.org/c/osmo-bts/+/32714/comment/cffdfbcd_ae7c04dd PS3, Line 1308: static inline bool nonamr_mand_sid_position(struct gsm_lchan *lchan,
In the latest patchset just uploaded, I renamed this function to fr_hr_efr_sid_position().
Done
https://gerrit.osmocom.org/c/osmo-bts/+/32714/comment/2dc50c4e_60b78772 PS3, Line 1441: if (!resp_msg && lchan->tch.dtx_fr_efr.last_rtp_input_was_sid &&
In the new patchset I added a bunch of comments, extensively documenting the flow of that complex co […]
maybe just putting the whole code path in a static function would already improve the readability of the code.
File src/common/l1sap.c:
https://gerrit.osmocom.org/c/osmo-bts/+/32714/comment/bfe2abc9_78f645a3 PS4, Line 1384: /* non-AMR SID handling */ maybe /* FR, HR, EFR SID handling */, or just /* SID handling */ (I think is obvious that this only affects HR/FR and EFR.)
https://gerrit.osmocom.org/c/osmo-bts/+/32714/comment/282d3d2f_d818b0d1 PS4, Line 1393: * until OS#5688 is resolved. */ I guess the problem is that you do not know which of the two payload formats you will see here. I have submitted a patch (I25728299b757fbc87dd1b3f5adaec9b8b240c5d1) to make osmo_hr_check_sid() compatible with both payload types. So you can use it here without worrying about in which format the payload is.