Change in osmocom-bb[master]: layer23/common: introduce L23SAP API for L1CTL
Vadim Yanitskiy has abandoned this change. ( https://gerrit.osmocom.org/11246 ) Change subject: layer23/common: introduce L23SAP API for L1CTL .. Abandoned -- To view, visit https://gerrit.osmocom.org/11246 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I22d7932ddc03c692f2616726ced53b6e8eef822d Gerrit-Change-Number: 11246 Gerrit-PatchSet: 2 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-CC: Harald Welte
Change in osmocom-bb[master]: layer23/common: introduce L23SAP API for L1CTL
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/11246 ) Change subject: layer23/common: introduce L23SAP API for L1CTL .. Patch Set 2: > I still don't see the value of this, sorry. And actually I think > adding another layer makes the code even harder to read compared > with the existing code. If you are strongly in fasvor, I'll. +2 > thixz series, as you are the most active developer in this area. > Let me know. Well, this patch set doesn't prevent me from implementing something else. So I'll abandon it then. -- To view, visit https://gerrit.osmocom.org/11246 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22d7932ddc03c692f2616726ced53b6e8eef822d Gerrit-Change-Number: 11246 Gerrit-PatchSet: 2 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-CC: Harald Welte Gerrit-Comment-Date: Wed, 31 Oct 2018 07:38:58 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmocom-bb[master]: layer23/common: introduce L23SAP API for L1CTL
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/11246 ) Change subject: layer23/common: introduce L23SAP API for L1CTL .. Patch Set 2: I still don't see the value of this, sorry. And actually I think adding another layer makes the code even harder to read compared with the existing code. If you are strongly in fasvor, I'll. +2 thixz series, as you are the most active developer in this area. Let me know. -- To view, visit https://gerrit.osmocom.org/11246 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22d7932ddc03c692f2616726ced53b6e8eef822d Gerrit-Change-Number: 11246 Gerrit-PatchSet: 2 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-CC: Harald Welte Gerrit-Comment-Date: Tue, 30 Oct 2018 21:23:24 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmocom-bb[master]: layer23/common: introduce L23SAP API for L1CTL
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/11246 ) Change subject: layer23/common: introduce L23SAP API for L1CTL .. Patch Set 2: > I don't really understand what we gain from this. Sure, I can read > your statement and agree that it removes the need to have lapdm.h > included and used. But why is that a problem? > Whar are you trying to fix here? Removing 'lapdm*.h' is not the main idea of this change. The main idea is to make the code a bit more modular and cleaner. Here is a little example: Let's say I am a new developer, and I need to understand how the patch loss criteria is implemented. Where should I look? It's quite odd that I would find it in the L1CTL handling implementation. How is it related to L1CTL?!? Moreover, changing / fixing the logic of the mentioned parts (unrelated to L1CTL itself) would look like changing the L1CTL logic in the git history, because everything is mixed in a single file. -- To view, visit https://gerrit.osmocom.org/11246 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22d7932ddc03c692f2616726ced53b6e8eef822d Gerrit-Change-Number: 11246 Gerrit-PatchSet: 2 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-CC: Harald Welte Gerrit-Comment-Date: Mon, 29 Oct 2018 23:53:12 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmocom-bb[master]: layer23/common: introduce L23SAP API for L1CTL
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/11246 ) Change subject: layer23/common: introduce L23SAP API for L1CTL .. Patch Set 2: I don't really understand what we gain from this. Sure, I can read your statement and agree that it removes the need to have lapdm.h included and used. But why is that a problem? Whar are you trying to fix here? -- To view, visit https://gerrit.osmocom.org/11246 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22d7932ddc03c692f2616726ced53b6e8eef822d Gerrit-Change-Number: 11246 Gerrit-PatchSet: 2 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-CC: Harald Welte Gerrit-Comment-Date: Mon, 29 Oct 2018 23:21:19 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmocom-bb[master]: layer23/common: introduce L23SAP API for L1CTL
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/11246 ) Change subject: layer23/common: introduce L23SAP API for L1CTL .. Patch Set 1: > I'm not sure I'm following you here. The existing "Layer2/3" SAP > is the RSL/RSLms SAP boundary on top of LAPDm, as provided by > libosmocore. > > If thre are some helpers missing, fine. But calling that a new SAP > (which is basically 90% only using the existing RSLms SAP) seems > more confusing to me? TBH, I was not sure how to call this new layer properly. The main idea is to clean up and abstract the L1CTL code from the things (e.g. GSMTAP) that are not related to L1CTL at all. How would you call this? :) And do you support the idea of introducing this layer? -- To view, visit https://gerrit.osmocom.org/11246 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22d7932ddc03c692f2616726ced53b6e8eef822d Gerrit-Change-Number: 11246 Gerrit-PatchSet: 1 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-CC: Harald Welte Gerrit-Comment-Date: Sun, 07 Oct 2018 15:13:16 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmocom-bb[master]: layer23/common: introduce L23SAP API for L1CTL
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/11246 ) Change subject: layer23/common: introduce L23SAP API for L1CTL .. Patch Set 1: I'm not sure I'm following you here. The existing "Layer2/3" SAP is the RSL/RSLms SAP boundary on top of LAPDm, as provided by libosmocore. If thre are some helpers missing, fine. But calling that a new SAP (which is basically 90% only using the existing RSLms SAP) seems more confusing to me? -- To view, visit https://gerrit.osmocom.org/11246 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22d7932ddc03c692f2616726ced53b6e8eef822d Gerrit-Change-Number: 11246 Gerrit-PatchSet: 1 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Jenkins Builder (102) Gerrit-CC: Harald Welte Gerrit-Comment-Date: Sun, 07 Oct 2018 09:47:03 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmocom-bb[master]: layer23/common: introduce L23SAP API for L1CTL
Vadim Yanitskiy has uploaded this change for review. (
https://gerrit.osmocom.org/11246
Change subject: layer23/common: introduce L23SAP API for L1CTL
..
layer23/common: introduce L23SAP API for L1CTL
Having a separate abstraction layer between both L1CTL interface
and the upper layers would allow to keep the L1CTL implementation
clean from GSMTAP / LAPDm / measurement specific stuff.
Change-Id: I22d7932ddc03c692f2616726ced53b6e8eef822d
---
M src/host/layer23/include/osmocom/bb/common/Makefile.am
A src/host/layer23/include/osmocom/bb/common/l23sap.h
M src/host/layer23/include/osmocom/bb/common/logging.h
M src/host/layer23/src/common/Makefile.am
M src/host/layer23/src/common/l1ctl.c
A src/host/layer23/src/common/l23sap.c
M src/host/layer23/src/common/logging.c
7 files changed, 134 insertions(+), 51 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/46/11246/1
diff --git a/src/host/layer23/include/osmocom/bb/common/Makefile.am
b/src/host/layer23/include/osmocom/bb/common/Makefile.am
index cd3437e..5a137df 100644
--- a/src/host/layer23/include/osmocom/bb/common/Makefile.am
+++ b/src/host/layer23/include/osmocom/bb/common/Makefile.am
@@ -1,2 +1,2 @@
noinst_HEADERS = l1ctl.h l1l2_interface.h l23_app.h logging.h \
-networks.h gps.h sysinfo.h osmocom_data.h utils.h
+networks.h gps.h sysinfo.h osmocom_data.h utils.h l23sap.h
diff --git a/src/host/layer23/include/osmocom/bb/common/l23sap.h
b/src/host/layer23/include/osmocom/bb/common/l23sap.h
new file mode 100644
index 000..814fb7d
--- /dev/null
+++ b/src/host/layer23/include/osmocom/bb/common/l23sap.h
@@ -0,0 +1,15 @@
+#pragma once
+
+#include
+#include
+
+/* Logical channel link ID */
+#define LID_SACCH 0x40
+#define LID_DEDIC 0x00
+
+#define CHAN_IS_SACCH(link_id) \
+ ((link_id & 0xc0) == LID_SACCH)
+
+int l23sap_data_ind(struct osmocom_ms *ms, struct msgb *msg);
+int l23sap_data_conf(struct osmocom_ms *ms, struct msgb *msg);
+int l23sap_rach_conf(struct osmocom_ms *ms, struct msgb *msg);
diff --git a/src/host/layer23/include/osmocom/bb/common/logging.h
b/src/host/layer23/include/osmocom/bb/common/logging.h
index bf6e6aa..10b2f7f 100644
--- a/src/host/layer23/include/osmocom/bb/common/logging.h
+++ b/src/host/layer23/include/osmocom/bb/common/logging.h
@@ -25,6 +25,7 @@
DMOB,
DPRIM,
DLUA,
+ DL23SAP,
};
extern const struct log_info log_info;
diff --git a/src/host/layer23/src/common/Makefile.am
b/src/host/layer23/src/common/Makefile.am
index b76094c..e1b7b44 100644
--- a/src/host/layer23/src/common/Makefile.am
+++ b/src/host/layer23/src/common/Makefile.am
@@ -3,4 +3,4 @@
noinst_LIBRARIES = liblayer23.a
liblayer23_a_SOURCES = l1ctl.c l1l2_interface.c sap_interface.c \
- logging.c networks.c sim.c sysinfo.c gps.c l1ctl_lapdm_glue.c utils.c
+ logging.c networks.c sim.c sysinfo.c gps.c l1ctl_lapdm_glue.c utils.c
l23sap.c
diff --git a/src/host/layer23/src/common/l1ctl.c
b/src/host/layer23/src/common/l1ctl.c
index 6f4a6d8..8a45ebe 100644
--- a/src/host/layer23/src/common/l1ctl.c
+++ b/src/host/layer23/src/common/l1ctl.c
@@ -43,9 +43,9 @@
#include
#include
+#include
#include
#include
-#include
#include
extern struct gsmtap_inst *gsmtap_inst;
@@ -117,34 +117,21 @@
static int rx_l1_rach_conf(struct osmocom_ms *ms, struct msgb *msg)
{
- struct lapdm_entity *le = >lapdm_channel.lapdm_dcch;
- struct osmo_phsap_prim pp;
- struct l1ctl_info_dl *dl;
-
- if (msgb_l1len(msg) < sizeof(*dl)) {
+ if (msgb_l1len(msg) < sizeof(struct l1ctl_info_dl)) {
LOGP(DL1C, LOGL_ERROR, "RACH CONF MSG too short "
"(len=%u), missing DL info header\n", msgb_l1len(msg));
msgb_free(msg);
return -1;
}
- dl = (struct l1ctl_info_dl *) msg->l1h;
- msg->l2h = msg->l3h = dl->payload;
-
- osmo_prim_init(, SAP_GSM_PH, PRIM_PH_RACH,
- PRIM_OP_CONFIRM, msg);
- pp.u.rach_ind.fn = ntohl(dl->frame_nr);
-
- return lapdm_phsap_up(, le);
+ return l23sap_rach_conf(ms, msg);
}
/* Receive L1CTL_DATA_IND (Data Indication from L1) */
-static int rx_ph_data_ind(struct osmocom_ms *ms, struct msgb *msg)
+static int rx_data_ind(struct osmocom_ms *ms, struct msgb *msg)
{
- struct osmo_phsap_prim pp;
struct l1ctl_info_dl *dl;
struct l1ctl_data_ind *ccch;
- struct lapdm_entity *le;
struct rx_meas_stat *meas = >meas;
uint8_t chan_type, chan_ts, chan_ss;
uint8_t gsmtap_chan_type;
@@ -238,46 +225,22 @@
gsmtap_chan_type, chan_ss, tm.fn, dl->rx_level-110,
dl->snr, ccch->data, sizeof(ccch->data));
- /* determine LAPDm entity based on SACCH or not */
- if (dl->link_id & 0x40)
- le = >lapdm_channel.lapdm_acch;
- else
-
