<p style="white-space: pre-wrap; word-wrap: break-word;">(force gerrit to post all comments!)</p><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/21014">View Change</a></p><p>10 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/include/osmo-bts/gsm_data.h">File include/osmo-bts/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/osmo-bts/+/21014/2/include/osmo-bts/gsm_data.h@159">Patch Set #2, Line 159:</a> <code style="font-family:monospace,monospace">gsm_rep_facch</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I guess you could just store fn within msgb->cb?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Its probably more clear if I keep it the way it is.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/21014/1/include/osmo-bts/gsm_data.h">File include/osmo-bts/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/osmo-bts/+/21014/1/include/osmo-bts/gsm_data.h@261">Patch Set #1, Line 261:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">/* SLOT #1 */<br> struct msgb *msg_1;<br> uint32_t fn_1;<br><br> /* SLOT #2 */<br> struct msgb *msg_2;<br> uint32_t fn_2;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why not have an array[2] of a structure?</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/osmo-bts/+/21014/2/src/common/bts.c">File src/common/bts.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/osmo-bts/+/21014/2/src/common/bts.c@458">Patch Set #2, Line 458:</a> <code style="font-family:monospace,monospace"> if (lchan->repeated_acch_capability && (lchan->type == GSM_LCHAN_TCH_F || lchan->type == GSM_LCHAN_TCH_H))</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Not sure if we would ever have other scale values, so I would move this condition after the next blo […]</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/osmo-bts/+/21014/1/src/common/l1sap.c">File src/common/l1sap.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/osmo-bts/+/21014/1/src/common/l1sap.c@925">Patch Set #1, Line 925:</a> <code style="font-family:monospace,monospace"><= fn</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why <=8 ? The spec says "M+8 or M+9 (in case of SACCH or IDLE)". […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The stored frame number + 8 is compared against the current fn. The repetition is triggered as soon as at least 8 frames are passed. That should be correct. If not at the 8th frame, then it triggers at the 9th.</p><p style="white-space: pre-wrap; word-wrap: break-word;">We could have lookup tables that derive if it is exactly 8 or 9 frames, but I don't think that this is necessary since all this needs to make sure is that the repeated FACCH does not slip into the next but in the next+1 slot. For HR it is even the next slot anyway.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/21014/1/src/common/l1sap.c@947">Patch Set #1, Line 947:</a> <code style="font-family:monospace,monospace">for TCH/H only one</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what do you mean by 'instances' here? Which part in Osmo-BTS is ensuring this never happens? If th […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think "blocks" fits better. The channel mapping and the interleaving limits the amount of FACCH transmissions. I think 3gpp wanted to make sure that the repeated version of the FACCH does not overlap with the original version when it is transmitted. That is why they leave one FACCH block inbetween. For HR the blocks overlap again, but only by two frames. Skipping one block here would probably increase the latency too much.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/src/common/l1sap.c">File src/common/l1sap.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/osmo-bts/+/21014/2/src/common/l1sap.c@141">Patch Set #2, Line 141:</a> <code style="font-family:monospace,monospace"> LOGPLCHAN(lchan, DRTP, LOGL_ERROR, "RTP clock out of sync with lower layer:"</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why?</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/osmo-bts/+/21014/2/src/common/l1sap.c@909">Patch Set #2, Line 909:</a> <code style="font-family:monospace,monospace">}</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">missing new line</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/osmo-bts/+/21014/2/src/common/l1sap.c@925">Patch Set #2, Line 925:</a> <code style="font-family:monospace,monospace">fn + 8 <= fn</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Are you sure this is safe? Don't we need GSM_TDMA_FN_SUM() / GSM_TDMA_FN_DIFF() here? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The spec says: 3GPP TS 44.006, section 10.2: A repeated FACCH block shall be sent in such a way that, if the first burst of the downlink FACCH block containing the first instance of a LAPDm frame is sent in TDMA frame M, the first burst of the downlink FACCH block containing the repeated instance of the LAPDm frame is sent in TDMA frame M+8 or M+9 (the latter corresponding to the case where the two FACCH blocks are separated by either a SACCH frame or an idle frame, see 3GPP TS 45.002).</p><p style="white-space: pre-wrap; word-wrap: break-word;">What we doe here is simply wait until the fn is exactly 8, 9 or more bursts later before we trigger the repeated version of the FACCH. The exact timing is done by the scheduler. If there is a SACCH burst in between this code path is simply not executed but then we come along here one burst later (+9) - so I think it is safe. I also have checked it multiple times against my tables.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/21014/2/src/common/l1sap.c@929">Patch Set #2, Line 929:</a> <code style="font-family:monospace,monospace">fn + 8 <= fn</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same here.</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/osmo-bts/+/21014/2/src/common/vty.c">File src/common/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/osmo-bts/+/21014/2/src/common/vty.c@817">Patch Set #2, Line 817:</a> <code style="font-family:monospace,monospace"> "disable downlink FACCH repetition\n",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">missing NO_STR</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bts/+/21014">change 21014</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/c/osmo-bts/+/21014"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-bts </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I72f0cf7eaaef9f80fc35e752c90ae0e2d24d0c75 </div>
<div style="display:none"> Gerrit-Change-Number: 21014 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-CC: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 10 Nov 2020 20:27:47 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>