Currently RTP output_enabled is set to 1 on initialisation, which does not semantically match the initial value of conn_mode (MGCP_CONN_NONE).
This patch changes this initial value to 0.
Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_protocol.c | 2 +- openbsc/tests/mgcp/mgcp_test.ok | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index 9996ba7..9055bdb 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -1258,7 +1258,7 @@ static void mgcp_rtp_end_reset(struct mgcp_rtp_end *end) end->frames_per_packet = 0; /* unknown */ end->packet_duration_ms = DEFAULT_RTP_AUDIO_PACKET_DURATION_MS; end->rate = DEFAULT_RTP_AUDIO_DEFAULT_RATE; - end->output_enabled = 1; + end->output_enabled = 0; }
static void mgcp_rtp_end_init(struct mgcp_rtp_end *end) diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 7e8aa5c..3b645e6 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -51,7 +51,7 @@ Connection mode: 2, BTS output disabled, NET output enabled Testing DLCX Detected packet duration: 20 Requested packetization period not set -Connection mode: 0, BTS output enabled, NET output enabled +Connection mode: 0, BTS output disabled, NET output disabled Testing CRCX_ZYN Dummy packets: 1 Packet duration not set @@ -67,7 +67,7 @@ Testing RQNT2 Testing DLCX Detected packet duration: 20 Requested packetization period not set -Connection mode: 0, BTS output enabled, NET output enabled +Connection mode: 0, BTS output disabled, NET output disabled Testing CRCX Re-transmitting CRCX Testing RQNT1
This patch changes implementation and the mgcp_connection_mode enum in a way that net_end.output_enabled (bts_end.output_enabled) flag always matches the MGCP_CONN_SEND_ONLY (MGCP_CONN_RECV_ONLY) bit of conn_mode.
Based on this, the conn_mode bits are then used instead of the output_enabled fields within mgcp_protocol.c.
Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/mgcp_internal.h | 2 +- openbsc/src/libmgcp/mgcp_protocol.c | 31 +++++++---------------------- openbsc/tests/mgcp/mgcp_test.c | 33 ++++++++++++++++++++++++------- 3 files changed, 34 insertions(+), 32 deletions(-)
diff --git a/openbsc/include/openbsc/mgcp_internal.h b/openbsc/include/openbsc/mgcp_internal.h index b4899e4..28ea678 100644 --- a/openbsc/include/openbsc/mgcp_internal.h +++ b/openbsc/include/openbsc/mgcp_internal.h @@ -32,7 +32,7 @@ enum mgcp_connection_mode { MGCP_CONN_RECV_ONLY = 1, MGCP_CONN_SEND_ONLY = 2, MGCP_CONN_RECV_SEND = MGCP_CONN_RECV_ONLY | MGCP_CONN_SEND_ONLY, - MGCP_CONN_LOOPBACK = 4, + MGCP_CONN_LOOPBACK = 4 | MGCP_CONN_RECV_SEND, };
enum mgcp_trunk_type { diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index 9055bdb..5c88c9d 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -504,28 +504,10 @@ static int parse_conn_mode(const char *msg, struct mgcp_endpoint *endp) ret = -1; }
- switch (endp->conn_mode) { - case MGCP_CONN_NONE: - endp->net_end.output_enabled = 0; - endp->bts_end.output_enabled = 0; - break; - - case MGCP_CONN_RECV_ONLY: - endp->net_end.output_enabled = 0; - endp->bts_end.output_enabled = 1; - break; - - case MGCP_CONN_SEND_ONLY: - endp->net_end.output_enabled = 1; - endp->bts_end.output_enabled = 0; - break; - - default: - endp->net_end.output_enabled = 1; - endp->bts_end.output_enabled = 1; - break; - } - + endp->net_end.output_enabled = + endp->conn_mode & MGCP_CONN_SEND_ONLY ? 1 : 0; + endp->bts_end.output_enabled = + endp->conn_mode & MGCP_CONN_RECV_ONLY ? 1 : 0;
return ret; } @@ -877,7 +859,7 @@ mgcp_header_done: if (p->cfg->change_cb) p->cfg->change_cb(tcfg, ENDPOINT_NUMBER(endp), MGCP_ENDP_CRCX);
- if (endp->bts_end.output_enabled && tcfg->keepalive_interval != 0) + if (endp->conn_mode & MGCP_CONN_RECV_ONLY && tcfg->keepalive_interval != 0) mgcp_send_dummy(endp);
create_transcoder(endp); @@ -979,7 +961,8 @@ static struct msgb *handle_modify_con(struct mgcp_parse_data *p) if (p->cfg->change_cb) p->cfg->change_cb(endp->tcfg, ENDPOINT_NUMBER(endp), MGCP_ENDP_MDCX);
- if (endp->bts_end.output_enabled && endp->tcfg->keepalive_interval != 0) + if (endp->conn_mode & MGCP_CONN_RECV_ONLY && + endp->tcfg->keepalive_interval != 0) mgcp_send_dummy(endp);
if (silent) diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index 7eeef99..1f11b1d 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -321,6 +321,19 @@ ssize_t sendto(int sockfd, const void *buf, size_t len, int flags, return real_sendto(sockfd, buf, len, flags, dest_addr, addrlen); }
+#define CONN_UNMODIFIED (0x1000) + +static void test_values(void) +{ + /* Check that NONE disables all output */ + OSMO_ASSERT((MGCP_CONN_NONE & MGCP_CONN_RECV_SEND) == 0) + + /* Check that LOOPBACK enables all output */ + OSMO_ASSERT((MGCP_CONN_LOOPBACK & MGCP_CONN_RECV_SEND) == + MGCP_CONN_RECV_SEND) +} + + static void test_messages(void) { struct mgcp_config *cfg; @@ -341,9 +354,9 @@ static void test_messages(void) endp = &cfg->trunk.endpoints[i]; endp->net_end.payload_type = PTYPE_NONE; endp->net_end.packet_duration_ms = -1; - endp->bts_end.output_enabled = 0; - endp->net_end.output_enabled = 0; - endp->conn_mode = -1; + + OSMO_ASSERT(endp->conn_mode == MGCP_CONN_NONE); + endp->conn_mode |= CONN_UNMODIFIED; }
for (i = 0; i < ARRAY_SIZE(tests); i++) { @@ -386,7 +399,7 @@ static void test_messages(void) else printf("Requested packetization period not set\n");
- if (endp->conn_mode != -1) + if ((endp->conn_mode & CONN_UNMODIFIED) == 0) printf("Connection mode: %d, " "BTS output %sabled, NET output %sabled\n", endp->conn_mode, @@ -395,12 +408,17 @@ static void test_messages(void) else printf("Connection mode not set\n");
+ OSMO_ASSERT(endp->net_end.output_enabled == + (endp->conn_mode & MGCP_CONN_SEND_ONLY ? 1 : 0)); + OSMO_ASSERT(endp->bts_end.output_enabled == + (endp->conn_mode & MGCP_CONN_RECV_ONLY ? 1 : 0)); + endp->net_end.packet_duration_ms = -1; - endp->bts_end.output_enabled = 0; - endp->net_end.output_enabled = 0; endp->local_options.pkt_period_min = 0; endp->local_options.pkt_period_max = 0; - endp->conn_mode = -1; + endp->conn_mode = MGCP_CONN_NONE | CONN_UNMODIFIED; + endp->net_end.output_enabled = 0; + endp->bts_end.output_enabled = 0; }
@@ -784,6 +802,7 @@ int main(int argc, char **argv) osmo_init_logging(&log_info);
test_strline(); + test_values(); test_messages(); test_retransmission(); test_packet_loss_calc();
On Thu, Jan 16, 2014 at 04:50:40PM +0100, Jacob Erlbeck wrote:
Good Morning!
+#define CONN_UNMODIFIED (0x1000)
+static void test_values(void) +{
- /* Check that NONE disables all output */
- OSMO_ASSERT((MGCP_CONN_NONE & MGCP_CONN_RECV_SEND) == 0)
- /* Check that LOOPBACK enables all output */
- OSMO_ASSERT((MGCP_CONN_LOOPBACK & MGCP_CONN_RECV_SEND) ==
MGCP_CONN_RECV_SEND)+}
I think osmo_static_assert would work here too? I am applying your patches as is though.
if (endp->conn_mode != -1)
if ((endp->conn_mode & CONN_UNMODIFIED) == 0) printf("Connection mode: %d, " "BTS output %sabled, NET output %sabled\n", endp->conn_mode,
In the next step we could change the test output to illustrate the tri-state from on/off and not modified to previous test?
endp->conn_mode = MGCP_CONN_NONE | CONN_UNMODIFIED;endp->net_end.output_enabled = 0;endp->bts_end.output_enabled = 0;
do we still need these flags? Or only because of the above printf?
Hi
On 20.01.2014 08:18, Holger Hans Peter Freyther wrote:
On Thu, Jan 16, 2014 at 04:50:40PM +0100, Jacob Erlbeck wrote:
- /* Check that LOOPBACK enables all output */
- OSMO_ASSERT((MGCP_CONN_LOOPBACK & MGCP_CONN_RECV_SEND) ==
MGCP_CONN_RECV_SEND)I think osmo_static_assert would work here too?
Yes, it would.
if (endp->conn_mode != -1)
if ((endp->conn_mode & CONN_UNMODIFIED) == 0) printf("Connection mode: %d, " "BTS output %sabled, NET output %sabled\n", endp->conn_mode,In the next step we could change the test output to illustrate the tri-state from on/off and not modified to previous test?
Ok, I'd print the output_enabled values to stderr then.
endp->conn_mode = MGCP_CONN_NONE | CONN_UNMODIFIED;endp->net_end.output_enabled = 0;endp->bts_end.output_enabled = 0;do we still need these flags? Or only because of the above printf?
Yes, we do. Since conn_mode (the SEND_RECV bits) is modified, the output_enabled must be adjusted, so that the invariant holds (this is the central topic of this patch). I didn't want to have a modified behaviour of the test, since this ought to be a semantic-neutral modification (ideally with a unmodified ok file). See the next patch "mgcp/test: Don't reset conn_mode between messages", that addresses this issue.
Jacob
Currently, the conn_mode field is reset after it has been checked.
This patch disables this behaviour and only adds a mark (bit) to detect modifications.
Sponsored-by: On-Waves ehf --- openbsc/tests/mgcp/mgcp_test.c | 4 +--- openbsc/tests/mgcp/mgcp_test.ok | 1 + 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index 1f11b1d..fa68867 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -416,9 +416,7 @@ static void test_messages(void) endp->net_end.packet_duration_ms = -1; endp->local_options.pkt_period_min = 0; endp->local_options.pkt_period_max = 0; - endp->conn_mode = MGCP_CONN_NONE | CONN_UNMODIFIED; - endp->net_end.output_enabled = 0; - endp->bts_end.output_enabled = 0; + endp->conn_mode |= CONN_UNMODIFIED; }
diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 3b645e6..fbe2566 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -21,6 +21,7 @@ Detected packet duration: 40 Requested packetetization period: 20-20 Connection mode: 1, BTS output enabled, NET output disabled Testing MDCX3 +Dummy packets: 1 Packet duration not set Requested packetization period not set Connection mode not set
-
Holger Hans Peter Freyther -
Jacob Erlbeck