Change in osmo-hnodeb[master]: First implementation of the LLSK audio SAPI

[pespin]

pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502 )

Change subject: First implementation of the LLSK audio SAPI
......................................................................


Patch Set 2:

(4 comments)

https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502/2/include/osmocom/hnodeb/hnb_prim.h 
File include/osmocom/hnodeb/hnb_prim.h:

https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502/2/include/osmocom/hnodeb/hnb_prim.h@173 
PS2, Line 173: HNB_AUDIO_PRIM_CONN_ESTABLISH,
             : 	HNB_AUDIO_PRIM_CONN_RELEASE,
             : 	HNB_AUDIO_PRIM_CONN_DATA,
             : 	_HNB_AUDIO_PRIM_MAX
> IIRC "DATA" should be "UNITDATA" as there is no retransmission in RTP
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).


https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502/2/include/osmocom/hnodeb/hnb_prim.h@184 
PS2, Line 184: uint8_t remote_rtp_address_type;  /* enum u_addr_type */
             : 	union u_addr remote_rtp_addr;
> wouldn't it make sense to define a struct that encapsulates the address type and the union for the p […]
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.


https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502/2/include/osmocom/hnodeb/hnb_prim.h@217 
PS2, Line 217: 	struct osmo_prim_hdr hdr;
> as all of your primitives have a context_id, it might make sense to shift the context_id up one laye […]
Not sure if it's really worth it, since anyway we can find the UE from the context_id, and then use LOGUE().


https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502/2/src/osmo-hnodeb/llsk_audio.c 
File src/osmo-hnodeb/llsk_audio.c:

https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502/2/src/osmo-hnodeb/llsk_audio.c@180 
PS2, Line 180: 		LOGP(DLLSK, LOGL_ERROR, "Rx AUDIO-CONN_ESTABLISH.req: CS chan not active! ctx=%u rem_addr=%s\n",
> I think you could introduce a logging macro for printing the 'ctx' in a structured way. […]
This one should actually be LOGUE(), which already prints context_id.



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-hnodeb/+/26502
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnodeb
Gerrit-Branch: master
Gerrit-Change-Id: I9909a7c054ddaabb1bb63d7d06331cc79f642b5d
Gerrit-Change-Number: 26502
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Mon, 13 Dec 2021 10:58:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211213/afd9e273/attachment.htm>