Attention is currently required from: jolly, laforge, pespin, fixeria.
7 comments:
Patchset:
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:
Patch Set #7, 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:
Patch Set #6, 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.
Patch Set #6, 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:
Patch Set #7, 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".
Patch Set #7, 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.
Patch Set #7, 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 change 31417. To unsubscribe, or for help writing mail filters, visit settings.