<p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385">View Change</a></p><p>11 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3//COMMIT_MSG">Commit Message:</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-pcu/+/22385/3//COMMIT_MSG@25">Patch Set #3, Line 25:</a> <code style="font-family:monospace,monospace">bis</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">bits? not sure what you meant here. Siomply "the SI are received"?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3//COMMIT_MSG@26">Patch Set #3, Line 26:</a> <code style="font-family:monospace,monospace">the MS originating the MS</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The MS orignating the MS? you mean originating the CCN?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_bssgp_rim.c">File src/gprs_bssgp_rim.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-pcu/+/22385/3/src/gprs_bssgp_rim.c@104">Patch Set #3, Line 104:</a> <code style="font-family:monospace,monospace">{</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">should the 'bp' possibly be 'const' here, like all of the pointers derived from it below as local variables?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_bssgp_rim.c@112">Patch Set #3, Line 112:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">possibly some OSMO_ASSERT that it is the right type of osmo_bssgp_prim?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_bssgp_rim.c@115">Patch Set #3, Line 115:</a> <code style="font-family:monospace,monospace">RAN</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">the message doesnt say 'rx' or 'tx' or the like, so it's unclear whether "answered" menas we answered it, or somebody answered something we sent.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_bssgp_rim.c@116">Patch Set #3, Line 116:</a> <code style="font-family:monospace,monospace">nacc_cause</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">we just added value_string arrays for those cause values, please use.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_bssgp_rim.c@123">Patch Set #3, Line 123:</a> <code style="font-family:monospace,monospace">B</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">P instead of B, I guess?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_ms.c">File src/gprs_ms.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-pcu/+/22385/3/src/gprs_ms.c@912">Patch Set #3, Line 912:</a> <code style="font-family:monospace,monospace">EINVAL</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">an allocation failure is not ENOMEM?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/nacc_fsm.h">File src/nacc_fsm.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-pcu/+/22385/3/src/nacc_fsm.h@30">Patch Set #3, Line 30:</a> <code style="font-family:monospace,monospace">enum nacc_fsm_event {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would appraciate some clarity in event nameing. Ideally the name should clearly state if the event is the indication that something was received (like ...RX_IND_...) or that the event requests the FSM to TX something or the like.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also, I think RX_RESOLVE_RAC_CI and SI_INFO_RECEIVED are inconsistent.  If they both relate to receiving something, they both should either be RX_... or ...RECEIVED.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, details, but they help immensely anyone who did not write (or not remember writing) the code :)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/nacc_fsm.c">File src/nacc_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/osmo-pcu/+/22385/3/src/nacc_fsm.c@376">Patch Set #3, Line 376:</a> <code style="font-family:monospace,monospace">st_tx_neighbour_data_on_enter</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">why do we have empty onenter functions (also further below)? They just add one function call + return without benefit?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/neigh_cache.h">File src/neigh_cache.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-pcu/+/22385/3/src/neigh_cache.h@78">Patch Set #3, Line 78:</a> <code style="font-family:monospace,monospace">      char</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whi is this a 512 character text string and not a uint8_t of MAC_BLOCK_LEN (23 bytes)?  Or are you storing all the SI in there? Then why one long buffer instread of several byte-arrays, one for each SI type?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385">change 22385</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-pcu/+/22385"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Id35f40d05f3e081f32fddbf1fa34cb338db452ca </div>
<div style="display:none"> Gerrit-Change-Number: 22385 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 22 Jan 2021 20:22:05 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>