Attention is currently required from: jolly.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33674 )
Change subject: ASCI: Add option to switch on or off ASCI support
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
File src/libmsc/msc_a.c:
https://gerrit.osmocom.org/c/osmo-msc/+/33674/comment/4531ea71_152475d9
PS2, Line 2088: struct msc_a *msc_a
(
rather add a struct gsm_network * arg. Nothing in this function uses an msc_a->* member.
...or rather even only a 'bool asci_enable' arg?
some years ago i opposed it, but now my opinion is that a single global gsmnet pointer is a good idea. we have bsc_gsmnet in osmo-bsc, i think no such thing in osmo-msc ... yet. Maybe we can add that now, and then not need any arg at all here.
Up to you, not critical.
)
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33674
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Id68deb69f7395f0f8f50b3820e9d51052a34f753
Gerrit-Change-Number: 33674
Gerrit-PatchSet: 2
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Comment-Date: Thu, 13 Jul 2023 01:03:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: jolly.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33504 )
Change subject: ASCI: Add function to receive VGCS/VBS messages from BSS
......................................................................
Patch Set 14:
(2 comments)
File src/libmsc/ran_peer.c:
https://gerrit.osmocom.org/c/osmo-msc/+/33504/comment/e38abce0_8f331f33
PS14, Line 396:
so below, there is a priority: 1.msc_role 2.vgcs.bss 3.vgcs.cell.
When there is an active vgcs.bss connection, is it possible that something also adds an msc_role to the same conn, and then that hides the active VGCS from incoming messages, because msc_role outranks vgcs.bss?
I'm asking, because there are mechanisms where a subscriber re-uses an existing connection. For example, if some CM Service is already active (say USSD) and then also a call starts, then it opens another transaction on the same conn.
Also a Call Re-Establishment does some more aggressive re-using of transactions and conns.
Can this kind of thing happen at all, e.g. the active talker person happens to receive an SMS at the same time, and while the SMS is handled, an important VGCS message is lost?
https://gerrit.osmocom.org/c/osmo-msc/+/33504/comment/f5bd4f59_ff26aa55
PS14, Line 413: return
(now redundant)
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33504
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Ie4a2f19ba75140a6f2de02b709597239c01f02a2
Gerrit-Change-Number: 33504
Gerrit-PatchSet: 14
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Comment-Date: Thu, 13 Jul 2023 00:51:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: jolly, laforge, dexter.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33511 )
Change subject: ASCI: Add call control for VGCS/VBS
......................................................................
Patch Set 14:
(11 comments)
File src/libmsc/msc_vgcs.c:
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/3eb64441_587e42ec
PS14, Line 134: static char string[9];
none of these are critical, but i'd still like to mention them:
Since it is only one format string, it might be implemented as a #define GROUP_ID_FMT "%08u"
In case of a function,
i'd use the following pattern because it is very flexible:
int gsm44068_group_id_str_buf(char *buf, size_t buflen, uint32_t callref)
{
struct osmo_strbuf sb = { .buf = buf, .len = buflen };
OSMO_STRBUF_PRINTF(sb, "%08u", callref);
return sb.chars_needed;
}
- can be used for static alloc
- can be used for dynamic alloc with OSMO_NAME_C_IMPL
- can be used to append to arbitrary buffer position directly
- can be used with OSMO_STRBUF_APPEND()
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/4dbc4771_95119204
PS14, Line 179: OSMO_ASSERT(l3_msg);
this would abort the program on an encoding error?
rather log and return error.
These four lines from ran_a_encode to msgb_free seem to duplicate 1:1 many times in this file, put them in a static function?
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/59a868dd_a420ec16
PS14, Line 231: *callref = ntohl(*(uint32_t *)ie) >> 5;
maybe better use osmo_load32be() because it also works reliably when the pointer is not on a word boundary
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/71b5db22_ffc6a28b
PS14, Line 801: if (msc_a) {
(early exit pattern: 'if (!msc_a) return;' means less indent. some more candidates for early exit below)
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/e6c8b05f_69d73936
PS14, Line 1628: OSMO_ASSERT(l3_msg);
encoding error: do not abort the program, log and return error
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/577b08a5_6e8d079c
PS14, Line 1666: OSMO_ASSERT
.
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/060c9d51_155c550f
PS14, Line 1727: osmo_fsm_inst_free(bss->fi);
rather use osmo_fsm_inst_term(). It terminates the instance gracefully and then frees it.
(I like avoid doing anything at all after dispatching an event or changing a state to avoid possible use-after-free "loops", but I see BSS_ST_NULL has no onenter() action that could trigger those).
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/c8814bfa_c1dd2f9a
PS14, Line 1730: talloc_free(bss);
(hint, in case you like it too, i like to allocate the struct as a talloc child of the FSM instance, so an osmo_fsm_inst_term() automatically frees the struct, and they are "atomically" removed at the same time without any race conditions possible)
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/4c1901cf_986d9645
PS14, Line 1796: OSMO_ASSERT
.
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/1186fa77_1bee1bdb
PS14, Line 1938: OSMO_ASSERT
.
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/c77a36c0_c57383a6
PS14, Line 1949: OSMO_ASSERT
.
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33511
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I9947403fde8212b66758104443c60aaacc8b1e7b
Gerrit-Change-Number: 33511
Gerrit-PatchSet: 14
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: dexter <pmaier(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 13 Jul 2023 00:27:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: jolly.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33505 )
Change subject: ASCI: Add functions to transcode VGCS/VBS messages on A-interface
......................................................................
Patch Set 11: Code-Review+2
(1 comment)
Patchset:
PS4:
> I put it there, because all other A-interface messages are also there. […]
no, completely fine as it is
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33505
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I6b1f088201e7ef4a58762937855a1d358973882c
Gerrit-Change-Number: 33505
Gerrit-PatchSet: 11
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Comment-Date: Wed, 12 Jul 2023 23:26:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: jolly <andreas(a)eversberg.eu>
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: jolly.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33510 )
Change subject: ASCI: Add simple implementation of Group Call Register
......................................................................
Patch Set 11:
(1 comment)
File src/libmsc/asci_gcr.c:
https://gerrit.osmocom.org/c/osmo-msc/+/33510/comment/84ff1bd7_a68bc7b6
PS11, Line 163: OSMO_ASSERT(l < ARRAY_SIZE(pow10));
can this assertion abort the program based on user input?
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33510
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Ia74a4a865f943c5fb388cd28f9406005c92e663e
Gerrit-Change-Number: 33510
Gerrit-PatchSet: 11
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Comment-Date: Wed, 12 Jul 2023 23:25:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bts/+/33730 )
Change subject: osmo-bts-trx: rx_tchf_fn(): move compute_ber10k() above
......................................................................
osmo-bts-trx: rx_tchf_fn(): move compute_ber10k() above
... for the sake of consistency with rx_tchh_fn().
Change-Id: Ie331da78eb3831e35d255583466e0d09b093b132
---
M src/osmo-bts-trx/sched_lchan_tchf.c
1 file changed, 13 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/30/33730/1
diff --git a/src/osmo-bts-trx/sched_lchan_tchf.c b/src/osmo-bts-trx/sched_lchan_tchf.c
index bdc5140..3973cd0 100644
--- a/src/osmo-bts-trx/sched_lchan_tchf.c
+++ b/src/osmo-bts-trx/sched_lchan_tchf.c
@@ -225,6 +225,8 @@
return -EINVAL;
}
+ ber10k = compute_ber10k(n_bits_total, n_errors);
+
/* average measurements of the last N (depends on mode) bursts */
trx_sched_meas_avg(chan_state, &meas_avg, meas_avg_mode);
/* meas_avg.fn now contains TDMA frame number of the first burst */
@@ -240,8 +242,6 @@
rc = 0; /* this is how we signal BFI to l1sap */
}
- ber10k = compute_ber10k(n_bits_total, n_errors);
-
/* FACCH */
if (rc == GSM_MACBLOCK_LEN) {
_sched_compose_ph_data_ind(l1ts, fn_begin, bi->chan,
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/33730
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ie331da78eb3831e35d255583466e0d09b093b132
Gerrit-Change-Number: 33730
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange