<p style="white-space: pre-wrap; word-wrap: break-word;">sorry, there are unfortunately some situations not taken in consideration :(</p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/10492">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10492/1/include/osmo-bts/gsm_data_shared.h">File include/osmo-bts/gsm_data_shared.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/10492/1/include/osmo-bts/gsm_data_shared.h@263">Patch Set #1, Line 263:</a> <code style="font-family:monospace,monospace">mesaurement</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">measurement</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10492/1/include/osmo-bts/gsm_data_shared.h@264">Patch Set #1, Line 264:</a> <code style="font-family:monospace,monospace">last_fn</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I can see you are writing to this field, but I cannot see that you ever reset/reinitialize it after a lchan is closed.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10492/1/src/common/measurement.c">File src/common/measurement.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/10492/1/src/common/measurement.c@280">Patch Set #1, Line 280:</a> <code style="font-family:monospace,monospace">is_meas_overdue</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what about the start condition? lchan->meas.last_fn will be zero, but the current frame number will be e.g. 200000. Then your current logic would consider a measurement overdue at the very first measurement, as there's a distance of 200000. I think lchan->meas.last_fn needs to be turned into a signed integer and initialized to -1 every time the lchan is reset. This way you can check for -1 here and if that's the case, assume that nothing is overdue.<br>Or you keep it unsigned and use the magic value 0xffffffff as initializer. This is much larger than the largest permitted framenumber.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10492/1/src/common/measurement.c@285">Patch Set #1, Line 285:</a> <code style="font-family:monospace,monospace">abs(fn - lchan->meas.last_fn)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think you need to do this in modulo-arithmetic of the GSM hyperframe duration. Keep in mind frame numbers wrap at that point, and arithemtically (in "unsigned int" math), without the modulo, your "distance" is way larger than in reality is is (think of last_fn = HYPERFRAME-1 and fn = HYPERFRAME+1. Then the distance is 2, but only in modulo-hyperframe math.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Unless I'm wrong, this must be fixed before merge.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10492/1/src/common/measurement.c@286">Patch Set #1, Line 286:</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;">for (i = 1; i < distance; i++) {<br> if (is_meas_complete(lchan, i + lchan->meas.last_fn)) {<br> *fn_missed_end = i + lchan->meas.last_fn;<br> return true;<br> }<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">this is a potentially quite expensive / time-consuming loop, particularly if the distance is quite large. I wish there was some obvious way to solve this without the expensive and lengthy iteration.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If the distance is larger than the sacch reporting interval, it is for sure overdue, So that could be a short-cut, limiting the maximum number of iterations here to 104. That would be a good start. Please add this and a comment as rationale.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think even for distance <= 102/104, it can even be done more efficiently by determining if the end of the measurement period for this lchan (see lookup tables) is within the interval [ lchan->meas.last_fn .. fn ]. So two if statement should be sufficient to avoid the iteration. Feel free to do this optimization as a follow-up patch, to not delay this any further.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10492/1/src/common/measurement.c@294">Patch Set #1, Line 294:</a> <code style="font-family:monospace,monospace">*fn_missed_end = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"0" is a valid frame number like anything else below the value of the maximum hyperframe number. I think setting it to "0" is no different than simply leaving it at whatever random value it was before calling the function. A caller must never use fn_missed_end when this function returns false.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/10492">change 10492</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/10492"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I3a86cd8185cc6b94258373fe929f0c2f1cf27cfa </div>
<div style="display:none"> Gerrit-Change-Number: 10492 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 17 Aug 2018 16:24:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>