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

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

pravin gerrit-no-reply at lists.osmocom.org
Thu Sep 29 11:27:49 UTC 2016


Patch Set 24:

(26 comments)

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

Line 700: 		LOGP(DRLCMACDL, LOGL_DEBUG, "Compress bitmap exist, "
> 'exists'?
Done


Line 701: 			"CRBB LEN =%d and Starting color code =%d",
> 'LEN = %d' or 'LEN=%d', same with 'code=%d'
Done


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

Line 36: /* Function to build the codeword tree
> drop the 'Function to', we can see that it's a function. Furthermore, it is
Done


Line 43: 	int  len;
> whitespace: only single spaces please
Done


Line 50: 		for (i = 0;  i < len;  i++) {
> single spaces
Done


Line 55: 			} else if (cdwd[idx][i] == '1') {
> it's only '0' or '1', so a simple else would suffice.
Done


Line 63: 				(iter->run_length) = idx;
> drop the braces around iter->run_length
Done


Line 65: 				(iter->run_length) = (idx - 63) * 64;
> This looks odd. The first 64 run lenghts are 0, 1, 2, ..., 63, and the foll
Done


Line 69: 
> add a spec paragraph or table reference?
Done


Line 234: /* search_runlen function will return the runlength for the codeword
> We see that it is called search_runlen and that it is a function, there is 
Done


Line 236:  * \param bmbuf[in] Recevied compressed bitmap buf
> "Received"
Done


Line 239:  * \param rlen[out] Run length value
> "Calculated run length"
Done


Line 259: 				>>(7-(MOD8(bit_pos)))) & 0x01);
> add spaces after '>>' and around the '-'. No need to put (MOD8()) in braces
Done


Line 274: /* Function to decompress crbb
> 'Function to' is obsolete, so is 'decompress crbb' since it merely repeats 
Done


Line 276:  * \param clr_code_bit[in] Color code 1 for Ones runlength 0 for Zero runlength
> clr generally means 'clear', rather name it 'color_code_bit' instead. Consi
Done


Line 329: /* init function to build codeword */
> obsolete comment
Done


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

Line 6: 
> In case below defines are used only in egprs_rlc_compression.c, they should
Done


Line 7: #define MAX_CDWDTBL_LEN		79 /* total number of codewords */
> since this is not for all thinkable codeword tables, it should have a prefi
Done


Line 8: #define BITS_TO_BYTES(X)	((X ? (X/8):0)+1)
> Make sure to add braces around every X.
Here we are passing position of the bit to the macro and expecting the byte number. For example if the bit position is between 0-7 then it should return 1. If the bit position is between 8-15 it should return 2 and so on.


Line 9: #define MOD8(X)			(((X)+8) & (0x07))
> mod 8 is simply '((X) & 0x7)' -- what is the +8 for??
Done


Line 11: typedef struct node {
> this struct could be opaque, with only a 'typedef struct <name>' here, and 
I will change the structure instance name as you suggested.
In normally all structures in osmo-pcu files are defined in .h file itself eg:gsm_rlcmac.h.


Line 17: int decompress_crbb(int8_t compress_bmap_len, uint8_t clr_code_bit,
> I'd make this a static member function of class egprs_compress
Done


Line 20: /* Creating singleton class*/
> whitespace: 'class */'
Done


Line 23: public:
> actually, decompress_crbb could be the *only* public member of this class (
Compression support will be added in next patch.


Line 41: 		if (decode_tree_init() < 0) {
> decode_tree_init() *always* returns zero.
Done


Line 48: 	/* Private because nobody is allow to delete on object of this type */
> I'd prefer
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: 24
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



More information about the gerrit-log mailing list