[5/5] Change internal API for consistency

Submitted by msuraev@sysmocom.de on April 21, 2016, 12:35 p.m.

Details

Message ID 1461242157-14223-5-git-send-email-msuraev@sysmocom.de
State New
Series "Series without cover letter"
Headers show

Commit Message

msuraev@sysmocom.de April 21, 2016, 12:35 p.m.
From: Max <msuraev@sysmocom.de>

Use uint8_t for TRX numbering everywhere (we don't expect hardware with
more than 256 transceivers in the near future). This change helps to
avoid unnecessary casts and make API much clearer.
---
 src/osmo-bts-sysmo/sysmo_l1_if.c | 8 ++++----
 src/osmo-bts-sysmo/sysmo_l1_if.h | 2 +-
 src/pcu_l1_if.cpp                | 9 ++++-----
 3 files changed, 9 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/osmo-bts-sysmo/sysmo_l1_if.c b/src/osmo-bts-sysmo/sysmo_l1_if.c
index c7c54dd..4bfcecd 100644
--- a/src/osmo-bts-sysmo/sysmo_l1_if.c
+++ b/src/osmo-bts-sysmo/sysmo_l1_if.c
@@ -155,7 +155,7 @@  static int handle_ph_readytosend_ind(struct femtol1_hdl *fl1h,
 	switch (rts_ind->sapi) {
 	case GsmL1_Sapi_Pdtch:
 	case GsmL1_Sapi_Pacch:
-		rc = pcu_rx_rts_req_pdtch((long)fl1h->priv, rts_ind->u8Tn,
+		rc = pcu_rx_rts_req_pdtch(fl1h->trx, rts_ind->u8Tn,
 			rts_ind->u16Arfcn, rts_ind->u32Fn, rts_ind->u8BlockNbr);
 	case GsmL1_Sapi_Ptcch:
 		// FIXME
@@ -215,7 +215,7 @@  static int handle_ph_data_ind(struct femtol1_hdl *fl1h,
 			!= GsmL1_PdtchPlType_Full)
 			break;
 		/* PDTCH / PACCH frame handling */
-		pcu_rx_data_ind_pdtch((long)fl1h->priv, data_ind->u8Tn,
+		pcu_rx_data_ind_pdtch(fl1h->trx, data_ind->u8Tn,
 			data_ind->msgUnitParam.u8Buffer + 1,
 			data_ind->msgUnitParam.u8Size - 1,
 			data_ind->u32Fn,
@@ -357,7 +357,7 @@  int l1if_pdch_req(void *obj, uint8_t ts, int is_ptcch, uint32_t fn,
 	return 0;
 }
 
-void *l1if_open_pdch(void *priv, uint32_t hlayer1, struct gsmtap_inst *gsmtap)
+void *l1if_open_pdch(uint8_t trx, uint32_t hlayer1, struct gsmtap_inst *gsmtap)
 {
 	struct femtol1_hdl *fl1h;
 	int rc;
@@ -367,7 +367,7 @@  void *l1if_open_pdch(void *priv, uint32_t hlayer1, struct gsmtap_inst *gsmtap)
 		return NULL;
 
 	fl1h->hLayer1 = hlayer1;
-	fl1h->priv = priv;
+	fl1h->trx = trx;
 	fl1h->clk_cal = 0;
 	/* default clock source: OCXO */
 	fl1h->clk_src = SuperFemto_ClkSrcId_Ocxo;
diff --git a/src/osmo-bts-sysmo/sysmo_l1_if.h b/src/osmo-bts-sysmo/sysmo_l1_if.h
index 6b50d4e..83ca481 100644
--- a/src/osmo-bts-sysmo/sysmo_l1_if.h
+++ b/src/osmo-bts-sysmo/sysmo_l1_if.h
@@ -38,7 +38,7 @@  struct femtol1_hdl {
 	struct gsmtap_inst *gsmtap;
 	uint32_t gsmtap_sapi_mask;
 
-	void *priv;			/* user reference */
+	uint8_t trx;
 
 	struct osmo_timer_list alive_timer;
 	unsigned int alive_prim_cnt;
diff --git a/src/pcu_l1_if.cpp b/src/pcu_l1_if.cpp
index 67272ab..2acf8ce 100644
--- a/src/pcu_l1_if.cpp
+++ b/src/pcu_l1_if.cpp
@@ -44,7 +44,7 @@  extern "C" {
 
 // FIXME: move this, when changed from c++ to c.
 extern "C" {
-void *l1if_open_pdch(void *priv, uint32_t hlayer1, struct gsmtap_inst *gsmtap);
+void *l1if_open_pdch(uint8_t trx, uint32_t hlayer1, struct gsmtap_inst *gsmtap);
 int l1if_connect_pdch(void *obj, uint8_t ts);
 int l1if_pdch_req(void *obj, uint8_t ts, int is_ptcch, uint32_t fn,
         uint16_t arfcn, uint8_t block_nr, uint8_t *data, uint8_t len);
@@ -330,9 +330,8 @@  static int pcu_rx_info_ind(struct gsm_pcu_if_info_ind *info_ind)
 	struct gprs_bssgp_pcu *pcu;
 	struct gprs_rlcmac_pdch *pdch;
 	struct in_addr ia;
-	int rc = 0;
-	int trx, ts;
-	int i;
+	int rc = 0, ts, i;
+	uint8_t trx;
 
 	if (info_ind->version != PCU_IF_VERSION) {
 		fprintf(stderr, "PCU interface version number of BTS (%d) is "
@@ -450,7 +449,7 @@  bssgp_failed:
 				info_ind->trx[trx].hlayer1);
 				if (!bts->trx[trx].fl1h)
 					bts->trx[trx].fl1h = l1if_open_pdch(
-						(void *)trx,
+						trx,
 						info_ind->trx[trx].hlayer1,
 						bts->gsmtap);
 			if (!bts->trx[trx].fl1h) {

Comments

Neels Hofmeyr April 26, 2016, 11:06 a.m.
On Thu, Apr 21, 2016 at 02:35:57PM +0200, msuraev@sysmocom.de wrote:
> -	void *priv;			/* user reference */
> +	uint8_t trx;

So I presume you are certain that priv was so far only used for the trx
number, that there are no other callers using priv, and that it makes
sense to not have a priv pointer anymore?

Looking at the patch it does look like priv was never really a priv in the
arbitrary-backpointer case, but IMHO it would be good to clarify that in
the log message.

> -	int rc = 0;
> -	int trx, ts;
> -	int i;
> +	int rc = 0, ts, i;

Unrelated cosmetic change, keep that out of this patch, i.e. only remove
the trx int, the point being that I noticed only on the second glance.
Also I believe we prefer to have each variable on its own line.

~Neels
msuraev@sysmocom.de April 26, 2016, 11:14 a.m.
Thanks. Comments are inline.

On 04/26/2016 01:06 PM, Neels Hofmeyr wrote:
> On Thu, Apr 21, 2016 at 02:35:57PM +0200, msuraev@sysmocom.de wrote:
>> -	void *priv;			/* user reference */
>> +	uint8_t trx;
> So I presume you are certain that priv was so far only used for the trx
> number, that there are no other callers using priv, and that it makes
> sense to not have a priv pointer anymore?

Yes.

> Looking at the patch it does look like priv was never really a priv in the
> arbitrary-backpointer case, but IMHO it would be good to clarify that in
> the log message.
>
>> -	int rc = 0;
>> -	int trx, ts;
>> -	int i;
>> +	int rc = 0, ts, i;
> Unrelated cosmetic change, keep that out of this patch, i.e. only remove
> the trx int, the point being that I noticed only on the second glance.

Noted.

> Also I believe we prefer to have each variable on its own line.
>
It doesn't look that way from looking at the other code. It also
decreases readability by cluttering the code with useless repetitions.
Neels Hofmeyr April 28, 2016, 11:31 a.m.
On Tue, Apr 26, 2016 at 01:14:54PM +0200, Max wrote:
> > Also I believe we prefer to have each variable on its own line.
> >
> It doesn't look that way from looking at the other code. It also
> decreases readability by cluttering the code with useless repetitions.

but removing/adding variables where there are more than one per line makes
the patches less readable, and potentially introduces more merge conflicts
than necessary...

I used to be against one-per-line but have changed my opinion quite some
time ago :)

~Neels