hi,
(somehow i already sent this patch to the list. but it seems to be gone.)
this patch should fix the underrun problem. it will only send radio link failure to bsc one and then stop processing timeout counter.
regards,
andreas
On Wed, Mar 13, 2013 at 05:47:26PM +0100, jolly wrote:
Dear jolly,
this patch should fix the underrun problem. it will only send radio link failure to bsc one and then stop processing timeout counter.
is the semantic of btsb->link_time_out == 0 == disabled something from the specification or something the implementor has decided)? Either way is fine, I would just have the commit message reflect that.
thank you for the fix
kind regards holger
PS: So before and after the patch it appears possible that the connection failure is sent more than once? (e.g. if the BSC doesn't release the channel within 3 SACCH frames?). Is that intended? In both cases it is fine, it is just that the commit message should reflect the decision you took.
On Thu, Mar 14, 2013 at 07:47:25AM +0100, jolly wrote:
Holger Hans Peter Freyther wrote:
Hi,
this must be frustrating for you, but it really is for me as well. I take absolutely no joy in pointing out these issues. I would prefer to work on my code but it is something that will bite us in a commercial setup so I a have to be the PITA here.
/* 9.4.14 Connection Failure Criterion */ if (TLVP_PRESENT(&tp, NM_ATT_CONN_FAIL_CRIT) &&
(TLVP_LEN(&tp, NM_ATT_CONN_FAIL_CRIT) >= 2) &&*TLVP_VAL(&tp, NM_ATT_CONN_FAIL_CRIT) == 0x01) {
(TLVP_LEN(&tp, NM_ATT_CONN_FAIL_CRIT) >= 2)) {
btsb->radio_link_timeout = val[1];
if (val[0] == 0x01 && val[1] >= 4)btsb->radio_link_timeout = val[1];
Here you point out that the range of 4 to UINT8_MAX is coming from the specification. Which is very good. The way it is done is a violation of the principle of least surprise though. If a BSC sends the value '2' it doesn't make sense that '32' is used. The configurator/implementor certainly wanted to have a low value.
IMHO the right thing to do is to NACK the set bts attributes with a descriptive error message. The same goes for the conn fail crit being present but not of the type we support.
holger
PS:
the lchan->s handling is fine. The switch/case has grown to a state I would move the link failure handling to a new helper function though, this way one could even write a unit test for the handling...
switch (data_ind->sapi) { case GsmL1_Sapi_Sacch:
/* process radio link timeout coniter S */
/* process radio link timeout counter S */
Holger Hans Peter Freyther wrote:
Here you point out that the range of 4 to UINT8_MAX is coming from the specification. Which is very good. The way it is done is a violation of the principle of least surprise though. If a BSC sends the value '2' it doesn't make sense that '32' is used. The configurator/implementor certainly wanted to have a low value.
IMHO the right thing to do is to NACK the set bts attributes with a descriptive error message. The same goes for the conn fail crit being present but not of the type we support.
hi holger,
the range is actually 4..64, so i send a NACK now, if the given attribute is not is that range. also the counting of S value is moved to a seperate function. check out the attached patch.
regards,
andreas
On Thu, Mar 14, 2013 at 11:45:42AM +0100, jolly wrote:
the range is actually 4..64, so i send a NACK now, if the given attribute is not is that range. also the counting of S value is moved to a seperate function. check out the attached patch.
thank you. this looks good. feel free to push it.
holger
-
Andreas Eversberg -
Holger Hans Peter Freyther -
jolly