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/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Thu Sep 29 01:15:53 UTC 2016


Patch Set 24:

(26 comments)

Also, still waiting for Holger to remove his -2.

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

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


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


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 called 'build_codeword', so this adds no information to what is already there. Instead you should say that, something like, the function expands the given tree by incorporating the given codeword.

Wait, it actually iterates the *entire list* of codewords, so you should use plurals here: build_codewords(root, cdwds)


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


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


Line 55: 			} else if (cdwd[idx][i] == '1') {
it's only '0' or '1', so a simple else would suffice.
(unless you want to catch odd characters in the code list, but since you're not doing that either...)


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


Line 65: 				(iter->run_length) = (idx - 63) * 64;
This looks odd. The first 64 run lenghts are 0, 1, 2, ..., 63, and the following ones are 64, 128, 192, ...? Is this defined by the spec? Maybe it deserves a comment with a reference.


Line 69: 
add a spec paragraph or table reference?


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 no need to restate the obvious.

Make the first line something like:
 "Calculate the run length of a code word."


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


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


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


Line 274: /* Function to decompress crbb
'Function to' is obsolete, so is 'decompress crbb' since it merely repeats the function's name. At least write out what crbb stands for.


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. Consider using 'bool' instead of uint8_t.

Is "color code" a term from the spec? From this doc alone I would not understand what to pass for this parameter, is this a technical detail everyone familiar with the spec would understand right away?


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


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 be moved to that file.


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


Line 8: #define BITS_TO_BYTES(X)	((X ? (X/8):0)+1)
Make sure to add braces around every X.

Logic error: 0 bits is 1 byte?
More generally true would be
(((X)? (X)/8 : 0) + ((X) & 0x7 ? 1 : 0))


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


Line 11: typedef struct node {
this struct could be opaque, with only a 'typedef struct <name>' here, and the full definition in egprs_rlc_compression.c.

Actually, for a publicly seen name, just 'Node' is too general. It should be something like 'struct egprs_compress_node'.


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


Line 20: /* Creating singleton class*/
whitespace: 'class */'
What do you mean by "Creating"?
Maybe say

/* Singleton to manage the EGPRS compression algorithm. */


Line 23: public:
actually, decompress_crbb could be the *only* public member of this class (and static), with all else handled privately: the ones and zeros lists, instance(), tree init.

BTW, so far this supports only decompression, and compression is not needed?


Line 41: 		if (decode_tree_init() < 0) {
decode_tree_init() *always* returns zero.
Just call it and make its return value void.

If at some point a possible error is added in decode_tree_init(), you can abort using OSMO_ASSERT() from within decode_tree_init().


Line 48: 	/* Private because nobody is allow to delete on object of this type */
I'd prefer
"/* singleton class, so this private destructor is left unimplemented. */"


-- 
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