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=a8299573830... is fine, as it just removes old code.
http://cgit.osmocom.org/cgit/osmo-bts/commit/?h=jolly/trx&id=9be20998555... 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=7322254e68b... 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=1c371296219... 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
hi harald,
http://cgit.osmocom.org/cgit/osmo-bts/commit/?h=jolly/trx&id=9be20998555... 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.
yes, i did not noticed that there is an OML IE for that. the specs leave open how to measure link at BTS side, so I decided to use same algorithm as the MS uses. (the MS gets initial value for radio link timeout via system information message.) i have just improved this patch (see jolly/trx branch). additionally I added an option to openbsc to configure radio link timeout via VTY. this value is sent down to BTS (via OML) also. (see jolly/testing branch of openbsc)
http://cgit.osmocom.org/cgit/osmo-bts/commit/?h=jolly/trx&id=7322254e68b... 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?
i have tested everything with osmo-bts-sysmo - at least with TCH/F call. the behavior was the same.
http://cgit.osmocom.org/cgit/osmo-bts/commit/?h=jolly/trx&id=1c371296219... 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?
as above, i tested TCH/F calls with osmo-bts-sysmo. because gsmtap is now provided at common part, and because each application (osmo-bts-sysmo in this case) handles gsmtap command line option itself (there is no option handling at common part), i added the command line options, so this patch does not remove any feature. i can split it of course.
i think that my branches are still quite experimental and not really made for merging yet. once i am done with the trx stuff, i would like to test osmo-bts-sysmo again with all voice codecs and then create a new branch to rebase everything onto the current master.
regards,
andreas
Hi Andreas,
On Sun, Mar 10, 2013 at 12:41:10PM +0100, jolly wrote:
hi harald,
http://cgit.osmocom.org/cgit/osmo-bts/commit/?h=jolly/trx&id=9be20998555... 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.
yes, i did not noticed that there is an OML IE for that. the specs leave open how to measure link at BTS side, so I decided to use same algorithm as the MS uses. (the MS gets initial value for radio link timeout via system information message.) i have just improved this patch (see jolly/trx branch).
Please note that the NM_ATT_CONN_FAIL_CRIT does not _have_ to be the radio link timeout, i.e. just blindly dereferencing its value is wrong in two parts: 1) it may be that the value part is not even two bytes long 2) it may be that the val[0] != 0x01, i.e. not RADIO_LINK_TIMEOUT
Please see my improved version of the patch in osmo-bts master as commit 294fd1b650e4482775fdd604288fc928e66ef81c.
i think that my branches are still quite experimental and not really made for merging yet.
I would like to avoid keeping the l1sap in a separate branch for too long time as it will start to clash with other work that is being done.
So independent of the 'trx' branch, it would be good if we could get the l1sap part into a state where you consider it worth merging at some not too distant future. Thanks for offering to test + rebase everything once you're done, though.
Regards, Harald
Harald Welte wrote:
So independent of the 'trx' branch, it would be good if we could get the l1sap part into a state where you consider it worth merging at some not too distant future.
i can rebase the jolly/l1sap branch on top of master. also i can apply all common/sysmo fixes from jolly/trx branch to the l1sap branch. the l1sap branch will then have a clean set of patches. i will test full rate and half rate (amr) calls with sysmo-bts. i can rebase the trx branch on top of the cleaned l1sap branch.
after that, the branches at origin need to be removed and pushed again. i like to know if this is a good way to do it. (i don't want to create even more branches.)
On Mon, Mar 11, 2013 at 07:41:12PM +0100, Andreas Eversberg wrote:
Hi,
Harald Welte wrote:
So independent of the 'trx' branch, it would be good if we could get the l1sap part into a state where you consider it worth merging at some not too distant future.
I would like to test the sapi queuing first. The integration process on my side is a bit longish as it goes through a multi hour stress test with the modem bank.
holger
-
Andreas Eversberg -
Harald Welte -
Holger Hans Peter Freyther -
jolly