| Message ID | 1458657612-12669-4-git-send-email-msuraev@sysmocom.de |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 22, 2016 at 03:40:11PM +0100, msuraev@sysmocom.de wrote: > - int is_system_prim, l1if_compl_cb *cb, void *data) > + bool is_system_prim, l1if_compl_cb *cb, void *data) Hey, if Max is using bool, I also want to use bool! The stance a few months ago was that I would be the first to use the bool type instead of int to indicate boolean values. Is there a consensus that using bool is fine? Thanks! ~Neels
Hi Max, please refrain from refactoring litecell15 code without also refactoring the osmo-bts-sysmo code (whihc was used as a template for osmo-bts-litecell15) at the same time, too. We don't want those two to diverge any more than absolutely neccessary. Thanks!
On Tue, Mar 22, 2016 at 04:39:07PM +0100, Neels Hofmeyr wrote:
> Is there a consensus that using bool is fine?
true.
> On 22 Mar 2016, at 18:40, Harald Welte <laforge@gnumonks.org> wrote: > > Hi Max, > > please refrain from refactoring litecell15 code without also refactoring > the osmo-bts-sysmo code (whihc was used as a template for > osmo-bts-litecell15) at the same time, too. We don't want those two to > diverge any more than absolutely neccessary. Thanks! and I prefer a minimal diff even if you change it to if (!is..) else to avoid moving the two branches. holger
Shouldn't we move the code in question to src/common than? On 03/22/2016 06:40 PM, Harald Welte wrote: > Hi Max, > > please refrain from refactoring litecell15 code without also refactoring > the osmo-bts-sysmo code (whihc was used as a template for > osmo-bts-litecell15) at the same time, too. We don't want those two to > diverge any more than absolutely neccessary. Thanks! >
Hi Max,
On Wed, Mar 23, 2016 at 03:44:08PM +0100, Max wrote:
> Shouldn't we move the code in question to src/common than?
No, the code is similar but not identical, and the L1 interfaces of the
underlying PHY are not guaranteed to stay as similar as they are.
So some of the general structure can probably be further common-ized,
but the details regarding the L1 interface primitives will still stay
PHY (bts-model-)specific.
Also, the point is that so far nobody has volunteered to do (or fund)
such an effort.
It is typically relatively easy for companies to take care / ownership /
funding of the driver for their specific hardware, rather it is for
refactoring and general code improvement. This is not specific to
OsmoBTS, but you can also see this quite a lot if you look at Linux
kernel development until fairly recently.
It takes both an interest in long-term effects as well as a deep
understanding about the nature of collaborative free software
development processes to invest in the improvement of the (core/common)
code.
Regards,
Harald
diff --git a/src/osmo-bts-litecell15/l1_if.c b/src/osmo-bts-litecell15/l1_if.c index d89cc29..d810248 100644 --- a/src/osmo-bts-litecell15/l1_if.c +++ b/src/osmo-bts-litecell15/l1_if.c @@ -28,7 +28,7 @@ #include <unistd.h> #include <errno.h> #include <fcntl.h> - +#include <stdbool.h> #include <sys/types.h> #include <sys/stat.h> @@ -96,7 +96,7 @@ static void l1if_req_timeout(void *data) } static int _l1if_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, - int is_system_prim, l1if_compl_cb *cb, void *data) + bool is_system_prim, l1if_compl_cb *cb, void *data) { struct wait_l1_conf *wlc; struct osmo_wqueue *wqueue; @@ -108,23 +108,7 @@ static int _l1if_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, wlc->cb_data = data; /* Make sure we actually have received a REQUEST type primitive */ - if (is_system_prim == 0) { - GsmL1_Prim_t *l1p = msgb_l1prim(msg); - - LOGP(DL1P, LOGL_INFO, "Tx L1 prim %s\n", - get_value_string(lc15bts_l1prim_names, l1p->id)); - - if (lc15bts_get_l1prim_type(l1p->id) != L1P_T_REQ) { - LOGP(DL1C, LOGL_ERROR, "L1 Prim %s is not a Request!\n", - get_value_string(lc15bts_l1prim_names, l1p->id)); - talloc_free(wlc); - return -EINVAL; - } - wlc->is_sys_prim = 0; - wlc->conf_prim_id = lc15bts_get_l1prim_conf(l1p->id); - wqueue = &fl1h->write_q[MQ_L1_WRITE]; - timeout_secs = 30; - } else { + if (is_system_prim) { Litecell15_Prim_t *sysp = msgb_sysprim(msg); LOGP(DL1C, LOGL_INFO, "Tx SYS prim %s\n", @@ -139,9 +123,25 @@ static int _l1if_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, wlc->is_sys_prim = 1; wlc->conf_prim_id = lc15bts_get_sysprim_conf(sysp->id); wqueue = &fl1h->write_q[MQ_SYS_WRITE]; - timeout_secs = 30; + } else { + GsmL1_Prim_t *l1p = msgb_l1prim(msg); + + LOGP(DL1P, LOGL_INFO, "Tx L1 prim %s\n", + get_value_string(lc15bts_l1prim_names, l1p->id)); + + if (lc15bts_get_l1prim_type(l1p->id) != L1P_T_REQ) { + LOGP(DL1C, LOGL_ERROR, "L1 Prim %s is not a Request!\n", + get_value_string(lc15bts_l1prim_names, l1p->id)); + talloc_free(wlc); + return -EINVAL; + } + wlc->is_sys_prim = 0; + wlc->conf_prim_id = lc15bts_get_l1prim_conf(l1p->id); + wqueue = &fl1h->write_q[MQ_L1_WRITE]; } + timeout_secs = 30; + /* enqueue the message in the queue and add wsc to list */ if (osmo_wqueue_enqueue(wqueue, msg) != 0) { /* So we will get a timeout but the log message might help */ @@ -163,13 +163,13 @@ static int _l1if_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, int l1if_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, l1if_compl_cb *cb, void *data) { - return _l1if_req_compl(fl1h, msg, 1, cb, data); + return _l1if_req_compl(fl1h, msg, true, cb, data); } int l1if_gsm_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, l1if_compl_cb *cb, void *data) { - return _l1if_req_compl(fl1h, msg, 0, cb, data); + return _l1if_req_compl(fl1h, msg, false, cb, data); } /* allocate a msgb containing a GsmL1_Prim_t */
From: Max <msuraev@sysmocom.de> Use bool type for boolean values. Make if order more natural. --- src/osmo-bts-litecell15/l1_if.c | 44 ++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 22 deletions(-)