Change in osmo-bsc[master]: refactor lchan counting

historical

neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/25972 )

Change subject: refactor lchan counting
......................................................................

Patch Set 1:

(6 comments)

feedback on my feedback to your feedback is welcome...

https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/include/osmocom/bsc/gsm_data.h 
File include/osmocom/bsc/gsm_data.h:

https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/include/osmocom/bsc/gsm_data.h@1439 
PS1, Line 1439: 	CHAN_COUNTS2_N
> is this simply to flag the end? please document so, since it's a bit confusing as this is an enum en […]
it's to know how large to define the array (N as in number of indexes)

https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/include/osmocom/bsc/gsm_data.h@1442 
PS1, Line 1442: extern const struct value_string chan_count2_strs[];
> Lots of new infra, sounds like all this can be moved to a new header file.
oh i thought i had kept those *str* functions in a separate patch, just needed it for debugging and haven't decided yet whether to commit at all. these shouldn't be here

but ack, maybe a separate .h/.c pair could be nice

https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/src/osmo-bsc/abis_rsl.c 
File src/osmo-bsc/abis_rsl.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/src/osmo-bsc/abis_rsl.c@1961 
PS1, Line 1961: 	chan_counts_t bts_counts;
> we don't tend to use *_t types in osmocom (see kernel programming style guide). […]
well, we usually use 'struct foo' or 'enum foo', but this typedef (used to pinpoint the array sizes) can't be used as 'typedef chan_counts', just 'chan_counts'.

So what I often like to do, name the instance exactly like the type:

  struct foo foo;

is not possible with a typedef like this.
That's why I added the _t. Not harmful, is it?

https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/src/osmo-bsc/abis_rsl.c@1974 
PS1, Line 1974: 	free_tchf = bts_counts[CHAN_COUNTS1_ALL][CHAN_COUNTS2_FREE][GSM_LCHAN_TCH_F];
> I'd rather prefer getters here, this way the array can be kept internal to its own module and no nee […]
To me it all seems super obvious, could it be about improving readability?
Maybe abbreviating CHAN_COUNTS1_ to CC1_ could make it more readable?
(but CC also means Call Control ...)
CHANS1_ CHANS2_ ?

...you mean a getter like this?

  static inline unsigned int chan_counts_get(chan_counts vals,
        enum chan_counts_dim1 dim1, enum chan_counts_dim2 dim2, enum gsm_chan_t dim3)
  {
       return vals[dim1][dim2][dim3];
  }

  free_tchf = chan_counts_get(bts_counts, CHAN_COUNTS1_ALL, CHAN_COUNTS2_FREE, GSM_LCHAN_TCH_F);

Does that help? From typedef chan_counts_t it's obvious which enum values to use in which array dimension??

"some comment here describing what you fetch here"
I fetch literally the count of "ALL FREE TCH_F" as the enum values indicate.
Besides ALL, there could be STATIC or DYNAMIC, as seen in the enum definition, but here i get ALL.
FREE is obvious, TCH_F is obvious ... what is there to explain?

https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/src/osmo-bsc/bts_trx.c 
File src/osmo-bsc/bts_trx.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/src/osmo-bsc/bts_trx.c@327 
PS1, Line 327: void trx_count_lchans(chan_counts_t ret, const struct gsm_bts_trx *trx)
> All this could go into a new counts.c file, to have all this logic in one place. […]
re "all logic in one file" -- who introduced the separation of bts and trx? I dimly remember that this used to be in the same place at some point? The only reason why this is here is because previous bts lchan counting and trx lchan counting code was in these separate places. I assumed that is so for a reason, so rather leave all trx logic in the same file...?

we usually put out-arguments first, right? 'ret' is a returned argument, maybe I should doc that

https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/src/osmo-bsc/gsm_data.c 
File src/osmo-bsc/gsm_data.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/src/osmo-bsc/gsm_data.c@650 
PS1, Line 650:  * TODO: include possible VAMOS secondary lchans? */
> what about this TODO?
wait, this bit of patch may be an unrelated leftover from a previous patch version...

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/25972
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I2fb48c549186db812b1e9d6b735a92e80f27b8d3
Gerrit-Change-Number: 25972
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 27 Oct 2021 20:57:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211027/c20a96e6/attachment.htm>