osmo-pcu[master]: EGPRS: Add EPDAN CRBB Tree based decoding

[pravin]

Patch Set 25:

(22 comments)

https://gerrit.osmocom.org/#/c/416/25/src/decoding.cpp
File src/decoding.cpp:

Line 697: 		LOGP(DRLCMACDL, LOGL_DEBUG, "Compress bitmap exists, "
> Actually ... what do you mean by 'exists'? Is received?
This means compress bitmap is present in the received bitmap.


https://gerrit.osmocom.org/#/c/416/25/src/egprs_rlc_compression.cpp
File src/egprs_rlc_compression.cpp:

Line 22: extern const char *zero_run_len_code_list[EGPRS_CODEWORDS];
> the two arrays are not extern but defined further below.
Done


Line 37: /* The function expands the given tree by incorporating
> Still no need to state that it is a function! For example:
Done


Line 40:  * \param cdwd[in] Code word
> plural, and indicate the length:
Done


Line 74:  
> (remove space from the blank line)
Done


Line 248:  * \param rlen[out] Caluculate run length
> fix typo. As I said before, it should say 'Calculated', not 'Calculate' (wh
Done


Line 267: 		dir = ((bmbuf[BITS_TO_BYTES(bit_pos) - 1]
> Come on, in BITS_TO_BYTES you '+1' and here you '-1' again? Since this is t
Done


Line 268: 				>> (7 - MOD8(bit_pos))) & 0x01);
> Drop outermost braces.
Done


Line 271: 		if (((dir&0x01) == 0) && (iter->left != NULL))
> dir is already & 0x01 above.
Done


Line 273: 		else if (((dir&0x01) == 1) && (iter->right != NULL))
> 'if (dir && (iter...'
Done


Line 279: 	(*rlen) = (iter->run_length);
> drop all braces
Done


Line 283: /* Function to decompress compressed received block bitmap
> I *see* that it is a function! Start with
Done


Line 285:  * \param color_code_bit[in] Color code 1 for Ones runlength 0 for Zero runlength
> As asked before, please explain why you call it 'color code' in the first p
Done


Line 297: 	uint8_t data = 0x0;
> '= 0' ?
Done


Line 304: 	egprs_compress *compress = egprs_compress::instance();
> just ' = instance()' should suffice?
Done


Line 307: 		if (color_code_bit == 1) {
> 'if (color_code_bit) {'
Done


Line 320: 			color_code_bit ? color_code_bit = 0 : color_code_bit = 1;
> "color_code_bit = !color_code_bit;"
Done


https://gerrit.osmocom.org/#/c/416/25/src/egprs_rlc_compression.h
File src/egprs_rlc_compression.h:

Line 7: #define BITS_TO_BYTES(X)	(((X) ? (X)/8 : 0) + 1)
> Then this should be called BITPOS_TO_BYTES instead.
Done


Line 8: #define MOD8(X)			((X) & (0x07))
> you're using this define only once, just drop it and use '& 0x7' directly i
Done


Line 14: } egprs_compress_node;
> What I meant: here just say "struct egprs_compress_node;",
Done


Line 32: 	{
> I would move the function definition to .cpp as well
Done


Line 41: 		decode_tree_init();
> and move this one to .cpp as well.
Done


-- 
To view, visit https://gerrit.osmocom.org/416
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieae1992ed4b02bb1e09eec2d3de1a030eabd16ce
Gerrit-PatchSet: 25
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: pravin <pravin.manoharan at radisys.com>
Gerrit-Reviewer: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: arvind.sirsikar <arvind.sirsikar at radisys.com>
Gerrit-Reviewer: pravin <pravin.manoharan at radisys.com>
Gerrit-HasComments: Yes