[osmo-bts] Review of jolly/trx jolly/l1sap

historical

Hi Andreas,

I've found some time to review the trx and l1sap branches you have
created in the osmo-bts.git repository.

Let me start by mentioning that I support and encourage your work.
However, I think we have to set some general rules before we can merge
the code.

For us, the most important factor is that we do not loose features or
introduce regressions with more or less every new commit to osmo-bts.

Furthermore, we would like to stay with 'one commit, one feature/fix'.

I understand your primary goal ist go generalize some code and then
create a working solution for the OpenBTS or OsmocomBB based
transceiver.  However,  for the way to get the changes are integrated is
to first introduce all the infrastructure/core changes while keeping
osmo-bts-sysmo functional and that with all existing features.  Once
those core/infrastructure changes are merged, we can easily include
support the extra code for the osmo-bts-trx and in fact even do all
development commits to master, as long as they don't touch the core
code.

Looking at your commits:
http://cgit.osmocom.org/cgit/osmo-bts/commit/?h=jolly/trx&id=a82995738303d75647aa064bd52cccaf76d249ef
is fine, as it just removes old code.

http://cgit.osmocom.org/cgit/osmo-bts/commit/?h=jolly/trx&id=9be2099855532cff4313e132375c63c71fa8c0ff
looks fine to me, too, we should be able to merge that.  My only comment
would be:  Are you sure this should not be configured via OML from the
BSC?  I think TS 12.21 Chapter 9.4.14 should be the proper way to
configure it.  Do you agree?  If yes, please use OML configuration and
remove the VTY option.

http://cgit.osmocom.org/cgit/osmo-bts/commit/?h=jolly/trx&id=7322254e68b7d5ebf1098a902d99bc0e5965e2f4
seems fine to me.  Can you pleaes confirm from your side that it
_should_ not alter/remove any existing code/behavior, but just introduce
that L1SAP intermediate interface?  Have you tested it with
osmo-bts-sysmo?

http://cgit.osmocom.org/cgit/osmo-bts/commit/?h=jolly/trx&id=1c3712962195d1988c2937481c98592d2705464f
seems to not just remove the direct handling of primitives, but also add
some code, particularly the gsmtap_ip command line argument.  Can you
please split that in a separate patch?  Also, have you tested
osmo-bts-sysmo after this commit?

Regards,
	Harald
-- 
- Harald Welte <laforge at gnumonks.org>           http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)