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/.
Vadim Yanitskiy gerrit-no-reply at lists.osmocom.org
Patch Set 1: Code-Review-1
(24 comments)
Regarding to the gprs_* files, i.e. samples and decoding references,
I would prefer to keep them in a separate directory, for example,
called 'samples' or 'test', since they only used for testing.
Regarding to the CRC code, may we use the code from libosmocore?
Probably, we don't need CRC-related code here, because the most
part of 'gprs.c' could be rewritten to use the libosmocoding API.
You only need to collect a few bursts in buffer, and call a proper
*_decode function, which would return L2 payload.
Please see my comments. Lots of them are mostly related to the
Osmocom coding style...
https://gerrit.osmocom.org/#/c/5992/1/src/host/gprsdecode/Makefile
File src/host/gprsdecode/Makefile:
Line 15
I think, it would be better to use automake here.
https://gerrit.osmocom.org/#/c/5992/1/src/host/gprsdecode/README
File src/host/gprsdecode/README:
Line 3: Based on the version from https://srlabs.de/gprs/
Let's add a minimalistic description and usage examples here?
Example: http://git.osmocom.org/osmocom-bb/tree/src/target/fake_trx/README?h=fixeria/trx
This link is broken :(
I think it would be enough to only mention SRLabs as the authors,
and you as the patch customizer / integrator, e.g. customized by...
And BTW: what about the license?
https://gerrit.osmocom.org/#/c/5992/1/src/host/gprsdecode/burst_desc.h
File src/host/gprsdecode/burst_desc.h:
PS1, Line 7: #define BI_FLG_DUMMY (1<<4)
: #define BI_FLG_SACCH (1<<5)
:
: struct l1ctl_burst_ind {
: uint32_t frame_nr;
: uint16_t band_arfcn; /* ARFCN + band + ul indicator */
: uint8_t chan_nr; /* GSM 08.58 channel number (9.3.1) */
: uint8_t flags; /* BI_FLG_xxx + burst_id = 2LSBs */
: uint8_t rx_level; /* 0 .. 63 in typical GSM notation (dBm+110) */
: uint8_t snr; /* Reported SNR >> 8 (0-255) */
: uint8_t bits[15]; /* 114 bits + 2 steal bits. Filled MSB first */
: } __attribute__((packed));
This part should be migrated to the l1ctl_proto.h.
https://gerrit.osmocom.org/#/c/5992/1/src/host/gprsdecode/crc.c
File src/host/gprsdecode/crc.c:
Line 2: * Code imported from GNURadio, GSMSTACK and Linux Kernel.
The license header from crc.h shout be (also) here.
Line 15: int FC_init(FC_CTX *ctx, unsigned int crc_size, unsigned int data_size)
Return value is always zero => void is better..
Line 24: int FC_check_crc(FC_CTX *ctx, unsigned char *input_bits, unsigned char *control_data)
Mix of spaces and tabs...
Line 30: // reset the syndrome register
Comment style /* ... */ is preferred.
Line 80: }
... } else if { ...
https://gerrit.osmocom.org/#/c/5992/1/src/host/gprsdecode/crc.h
File src/host/gprsdecode/crc.h:
Line 24: #include <stdint.h>
Useless header...
https://gerrit.osmocom.org/#/c/5992/1/src/host/gprsdecode/gprs.c
File src/host/gprsdecode/gprs.c:
Line 43: return "LOL";
LOL :)
Line 98: const uint8_t usf_pattern[][6] = {{0, 0, 0, 0, 0, 0},
Could be separated as a static symbol.
Line 122: const uint8_t usf_pattern[][12] = {{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0},
Also could be separated...
PS1, Line 142: static inline void gsm0503_xcch_deinterleave(sbit_t *cB, sbit_t *iB)
: {
: int j, B;
:
: for (int k = 0; k < 456; k++) {
: B = k & 3;
: j = 2 * ((49 * k) % 57) + ((k & 7) >> 2);
: cB[k] = iB[B * 114 + j];
: }
: }
Already in libosmocoding.
PS1, Line 153: static inline int decode_signalling(const uint8_t *conv_data, uint8_t *msg)
: {
: uint8_t decoded_data[PARITY_OUTPUT_SIZE];
: int8_t soft_input[CONV_SIZE];
:
: /* convert to soft bits */
: osmo_ubit2sbit(soft_input, conv_data, CONV_SIZE);
:
: /* Viterbi decoding */
: osmo_conv_decode(&gsm0503_xcch, soft_input, decoded_data);
:
: /* parity check: if error detected try to fix it */
: int ret = osmo_crc64gen_check_bits(&gsm0503_fire_crc40, decoded_data, 184, decoded_data + 184);
: if (ret) {
: FC_CTX fc_ctx;
: FC_init(&fc_ctx, PARITY_SIZE, DATA_BLOCK_SIZE);
: /**/
: unsigned char crc_result[DATA_BLOCK_SIZE + PARITY_SIZE];
: ret = FC_check_crc(&fc_ctx, decoded_data, crc_result);
: if (!ret)
: return 0;
: /*
: ubit_t crc_result[DATA_BLOCK_SIZE + PARITY_SIZE];
: osmo_crc64gen_set_bits(&gsm0503_fire_crc40, decoded_data, 184, crc_result);
: */
: memcpy(decoded_data, crc_result, sizeof crc_result);
: }
:
: osmo_ubit2pbit_ext(msg, 0, decoded_data, 0, DATA_BLOCK_SIZE, 1);
:
: return 23;
: }
Also looks like a code duplication, should be in libosmocoding.
Line 186: int process_pdch(struct l1ctl_burst_ind *bi, bool print)
The most part of this function could be rewritten to use
the libosmocoding API, like OsmoBTS does. Reference:
http://git.osmocom.org/libosmocore/tree/include/osmocom/coding/gsm0503_coding.h#n37
https://gerrit.osmocom.org/#/c/5992/1/src/host/gprsdecode/gprs.h
File src/host/gprsdecode/gprs.h:
PS1, Line 13: #define DATA_BLOCK_SIZE 184
: #define PARITY_SIZE 40
: #define FLUSH_BITS_SIZE 4
: #define PARITY_OUTPUT_SIZE (DATA_BLOCK_SIZE + PARITY_SIZE + FLUSH_BITS_SIZE)
: #define DEBUG_PRINT false
: #define PREFER_MCS 0
: #define CONV_SIZE (2 * PARITY_OUTPUT_SIZE)
: #define MAX_MCS 5
Tabs are welcome for alignment ;)
PS1, Line 25: /*
: * GSM PDTCH CS-2, CS-3, CS-4 parity
: *
: * g(x) = x^16 + x^12 + x^5 + 1
: */
: static const struct osmo_crc16gen_code gsm0503_cs234_crc16 = {
: .bits = 16,
: .poly = 0x1021,
: .init = 0x0000,
: .remainder = 0xffff,
: };
:
: /*
: * GSM (SACCH) parity (FIRE code)
: *
: * g(x) = (x^23 + 1)(x^17 + x^3 + 1)
: * = x^40 + x^26 + x^23 + x^17 + x^3 + a1
: */
: static const struct osmo_crc64gen_code gsm0503_fire_crc40 = {
: .bits = 40,
: .poly = 0x0004820009ULL,
: .init = 0x0000000000ULL,
: .remainder = 0xffffffffffULL,
: };
Both are in libosmocore now.
Line 60: struct burst_buf gprs[16];
WTF? Shouldn't be here.
https://gerrit.osmocom.org/#/c/5992/1/src/host/gprsdecode/main.c
File src/host/gprsdecode/main.c:
Line 6: #include <osmocom/core/select.h>
Select API isn't used here.
PS1, Line 26: /* printf("TS = %d, FN = %d (%d)\n", ts, fn, process_pdch(bi));
: else
: printf("TS = %d, FN = %d [%d]\n", ts, fn, fn % 13);
: */
Why this part is commented out?
PS1, Line 30: case RSL_CHAN_Lm_ACCHs:
: case RSL_CHAN_BCCH:
: case RSL_CHAN_SDCCH4_ACCH:
: case RSL_CHAN_SDCCH8_ACCH:
: case RSL_CHAN_RACH:
: case RSL_CHAN_PCH_AGCH:
: default:
: break;
: //printf("Type not handled! %.02x\n", type);
We are not interested in other than GPRS channels, right?
So, we can merely skip them, putting only 'default' label.
Line 64: ret = fread(&bi, sizeof(bi), 1, burst_fd);
Would be great to put a error message here.
Line 71: fflush(NULL);
Why do we need this?
https://gerrit.osmocom.org/#/c/5992/1/src/host/gprsdecode/output.c
File src/host/gprsdecode/output.c:
Line 64: while (osmo_select_main(1));
Shouldn't this be in main.c, like all other application do?
--
To view, visit https://gerrit.osmocom.org/5992
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I12234d37c66b83b8abd60f7511fa1d7837db1856
Gerrit-PatchSet: 1
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Owner: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-HasComments: Yes