osmo-bts[master]: osmo-bts-trx: add error concealment unit for GSM-FR

[dexter]

Patch Set 2:

(15 comments)

> And finally, sorry for a lot of messages, but what if this would be
 > implemented as a part of libosmocodec? Harald?

At least I think that would make a lot of sense. Also the enum with the bit offsets could be put in a header file so others can use it.

https://gerrit.osmocom.org/#/c/5214/1//COMMIT_MSG
Commit Message:

Line 10: ith a silence frame. This may cause unpleasant audio effects.
> ith slince?
Done


Line 12: Add a functionality (see ecu_fr.c, ecu_fr_conceal() and
> ideally name the function
Done


Line 13: ecu_fr_reset() to craft a replacement frame from the last
> Add ecu_fr_test.
Done


https://gerrit.osmocom.org/#/c/5214/1/include/osmo-bts/ecu_fr.h
File include/osmo-bts/ecu_fr.h:

Line 19
> (usually no license header in .h files, but a short summary would be nice)
Done


https://gerrit.osmocom.org/#/c/5214/2/include/osmo-bts/ecu_fr.h
File include/osmo-bts/ecu_fr.h:

Line 2: 
> I think, this header should satisfy at least the following dependences itse
Done


https://gerrit.osmocom.org/#/c/5214/1/include/osmo-bts/gsm_data_shared.h
File include/osmo-bts/gsm_data_shared.h:

Line 19: #include <osmocom/gsm/protocol/gsm_12_21.h>
> (unrelated ws)
Done


https://gerrit.osmocom.org/#/c/5214/1/src/common/ecu_fr.c
File src/common/ecu_fr.c:

Line 1: /* (C) 2017 by sysmocom - s.f.m.c. GmbH
> from Harald's recent comment we should put a dash before s.f.m.c.
Done


Line 303: /* Reduce the XMAXC field. When the XMAXC field reaches
> (almost any API is a "helper function" :) -- just say what it does, same be
Done


Line 364: 		goto leave;
> a bit more context, like at least say we failed to unpack an FR frame
Done


Line 377: 	}
> semantically it's not an abort, right?
Done


Line 383: 		goto leave;
> -1: unreachable code, the goto is not useful anyhow
Done


Line 384: 	}
> context
Done


https://gerrit.osmocom.org/#/c/5214/1/src/osmo-bts-trx/scheduler_trx.c
File src/osmo-bts-trx/scheduler_trx.c:

Line 1113: 	if (!bfi_flag && tch_mode == GSM48_CMODE_SPEECH_V1)
> if (!bfi_flag...
Done


https://gerrit.osmocom.org/#/c/5214/2/tests/ecu_fr/Makefile.am
File tests/ecu_fr/Makefile.am:

Line 3: LDADD = $(LIBOSMOCORE_LIBS) $(LIBOSMOGSM_LIBS) $(LIBOSMOCODEC_LIBS) $(LIBOSMOTRAU_LIBS) $(LIBOSMOABIS_LIBS)
> Both LIBOSMOTRAU and LIBOSMOABIS are not required here...
Done


https://gerrit.osmocom.org/#/c/5214/2/tests/ecu_fr/ecu_fr_test.c
File tests/ecu_fr/ecu_fr_test.c:

PS2, Line 1: #include <stdint.h>
           : #include <unistd.h>
           : #include <stdlib.h>
           : #include <errno.h>
           : #include <getopt.h>
           : #include <limits.h>
           : #include <sched.h>
           : #include <sys/signal.h>
           : #include <sys/types.h>
           : #include <sys/stat.h>
           : #include <sys/ioctl.h>
           : 
           : #include <netinet/in.h>
           : #include <arpa/inet.h>
           : #include <net/if.h>
           : 
           : #include <osmocom/core/talloc.h>
           : #include <osmocom/core/application.h>
           : #include <osmocom/vty/telnet_interface.h>
           : #include <osmocom/vty/logging.h>
           : 
           : #include <osmo-bts/gsm_data.h>
           : #include <osmo-bts/logging.h>
           : #include <osmo-bts/ecu_fr.h>
> So many useless includes... The following set would be enough:
I copied from another test and forgot to go through the includes. Thanks.


-- 
To view, visit https://gerrit.osmocom.org/5214
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iae9e69a9578ae305bca42f834694af96a29084e6
Gerrit-PatchSet: 2
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-HasComments: Yes