diff mbox

gsm48: factor out MCC+MNC BCD parsing for re-use in UMTS

Message ID 1458044890-3541-1-git-send-email-nhofmeyr@sysmocom.de
State New
Headers show

Commit Message

Neels Hofmeyr March 15, 2016, 12:28 p.m. UTC 
For 3G, I need a BCD composer/parser similar to gsm48_generate_lai()/
gsm48_decode_lai(). Those functions also handle a trivial extra
member (lac) which I don't need in this way for 3G.

So create new functions to take on the MCC+MNC BCD handling and call those
from gsm48_generate_lai() and gsm48_decode_lai(). In this way, the 3G code
in openbsc can use only the BCD functionality without code duplication.
---
 include/osmocom/gsm/gsm48.h |  3 +++
 src/gsm/gsm48.c             | 55 ++++++++++++++++++++++++++++-----------------
 src/gsm/libosmogsm.map      |  2 ++
 3 files changed, 39 insertions(+), 21 deletions(-)

Comments

Harald Welte March 16, 2016, 10:46 a.m. UTC | #1 
Hi Neels,

On Tue, Mar 15, 2016 at 01:28:10PM +0100, Neels Hofmeyr wrote:
> So create new functions to take on the MCC+MNC BCD handling and call those
> from gsm48_generate_lai() and gsm48_decode_lai(). In this way, the 3G code
> in openbsc can use only the BCD functionality without code duplication.

fine.  As this kind of bit/nibble shifting is something that can easily
get wrong, and we don't have a test case yet, it should be best to add a
test case testing gsm48_generate_lai() first, then perform the
factoring-out and confirm that the test result doesn't change.
Neels Hofmeyr March 16, 2016, 10:40 p.m. UTC | #2 
On Wed, Mar 16, 2016 at 11:46:01AM +0100, Harald Welte wrote:
> Hi Neels,
> 
> On Tue, Mar 15, 2016 at 01:28:10PM +0100, Neels Hofmeyr wrote:
> > So create new functions to take on the MCC+MNC BCD handling and call those
> > from gsm48_generate_lai() and gsm48_decode_lai(). In this way, the 3G code
> > in openbsc can use only the BCD functionality without code duplication.
> 
> fine.  As this kind of bit/nibble shifting is something that can easily
> get wrong, and we don't have a test case yet, it should be best to add a
> test case testing gsm48_generate_lai() first, then perform the
> factoring-out and confirm that the test result doesn't change.

I agree on that. I didn't check hence wasn't aware that no test for
gsm48_generate_lai() exists. Nevertheless, when Holger and I went thru the
patch review, we broke the BCD generation/parsing code on purpose and
actually managed to provoke a test failure with that. So these functions
seem to be tested, at least indirectly. I hope that's sufficient...?

~Neels
diff mbox

Patch

diff --git a/include/osmocom/gsm/gsm48.h b/include/osmocom/gsm/gsm48.h
index 74ac52c..d6e58c2 100644
--- a/include/osmocom/gsm/gsm48.h
+++ b/include/osmocom/gsm/gsm48.h
@@ -39,3 +39,6 @@  void gsm48_parse_ra(struct gprs_ra_id *raid, const uint8_t *buf);
 int gsm48_construct_ra(uint8_t *buf, const struct gprs_ra_id *raid);
 
 int gsm48_number_of_paging_subchannels(struct gsm48_control_channel_descr *chan_desc);
+
+void gsm48_mcc_mnc_to_bcd(uint8_t *bcd_dst, uint16_t mcc, uint16_t mnc);
+void gsm48_mcc_mnc_from_bcd(uint8_t *bcd_src, uint16_t *mcc, uint16_t *mnc);
diff --git a/src/gsm/gsm48.c b/src/gsm/gsm48.c
index 60937cb..d0a2286 100644
--- a/src/gsm/gsm48.c
+++ b/src/gsm/gsm48.c
@@ -278,25 +278,50 @@  static void to_bcd(uint8_t *bcd, uint16_t val)
 	val = val / 10;
 }
 
-void gsm48_generate_lai(struct gsm48_loc_area_id *lai48, uint16_t mcc,
-			uint16_t mnc, uint16_t lac)
+/* Convert given mcc and mnc to BCD and write to *bcd_dst, which must be an
+ * allocated buffer of (at least) 3 bytes length. */
+void gsm48_mcc_mnc_to_bcd(uint8_t *bcd_dst, uint16_t mcc, uint16_t mnc)
 {
 	uint8_t bcd[3];
 
 	to_bcd(bcd, mcc);
-	lai48->digits[0] = bcd[0] | (bcd[1] << 4);
-	lai48->digits[1] = bcd[2];
+	bcd_dst[0] = bcd[0] | (bcd[1] << 4);
+	bcd_dst[1] = bcd[2];
 
 	to_bcd(bcd, mnc);
 	/* FIXME: do we need three-digit MNC? See Table 10.5.3 */
 	if (mnc > 99) {
-		lai48->digits[1] |= bcd[2] << 4;
-		lai48->digits[2] = bcd[0] | (bcd[1] << 4);
+		bcd_dst[1] |= bcd[2] << 4;
+		bcd_dst[2] = bcd[0] | (bcd[1] << 4);
+	} else {
+		bcd_dst[1] |= 0xf << 4;
+		bcd_dst[2] = bcd[1] | (bcd[2] << 4);
+	}
+}
+
+/* Convert given 3-byte BCD buffer to integers and write results to *mcc and
+ * *mnc. The first three BCD digits result in the MCC and the remaining ones in
+ * the MNC. */
+void gsm48_mcc_mnc_from_bcd(uint8_t *bcd_src, uint16_t *mcc, uint16_t *mnc)
+{
+	*mcc = (bcd_src[0] & 0x0f) * 100
+	     + (bcd_src[0] >> 4) * 10
+	     + (bcd_src[1] & 0x0f);
+
+	if ((bcd_src[1] & 0xf0) == 0xf0) {
+		*mnc = (bcd_src[2] & 0x0f) * 10
+		     + (bcd_src[2] >> 4);
 	} else {
-		lai48->digits[1] |= 0xf << 4;
-		lai48->digits[2] = bcd[1] | (bcd[2] << 4);
+		*mnc = (bcd_src[2] & 0x0f) * 100
+		     + (bcd_src[2] >> 4) * 10
+		     + (bcd_src[1] >> 4);
 	}
+}
 
+void gsm48_generate_lai(struct gsm48_loc_area_id *lai48, uint16_t mcc,
+			uint16_t mnc, uint16_t lac)
+{
+	gsm48_mcc_mnc_to_bcd(&lai48->digits[0], mcc, mnc);
 	lai48->lac = htons(lac);
 }
 
@@ -304,20 +329,8 @@  void gsm48_generate_lai(struct gsm48_loc_area_id *lai48, uint16_t mcc,
 int gsm48_decode_lai(struct gsm48_loc_area_id *lai, uint16_t *mcc,
 		     uint16_t *mnc, uint16_t *lac)
 {
-	*mcc = (lai->digits[0] & 0x0f) * 100
-	     + (lai->digits[0] >> 4) * 10
-	     + (lai->digits[1] & 0x0f);
-
-	if ((lai->digits[1] & 0xf0) == 0xf0) {
-		*mnc = (lai->digits[2] & 0x0f) * 10
-		     + (lai->digits[2] >> 4);
-	} else {
-		*mnc = (lai->digits[2] & 0x0f) * 100
-		     + (lai->digits[2] >> 4) * 10
-		     + (lai->digits[1] >> 4);
-	}
+	gsm48_mcc_mnc_from_bcd(&lai->digits[0], mcc, mnc);
 	*lac = ntohs(lai->lac);
-
 	return 0;
 }
 
diff --git a/src/gsm/libosmogsm.map b/src/gsm/libosmogsm.map
index 7eebe7f..0aeefbb 100644
--- a/src/gsm/libosmogsm.map
+++ b/src/gsm/libosmogsm.map
@@ -138,6 +138,8 @@  gsm48_number_of_paging_subchannels;
 gsm48_parse_ra;
 gsm48_rr_att_tlvdef;
 gsm48_mi_type_name;
+gsm48_mcc_mnc_to_bcd;
+gsm48_mcc_mnc_from_bcd;
 
 gsm_7bit_decode;
 gsm_7bit_decode_ussd;