osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support:

[allesklar2]

Patch Set 3:

> Patch Set 3: Published edit on patch set 2.

I found out that a file (tetra_burst.h) was uploaded without changes in my 
first commit and there was also a type in tetra-rx.c

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
Gerrit-PatchSet: 3
Gerrit-Project: osmo-tetra
Gerrit-Branch: laforge/sq5bpf-rebase-20161218
Gerrit-Owner: allesklar2 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Reviewer: allesklar2 
Gerrit-HasComments: No

[PATCH] osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support:

[allesklar2]

Hello Harald Welte,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/2817

to look at the new patch set (#3).

adding dmo support:

physical layer: If DMO preamble bits are present, burst are send to DMO SAPs

lower mac layer: Decoding DMO Bursts and adding a new SAP for DMO. This SAP 
forwards all requests to the TMO SAP because the processing is quite similar. 
Nevertheless this SAP is important because in further developments you can 
change the lower mac without addopting the physical layer.

Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
---
M AUTHORS
M src/conv_enc_test.c
M src/lower_mac/tetra_lower_mac.c
M src/lower_mac/tetra_scramb.c
M src/lower_mac/tetra_scramb.h
M src/phy/tetra_burst.c
M src/phy/tetra_burst.h
M src/phy/tetra_burst_sync.c
M src/phy/tetra_burst_sync.h
M src/tetra-rx.c
M src/tetra_common.h
M src/tetra_gsmtap.c
12 files changed, 477 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-tetra refs/changes/17/2817/3

diff --git a/AUTHORS b/AUTHORS
index a0c987b..7702664 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -1,3 +1,5 @@
 Harald Welte 
 Sylvain Munaut 
 Holger Hans Peter Freyther 
+Jan-Pascal Kwiotek 
+Jannik Jürgens 
diff --git a/src/conv_enc_test.c b/src/conv_enc_test.c
index 303ef31..e714ec6 100644
--- a/src/conv_enc_test.c
+++ b/src/conv_enc_test.c
@@ -49,6 +49,11 @@
 {
 }
 
+/* incoming DP-SAP UNITDATA.ind  from PHY into lower MAC */
+void dp_sap_udata_ind(enum tp_sap_data_type type, const uint8_t *bits, 
unsigned int len, void *priv)
+{
+}
+
 static void decode_schf(const uint8_t *bits)
 {
uint8_t type4[1024];
diff --git a/src/lower_mac/tetra_lower_mac.c b/src/lower_mac/tetra_lower_mac.c
index 53fe73a..b0f95ed 100644
--- a/src/lower_mac/tetra_lower_mac.c
+++ b/src/lower_mac/tetra_lower_mac.c
@@ -103,6 +103,42 @@
.type2_bits = 30,
.type1_bits = 14,
},
+   /* DMO Synchronization Burst Block 1 */
+   [DPSAP_SCH_S] = {
+   .name   = "SCH/S",
+   .type345_bits   = 120,
+   .type2_bits = 80,
+   .type1_bits = 60,
+   .interleave_a   = 11,
+   .have_crc16 = 1,
+   },
+   /* DMO Synchronization Burst Block 2 */
+   [DPSAP_SCH_H] = {
+   .name   = "SCH/H",
+   .type345_bits   = 216,
+   .type2_bits = 144,
+   .type1_bits = 124,
+   .interleave_a   = 101,
+   .have_crc16 = 1,
+   },
+   /* DMO Normal Burst Block 1+2 */
+   [DPSAP_SCH_F] = {
+   .name   = "SCH/F",
+   .type345_bits   = 432,
+   .type2_bits = 288,
+   .type1_bits = 268,
+   .interleave_a   = 103,
+   .have_crc16 = 1,
+   },
+   /* DMO Normal Burst with Slot Flag */
+   [DPSAP_STCH] = {
+   .name   = "STCH",
+   .type345_bits   = 216,
+   .type2_bits = 144,
+   .type1_bits = 124,
+   .interleave_a   = 101,
+   .have_crc16 = 1,
+   },
 };
 
 struct tetra_cell_data {
@@ -112,6 +148,7 @@
struct tetra_tdma_time time;
 
uint32_t scramb_init;
+   uint32_t textmessage_length;
 };
 
 static struct tetra_cell_data _tcd, *tcd = &_tcd;
@@ -151,6 +188,7 @@
uint8_t type3dp[512*4];
uint8_t type3[512];
uint8_t type2[512];
+   uint8_t frag_offset;/* for variable fields in DPSAP_SCH_H */
 
const struct tetra_blk_param *tbp = _blk_param[type];
struct tetra_mac_state *tms = priv;
@@ -181,7 +219,7 @@
 
/* De-scramble, pay special attention to SB1 pre-defined scrambling */
memcpy(type4, bits, tbp->type345_bits);
-   if (type == TPSAP_T_SB1) {
+   if (type == TPSAP_T_SB1 || type == DPSAP_SCH_S  || type == DPSAP_SCH_H) 
{
tetra_scramb_bits(SCRAMB_INIT, type4, tbp->type345_bits);
tup->scrambling_code = SCRAMB_INIT;
} else {
@@ -235,7 +273,7 @@
printf("MN %s(%2u) ", osmo_ubit_dump(type2+17, 6), 
bits_to_uint(type2+17, 6));
printf("MCC %s(%u) ", osmo_ubit_dump(type2+31, 10), 
bits_to_uint(type2+31, 10));
printf("MNC %s(%u)\n", osmo_ubit_dump(type2+41, 14), 
bits_to_uint(type2+41, 14));
-   /* obtain information from SYNC PDU */
+   /* obtain information from SYNC PDU - 21.4.4.2 */
tcd->colour_code = bits_to_uint(type2+4, 6);
tcd->time.tn = bits_to_uint(type2+10, 2);
tcd->time.fn = bits_to_uint(type2+12, 5);
@@ -296,6 +334,294 @@
}
/* sq5bpf: koniec */
break;
+   case DPSAP_SCH_F:
+   /* 396-3 9.2 */

osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support:

[allesklar2]

Patch Set 2:

(1 comment)

> Patch Set 2: Published edit on patch set 1.

https://gerrit.osmocom.org/#/c/2817/1/src/conv_enc_test.c
File src/conv_enc_test.c:

PS1, Line 53: void dp_sap_udata_ind(enum tp_sap_data_type type, const uint8_t 
*bits, unsigned int len, void *priv)
: {
: }
> Just a hint: you can use grep to find, where this
During the development I have oriented to TMO and the function 
tp_sap_udata_ind() is also empty. I suspect that these functions must exist in 
order to preserve the independence from the MAC Layer. Since the function is 
specified in lower_mac_layer and is defined as extern in tetra_burst.h, it must 
be listed as a dummy at this point.

Does this make sense for you?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
Gerrit-PatchSet: 2
Gerrit-Project: osmo-tetra
Gerrit-Branch: laforge/sq5bpf-rebase-20161218
Gerrit-Owner: allesklar2 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Reviewer: allesklar2 
Gerrit-HasComments: Yes

[PATCH] osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support:

[allesklar2]

Hello Harald Welte,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/2817

to look at the new patch set (#2).

adding dmo support:

physical layer: If DMO preamble bits are present, burst are send to DMO SAPs

lower mac layer: Decoding DMO Bursts and adding a new SAP for DMO. This SAP 
forwards all requests to the TMO SAP because the processing is quite similar. 
Nevertheless this SAP is important because in further developments you can 
change the lower mac without addopting the physical layer.

Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
---
M AUTHORS
M src/conv_enc_test.c
M src/lower_mac/tetra_lower_mac.c
M src/lower_mac/tetra_scramb.c
M src/lower_mac/tetra_scramb.h
M src/phy/tetra_burst.c
M src/phy/tetra_burst_sync.c
M src/phy/tetra_burst_sync.h
M src/tetra-rx.c
M src/tetra_common.h
M src/tetra_gsmtap.c
11 files changed, 464 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-tetra refs/changes/17/2817/2

diff --git a/AUTHORS b/AUTHORS
index a0c987b..7702664 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -1,3 +1,5 @@
 Harald Welte 
 Sylvain Munaut 
 Holger Hans Peter Freyther 
+Jan-Pascal Kwiotek 
+Jannik Jürgens 
diff --git a/src/conv_enc_test.c b/src/conv_enc_test.c
index 303ef31..e714ec6 100644
--- a/src/conv_enc_test.c
+++ b/src/conv_enc_test.c
@@ -49,6 +49,11 @@
 {
 }
 
+/* incoming DP-SAP UNITDATA.ind  from PHY into lower MAC */
+void dp_sap_udata_ind(enum tp_sap_data_type type, const uint8_t *bits, 
unsigned int len, void *priv)
+{
+}
+
 static void decode_schf(const uint8_t *bits)
 {
uint8_t type4[1024];
diff --git a/src/lower_mac/tetra_lower_mac.c b/src/lower_mac/tetra_lower_mac.c
index 53fe73a..b0f95ed 100644
--- a/src/lower_mac/tetra_lower_mac.c
+++ b/src/lower_mac/tetra_lower_mac.c
@@ -103,6 +103,42 @@
.type2_bits = 30,
.type1_bits = 14,
},
+   /* DMO Synchronization Burst Block 1 */
+   [DPSAP_SCH_S] = {
+   .name   = "SCH/S",
+   .type345_bits   = 120,
+   .type2_bits = 80,
+   .type1_bits = 60,
+   .interleave_a   = 11,
+   .have_crc16 = 1,
+   },
+   /* DMO Synchronization Burst Block 2 */
+   [DPSAP_SCH_H] = {
+   .name   = "SCH/H",
+   .type345_bits   = 216,
+   .type2_bits = 144,
+   .type1_bits = 124,
+   .interleave_a   = 101,
+   .have_crc16 = 1,
+   },
+   /* DMO Normal Burst Block 1+2 */
+   [DPSAP_SCH_F] = {
+   .name   = "SCH/F",
+   .type345_bits   = 432,
+   .type2_bits = 288,
+   .type1_bits = 268,
+   .interleave_a   = 103,
+   .have_crc16 = 1,
+   },
+   /* DMO Normal Burst with Slot Flag */
+   [DPSAP_STCH] = {
+   .name   = "STCH",
+   .type345_bits   = 216,
+   .type2_bits = 144,
+   .type1_bits = 124,
+   .interleave_a   = 101,
+   .have_crc16 = 1,
+   },
 };
 
 struct tetra_cell_data {
@@ -112,6 +148,7 @@
struct tetra_tdma_time time;
 
uint32_t scramb_init;
+   uint32_t textmessage_length;
 };
 
 static struct tetra_cell_data _tcd, *tcd = &_tcd;
@@ -151,6 +188,7 @@
uint8_t type3dp[512*4];
uint8_t type3[512];
uint8_t type2[512];
+   uint8_t frag_offset;/* for variable fields in DPSAP_SCH_H */
 
const struct tetra_blk_param *tbp = _blk_param[type];
struct tetra_mac_state *tms = priv;
@@ -181,7 +219,7 @@
 
/* De-scramble, pay special attention to SB1 pre-defined scrambling */
memcpy(type4, bits, tbp->type345_bits);
-   if (type == TPSAP_T_SB1) {
+   if (type == TPSAP_T_SB1 || type == DPSAP_SCH_S  || type == DPSAP_SCH_H) 
{
tetra_scramb_bits(SCRAMB_INIT, type4, tbp->type345_bits);
tup->scrambling_code = SCRAMB_INIT;
} else {
@@ -235,7 +273,7 @@
printf("MN %s(%2u) ", osmo_ubit_dump(type2+17, 6), 
bits_to_uint(type2+17, 6));
printf("MCC %s(%u) ", osmo_ubit_dump(type2+31, 10), 
bits_to_uint(type2+31, 10));
printf("MNC %s(%u)\n", osmo_ubit_dump(type2+41, 14), 
bits_to_uint(type2+41, 14));
-   /* obtain information from SYNC PDU */
+   /* obtain information from SYNC PDU - 21.4.4.2 */
tcd->colour_code = bits_to_uint(type2+4, 6);
tcd->time.tn = bits_to_uint(type2+10, 2);
tcd->time.fn = bits_to_uint(type2+12, 5);
@@ -296,6 +334,294 @@
}
/* sq5bpf: koniec */
break;
+   case DPSAP_SCH_F:
+   /* 396-3 9.2 */
+   if 

osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support

[Max]

Patch Set 1:

> you can use grep to find

git grep -n func-name
is better because it looks only into version-controlled files :)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
Gerrit-PatchSet: 1
Gerrit-Project: osmo-tetra
Gerrit-Branch: laforge/sq5bpf-rebase-20161218
Gerrit-Owner: allesklar2 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Reviewer: allesklar2 
Gerrit-HasComments: No

osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support

[Vadim Yanitskiy]

Patch Set 1:

(1 comment)

> Do you see my updated code or do I have to push a button somewhere?

Nope. See https://osmocom.org/projects/cellular-infrastructure/wiki/Gerrit for 
details.

https://gerrit.osmocom.org/#/c/2817/1/src/conv_enc_test.c
File src/conv_enc_test.c:

PS1, Line 53: void dp_sap_udata_ind(enum tp_sap_data_type type, const uint8_t 
*bits, unsigned int len, void *priv)
: {
: }
> I have not figured out why yet, but without this function I get some error 
Just a hint: you can use grep to find, where this
function is being called, and drop corresponding line(s).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
Gerrit-PatchSet: 1
Gerrit-Project: osmo-tetra
Gerrit-Branch: laforge/sq5bpf-rebase-20161218
Gerrit-Owner: allesklar2 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Reviewer: allesklar2 
Gerrit-HasComments: Yes

osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support

[allesklar2]

Patch Set 1:

Do you see my updated code or do I have to push a button somewhere?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
Gerrit-PatchSet: 1
Gerrit-Project: osmo-tetra
Gerrit-Branch: laforge/sq5bpf-rebase-20161218
Gerrit-Owner: allesklar2 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Reviewer: allesklar2 
Gerrit-HasComments: No

osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support

[allesklar2]

Patch Set 1:

(1 comment)

> onee question though: This is submitted for the sq5bpf-rebase
 > branch (which was basically just a quick test by me some time ago),
 > but should actually go in master, or is there any specific
 > dependency?  If so, please explain.

There is no special dependency. I just stated developing on the 
sq5bpf-rebase-branch for other reasons. Now I decided to first commit my code 
here and wait for feedback. I through it would end in a mess if I start 
committing the same stuff in two branches.

https://gerrit.osmocom.org/#/c/2817/1/src/conv_enc_test.c
File src/conv_enc_test.c:

PS1, Line 53: void dp_sap_udata_ind(enum tp_sap_data_type type, const uint8_t 
*bits, unsigned int len, void *priv)
: {
: }
> Why this is empty?
I have not figured out why yet, but without this function I get some error 
during the compilation.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
Gerrit-PatchSet: 1
Gerrit-Project: osmo-tetra
Gerrit-Branch: laforge/sq5bpf-rebase-20161218
Gerrit-Owner: allesklar2 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Reviewer: allesklar2 
Gerrit-HasComments: Yes

osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support

[allesklar2]

Patch Set 1:

(1 comment)

> (4 comments)
 > 
 > Thnaks a lot for your contribution! It is very much appreciated,
 > please forgitve me missing it in the patch review  so far.  It's
 > not usual that it waits for three weeks.  If you hsould see that
 > again, feel free to send a "ping" either by private mail or here as
 > a "reply" on the patch.

Thanks for your comments. I revised my patch and fixed the most things.

https://gerrit.osmocom.org/#/c/2817/1/src/phy/tetra_burst.c
File src/phy/tetra_burst.c:

Line 305: #if 0 /* not used */
> not used by whom? why? I think more context is needed here.  also, it seems
You are right, it is not necessary for DMO. I comment it out because the code 
runs faster and it is not used in "tetra_burst_sync.c".


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
Gerrit-PatchSet: 1
Gerrit-Project: osmo-tetra
Gerrit-Branch: laforge/sq5bpf-rebase-20161218
Gerrit-Owner: allesklar2 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Reviewer: allesklar2 
Gerrit-HasComments: Yes

osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support

[Vadim Yanitskiy]

Patch Set 1:

(3 comments)

https://gerrit.osmocom.org/#/c/2817/1//COMMIT_MSG
Commit Message:

Line 7: adding dmo support
Not so critical, but it would be great to have a bit
more information about the change and DMO in general here.


https://gerrit.osmocom.org/#/c/2817/1/src/conv_enc_test.c
File src/conv_enc_test.c:

PS1, Line 53: void dp_sap_udata_ind(enum tp_sap_data_type type, const uint8_t 
*bits, unsigned int len, void *priv)
: {
: }
Why this is empty?

Could you please explain, writing a few words comment
inside the body, if this function makes sense.

Otherwise, it shouldn't be here.


https://gerrit.osmocom.org/#/c/2817/1/src/lower_mac/tetra_scramb.c
File src/lower_mac/tetra_scramb.c:

PS1, Line 34: #define SCRAMB_INIT   3
: #define SCRAMB_ZERO   0
Both already defined in lower_mac/tetra_scramb.h,
so we can clean them out.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
Gerrit-PatchSet: 1
Gerrit-Project: osmo-tetra
Gerrit-Branch: laforge/sq5bpf-rebase-20161218
Gerrit-Owner: allesklar2 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-HasComments: Yes

osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support

[Harald Welte]

Patch Set 1:

onee question though: This is submitted for the sq5bpf-rebase branch (which was 
basically just a quick test by me some time ago), but should actually go in 
master, or is there any specific dependency?  If so, please explain.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
Gerrit-PatchSet: 1
Gerrit-Project: osmo-tetra
Gerrit-Branch: laforge/sq5bpf-rebase-20161218
Gerrit-Owner: allesklar2 
Gerrit-Reviewer: Harald Welte 
Gerrit-HasComments: No

osmo-tetra[laforge/sq5bpf-rebase-20161218]: adding dmo support

[Harald Welte]

Patch Set 1: Code-Review+1

(4 comments)

Thnaks a lot for your contribution! It is very much appreciated, please 
forgitve me missing it in the patch review  so far.  It's not usual that it 
waits for three weeks.  If you hsould see that again, feel free to send a 
"ping" either by private mail or here as a "reply" on the patch.

https://gerrit.osmocom.org/#/c/2817/1/src/lower_mac/tetra_lower_mac.c
File src/lower_mac/tetra_lower_mac.c:

Line 106:   [DPSAP_SCH_S] = {   //DMO Synchronization Burst 
Block 1
I would appreciate if you could avoid introducing // style comments, we don't 
use them in osmo-tetra (or the GSM related projects).  It's not a critical 
requirement, but I would appreciate it.


https://gerrit.osmocom.org/#/c/2817/1/src/phy/tetra_burst.c
File src/phy/tetra_burst.c:

Line 305: #if 0 /* not used */
not used by whom? why? I think more context is needed here.  also, it seems 
like an unrelated change to DMO support, right? If so, please have separate 
patch explaining rationale in commitlog.


Line 367:   break;  
extra whitspace at end of break


https://gerrit.osmocom.org/#/c/2817/1/src/phy/tetra_burst_sync.c
File src/phy/tetra_burst_sync.c:

Line 147:   case (TETRA_TRAIN_NORM_1): /* fall through */
why do we add patenthesis here? The other cases work without?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa5521d7313595384e74dd790a56550755b93fe9
Gerrit-PatchSet: 1
Gerrit-Project: osmo-tetra
Gerrit-Branch: laforge/sq5bpf-rebase-20161218
Gerrit-Owner: allesklar2 
Gerrit-Reviewer: Harald Welte 
Gerrit-HasComments: Yes