<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><ul><li>See comment about the unsupported value, any reason for this ?</li><li>I have now added that value, it was indeed missing.</li></ul></blockquote><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><ul><li>The preference of codec expressed by the MSC in the assignement</li><li>Yes thats true, when I was implementing it it was not clear to me how to</li><li>handle the preference and if there is any preference at all so I did not</li><li>take that into consideration. I also can not remember seeing anything in</li><li>the spec that dictates that the order of the codecs needs to be retained</li><li>when making the codec decision. I think thats also not in scope of the</li><li>spec since at the end of the day it will be up to the vendor how to</li><li>optimize the resource management at the BSC anyway.</li></ul></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">However, I think that if we decide to analyze and possibly fix/optimize<br>the codec selection behavior we should do that in a separate patch.</p><p><a href="https://gerrit.osmocom.org/12625">View Change</a></p><p>38 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/codec_pref.h">File include/osmocom/bsc/codec_pref.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/codec_pref.h@24">Patch Set #2, Line 24:</a> <code style="font-family:monospace,monospace"> const struct gsm_bts *bts, enum rate_pref rate_pref);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">in ch_mode_rate, we add a 'bool full_rate', and we also add a 'rate_pref'? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The bool in ch_mode_rate and the enum rate_pref are two different things. The bool symbols an absolute choice between FR and HR, the enum rate_pref members symbolize a set of three different preference choices. I don't think that it is a good idea to mix this together.</p><p style="white-space: pre-wrap; word-wrap: break-word;">match_codec_pref() is used here to make an absolute choice based on a preference. If the MSC asks for FR and HR (with a preference) we execute match_codec_pref() two times to get the choices for the two situations. These two are then handed over to the channel allocator which makes the final decision based on the channel load.</p><p style="white-space: pre-wrap; word-wrap: break-word;">There is also a problem with the FR/HR flag that not every codec is available for FR and HR. So we can not first choose a codec with match_codec_pref() and then say if the TCH/F version of the codec is not available, just pick the TCH/H version. This does not work for EFR and even for AMR there are problems with the S-Bits. Thats why we need the full information for both possible situations.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/codec_pref.h@24">Patch Set #2, Line 24:</a> <code style="font-family:monospace,monospace"> const struct gsm_bts *bts, enum rate_pref rate_pref);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Also talked about this in person, and obviously ch_mode_rate is an out-argument, while the rate_pref […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h">File include/osmocom/bsc/gsm_data.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h@104">Patch Set #8, Line 104:</a> <code style="font-family:monospace,monospace"> bool full_rate;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Just wondering if it wouldn't be worth directly using GSM_LCHAN_xxx constant in there rather than fu […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h@104">Patch Set #8, Line 104:</a> <code style="font-family:monospace,monospace"> bool full_rate;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">We ignore the MS channel preference on purpose and do "late assignment" instead. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h@104">Patch Set #8, Line 104:</a> <code style="font-family:monospace,monospace"> bool full_rate;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I hope I understood this right, though. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h">File include/osmocom/bsc/gsm_data.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h@121">Patch Set #4, Line 121:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">We talked in person, and there really are only two possible outcomes: one for an FR and one for an H […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h@300">Patch Set #4, Line 300:</a> <code style="font-family:monospace,monospace"> /* LCLS FSM */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this ^ should probably go inside the assignment{} scope instead, see other comments</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/gsm_data.h">File include/osmocom/bsc/gsm_data.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/gsm_data.h@121">Patch Set #2, Line 121:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Are there really only two possible mode,rate combinations?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, the MSC can only ask for TCH/F and TCH/H at the same time. It may also ask only for one of the two. The codec is a different thing. There are more combinations possible, but thats a pure capability thing since a TCH may be used with several different codecs.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/1/src/osmo-bsc/abis_rsl.c">File src/osmo-bsc/abis_rsl.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/1/src/osmo-bsc/abis_rsl.c@1378">Patch Set #1, Line 1378:</a> <code style="font-family:monospace,monospace">a </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Spelling: 'to' is not needed here I think.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/abis_rsl.c">File src/osmo-bsc/abis_rsl.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/abis_rsl.c@1391">Patch Set #2, Line 1391:</a> <code style="font-family:monospace,monospace"> if (!lchan) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">These changes above make up a separate fix by themselves. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have now separated this.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/1/src/osmo-bsc/assignment_fsm.c">File src/osmo-bsc/assignment_fsm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/1/src/osmo-bsc/assignment_fsm.c@258">Patch Set #1, Line 258:</a> <code style="font-family:monospace,monospace"> sw</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Let's use bool.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/assignment_fsm.c">File src/osmo-bsc/assignment_fsm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/assignment_fsm.c@255">Patch Set #2, Line 255:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">const struct ...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/assignment_fsm.c@305">Patch Set #2, Line 305:</a> <code style="font-family:monospace,monospace"> case GSM48_CMODE_SPEECH_AMR:</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(we already have a flag around called "requires_voice_stream", so prefer if this is called "check_re […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/assignment_fsm.c@324">Patch Set #2, Line 324:</a> <code style="font-family:monospace,monospace"> bool result_requires_voice_pref;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">don't understand the name 'result_pref' ... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have added 'requires_voice' to the variable names to make it more clear.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c">File src/osmo-bsc/assignment_fsm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@a351">Patch Set #4, Line 351:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">noticing a bug in this code path: it is leaving the assignment fi allocated. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@304">Patch Set #4, Line 304:</a> <code style="font-family:monospace,monospace"> case GSM48_CMODE_SPEECH_EFR:</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">-1: code dup: please write one function that takes a chan_mode as input and returns a requires_voice […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The requires voice stream must be identical for both possible choices. If it is not identical there is a protocol error since it is not possible mix signaling and voice here. There is no "Full rate TCH channel or Signalling channel, signalling prefered..."</p><p style="white-space: pre-wrap; word-wrap: break-word;">This means if we request for voice/TCH, both choices must always be voice channels.</p><p style="white-space: pre-wrap; word-wrap: break-word;">See also: 3GPP TS 48.008, chapter 3.2.2.11</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@377">Patch Set #4, Line 377:</a> <code style="font-family:monospace,monospace"> /* Check if the currently existing lchan is compatible with the</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">mistake in my comment: should be […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't get that. Why check the req->ch_mode_rate_pref and req->ch_mode_rate_alt in the same statement if it is not checked if req->ch_mode_rate_alt is even populated. One must first check the _pref, and only if an _alt exists one may move on.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@443">Patch Set #4, Line 443:</a> <code style="font-family:monospace,monospace"> return;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">-1: code dup (same)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Its similar, but also different enough. I am not sure if it makes sense to split this up into a helper function, we wouldn't save much.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@479">Patch Set #4, Line 479:</a> <code style="font-family:monospace,monospace"> conn->lchan->ch_mode_rate.full_rate,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(FYI, I considered whether it is better to allocate the FSM instance only *after* we checked whether […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@495">Patch Set #4, Line 495:</a> <code style="font-family:monospace,monospace">static void assignment_fsm_wait_lchan(struct osmo_fsm_inst *fi, uint32_t event, void *data)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">^ still valid, but don't worry about this one, see https://gerrit.osmocom. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@495">Patch Set #4, Line 495:</a> <code style="font-family:monospace,monospace">static void assignment_fsm_wait_lchan(struct osmo_fsm_inst *fi, uint32_t event, void *data)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">...maybe we can deallocate the assignment. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I see, however since it is allocated as a child we sill won't run into serious memory leaks. I have deallocated the fi now.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@501">Patch Set #4, Line 501:</a> <code style="font-family:monospace,monospace"> if (data != conn->assignment.new_lchan)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">-1: the ch_mode_rate use must not be ambiguous: does it apply to the current conn->lchan or to the n […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have put it now into the assignment scope as you suggested. I am not sure if it makes sense to put it into the lchan. Probably worth to discuss this on Thursday?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@503">Patch Set #4, Line 503:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">("alternate". use line width. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/assignment_fsm.c">File src/osmo-bsc/assignment_fsm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/assignment_fsm.c@340">Patch Set #5, Line 340:</a> <code style="font-family:monospace,monospace"> return -EINVAL;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">before, I said "it doesn't make sense to fail the assignment if only one of the choices is not suppo […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/assignment_fsm.c@497">Patch Set #5, Line 497:</a> <code style="font-family:monospace,monospace"> struct gsm_subscriber_connection *conn = assignment_fi_conn(fi);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">instead of spraying CR comments I pushed a suggestion instead. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have checked that out, looks good to me, also the tests pass fine.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/10/src/osmo-bsc/bsc_vty.c">File src/osmo-bsc/bsc_vty.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/10/src/osmo-bsc/bsc_vty.c@4805">Patch Set #10, Line 4805:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I am wondering how the changes in this file related to "channel allocator preferences"?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/10/src/osmo-bsc/bsc_vty.c@4805">Patch Set #10, Line 4805:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">indeed, seems like whitespace tweaks related to the previous patch that allows disabling specific lc […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/handover_fsm.c">File src/osmo-bsc/handover_fsm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/handover_fsm.c@716">Patch Set #5, Line 716:</a> <code style="font-family:monospace,monospace"> sc.cfg = conn->lchan->ch_mode_rate.s15_s0;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Noticing this problem that exists before this patch: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I see, then ch_mode_rate should be really seen as an lchan property. I have moved the struct now to gsm_lchan.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/handover_fsm.c@716">Patch Set #5, Line 716:</a> <code style="font-family:monospace,monospace"> sc.cfg = conn->lchan->ch_mode_rate.s15_s0;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Noticing this problem that exists before this patch: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Then its</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c">File src/osmo-bsc/osmo_bsc_bssap.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c@609">Patch Set #4, Line 609:</a> <code style="font-family:monospace,monospace"> lcls_apply_config(conn);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">"select_codecs" ?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c@632">Patch Set #4, Line 632:</a> <code style="font-family:monospace,monospace"> msc = conn->sccp.msc;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(if full rate didn't work, then this must end up being half rate? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, thats correct. Its possibly more logical to express it explicitly.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c@647">Patch Set #4, Line 647:</a> <code style="font-family:monospace,monospace"> RATE_PREF_FR);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(RATE_PREF_FR?)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c">File src/osmo-bsc/osmo_bsc_bssap.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c@635">Patch Set #8, Line 635:</a> <code style="font-family:monospace,monospace"> switch (ct->ch_rate_type) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">do you really need the mask at all?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, we need to mask off the bits that define that a later change is between the two rates is allowed or not. This information plays no role here, but is contained in the byte.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c@635">Patch Set #8, Line 635:</a> <code style="font-family:monospace,monospace"> switch (ct->ch_rate_type) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Wait what ? GSM0808_SPEECH_PERM is not a mask, it's an actual possible value of the field defined in […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have looked up 3GPP TS 48.008 version 14.2.0 Release 14 3.2.2.11 and 0x0F is not a valid value, this is indeed a mask. The original author of the file seems to have forgotten to add a suffix to make that clear. If you look at enum gsm0808_chan_rate_type_data it should be more clear how the intension was. I know this is odd, but unfortunately its now the way it is. I can of course change it but I am not sure if this is wise, we might break the backward compatibility of the API.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c@637">Patch Set #8, Line 637:</a> <code style="font-family:monospace,monospace"> rc = match_codec_pref(&req->ch_mode_rate_pref, ct, &conn->codec_list, msc, conn_get_bts(conn),</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Given this is used for speech, I'd use the GSM0808_SPEECH_xxx constants. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c@637">Patch Set #8, Line 637:</a> <code style="font-family:monospace,monospace"> rc = match_codec_pref(&req->ch_mode_rate_pref, ct, &conn->codec_list, msc, conn_get_bts(conn),</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">agree</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/9/src/osmo-bsc/osmo_bsc_bssap.c">File src/osmo-bsc/osmo_bsc_bssap.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/9/src/osmo-bsc/osmo_bsc_bssap.c@635">Patch Set #9, Line 635:</a> <code style="font-family:monospace,monospace"> switch (ct->ch_rate_type) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Still that weird masking ...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The purpose of the masking is to get rid of the bit that configures if later changes are allowed or not. As I said, I interpret GSM0808_SPEECH_PERM as a mask that is intended to do this.</p><p style="white-space: pre-wrap; word-wrap: break-word;">However, I looked at 3GPP TS 48.008 again, the mask would also mask off bits that define TCH multislot configurations, so now I think that just masking the bits off is probably not a very good Idea. I have added the fall through cases instead.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/11/src/osmo-bsc/osmo_bsc_bssap.c">File src/osmo-bsc/osmo_bsc_bssap.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/11/src/osmo-bsc/osmo_bsc_bssap.c@635">Patch Set #11, Line 635:</a> <code style="font-family:monospace,monospace"> switch (ct->ch_rate_type) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">GSM0808_SPEECH_PERM and GSM0808_SPEECH_PERM_NO_CHANGE are not handled ? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Now I finally understand this, i always thought that those were masks, but the spec indeed lists the constants for speech + CTM Text Telephony, but not for data. I thought that data and voice would have no difference in their options, but this is not the case.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I have never seen those constants anywhere in the wild. Presumably MSCs always seem to have a preference for the one or the other rate.</p><p style="white-space: pre-wrap; word-wrap: break-word;">However, I have now programmed it that way that it is handled like GSM0808_SPEECH_FULL_PREF, so if the MSC has no preference at all we would internally assume a preference for FR. I think this makes sense since FR has better quality and if we have resources available, we should just use FR.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12625">change 12625</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/12625"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 </div>
<div style="display:none"> Gerrit-Change-Number: 12625 </div>
<div style="display:none"> Gerrit-PatchSet: 11 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Assignee: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-CC: tnt <tnt@246tNt.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 21 Feb 2019 09:08:27 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>