Attention is currently required from: arehbein.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/34671?usp=email )
Change subject: stream (test): Fix Coverity CID 323456
......................................................................
Patch Set 1:
(1 comment)
File tests/stream/stream_test.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/34671/comment/dca0e02b_b5fdde32
PS1, Line 681: if (5 < ipa_msg_type) {
> I thought about it, but then I also thought somebody else might complain that leaving it the way it […]
feel free to change it if you want, it shouldn't have been in the other way before anyway imho.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/34671?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I2efb28feae4d4fa7516702f01026af09aa3777ac
Gerrit-Change-Number: 34671
Gerrit-PatchSet: 1
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 05 Oct 2023 17:09:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: arehbein.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email )
Change subject: Make RLC timing data configurable
......................................................................
Patch Set 2:
(1 comment)
File src/osmo-bsc/net_init.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/5a1a2b04_521c0d30
PS2, Line 51: { .T = 3142, .default_val = 20,
> I don't have a GSM background, let alone ever configured/tuned any GSM network settings. […]
I am (and others) available for questions if you are unsure where to put stuff :)
If you are uncertain, then better raising the topic before writing tons of lines next time may be good.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I2c24110d8c977d6cc74c3c8e77bcc709ad9d2675
Gerrit-Change-Number: 31878
Gerrit-PatchSet: 2
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 05 Oct 2023 17:09:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/34671?usp=email )
Change subject: stream (test): Fix Coverity CID 323456
......................................................................
Patch Set 1:
(1 comment)
File tests/stream/stream_test.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/34671/comment/67f9a54b_46126c34
PS1, Line 681: if (5 < ipa_msg_type) {
> this syntax for comparision is a bit weird imho. I'd say we usually have the vars on the left side. […]
I thought about it, but then I also thought somebody else might complain that leaving it the way it is would make reading the diff more easy, so I left it the way it was.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/34671?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I2efb28feae4d4fa7516702f01026af09aa3777ac
Gerrit-Change-Number: 34671
Gerrit-PatchSet: 1
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 05 Oct 2023 17:04:52 +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.
arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email )
Change subject: Make RLC timing data configurable
......................................................................
Patch Set 2:
(1 comment)
File src/osmo-bsc/net_init.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/aa4fc799_e07c215a
PS2, Line 51: { .T = 3142, .default_val = 20,
> > So, how do I know which timers are BTS-specific or depend on BTS support? […]
I don't have a GSM background, let alone ever configured/tuned any GSM network settings. These timers touch different kinds of procedures described across different specs, too, I would guess.
So while it may or may not be easy to do so once I've acquired the right knowledge to make these decisions, it will probably be quite time consuming for me to read up on all of that.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I2c24110d8c977d6cc74c3c8e77bcc709ad9d2675
Gerrit-Change-Number: 31878
Gerrit-PatchSet: 2
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 05 Oct 2023 17:02:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: arehbein.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/34671?usp=email )
Change subject: stream (test): Fix Coverity CID 323456
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
File tests/stream/stream_test.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/34671/comment/a4d7729e_1e75ea35
PS1, Line 681: if (5 < ipa_msg_type) {
this syntax for comparision is a bit weird imho. I'd say we usually have the vars on the left side.
That's basically: "if (ipa_msg_type > 5)" right?
not criticial though.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/34671?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I2efb28feae4d4fa7516702f01026af09aa3777ac
Gerrit-Change-Number: 34671
Gerrit-PatchSet: 1
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 05 Oct 2023 16:54:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: arehbein.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email )
Change subject: Make RLC timing data configurable
......................................................................
Patch Set 2:
(2 comments)
File src/osmo-bsc/net_init.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/0b3ff55e_64a5f16d
PS2, Line 51: { .T = 3142, .default_val = 20,
> So, how do I know which timers are BTS-specific or depend on BTS support?
Easy procedure: Would it make sense for a user/operator to set different values per BTS? Answer: yes! => per bts.
Wuthout looking in detail, some of thse values clearly could be tunned different on different BTS, with different operation constrains, different volume of users, different load, etc.
I think to have one group named "bts" should be enough, even if some bts don't support setting some of the params.
File tests/nanobts_omlattr/nanobts_omlattr_test.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/4931c325_0debbf22
PS2, Line 228: 0x02, 0x00, 0x02, 0xa3, 0x00, 0x09,
> No default values have been changed. […]
ah ok. Can you at least take the chance to align them to the same column?
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I2c24110d8c977d6cc74c3c8e77bcc709ad9d2675
Gerrit-Change-Number: 31878
Gerrit-PatchSet: 2
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 05 Oct 2023 16:51:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email )
Change subject: Make RLC timing data configurable
......................................................................
Patch Set 2:
(4 comments)
File include/osmocom/bsc/gsm_data.h:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/2a852ad1_4b2065fe
PS2, Line 50: #define GSM_N3101_STRICT_LOWER_BOUND 8UL
> STRICT_LOWER_BOUND means simply "MIN" ? :D
Well, no,
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/80fbf532_679c8459
PS2, Line 50: #define GSM_N3101_STRICT_LOWER_BOUND 8UL
> STRICT_LOWER_BOUND means simply "MIN" ? :D
The spec states that 'N3101 > 8', so it isn't a minimum (it's not an allowed value).
I put the strict lower bound and not the minimum as a define, because that's how it's stated in the spec.
File src/osmo-bsc/net_init.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/d1a74a6b_efb0cb0c
PS2, Line 51: { .T = 3142, .default_val = 20,
> I know tons of parameters are already added here, but we should not continue adding BTS-specific par […]
So, how do I know which timers are BTS-specific or depend on BTS support?
I first thought the groupings suggested on https://www.rfwireless-world.com/Terminology/GSM-timers.html (GSM Timers, GSM Timers Network Side(BTS), ...) might make sense, but T3142 isn't listed there as a BTS timer, it's listed as a timer for the mobile side (MSC I guess).
I was thinking about adding a group named "bts". Or am I supposed to add several groups for different kinds of BTSs/
File tests/nanobts_omlattr/nanobts_omlattr_test.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/623669e9_742473c6
PS2, Line 228: 0x02, 0x00, 0x02, 0xa3, 0x00, 0x09,
> why is this changing? are you changing the default values?
No default values have been changed. I just added changed the paragraph formatting a bit and added a comment line so that if anyone else reads the code in the future, it's possible to read which octet contains which information without having to go through the entire code.
It shouldn't be necessary for anybody to lose time over looking up this information again, at least not for that line.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I2c24110d8c977d6cc74c3c8e77bcc709ad9d2675
Gerrit-Change-Number: 31878
Gerrit-PatchSet: 2
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 05 Oct 2023 16:35:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment