Attention is currently required from: pespin, fixeria.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31612 )
Change subject: cosmetic: clarify test_codec_support_bts()
......................................................................
Patch Set 1:
(2 comments)
File src/osmo-bsc/codec_pref.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31612/comment/67eb48cf_c15be85c
PS1, Line 197: switch (pchan) {
> I prefer the new version :)
i always prefer switch() for enum values, because they enforce catching odd values.
https://gerrit.osmocom.org/c/osmo-bsc/+/31612/comment/92a42a84_6521a210
PS1, Line 225: return (bool)bts_codec->efr;
> You probably want to (bool)(!!bts_codec->efr); to make sure values are 1 or 0?
the return value is a bool, so casting to bool is the logic fit, and even that is just syntactic sugar. I guess when assigning to some 1-bit storage, then it should have the foo_bit = (my_bool ? 1 : 0). But it doesn't belong here IMHO. If we start doing that here, we'd have to add these things all over the place where ever bool is used, just in case some caller might assign to a single bit storage... it's the wrong place to do it. ... also who guarantees that !!(x) always returns 0 or 1.
btw i just checked and test_codec_support_bts() is only used in if(), no single bit storage involved.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31612
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I13d9b158d08f4938c5aa47ef3134819a4b1f7d29
Gerrit-Change-Number: 31612
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 01 Mar 2023 22:34:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: neels, pespin, dexter.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/31145 )
Change subject: bts: add IMMEDIATE ASSIGNMENT via PCH transmission
......................................................................
Patch Set 19:
(1 comment)
File src/bts.cpp:
https://gerrit.osmocom.org/c/osmo-pcu/+/31145/comment/499080da_8156c262
PS17, Line 1134: pcu_l1if_tx_pch(bts, immediate_assignment, plen, ms_paging_group(tbf_ms(tbf)));
> I like the idea of supporting both 0x0a and 0x0b in osmo-pcu, that's great. […]
If we go this way (backwards compatibility), we would have to keep both `PCU_IF_MSG_DATA_CNF` and `PCU_IF_MSG_DATA_CNF_DT` as well as both `PCU_IF_SAPI_PCH` and `PCU_IF_SAPI_{AGCH->PCH}_DT` in the protocol definition. There would still be two ways of doing the same thing (sending data over PCH), what renders the idea of bumping the PCUIF version equal to having a flag in the `INFO.ind`. "Here we go again".
We could use this version bump chance to clean up the protocol a bit and make life easier for those who will be working on the PCUIF in the future and trying to understand what message does what.
I don't really want to block getting this work merged, and will better stop adding even more comments/nitpicks. But I really don't see the benefits of making version 0x0b backwards compatible with 0x0a, given that the patch for osmo-bts is already in Gerrit and upgrading osmo-bsc to 0x0b is not going to take too much effort either...
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/31145
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I2a78651593323e8b9627c39918d949a33497b70f
Gerrit-Change-Number: 31145
Gerrit-PatchSet: 19
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 01 Mar 2023 22:25:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31610 )
Change subject: vty: msc / codec-list: forbid duplicate entries
......................................................................
Patch Set 1:
(3 comments)
File src/osmo-bsc/bsc_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31610/comment/3e60d27e_7e4ed6d8
PS1, Line 2730: || argv[i][1] != 'r'
> this indentation looks wrong.
not mine, just moving code around.
https://gerrit.osmocom.org/c/osmo-bsc/+/31610/comment/86a87b73_d47e2fb3
PS1, Line 2731: || (argv[i][0] != 'h' && argv[i][0] != 'f')
> comparing first argv[i][0] and after argv[i][1] is way more "expected" ;)
actually i'd prefer strncmp(argv[i], "fr" resp. "hr", 2)
so i agree, but i'm just moving this code.
https://gerrit.osmocom.org/c/osmo-bsc/+/31610/comment/e1a55faa_d7ba9351
PS1, Line 2740: if (strncmp("hr", argv[i], 2) == 0)
> tmp[i]. […]
still just moving code
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31610
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Ifdc9e04bf1d623da65bfb8a2fddea765601f6d9b
Gerrit-Change-Number: 31610
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 01 Mar 2023 22:25:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin, fixeria.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31609 )
Change subject: simplify storage of bsc_msc_data->audio_support
......................................................................
Patch Set 1:
(2 comments)
File include/osmocom/bsc/bsc_msc_data.h:
https://gerrit.osmocom.org/c/osmo-bsc/+/31609/comment/4de23d10_f582fc35
PS1, Line 140: struct gsm_audio_support audio_support[16];
> you say there's 14 but you put 16 here?
No, i write "Let's just make it:" ... 16; which is larger than 14, and has ample room for users with weird codec-lists. I never like to put the actual nr in the comment, because code changes, and typically the comments fail to change along with the code. (same as writing "for these two reasons:" and then five reasons follow -- just write "for these reasons:" and it won't go stale)
Actually there can't be more than 13, because fr1-fr7 plus hr1-hr7 less hr2 is 13, and soon duplicates will be disallowed. But 16 is a power of two, and 13 has ... idk ... religious meaning or something.
It doesn't really matter... want me to change it??
File src/osmo-bsc/bsc_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31609/comment/c4f150ff_4d96febe
PS1, Line 2729: int
> It's actually a size_t afaiu, so %zu.
no, the compiler complained that it is 'long unsigned int' -- but my guess is that it's architecture dependent? So whichever format I choose, it is bound to be wrong somewhere ... Now that I think of it, that might actually not be the case at all. our ARRAY_SIZE is defined as 'sizeof(..) / sizeof()', so actually I also expect it to be size_t... now I'm confused, why did my compiler say 'long unsigned int'
Casting to (int) is a simple way to be sure it is correct, because i absolutely know that the array size is in int range, and we can skip the discussion. what do you think?
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31609
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I625cedc4bb040d649fd6e1794ba468f4c6ad6adc
Gerrit-Change-Number: 31609
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 01 Mar 2023 22:21:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31607 )
Change subject: bsc_vty.c write_msc(): fix weird printf format
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> any explanation what was that doing and ther ein first place and why don't we want it?
the codec names like 'fr1' have numbers that are supposed to be single digit. Now, the '%.1u' may seem like it is enforcing a single digit, or something; There is obviously no printf format limiting the amount of printed integer digits, because that concept makes no sense. The '%.1u' format only applies to floating point numbers, limiting fractional digits *after* the decimal dot. So it is completely weird and lost and confusing.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31607
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I856c2d620b89efcf3239186ef53c6941577dbccc
Gerrit-Change-Number: 31607
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 01 Mar 2023 22:10:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31604 )
Change subject: cosmetic: use i++ instead of ++i in for loop
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Not sure what's wrong with it, but ok.
It makes no difference, but it makes the reader (me) stop and think hard why on earth the odd '++i' is used there instead of the usual 'i++', as if there were a hidden meaning to it.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31604
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I9903e54e3eb59db9b9cd22e017bc81b9b86e01e9
Gerrit-Change-Number: 31604
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 01 Mar 2023 22:05:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
neels has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31601 )
Change subject: examples: osmo-bsc-minimal.cfg: drop codec-list
......................................................................
examples: osmo-bsc-minimal.cfg: drop codec-list
A new VTY test is coming up that includes testing the default codec-list
setting. The VTY tests are using osmo-bsc-minimal.cfg, so let's not
overwrite the compile time default for codec-list.
Also, there is no need to define a codec-list, so it is actually minimal
to omit it.
Change-Id: I01bee711f21023e2eab0688f45ff68f81afe1831
---
M doc/examples/osmo-bsc/osmo-bsc-minimal.cfg
1 file changed, 16 insertions(+), 1 deletion(-)
Approvals:
fixeria: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/doc/examples/osmo-bsc/osmo-bsc-minimal.cfg b/doc/examples/osmo-bsc/osmo-bsc-minimal.cfg
index 5e1ac11..b4e6e4a 100644
--- a/doc/examples/osmo-bsc/osmo-bsc-minimal.cfg
+++ b/doc/examples/osmo-bsc/osmo-bsc-minimal.cfg
@@ -30,4 +30,3 @@
e1_line 0 driver ipa
msc 0
allow-emergency deny
- codec-list fr1
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31601
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I01bee711f21023e2eab0688f45ff68f81afe1831
Gerrit-Change-Number: 31601
Gerrit-PatchSet: 2
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
Attention is currently required from: fixeria.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31602 )
Change subject: add msc.vty to test 'msc' / 'codec-list' cfg
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> You need to add this new file to `EXTRA_DIST`.
whoa.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31602
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I04ce02dff7cadab826611bd6a0df5596a40578b5
Gerrit-Change-Number: 31602
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 01 Mar 2023 21:56:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment