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'm sorry but after a more detailed look, I'm really struggling with this > patch. […] 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 */ > To me this would be "Whether the BTS model supports sending ..." […] (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 > please break each condition into one line with uniform formatting. 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 > The GSM_HR_BYTES_RTP_ constants should be in libosmocore. […] 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); > the description said the internal flag is about "sending", but iiuc you are > using it here to decide […] 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))) { > so if the BTS supports rfc5993 (sending, receving, whatever, see previous > comment), then you check t […] (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, > There's no need for the "else" here, it is guaranteed by the early return in > the previous if. 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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: jolly <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: fixeria <[email protected]> Gerrit-Attention: jolly <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Comment-Date: Thu, 04 May 2023 15:43:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: dexter <[email protected]> Gerrit-MessageType: comment
