Attention is currently required from: jolly, laforge, pespin, fixeria. dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31417 ) Change subject: l1sap: Accept RFC5993 and TS 101.318 HR GSM payload ...................................................................... Patch Set 8: (7 comments) Patchset: PS7: I hope it is less confusing now. It is correct that the flags refer to RTP receiving and sending but in this patch we only care about the path that receives RTP packets. The path that sends RTP packets must be handled in a follow up patch. DL: that is correct. In addition to that we also have rtppayload_is_valid() as a gatekeeper to ensure invalid packets are not forwarded to lower layers. UL: that is not in the scope of this patch, it will be added in a follow up patch. File include/osmo-bts/bts.h: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/5e5c80ee_0d292e8b PS7, Line 69: /* Whether the BTS model uses HR GSM RTP payload in RFC 5993 format */ (see my other comments, I hope this is clear now.) File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/b5402a08_22e74794 PS6, Line 1940: if (lchan->type == GSM_LCHAN_TCH_H && rtp_pl_len == GSM_HR_BYTES The lines are much shorter now, I think we do not have to break the lines now. https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/a2faa838_ab48ec48 PS6, Line 1950: } else if (lchan->type == GSM_LCHAN_TCH_H && rtp_pl_len == GSM_HR_BYTES + 1 Done File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/c85625da_11518e5f PS7, Line 1271: bool rfc5993 = bts_internal_flag_get(lchan->ts->trx->bts, BTS_INTERNAL_FLAG_SPEECH_H_V1_RTP_RFC5993); As far as I am aware the flag description says only "uses". Which refers to what the hardware supports (this includes sending and receiving), however in the scope of this patch we only care about which RTP payload format the hardware understands. The comment above says "sending" which is indeed a bit misleading. Let's better use the term "forwarding". https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/976c91a2_e9ebbf45 PS7, Line 1274: if (OSMO_UNLIKELY(rfc5993 && (resp_msg->len != GSM_HR_BYTES_RTP_RFC5993))) { (The function rtppayload_is_valid sits in the RTP receiving path) I probably get now where the confusion is coming from. My idea about the flags was to express a preference. In case both formats are supported none of the two flags would be set. (no check would be carried out, which is probably also not so good) That might be indeed confusing but we can fix this, in case both or none of the flags are set both encodings are recognized as valid formats. I also added a check so that we check with both length if we support both formats. https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/6a7490ee_0531f2d6 PS7, Line 1280: LOGPLCHAN(lchan, DL1P, LOGL_NOTICE, Done -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31417 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I453562da412fde5b928bd2b588129c58ec8e2a7e Gerrit-Change-Number: 31417 Gerrit-PatchSet: 8 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Thu, 04 May 2023 15:43:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: comment