libosmocore[master]: gsm0503: migrate transcoding routines from OsmoBTS

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
Tue Sep 6 18:00:58 UTC 2016


Patch Set 4:

(15 comments)

https://gerrit.osmocom.org/#/c/816/4//COMMIT_MSG
Commit Message:

Line 12: beeter to clean up the OsmoBTS source code, sharing this code.
> Better than what?
Yes, thanks. I'll change it.


Line 28: in generated state.
> The general opinion about generated codes was to rather not
In OsmoBTS there are already generated convolutional codes. So, I spent some time finding corresponding polynomial sets for some data types, which wasn't defined in the conv_gen.py generator.

Despite the EDGE's MCS levels use the same generator polynomial with different lengths, I wasn't able to find any details about EDGE convolutional transcoding, so I merely copied those tables from OsmoBTS into a separate file.

I still have some doubts, related to MCS codes...
They aren't covered by GSM 05.03, so should I migrate them in separate commit? Should I use a separate file? Which name to use for this file?


https://gerrit.osmocom.org/#/c/816/4/include/osmocom/gsm/gsm0503_coding.h
File include/osmocom/gsm/gsm0503_coding.h:

Line 6:  * (C) 2016 by Tom Tsou <tom.tsou at ettus.com>
> we usually have the copyright and license info only in the .c files...
I just followed the gsm0503.h as an example, because there was such information... But I am agree with you.


Line 46: int gsm0503_xcch_encode(ubit_t *bursts, uint8_t *l2_data);
> you're using stdint.h types, yet this header file has no #includes at all. 
Ok, I need to add the following:

#include <stdint.h>
#include <osmocom/core/bits.h>


https://gerrit.osmocom.org/#/c/816/4/src/gsm/Makefile.am
File src/gsm/Makefile.am:

Line 21: 			gsm610.c gsm620.c gsm660.c gsm0503_conv.c \
> Is the gsm610.c in src/codec related to these?
This is because the gsm0503_coding.c has some dependences from libosmocodec. Maybe there is a better way to link these dependences?


Line 30: 
> don't add a blank line here:
Ok, I will fix it.


Line 42: 	$(AM_V_GEN)python2 ../../utils/conv_gen.py
> for 'make distcheck': rather use $(top_srcdir)/... ?
Ok, it would be better.


https://gerrit.osmocom.org/#/c/816/4/src/gsm/gsm0503_conv_edge.c
File src/gsm/gsm0503_conv_edge.c:

Line 62: 
> one blank line too many -- but if the file is moving from
Yes, it is a code part from OsmoBTS, and there was even more blank lines, twice after every 'const struct osmo_conv_code...' definition. :) I used this separation (two blank lines) to separate tables from definitions and to increase readability...


https://gerrit.osmocom.org/#/c/816/4/src/gsm/gsm0503_interleaving.c
File src/gsm/gsm0503_interleaving.c:

Line 304: 
> (two blank lines, same as before)
Just copied 'as is', but I can fix it.


https://gerrit.osmocom.org/#/c/816/4/src/gsm/gsm0503_mapping.c
File src/gsm/gsm0503_mapping.c:

Line 29: 			      sbit_t *hl, sbit_t *hn)
> this file mixes single-tab indent and aligned indent styles.
Ok, will be fixed.


https://gerrit.osmocom.org/#/c/816/4/src/gsm/gsm0503_parity.c
File src/gsm/gsm0503_parity.c:

Line 33: 
> would be nicer to not have a blank line between comment-for-struct and stru
It also was taken 'as is', will be fixed.


https://gerrit.osmocom.org/#/c/816/4/utils/conv_gen.py
File utils/conv_gen.py:

Line 30: 				 name = "call-me", description = False, puncture = []):
> IMHO pretty weird indenting choice.
How to separate this big line better? I have some doubts... How many tabs should I use?


Line 53: 				raise ValueError("Bad polynoms: "
> The proper English term would be polynomial / polynomials.
I am not an author of this code, but I will fix it.


Line 59: 					"in a recursive code")
> separate commit.
Ok, I was thinking about that...


Line 298: 			 15,  19,  23,  27,  31,  35,  43,  47,  51,  55,  59,  63,  67,  71,
> I hope this is correct :)
For me this is magic too, and this is why I merely copied the puncture tables 'as is'. Just decreased tabs count...


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I257a5d015798ee9e690fd035ca97fd971cf9f60a
Gerrit-PatchSet: 4
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-HasComments: Yes



More information about the gerrit-log mailing list