Attention is currently required from: jolly, laforge, fixeria, dexter. pespin 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 7: (5 comments) Patchset: PS7: I'm sorry but after a more detailed look, I'm really struggling with this patch. I think the oerall behavior is blurry and not clear. IMHO, we should have the 2 features as you added, but they would mean "this BTS model supports receiving AND sending format XYZ to the upper layers". That means a given BTS model can also support both formats. Then, the upper layers, in the 2 paths: - DL: MGW->MS: if the received message is of a format not supported by the BTS model, then it has to be converted before passing it to lower layers - UL: MS->MGW: The BTS has a VTY command "hr-format XYZ" which specifies which output format to use to transmit towards the MGW. If the BTS model doesn't support that format, then convert it before sending to MGW. File include/osmo-bts/bts.h: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/8b14f5af_ec6e53ad 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 ..." If at some point one model supports both (which I think is expected at some point?) we will have a VTY command to select which one to be used. File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/597b8547_3992b2fa 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 whether the BTS model supports "receiving" it? This is all a bit confusing, I think the scope of the feature is not clear tbh. https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/9ed346fa_8f252c46 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 that if the received message is not rfc5993 (aka potentially is TS101.318) you are dropping it. But it could be that a BTS supports both formats, which means in here you would be dropping a received pkt containing TS101.318 for a BTS which supports that format. https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/de2d8768_dbad05e2 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. -- 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: 7 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: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Wed, 03 May 2023 14:36:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment