| Message ID | 1458211869-16586-2-git-send-email-msuraev@sysmocom.de |
|---|---|
| State | Accepted |
| Headers | show |
Hi Max,
I have already merged the patch, but now have one more comment that
should definitely be addressed ASAP:
On Thu, Mar 17, 2016 at 11:51:09AM +0100, msuraev@sysmocom.de wrote:
> +struct earfcn {
1) this misses the osmo_ prefix which must be used for all new data
types and symbols in the libraries
2) it is not just an EARFCN (which is an integer), but it is actually
some SI-specific EARFCN parameters. So please let's give it a proper
name. like osmo_earfcn_meas_pars (for measurement parameters) or the
like.
Please provide a follow-up patch quickly so we can resolve this before
any users rely on the old naming. Thanks.
Regards,
Harald
Done - see Fix earfcn struct header <https://patchwork.ozlabs.org/patch/599042/> patch. On 03/17/2016 02:11 PM, Harald Welte wrote: > Hi Max, > > I have already merged the patch, but now have one more comment that > should definitely be addressed ASAP: > > On Thu, Mar 17, 2016 at 11:51:09AM +0100, msuraev@sysmocom.de wrote: >> +struct earfcn { > 1) this misses the osmo_ prefix which must be used for all new data > types and symbols in the libraries > > 2) it is not just an EARFCN (which is an integer), but it is actually > some SI-specific EARFCN parameters. So please let's give it a proper > name. like osmo_earfcn_meas_pars (for measurement parameters) or the > like. > > Please provide a follow-up patch quickly so we can resolve this before > any users rely on the old naming. Thanks. > > Regards, > Harald
> On 17 Mar 2016, at 11:51, msuraev@sysmocom.de wrote: > > +struct earfcn { > + /* EARFCN (16 bits) array */ > + uint16_t *arfcn; Who do you write the comments for? If for a developer, have you checked they turn up in doxygen? > + bool thresh_lo_valid; this breaks the osmo-bts build: http://jenkins.osmocom.org/jenkins/job/osmo-bts/2029/ In file included from sysinfo.c:23: /home/builder/source/workspace/osmo-bts/FIRMWARE_VERSION/master/label/linux_i386_debian_squeeze/deps/libosmocore/../install/include/osmocom/gsm/sysinfo.h:56: error: expected specifier-qualifier-list before 'bool'
On 03/17/2016 03:50 PM, Holger Freyther wrote: >> On 17 Mar 2016, at 11:51, msuraev@sysmocom.de wrote: >> >> +struct earfcn { >> + /* EARFCN (16 bits) array */ >> + uint16_t *arfcn; > > Who do you write the comments for? If for a developer, have you checked they turn up in doxygen? For myself mostly. > > >> + bool thresh_lo_valid; > this breaks the osmo-bts build: > > http://jenkins.osmocom.org/jenkins/job/osmo-bts/2029/ > > In file included from sysinfo.c:23: > /home/builder/source/workspace/osmo-bts/FIRMWARE_VERSION/master/label/linux_i386_debian_squeeze/deps/libosmocore/../install/include/osmocom/gsm/sysinfo.h:56: error: expected specifier-qualifier-list before 'bool' > > > Fixed in "Fix earfcn struct header" patch.
On Thu, Mar 17, 2016 at 03:50:52PM +0100, Holger Freyther wrote:
> > + bool thresh_lo_valid;
why don't we have a bool type though? is there a bool in linux kernel
code?
~Neels
I've forgot to add stdbool.h to header - fixed in separate patch already. On 03/17/2016 04:33 PM, Neels Hofmeyr wrote: > On Thu, Mar 17, 2016 at 03:50:52PM +0100, Holger Freyther wrote: >>> + bool thresh_lo_valid; > why don't we have a bool type though? is there a bool in linux kernel > code? > > ~Neels >
diff --git a/include/osmocom/gsm/sysinfo.h b/include/osmocom/gsm/sysinfo.h index 7d1fe2b..346194e 100644 --- a/include/osmocom/gsm/sysinfo.h +++ b/include/osmocom/gsm/sysinfo.h @@ -3,6 +3,9 @@ #include <osmocom/core/utils.h> #include <osmocom/gsm/protocol/gsm_04_08.h> +#define OSMO_EARFCN_INVALID 666 +#define OSMO_EARFCN_MEAS_INVALID 0xff + enum osmo_sysinfo_type { SYSINFO_TYPE_NONE, SYSINFO_TYPE_1, @@ -32,9 +35,36 @@ enum osmo_sysinfo_type { _MAX_SYSINFO_TYPE }; +struct earfcn { + /* EARFCN (16 bits) array */ + uint16_t *arfcn; + /* Measurement Bandwidth (3 bits), might be absent + (OSMO_EARFCN_MEAS_INVALID is stored in this case) */ + uint8_t *meas_bw; + /* length of arfcn and meas_bw arrays (got to be the same) */ + size_t length; + /* THRESH_E-UTRAN_high (5 bits) */ + uint8_t thresh_hi; + /* THRESH_E-UTRAN_low (5 bits) */ + uint8_t thresh_lo; + /* E-UTRAN_PRIORITY (3 bits) */ + uint8_t prio; + /* E-UTRAN_QRXLEVMIN */ + uint8_t qrxlm; + /* indicates whether thresh_lo value is valid + thresh_hi is mandatory and hence always considered valid */ + bool thresh_lo_valid; + /* indicates whether prio value is valid */ + bool prio_valid; + /* indicates whether qrxlm value is valid */ + bool qrxlm_valid; +}; + typedef uint8_t sysinfo_buf_t[GSM_MACBLOCK_LEN]; extern const struct value_string osmo_sitype_strs[_MAX_SYSINFO_TYPE]; - +int osmo_earfcn_add(struct earfcn *e, uint16_t arfcn, uint8_t meas_bw); +int osmo_earfcn_del(struct earfcn *e, uint16_t arfcn); +void osmo_earfcn_init(struct earfcn *e); uint8_t osmo_sitype2rsl(enum osmo_sysinfo_type si_type); enum osmo_sysinfo_type osmo_rsl2sitype(uint8_t rsl_si); diff --git a/src/gsm/libosmogsm.map b/src/gsm/libosmogsm.map index 7eebe7f..9b6e8ea 100644 --- a/src/gsm/libosmogsm.map +++ b/src/gsm/libosmogsm.map @@ -29,6 +29,10 @@ osmo_sitype_strs; comp128; dbm2rxlev; +osmo_earfcn_add; +osmo_earfcn_del; +osmo_earfcn_init; + gprs_cipher_gen_input_i; gprs_cipher_gen_input_ui; gprs_cipher_load; diff --git a/src/gsm/sysinfo.c b/src/gsm/sysinfo.c index 1408f6b..e4d0ddf 100644 --- a/src/gsm/sysinfo.c +++ b/src/gsm/sysinfo.c @@ -125,6 +125,56 @@ const struct value_string osmo_sitype_strs[_MAX_SYSINFO_TYPE] = { { 0, NULL } }; +/*! \brief Add pair of arfcn and measurement bandwith value to earfcn struct + * \param[in,out] e earfcn struct + * \param[in] arfcn EARFCN value, 16 bits + * \param[in] meas_bw measurement bandwith value + * \returns 0 on success, error otherwise + */ +int osmo_earfcn_add(struct earfcn *e, uint16_t arfcn, uint8_t meas_bw) +{ + size_t i; + for (i = 0; i < e->length; i++) { + if (OSMO_EARFCN_INVALID == e->arfcn[i]) { + e->arfcn[i] = arfcn; + e->meas_bw[i] = meas_bw; + return 0; + } + } + return -ENOMEM; +} + +/*! \brief Delete arfcn (and corresponding measurement bandwith) from earfcn + * struct + * \param[in,out] e earfcn struct + * \param[in] arfcn EARFCN value, 16 bits + * \returns 0 on success, error otherwise + */ +int osmo_earfcn_del(struct earfcn *e, uint16_t arfcn) +{ + size_t i; + for (i = 0; i < e->length; i++) { + if (arfcn == e->arfcn[i]) { + e->arfcn[i] = OSMO_EARFCN_INVALID; + e->meas_bw[i] = OSMO_EARFCN_MEAS_INVALID; + return 0; + } + } + return -ENOENT; +} + +/*! \brief Initialize earfcn struct + * \param[in,out] e earfcn struct + */ +void osmo_earfcn_init(struct earfcn *e) +{ + size_t i; + for (i = 0; i < e->length; i++) { + e->arfcn[i] = OSMO_EARFCN_INVALID; + e->meas_bw[i] = OSMO_EARFCN_MEAS_INVALID; + } +} + uint8_t osmo_sitype2rsl(enum osmo_sysinfo_type si_type) { return sitype2rsl[si_type];
From: Max <msuraev@sysmocom.de> Add structure representing group of EARFCNs with common priority, threshold etc. Add functions to populate this structure. --- include/osmocom/gsm/sysinfo.h | 32 ++++++++++++++++++++++++++- src/gsm/libosmogsm.map | 4 ++++ src/gsm/sysinfo.c | 50 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 1 deletion(-)