hi,
while re-basing the osmo-bts-trx code with master, i saw that the ABIS code has been replaced by libosmo-abis. libosmo-abis is not capable of handling multiple RSL connections, so the multi-TRX support is broke now. the attached patch will add multiple RSL connection support to libosmo-abis.
regards,
andreas
On Tue, Jan 14, 2014 at 12:58:11PM +0100, Andreas Eversberg wrote:
Good Evening Andreas,
The code was tested with osmobts-trx.
What does that it mean? What was the result of the test? What was the content of the test. "I tested it" is a sentence without any content. ;)
The user of e1inp_ipa_bts_rsl_connect() (which is osmo-bts) must be upgraded after applying the patch.
okay. thanks for the patch.
dev->site_id, dev->bts_id, dev->trx_id);
dev->site_id, dev->bts_id, trx_nr);
We have an inconsistency problem here. From a design point of view it is bad to have dev->trx_id and a way to pass in the trx_nr. We can't remove dev->trx_id as OpenBSC is using it. Why can't we have more than one instance of the ipaccess_unit structure? For the sysmoBTS 2050 the mac addresses might/will be different too.
if (link->ofd->priv_nr >= E1INP_SIGN_RSL)trx_nr = link->ofd->priv_nr - E1INP_SIGN_RSL;
e1i_ts = &link->line->ts[link->ofd->priv_nr-1];
Where does this difference come from?
What are the alternatives to this patch? Can we copy the cfg.ipa fields into a per RSL line field and set the trx_id there before the first time we use it?
holger
Holger Hans Peter Freyther wrote:
hi holger,
What does that it mean? What was the result of the test? What was the content of the test. "I tested it" is a sentence without any content. ;)
i setup a UmTRX with two transceivers and used the jolly/trx_rebased branch to develop and test this patch. then i made calls on timeslots of different TRX, to see if the correct RSL connection is used to handle the channel. RSL connections on both TRX worked.
if (link->ofd->priv_nr >= E1INP_SIGN_RSL)trx_nr = link->ofd->priv_nr - E1INP_SIGN_RSL;e1i_ts = &link->line->ts[link->ofd->priv_nr-1];Where does this difference come from?
the priv_nr is set to E1INP_SIGN_OML (1) or E1INP_SIGN_RSL+trx_nr (2..). in order to retrieve the trx_nr from priv_nr, i substract E1INP_SIGN_RSL. the priv_nr is also the index for selecting the ts structure. because we do not have a physical E1 line, we can use ts[0] also, so '1' is substracted.
What are the alternatives to this patch? Can we copy the cfg.ipa fields into a per RSL line field and set the trx_id there before the first time we use it?
i looked at the data structure. the ipaccess_unit structure, which is linked to cfg.ipa.dev could be removed. instead we could provide individual ipaccess_unit structure per RSL/OML link. when calling e1inp_ipa_bts_rsl_connect(), we could provide a pointer to the individual structure. this pointer must be stored inside ipa_client_conn structure then.
alternatively we could provide an array of ipaccess_unit, instead of a single structure and attach it to cfg.ipa.dev.
in both cases, the BTS code must care of filling the ipaccess_unit structures with data for OML and each RSL link.
regards,
andreas
On Wed, Jan 15, 2014 at 10:09:16AM +0100, Andreas Eversberg wrote:
alternatively we could provide an array of ipaccess_unit, instead of a single structure and attach it to cfg.ipa.dev.
in both cases, the BTS code must care of filling the ipaccess_unit structures with data for OML and each RSL link.
please do that.
dear holger,
i change the patch. also i attached the change that is required for osmo-bts (sysmobts) to work afterwards.
best regards,
andreas
On Mon, Jan 20, 2014 at 03:12:00PM +0100, Andreas Eversberg wrote:
dear holger,
i change the patch. also i attached the change that is required for osmo-bts (sysmobts) to work afterwards.
hmm, I might not understand what your patch does but the trx_id is still in more than a single place. Make the ipaccess_unit the primary holder of the trx_id.
e1inp_ipa_bts_rsl_connect(line, "127.0.0.1", IPA_TCP_PORT_RSL);
e1inp_ipa_bts_rsl_connect(line, "127.0.0.1", IPA_TCP_PORT_RSL, 0);
- line = abis_open(bts, btsb->bsc_oml_host, "sysmoBTS");
- line = abis_open(bts, btsb->bsc_oml_host, "sysmoBTS", 1);
Why do you start with trx_id == 1 in the BTS part but in the manual case with 0?
Holger Hans Peter Freyther wrote:
hmm, I might not understand what your patch does but the trx_id is still in more than a single place. Make the ipaccess_unit the primary holder of the trx_id.
in my last patch (for osmo-bts), the trx_id is set in each entry of an array of ipaccess_unit structure. the first entry is used for OML link, trx_id is not used there, so it is set to 0. the second (and subsequent) array entry is used for RSL. there the trx_id is set to the TRX number, starting with 0. (sysmobts only uses one TRX.)
e1inp_ipa_bts_rsl_connect(line, "127.0.0.1", IPA_TCP_PORT_RSL);
e1inp_ipa_bts_rsl_connect(line, "127.0.0.1", IPA_TCP_PORT_RSL, 0);
- line = abis_open(bts, btsb->bsc_oml_host, "sysmoBTS");
- line = abis_open(bts, btsb->bsc_oml_host, "sysmoBTS", 1);
Why do you start with trx_id == 1 in the BTS part but in the manual case with 0?
the abis_open() function gets the number of TRX to be used, not the trx_id. as shown above, the abis_open shall create the OML link and 1 RSL link for 1 TRX. the e1inp_ip_bts_rsl_connect() function, as shown above, gets the parameter 0, which means it should connect the first RSL link, which uses trx_id 0.
On Mon, Jan 20, 2014 at 08:04:22PM +0100, Andreas Eversberg wrote:
Dear Andreas,
my initial comment was that information should be at a single place. The reason is that due bitrot/misc/etc. can change and one might think one talks about the same thing when in fact one doesn't.
In the spirit of avoiding broken windows we have to avoid such inconsistencies when they come up.
in my last patch (for osmo-bts), the trx_id is set in each entry of an array of ipaccess_unit structure. the first entry is used for OML link, trx_id is not used there, so it is set to 0. the second (and subsequent) array entry is used for RSL. there the trx_id is set to the TRX number, starting with 0. (sysmobts only uses one TRX.)
e1inp_ipa_bts_rsl_connect(line, "127.0.0.1", IPA_TCP_PORT_RSL);
e1inp_ipa_bts_rsl_connect(line, "127.0.0.1", IPA_TCP_PORT_RSL, 0);
- line = abis_open(bts, btsb->bsc_oml_host, "sysmoBTS");
- line = abis_open(bts, btsb->bsc_oml_host, "sysmoBTS", 1);
Why do you start with trx_id == 1 in the BTS part but in the manual case with 0?
the abis_open() function gets the number of TRX to be used, not the trx_id. as shown above, the abis_open shall create the OML link and 1 RSL link for 1 TRX. the e1inp_ip_bts_rsl_connect() function, as shown above, gets the parameter 0, which means it should connect the first RSL link, which uses trx_id 0.
With your patch there is:
* trx->nr * trx_id in the parameters * trx_id in the ipaccess_unit
So how can we reduce the amount of duplication? trx->nr will not go away. Can we change e1inp_ipa_bts_rsl_connect to already work on a sign_link (that normally would be created more late, probably not)?
any ideas?
holger
Holger Hans Peter Freyther wrote:
th your patch there is:
- trx->nr
- trx_id in the parameters
- trx_id in the ipaccess_unit
So how can we reduce the amount of duplication? trx->nr will not go away. Can we change e1inp_ipa_bts_rsl_connect to already work on a sign_link (that normally would be created more late, probably not)?
dear holger,
i looked at the code again. my thoughts are:
the trx_id in the parameters (that is e1inp_ipa_bts_rsl_connect) should be renamed to "index". that is what the parameter is used for: the index of the RSL link instance. we could provide sign_link instead of some index, but inside e1inp_ipa_bts_rsl_connect we call ipa_client_conn_create, which also requires an index. (this index is stored at priv_nr of the osmo_fd, to resolve ABIS link when receiving data.)
the trx_id in the ipaccess_unit is something that is replied to the BSC, to identify the TRX. there should be no relation between the value of trx_id and the actual RSL link instance. this makes sense, if multiple devices provide different TRX for the same BTS. (e.g. first device provides two TRX with trx_id 0 and 1, and second device provides two additional TRX with trx_id 2 and 3. both devices have only two RSL link instances.)
best regards,
andreass
On Tue, Jan 21, 2014 at 11:19:17AM +0100, Andreas Eversberg wrote:
dear andreas,
i looked at the code again. my thoughts are:
the trx_id in the parameters (that is e1inp_ipa_bts_rsl_connect) should be renamed to "index". that is what the parameter is used for: the index of the RSL link instance. we could provide sign_link instead of some index, but inside e1inp_ipa_bts_rsl_connect we call ipa_client_conn_create, which also requires an index. (this index is stored at priv_nr of the osmo_fd, to resolve ABIS link when receiving data.)
thanks a lot. I went through the code as well and I think the first version you sent is better than the second one. On the first version what I am missing is some documentation on how we segment the TS array for OML/RSL and a range check for the trx/index. Do you think you could add this to the first patch?
thanks holger
Holger Hans Peter Freyther wrote:
thanks a lot. I went through the code as well and I think the first version you sent is better than the second one. On the first version what I am missing is some documentation on how we segment the TS array for OML/RSL and a range check for the trx/index. Do you think you could add this to the first patch?
dear holger,
see attachment.
best regards,
andreas
-
Andreas Eversberg -
Holger Hans Peter Freyther