| Message ID | 20140519094847.GV14533@nataraja |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, May 19, 2014 at 11:48:47AM +0200, Harald Welte wrote: > Rename NM_ATT_O_REDUCEPOWER to NM_ATT_OSMO_REDUCEPOWER, which > makes it more clear that this is an osmcoom specific attribute. > > Also, we cannot simply overload 0x01 as an already defined OML > attribute. The problem is quite simple: When we use abis_nm_att_tlvdef > during the TLV parse, 0x01 will match to NM_ATT_ABIS_CHANNEL, > which is defined as { TLV_TYPE_FIXED, 3 }. But we already do have overlap. At least between BS11 and ipaccess. How is it different from GSM to osmocom? We could argue that the vendor values should not be in the gsm enum but sit somewhere else? Can you please elaborate? > I'm using 0xfe for the attribute, as 0xfe doesn't overlap with the IPA > specific attribues (and we might want to combine/merge the 12.21 plus > IPA plus osmocom spefici attributes) typo(s). :)
Hi Holger, On Mon, May 19, 2014 at 01:49:27PM +0200, Holger Hans Peter Freyther wrote: > On Mon, May 19, 2014 at 11:48:47AM +0200, Harald Welte wrote: > > > Rename NM_ATT_O_REDUCEPOWER to NM_ATT_OSMO_REDUCEPOWER, which > > makes it more clear that this is an osmcoom specific attribute. > > > > Also, we cannot simply overload 0x01 as an already defined OML > > attribute. The problem is quite simple: When we use abis_nm_att_tlvdef > > during the TLV parse, 0x01 will match to NM_ATT_ABIS_CHANNEL, > > which is defined as { TLV_TYPE_FIXED, 3 }. > > But we already do have overlap. At least between BS11 and ipaccess. > How is it different from GSM to osmocom? Yes, this is not an issue as e.g. openbsc uses the tlv_def_patch() function to merge the generic 12.21 ones with either the BS11 ones, or the ip.access ones. There is no overlapping in those respective sets. You can of couse not merge all three of them. During the patching, the vendor-specific IEI can override a 12.21 IEI / TLV definition. I think Siemens has in some cases used TV where the spec says TV16 or vice versa. But even if it was the intention of the osmocom IEI to override the NM_ATT_ABIS_CHANNEL, then the code would first have to create a patched tlv definition table, and then use tlv_parse with that patched table, rather than the pristine 12.21 one. > We could argue that the vendor values should not be in the gsm enum > but sit somewhere else? That might be an option, but its a mere difference in style. If they're all in one large enum, one can at least see their values/ranges in one spot. > > I'm using 0xfe for the attribute, as 0xfe doesn't overlap with the IPA > > specific attribues (and we might want to combine/merge the 12.21 plus > > IPA plus osmocom spefici attributes) > > typo(s). :) my apologies. well, as long as people can unserstand what I read, I don't really think it is the most useful way to use my time to re-read everything in order to spot + fix them.
On Mon, May 19, 2014 at 03:44:00PM +0200, Harald Welte wrote: > Yes, this is not an issue as e.g. openbsc uses the tlv_def_patch() > function to merge the generic 12.21 ones with either the BS11 ones, or > the ip.access ones. There is no overlapping in those respective sets. > You can of couse not merge all three of them. During the patching, the > vendor-specific IEI can override a 12.21 IEI / TLV definition. I think > Siemens has in some cases used TV where the spec says TV16 or vice > versa. > > But even if it was the intention of the osmocom IEI to override the > NM_ATT_ABIS_CHANNEL, then the code would first have to create a patched > tlv definition table, and then use tlv_parse with that patched table, > rather than the pristine 12.21 one. My intention of starting with using 0x1 was for the usage in vendor defined messages. I didn't intend to use these values to add IEs to existing GSM 12.21 messages. So maybe this is an argument to move the enum somewhere else. I didn't think of the tlv_ table for this enum and didn't notice it in the review. From my point of view I would favor to move this value into a new enum and introduce a new TLV table for these manufacturer messages?
From 5b5650f3de0213a459b4184bab3ab2d0d833c4a4 Mon Sep 17 00:00:00 2001
From: Harald Welte <laforge@gnumonks.org>
Date: Mon, 19 May 2014 11:25:46 +0200
Subject: [PATCH] Fix introducing osmocom speficic OML attributes
Rename NM_ATT_O_REDUCEPOWER to NM_ATT_OSMO_REDUCEPOWER, which
makes it more clear that this is an osmcoom specific attribute.
Also, we cannot simply overload 0x01 as an already defined OML
attribute. The problem is quite simple: When we use abis_nm_att_tlvdef
during the TLV parse, 0x01 will match to NM_ATT_ABIS_CHANNEL,
which is defined as { TLV_TYPE_FIXED, 3 }.
So instead, we need to introduce a new abis_nm_osmo_att_tlvdef[],
which has to be patched into abis_nm_att_tlvdef[] by the means of
tlv_def_patch(), exactly how we do it for bs-11 and nanobts specific
attributes.
I'm using 0xfe for the attribute, as 0xfe doesn't overlap with the IPA
specific attribues (and we might want to combine/merge the 12.21 plus
IPA plus osmocom spefici attributes)
---
include/osmocom/gsm/protocol/gsm_12_21.h | 4 +++-
src/gsm/abis_nm.c | 7 +++++++
src/gsm/libosmogsm.map | 1 +
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/include/osmocom/gsm/protocol/gsm_12_21.h b/include/osmocom/gsm/protocol/gsm_12_21.h
index b1725d5..ad0890c 100644
--- a/include/osmocom/gsm/protocol/gsm_12_21.h
+++ b/include/osmocom/gsm/protocol/gsm_12_21.h
@@ -487,7 +487,9 @@ enum abis_nm_attr {
NM_ATT_BS11_PLL_MODE = 0xfc,
NM_ATT_BS11_PASSWORD = 0xfd,
- NM_ATT_O_REDUCEPOWER = 0x01,
+ /* osmocom (osmo-bts) specific attributes, used in combination
+ * with the "org.osmocom" manufacturer identification */
+ NM_ATT_OSMO_REDUCEPOWER = 0xfe, /* TLV_TYPE_TV */
};
#define NM_ATT_BS11_FILE_DATA NM_ATT_EVENT_TYPE
diff --git a/src/gsm/abis_nm.c b/src/gsm/abis_nm.c
index 2c23a64..7a1f664 100644
--- a/src/gsm/abis_nm.c
+++ b/src/gsm/abis_nm.c
@@ -323,6 +323,13 @@ const struct tlv_definition abis_nm_att_tlvdef = {
},
};
+/*! \brief org.osmocom GSM A-bis OML TLV parser definition */
+const struct tlv_definition abis_nm_osmo_att_tlvdef = {
+ .def = {
+ [NM_ATT_OSMO_REDUCEPOWER] = { TLV_TYPE_TV },
+ },
+};
+
/*! \brief Human-readable strings for A-bis OML Object Class */
const struct value_string abis_nm_obj_class_names[] = {
{ NM_OC_SITE_MANAGER, "SITE-MANAGER" },
diff --git a/src/gsm/libosmogsm.map b/src/gsm/libosmogsm.map
index 3c5025d..cab4fc4 100644
--- a/src/gsm/libosmogsm.map
+++ b/src/gsm/libosmogsm.map
@@ -10,6 +10,7 @@ abis_nm_event_type_name;
abis_nm_nack_cause_name;
abis_nm_nack_name;
abis_nm_att_tlvdef;
+abis_nm_osmo_att_tlvdef;
abis_nm_obj_class_names;
abis_nm_opstate_name;
abis_nm_nacks;
--
2.0.0.rc2