Attention is currently required from: jolly, laforge, fixeria, dexter.

View Change

5 comments:

• Patchset:

  • Patch Set #7:

    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:

  • 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 ..."

    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:

  • 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 whether the BTS model supports "receiving" it? This is all a bit confusing, I think the scope of the feature is not clear tbh.

  • 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 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.

  • 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.

To view, visit change 31417. To unsubscribe, or for help writing mail filters, visit settings.