Cellular Network Infrastructure » OsmoMGW

.

(wrong ticket, please ignore previous close)

Current status:

I am currently Unifying the RTP information in struct mgcp_endpoint. All RTP related stuff is generalized in struct mgcp_conn_rtp so that we just instaniate this struct twice for two independend RTP connections (currntly one for the BTS and one for the NET. Currently the instances are kept in an array. However, having them inside a list is much nicer. I already have some code to handle the lists. The only thing thats left is to make use of the list handling functions and replace the array with the list.

In the past week I have restructured and generalized the code. I tried to get rid of the differentiation between network and bts side wherever possible. While doing this I tried to keep the compatibility to the old omso-bsc_mgcp style for as long as possible, so I could use the AoIP setup for testing. I think this worked out quite well. I was able to generalize most of the core functionality. The two connections on the endpoint are now held inside a list, but are still not dynamically allocated. I was not able to fix this right now, because it would have broken compatibility.

Today I decided that it is time to give up the compatibility and started removing the differentation between network and bts side everywhere. Sending a CRCX, followed by an MDCX seems to work ok. The CRCX yields a port number and after the MDCX the dummy frame is tramsitted. Data I send is received by osmo-mgw, but certainly not processed yet.

The next goal is to be able to create two connections on one endpoint. Which means we sent two times a CRCX, but with a different CI. This will create the two connections. We will complete the connections with an MDCX, we reference the connections by the CI again. The two connections on the same endpoint are then implicitly connected. I am not 100% sure if this thinking is correct. It makes sense, and is also simple to implement, but I can read here https://tools.ietf.org/html/rfc3435#section-2.3.5 that there is a SecondEndPointId to be given with the CRCX command.

I managed to refactor a lot yesterday. We now have dynamically allocated and freed RTP connections. Also revisited all the commands. We can do CRCX, DLCX, MDCX, AUEP and RSIP. When I do a CRCX I can transmit RTP data using GAPK into the port that had just opend. The data is arriving inside osmo-mgw, but is not processed in a meaningful way yet. I am not done with all the cleanups yet, my main problem is that the code is not so well modularized, so when I touch something, I break 10 other things, but I think most things should be ok by now.

• Getting able to allocate two connections, which are actually connected
• Add doxygen comments to all public functions (partially done)
• Fix osmux (very endpoint centric, but logically it focusses on connections)
• If possible, modularize the message parser, I see repeating code and the functions are very big)
• VTY options for the plausibility check (checking plausibility is a good idea, but makes debugging difficult)
• Untangle mgcp_internal.h, this header file is a giant blob of structs used by everyone, I am sure we can modularize this.

There are some news:

• We are now at a point where two connections can be created (same callid, different connection ids). When I use my (throtteled, 1 RTP packet per second) GAPK I can see the packet entering at the one connection and leaving it at the other connection at the port and vice versa.

• Also did some refactoring on the OSMUX side. Unfortunately I think OSMUX can be considered as broken by now. We will have to test this to see if it still works, but I expect it to fail, there were just too much changes going on recently.

• in mgcp_network.c and mgcp_protocol.c I finally got rid of the BTS/NET centric view, so there its no only up to two RTP connections that are implicitly connected.

Cleaned up most of the remaining problems. We can now turn off the RTP packet filtering (packets where originating IP/Port does not match) on and off via VTY. The filtering is on by default and can be turned off when debugging is necessary. We also have the "force-realloc" feature back in place. I think the code has now reached an acceptable state so we can focus on making the tests work again.

laforge: Lets have another look at OSMUX when I am done with fixing the the tests.

Gone through the tests and changed them to match the new sitation. Also fixed some bugs while doing that. The current state is on pmaier/mgw3

dexter wrote:

Gone through the tests and changed them to match the new sitation. Also fixed some bugs while doing that. The current state is on pmaier/mgw3

Unfortunately it's not building:

make[4]: *** No rule to make target 'mgcp_sdp.h', needed by 'all-am'.  Stop.

and find . -name mgcp_sdp.h is unable to locate said file.

copy that, it seems that commit 'sdp: refactoring sdp parser/generator' forgets to add the new mgcp_sdp.h file.
dexter, in general, we should sync your osmo-mgw branch up with master now and continue to work with gerrit patch review.
Then you will catch these errors with our jenkins build checking.

Updated osmo-mgw branch neels/osmo-mgw to reflect the current state of pmaier/mgw3 (83d90ec7272a63fa9046cb9e3ad605a55662a2cb) rebased onto master and a few commits squashed.

We need to fix the tests, which I see you have, but the commit comes late in the chain. The fix must come as soon as with commit 'Initially implement the new osmo-mgw'.
I was able to rebase the fixes to follow right after that commit, so can be squashed easily, but I disagree with aspects of it:
Some changes seem to be cosmetic only for no apparent reason, e.g.

@@ -41,11 +42,7 @@ const char *strline_test_data =
     "\r" 
     "one CRLF\r\n" 
     "two CRLF\r\n" 
-    "\r\n" 
-    "one LF\n" 
-    "two LF\n" 
-    "\n" 
-    "mixed (4 lines)\r\r\n\n\r\n";
+    "\r\n" "one LF\n" "two LF\n" "\n" "mixed (4 lines)\r\r\n\n\r\n";

^ Here it is hard to see that nothing even changed.

-#define MDCX3 "MDCX 18983215 1@mgw MGCP 1.0\r\n" 
-#define MDCX3_RET "200 18983215 OK\r\n"                \
-                "I: 1\n"                       \
-                "\n"                           \
[...]
+
+#define MDCX3 \
+       "MDCX 18983215 1@mgw MGCP 1.0\r\n" \
+       "I: 1\n" 
+
+#define MDCX3_RET \
+       "200 18983215 OK\r\n" \
+       "I: 1\n" \
+       "\n" \

^ here it is hard to see what changed, e.g. only "I: 1\n" was added in MDCX3.

We tend to avoid mixing functional changes with cosmetic changes, to make it easier for others to read what the patch is doing. It would take me a long time to figure out what is fixing the tests ... can you make the patches shorter? unless you have a good reason for why they are this way, of course.

The missing header file has now been added.

We should discuss on how to proceed with the tests on Tuesday.

Update: Extended CRCX/MDCX with the possibility to include a connectionIdentifier. This is now mandatory since every endpoint now holds two connections which have to be addressed individually. We may also update the other commands too.

• Fixed the most of the review comments in gerrit #4003 https://gerrit.osmocom.org/#/c/4003/ We now got rid of the expensive lookups. We only lookup once and memorize the pointer. We have a callback function now that is executed every time an RTP packet arrives. The callback then decideds what to do with the packet. In our current case we have an implementation that finds the egress connection and sends it out there. Later we can also implement callbacks that send the packet through an E1 connection for example.
• Added another function to generate a CRCX message with SDP. I need that to do a single phase connection creation (a single CRCX instead of CRCX+MDCX). All in all I am not super happy with the API implementation. We have three CRCX variants now. It would be much smarter to have one function that takes a struct. Maybe we can still change the API, the fallout would be moderate.

• Right now we are still busy with https://gerrit.osmocom.org/#/c/4003/. This patch must be merged before we can proceed.

Status update:

• Since the mgcp_client needed a proper way to build up the string oriented messages we added a printf derivate to the mgcp utilities. This patch is still under review, but should be merged soon so that the related osmo-mgw patches can be built by jenkins: https://gerrit.osmocom.org/4200

• There are a few changes under review that are required by osmo-bsc. Most importantly https://gerrit.osmocom.org/#/c/4146/ and https://gerrit.osmocom.org/#/c/4227/

I think it makes sense to keep this ticket open until those changes passed through revew. All following changes should get own tasks. The man restructuring is done now.

We are now at a point where all further changes go through

The patch is merged, so we can set this to resolved.

Oh, I see https://gerrit.osmocom.org/#/c/4146/ and https://gerrit.osmocom.org/#/c/4227/ are still open. Please ignore the previous post.