<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 style="white-space: pre-wrap; word-wrap: break-word;">IIRC "DATA" should be "UNITDATA" as there is no retransmission in RTP</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 style="white-space: pre-wrap; word-wrap: break-word;">wouldn't it make sense to define a struct that encapsulates the address type and the union for the payload?  IIRC we alreay have something like this in libosmocore, but of course we want a packed version here :/</p><p style="white-space: pre-wrap; word-wrap: break-word;">In fact, one could also think of adding the port into it,  At that point, you could actually pass a sockaddr_storage around, as it includes type, port and l3 address?</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 style="white-space: pre-wrap; word-wrap: break-word;">as all of your primitives have a context_id, it might make sense to shift the context_id up one layer, out of the union.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This introduced possibilities like a generic function/macro for logigng, to which you pass the entire hnb_audio_prim, and which then dereferences not just the prim-op/... but also the context_id.</p><p style="white-space: pre-wrap; word-wrap: break-word;">You could then have LOGHAP(struct hnb_audio_prim, format, ...)</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 style="white-space: pre-wrap; word-wrap: break-word;">I think you could introduce a logging macro for printing the 'ctx' in a structured way. Like LOGPCTX() or so?  This way it's guaranteed the formatting is identical everywhere.  Now  you repeate the %u in every line, and most have ctx=%u while at leat two have context_id=%u.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This is important later on if somebody wants to grep the logs.</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: Sun, 12 Dec 2021 07:50:27 +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>