[XL] Change in osmocom-bb[master]: ASCI: Add group receive and transmit mode support to RR layer
Jenkins Builder has posted comments on this change. (
https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email )
Change subject: ASCI: Add group receive and transmit mode support to RR layer
..
Patch Set 1:
(5 comments)
File src/host/layer23/src/mobile/gsm48_rr.c:
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-11164):
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/0a85918b_6c805ed3
PS1, Line 510: while((msg = msgb_dequeue(&rr->downqueue)))
space required before the open parenthesis '('
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-11164):
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/181b3ab1_a88dd160
PS1, Line 1529: if (bitvec_get_bit_high(&bv) == H) {
braces {} are not necessary for single statement blocks
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-11164):
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/47e0d30d_0665a654
PS1, Line 1963: /* Note: Other informations are not used. */
'informations' may be misspelled - perhaps 'information'?
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-11164):
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/2be13072_755c2368
PS1, Line 5501: reject:
labels should not be indented
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-11164):
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/a15c330a_469edcb7
PS1, Line 6389: /* Leave uplink, wait for uplink beeing free, channel
release or channel/link failure. */
'beeing' may be misspelled - perhaps 'being'?
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Ia55c182a1b4cde777a0e16dcfe0cd32e0f2e38eb
Gerrit-Change-Number: 34491
Gerrit-PatchSet: 1
Gerrit-Owner: jolly
Gerrit-CC: Jenkins Builder
Gerrit-Comment-Date: Thu, 21 Sep 2023 09:39:31 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
[XL] Change in osmocom-bb[master]: ASCI: Add group receive and transmit mode support to RR layer
Attention is currently required from: jolly. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified-1 by Jenkins Builder Change subject: ASCI: Add group receive and transmit mode support to RR layer .. ASCI: Add group receive and transmit mode support to RR layer Related: OS#5364 Change-Id: Ia55c182a1b4cde777a0e16dcfe0cd32e0f2e38eb --- M src/host/layer23/include/osmocom/bb/common/settings.h M src/host/layer23/include/osmocom/bb/mobile/gsm48_rr.h M src/host/layer23/src/mobile/gsm48_rr.c 3 files changed, 1,386 insertions(+), 84 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/91/34491/2 -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Ia55c182a1b4cde777a0e16dcfe0cd32e0f2e38eb Gerrit-Change-Number: 34491 Gerrit-PatchSet: 2 Gerrit-Owner: jolly Gerrit-Reviewer: Jenkins Builder Gerrit-Attention: jolly Gerrit-MessageType: newpatchset
[XL] Change in osmocom-bb[master]: ASCI: Add group receive and transmit mode support to RR layer
Attention is currently required from: jolly.
Jenkins Builder has posted comments on this change. (
https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email )
Change subject: ASCI: Add group receive and transmit mode support to RR layer
..
Patch Set 2:
(5 comments)
File src/host/layer23/src/mobile/gsm48_rr.c:
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-11187):
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/3c4cbcfc_3896902e
PS2, Line 510: while((msg = msgb_dequeue(&rr->downqueue)))
space required before the open parenthesis '('
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-11187):
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/a70ff070_d9203d79
PS2, Line 1529: if (bitvec_get_bit_high(&bv) == H) {
braces {} are not necessary for single statement blocks
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-11187):
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/7e4c8b7a_3a849f2a
PS2, Line 1963: /* Note: Other informations are not used. */
'informations' may be misspelled - perhaps 'information'?
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-11187):
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/55485bd5_cac4ac8e
PS2, Line 5501: reject:
labels should not be indented
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-11187):
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/286eef42_178bfe11
PS2, Line 6389: /* Leave uplink, wait for uplink beeing free, channel
release or channel/link failure. */
'beeing' may be misspelled - perhaps 'being'?
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Ia55c182a1b4cde777a0e16dcfe0cd32e0f2e38eb
Gerrit-Change-Number: 34491
Gerrit-PatchSet: 2
Gerrit-Owner: jolly
Gerrit-Reviewer: Jenkins Builder
Gerrit-Attention: jolly
Gerrit-Comment-Date: Thu, 21 Sep 2023 10:34:54 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
[XL] Change in osmocom-bb[master]: ASCI: Add group receive and transmit mode support to RR layer
Attention is currently required from: jolly. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified-1 by Jenkins Builder Change subject: ASCI: Add group receive and transmit mode support to RR layer .. ASCI: Add group receive and transmit mode support to RR layer Related: OS#5364 Change-Id: Ia55c182a1b4cde777a0e16dcfe0cd32e0f2e38eb --- M src/host/layer23/include/osmocom/bb/common/settings.h M src/host/layer23/include/osmocom/bb/mobile/gsm48_rr.h M src/host/layer23/src/mobile/gsm48_rr.c 3 files changed, 1,385 insertions(+), 84 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/91/34491/3 -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Ia55c182a1b4cde777a0e16dcfe0cd32e0f2e38eb Gerrit-Change-Number: 34491 Gerrit-PatchSet: 3 Gerrit-Owner: jolly Gerrit-Reviewer: Jenkins Builder Gerrit-Attention: jolly Gerrit-MessageType: newpatchset
[XL] Change in osmocom-bb[master]: ASCI: Add group receive and transmit mode support to RR layer
Attention is currently required from: jolly.
pespin has posted comments on this change. (
https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email )
Change subject: ASCI: Add group receive and transmit mode support to RR layer
..
Patch Set 5:
(8 comments)
File src/host/layer23/include/osmocom/bb/mobile/gsm48_rr.h:
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/9a764c4b_b77e84b1
PS5, Line 89: #define GSM48_RR_GST_OFF 0
can we have this as an enum?
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/67d7d1bf_7e586990
PS5, Line 220: /* group call */
what about putting all this inside "gc" substruct?
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/ea586a2e_80cfb3c9
PS5, Line 222: int group_state;/* extension to RR
state for group transmit/receive modes */
can we have this as an enum?
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/911c9204_ca1db00e
PS5, Line 230: int uplink_tries; /* Counts number of
tries to access the uplink. */
if it's a counter, do we need negative values (int)?
File src/host/layer23/src/mobile/gsm48_rr.c:
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/6933043e_360022c8
PS5, Line 73: * GSM48_MM_EVENT_UPLINK_BUSY: Notify MM layer about uplink
becoming busy.
I think you added these a few commits before. They should probably be in this
commit instead since this is where you are using them.
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/65122fac_c72b8f4e
PS5, Line 134: static int gsm48_rr_render_ma(struct osmocom_ms *ms, struct
gsm48_rr_cd *cd, uint16_t *ma, uint8_t *ma_len);
I bet some of this pointers can be const.
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/e5bc763e_f1826eea
PS5, Line 414: if (rr->state == state && state != GSM48_RR_ST_IDLE) {
are you sure this is correct' this should be == iiuc, not !=.
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/8da2ae0f_633b05ad
PS5, Line 7080:
we should start thinking about splitting this file into smaller pieces ;)
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Ia55c182a1b4cde777a0e16dcfe0cd32e0f2e38eb
Gerrit-Change-Number: 34491
Gerrit-PatchSet: 5
Gerrit-Owner: jolly
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin
Gerrit-Attention: jolly
Gerrit-Comment-Date: Thu, 21 Sep 2023 14:15:57 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
[XL] Change in osmocom-bb[master]: ASCI: Add group receive and transmit mode support to RR layer
Attention is currently required from: jolly, pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email ) Change subject: ASCI: Add group receive and transmit mode support to RR layer .. Patch Set 5: (1 comment) File src/host/layer23/include/osmocom/bb/mobile/gsm48_rr.h: https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/93099814_1baff3f0 PS5, Line 220: /* group call */ > what about putting all this inside "gc" substruct? Ack -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Ia55c182a1b4cde777a0e16dcfe0cd32e0f2e38eb Gerrit-Change-Number: 34491 Gerrit-PatchSet: 5 Gerrit-Owner: jolly Gerrit-Reviewer: Jenkins Builder Gerrit-CC: fixeria Gerrit-CC: pespin Gerrit-Attention: jolly Gerrit-Attention: pespin Gerrit-Comment-Date: Thu, 21 Sep 2023 22:39:48 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin Gerrit-MessageType: comment
[XL] Change in osmocom-bb[master]: ASCI: Add group receive and transmit mode support to RR layer
Attention is currently required from: fixeria, pespin.
jolly has posted comments on this change. (
https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email )
Change subject: ASCI: Add group receive and transmit mode support to RR layer
..
Patch Set 5:
(8 comments)
File src/host/layer23/include/osmocom/bb/mobile/gsm48_rr.h:
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/d7301427_57bd0550
PS5, Line 89: #define GSM48_RR_GST_OFF 0
> can we have this as an enum?
Done
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/5038e36f_e615c5c6
PS5, Line 220: /* group call */
> Ack
Done
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/82da7549_fe3f184a
PS5, Line 222: int group_state;/* extension to RR
state for group transmit/receive modes */
> can we have this as an enum?
Done
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/9b5fec6e_e5346a10
PS5, Line 230: int uplink_tries; /* Counts number of
tries to access the uplink. */
> if it's a counter, do we need negative values (int)?
Done
File src/host/layer23/src/mobile/gsm48_rr.c:
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/c9afe6b7_93ed519c
PS5, Line 73: * GSM48_MM_EVENT_UPLINK_BUSY: Notify MM layer about uplink
becoming busy.
> I think you added these a few commits before. […]
Only GSM48_MM_EVENT_NOTIFICATION has been added before. These comments describe
the new interface to the process implemented with this patch.
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/ff0008c2_33132097
PS5, Line 134: static int gsm48_rr_render_ma(struct osmocom_ms *ms, struct
gsm48_rr_cd *cd, uint16_t *ma, uint8_t *ma_len);
> I bet some of this pointers can be const.
It is out of the scope of this patch. The functions are not changed, they are
just defined here, because they are used in code above the implementation.
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/ddb252b8_9fbff766
PS5, Line 414: if (rr->state == state && state != GSM48_RR_ST_IDLE) {
> are you sure this is correct' this should be == iiuc, not !=.
It is allowed to change from IDLE to IDLE. The group sub-state may have
changed, but the state itself.
https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/68171800_e120be3e
PS5, Line 7080:
> we should start thinking about splitting this file into smaller pieces ;)
Maybe. This is one layer.
I changed the location of the new functions inside the gsm48_rr.c file. There
is also a group now for all functions releated to uplink control. Also I split
this patch into several smaller patches for easier review.
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Ia55c182a1b4cde777a0e16dcfe0cd32e0f2e38eb
Gerrit-Change-Number: 34491
Gerrit-PatchSet: 5
Gerrit-Owner: jolly
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria
Gerrit-CC: pespin
Gerrit-Attention: fixeria
Gerrit-Attention: pespin
Gerrit-Comment-Date: Tue, 26 Sep 2023 10:37:01 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin
Comment-In-Reply-To: fixeria
Gerrit-MessageType: comment
[XL] Change in osmocom-bb[master]: ASCI: Add group receive and transmit mode support to RR layer
jolly has abandoned this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email ) Change subject: ASCI: Add group receive and transmit mode support to RR layer .. Abandoned This patch has been split. -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Ia55c182a1b4cde777a0e16dcfe0cd32e0f2e38eb Gerrit-Change-Number: 34491 Gerrit-PatchSet: 5 Gerrit-Owner: jolly Gerrit-Reviewer: Jenkins Builder Gerrit-CC: fixeria Gerrit-CC: pespin Gerrit-MessageType: abandon
