<p><a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502/2/include/osmocom/hnodeb/hnb_prim.h">File include/osmocom/hnodeb/hnb_prim.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-hnodeb/+/26502/2/include/osmocom/hnodeb/hnb_prim.h@173">Patch Set #2, Line 173:</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;">HNB_AUDIO_PRIM_CONN_ESTABLISH,<br>  HNB_AUDIO_PRIM_CONN_RELEASE,<br>  HNB_AUDIO_PRIM_CONN_DATA,<br>     _HNB_AUDIO_PRIM_MAX<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">IIRC "DATA" should be "UNITDATA" as there is no retransmission in RTP</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm using CONN_DATA for payload forwarding which relates to an active connection or context, which was previously established (CONN_ESTABLISH) and later release (CONN_DATA). I do the same for Iuh signalling, where they are differentiated from UNITDATA which contain RANAP messages not related to any context (like paging).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502/2/include/osmocom/hnodeb/hnb_prim.h@184">Patch Set #2, Line 184:</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;">uint8_t remote_rtp_address_type;  /* enum u_addr_type */<br>        union u_addr remote_rtp_addr;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">wouldn't it make sense to define a struct that encapsulates the address type and the union for the p […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I preferred having an own format specified in the protocol, similar to what we have in PCUIF, rather than relying on some even more complex "opaque" data types provided by the system.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502/2/include/osmocom/hnodeb/hnb_prim.h@217">Patch Set #2, Line 217:</a> <code style="font-family:monospace,monospace">     struct osmo_prim_hdr hdr;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">as all of your primitives have a context_id, it might make sense to shift the context_id up one laye […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure if it's really worth it, since anyway we can find the UE from the context_id, and then use LOGUE().</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502/2/src/osmo-hnodeb/llsk_audio.c">File src/osmo-hnodeb/llsk_audio.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-hnodeb/+/26502/2/src/osmo-hnodeb/llsk_audio.c@180">Patch Set #2, Line 180:</a> <code style="font-family:monospace,monospace">           LOGP(DLLSK, LOGL_ERROR, "Rx AUDIO-CONN_ESTABLISH.req: CS chan not active! ctx=%u rem_addr=%s\n",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think you could introduce a logging macro for printing the 'ctx' in a structured way. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This one should actually be LOGUE(), which already prints context_id.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502">change 26502</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-hnodeb/+/26502"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-hnodeb </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I9909a7c054ddaabb1bb63d7d06331cc79f642b5d </div>
<div style="display:none"> Gerrit-Change-Number: 26502 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </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-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 13 Dec 2021 10:58:26 +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"> Gerrit-MessageType: comment </div>