[PATCH] Support of 11 bit RACH

[Bhargava Abhyankar]

Hello holger,
	          Thanks for your comments.
We see that in latest osmo-bts,  PCU_IF_VERSION is defined as 0x06 whereas in latest osmo-pcu, it is defined as 0x05.  Let us know the value  to be taken for 11 bit RACH support.

Regards
Bhargava Abhyankar

-----Original Message-----
From: Holger Freyther [mailto:holger at freyther.de] 
Sent: Tuesday, March 08, 2016 11:12 PM
To: Bhargava Abhyankar <Bhargava.Abhyankar at radisys.com>
Cc: osmocom-net-gprs at lists.osmocom.org; Prasad Kaup <Prasad.Kaup at radisys.com>
Subject: Re: [PATCH] Support of 11 bit RACH


> On 08 Mar 2016, at 14:15, Bhargava Abhyankar <Bhargava.Abhyankar at radisys.com> wrote:


Dear Bhargava,


> 
> This patch is first of series of patches to support 11 bit RACH.
> This includes interface structure changes between osmo-pcu and 
> osmo-bts to support 11 bit RACH.Processing of 11 bit RACH for GPRS is 
> added in bts.cpp.
> Unit test for the same shall be sent as separate patch.
> EGPRS PACKET CHANNEL REQUEST is not part of this patch.Note also that 
> this feature requires changes in osmo-bts and versioning needs to be 
> addressed further.


could you start by sending a patch for both osmo-bts and osmo-pcu to modify the pcuif_proto.h with the new protocol and the version number increase and osmo-bts to set it as GPRS_8_BIT_RACH (to match current behavior if that makes sense)?



> ---
> src/bts.cpp       | 36 +++++++++++++++++++++++++++---------
> src/bts.h         |  9 ++++++++-
> src/pcu_l1_if.cpp |  2 +-
> src/pcuif_proto.h |  3 ++-
> 4 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/src/bts.cpp b/src/bts.cpp index 715fb51..a999c48 100644
> --- a/src/bts.cpp
> +++ b/src/bts.cpp
> @@ -459,7 +459,7 @@ int BTS::rcv_imm_ass_cnf(const uint8_t *data, uint32_t fn)
> 	return 0;
> }
> 
> -int BTS::rcv_rach(uint8_t ra, uint32_t Fn, int16_t qta)
> +int BTS::rcv_rach(uint16_t ra, uint32_t Fn, int16_t qta, uint8_t 
> +rach_type)
> {
> 

> +	} else if(rach_type == GPRS_11_BIT_RACH ) {
> +		if((ra & RACH_CAUSE_SINGLE_BLOCK) == 0x0600) {

please have a look at the Linux Kernel CodingStyle guidelines about necessary spaces between the if keyword and the opening parenthesis.

In terms of style, could you move the code that select the 'sb' into a separate (static inline) helper function. Not that we do it right now but in general this allows us to unit test such things more easily.




> +	} else if (rach_type == EGPRS_11_BIT_RACH) {
> +		/*TODO*/
> +


Do you think a log message is necessary here? Also in terms of TODO, maybe describe what someone in the future should put there?


kind regards
	holger