osmocom-bb[master]: Import gprsdecode utility from SRLabs
Patch Set 7: Code-Review+2 -- 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: 7 Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Reviewer: fixeria Gerrit-HasComments: No
osmocom-bb[master]: Import gprsdecode utility from SRLabs
Patch Set 6: (3 comments) https://gerrit.osmocom.org/#/c/5992/6//COMMIT_MSG Commit Message: Line 2: Author: Max > I'm not sure if we should attribute max as author in git, if it's code orig Done https://gerrit.osmocom.org/#/c/5992/6/src/host/gprsdecode/gprs.c File src/host/gprsdecode/gprs.c: Line 3: * (C) 2017-2018 by Max > this should be copyright by sysmocom - s.f.m.c. GmbH with an Author: Max, s Done https://gerrit.osmocom.org/#/c/5992/6/src/host/gprsdecode/rlcmac.h File src/host/gprsdecode/rlcmac.h: Line 29:uint8_t data[53]; > might be interesting to have some description about the rationale of those I wish I could do this, but I am not so good in GPRS for now. This would be done in a separate commit, probably with some more improvements... -- 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: 6 Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Reviewer: fixeria Gerrit-HasComments: Yes
osmocom-bb[master]: Import gprsdecode utility from SRLabs
Patch Set 6: -Code-Review > Seems like 'authors' file was lost during update, as well as > 'copying' and 'news'. Unless it's intentional of course. The only (as I saw) project, known to use such files, is OsmoTRX. I am not sure they are really important here, moreover some of them are and will remain empty. So, this is intentional. -- 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: 6 Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Reviewer: fixeria Gerrit-HasComments: No
osmocom-bb[master]: Import gprsdecode utility from SRLabs
Patch Set 6: (3 comments) let's fix the copyright statements and then get it merged. https://gerrit.osmocom.org/#/c/5992/6//COMMIT_MSG Commit Message: Line 2: Author: Max I'm not sure if we should attribute max as author in git, if it's code originally from luca/srlabs? https://gerrit.osmocom.org/#/c/5992/6/src/host/gprsdecode/gprs.c File src/host/gprsdecode/gprs.c: Line 3: * (C) 2017-2018 by Max this should be copyright by sysmocom - s.f.m.c. GmbH with an Author: Max, similar to many other locations in Osmocom where sysmocom staff has been writing code during their work hours. https://gerrit.osmocom.org/#/c/5992/6/src/host/gprsdecode/rlcmac.h File src/host/gprsdecode/rlcmac.h: Line 29:uint8_t data[53]; might be interesting to have some description about the rationale of those magic numbers 53, 20 and 120 below. Is it known? -- 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: 6 Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Reviewer: fixeria Gerrit-HasComments: Yes
osmocom-bb[master]: Import gprsdecode utility from SRLabs
Patch Set 6: Seems like 'authors' file was lost during update, as well as 'copying' and 'news'. Unless it's intentional of course. -- 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: 6 Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Reviewer: fixeria Gerrit-HasComments: No
osmocom-bb[master]: Import gprsdecode utility from SRLabs
Patch Set 6: Code-Review+1 At this state it looks much better for me ;) Changed the code to use libosmocoding and avoid the code duplication (e.g. CRC, convolutional codes). Regression tests are passing. -- 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: 6 Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Reviewer: fixeria Gerrit-HasComments: No
osmocom-bb[master]: Import gprsdecode utility
Patch Set 5: Code-Review-1 (1 comment) > I'll leave it up to vadim. Once vadim is happy, let me know for > some final review and hopefully +2 I'll rewrite the code to use libosmocoding, because I have some experience with it and don't want to see the code duplication. The main idea is to use gsm0503_pdtch_egprs_(de|en)code like OsmoBTS does. This is what libosmocoding is intended for ;) https://gerrit.osmocom.org/#/c/5992/5/src/host/gprsdecode/Makefile.am File src/host/gprsdecode/Makefile.am: Line 2: AM_CFLAGS = -Wall -O3 -pipe -flto -std=gnu99 -g $(LIBOSMOCORE_CFLAGS) $(LIBOSMOGSM_CFLAGS) $(LIBOSMOCODING_CFLAGS) ws -- 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: 5 Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Reviewer: fixeria Gerrit-HasComments: Yes
osmocom-bb[master]: Import gprsdecode utility
Patch Set 1: I'll leave it up to vadim. Once vadim is happy, let me know for some final review and hopefully +2 -- 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 Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Reviewer: fixeria Gerrit-HasComments: No
osmocom-bb[master]: Import gprsdecode utility
Patch Set 1:
(5 comments)
Thank you for detailed review, I've tried to address most of it, don't hesitate
to remind if I've missed something.
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?
> This link is broken :(
Updated - they have problems with their website.
> what about the license?
Same as the rest of OsmocomBB.
https://gerrit.osmocom.org/#/c/5992/1/src/host/gprsdecode/gprs.c
File src/host/gprsdecode/gprs.c:
Line 98:const uint8_t usf_pattern[][6] = {{0, 0, 0, 0, 0, 0},
> Could be separated as a static symbol.
What for?
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.
I don't think so but if you can split it out and intergrate into libosmocoding
- feel free to send a patch and add me sa reviewer.
Line 186: int process_pdch(struct l1ctl_burst_ind *bi, bool print)
> The most part of this function could be rewritten to use
Perhaps, but it's certainly beyond the scope of "initial import" commit.
https://gerrit.osmocom.org/#/c/5992/1/src/host/gprsdecode/main.c
File src/host/gprsdecode/main.c:
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?
Debug aid, I'd prefer to keep it here until all MCS* are implemented. We can
drop it afterwards.
--
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
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max
Gerrit-Reviewer: Vadim Yanitskiy
Gerrit-Reviewer: fixeria
Gerrit-HasComments: Yes
osmocom-bb[master]: Import gprsdecode utility
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
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);
