Attention is currently required from: dexter.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/27391 )
Change subject: Revert "mgcp_codec: do not differentiate between oa and bwe when comparing codec"
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
See my comments in the follow up patch. I'm fine with merging this, but in general I'd go for a 2 step/function check:
1- codec is exactly the same (can be forwarded without modification. AMR OA != AMR BWE here)
2- codecs are different but can still be converted/transcoded: (AMR OA vs AMR BWE returns true here).
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/27391
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I0b2854ef2397f38606fab3425be586a3d0ca27d1
Gerrit-Change-Number: 27391
Gerrit-PatchSet: 3
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: msuraev <msuraev(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 24 Mar 2023 12:30:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: falconia, laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/32034 )
Change subject: codec: add osmo_efr_check_sid() function
......................................................................
Patch Set 1:
(3 comments)
File src/codec/gsm660.c:
https://gerrit.osmocom.org/c/libosmocore/+/32034/comment/dfb90092_a029972f
PS1, Line 271: static con
> not a request to change: just a question. […]
If you asked me my bet would be that a local non-static variable would always be allocated in the stack, which means CPU time storing the data. But I'm no expert here on internals so not 100% sure.
In the end it's a tradeoff between extra permanent mem use and CPU time here. I think it makes sense to have it static.
https://gerrit.osmocom.org/c/libosmocore/+/32034/comment/c75a17c6_d5e00661
PS1, Line 272: /* bit numbers relative to "pure" EFR frame beginning,
: * not counting the signature bits. */
: 45, 46, 48, 49, 50, 51, 52, 53, 54, 55,
: 56, 57, 58, 59, 60, 61, 62, 63, 64, 65,
: 66, 67, 68, 94, 95, 96, 98, 99, 100, 101,
: 102, 103, 104, 105, 106, 107, 108, 109, 110, 111,
: 112, 113, 114, 115, 116, 117, 118, 148, 149, 150,
: 151, 152, 153, 154, 155, 156, 157, 158, 159, 160,
: 161, 162, 163, 164, 165, 166, 167, 168, 169, 170,
: 171, 196, 197, 198, 199, 200, 201, 202, 203, 204,
: 205, 206, 207, 208, 209, 212, 213, 214, 215, 216,
: 217, 218, 219, 220, 221 };
> one tab indent too much, at least it looks that way in gerrit
Ack
https://gerrit.osmocom.org/c/libosmocore/+/32034/comment/f6d52ed8_28eb99c1
PS1, Line 293: for (i = 0; i < ARRAY_SIZE(sid_code_word_bits); i++)
In general we always use {} in code blocks with multiple lines in it.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32034
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Iab9fb60028f4135375287bc42f5da7ca7838b5f0
Gerrit-Change-Number: 32034
Gerrit-PatchSet: 1
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: falconia <falcon(a)freecalypso.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Fri, 24 Mar 2023 12:27:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: comment
Attention is currently required from: neels, laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/32045 )
Change subject: logging_vty.c: rename static function
......................................................................
Patch Set 1:
(1 comment)
File src/vty/logging_vty.c:
https://gerrit.osmocom.org/c/libosmocore/+/32045/comment/cb15b968_ff280fb2
PS1, Line 193: logging_prnt_timezone,
: logging_timezone_cmd,
> this newly-introduced command follows the same naming pattern: it has _prnt_ in the name, but not in […]
Ack
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32045
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I5a651e393bd438ea236659ed2c86188a8f9885d1
Gerrit-Change-Number: 32045
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Fri, 24 Mar 2023 12:16:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: comment
Attention is currently required from: neels.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/32043 )
Change subject: logging: add 'logging timezone (localtime|utc)'
......................................................................
Patch Set 1:
(6 comments)
File include/osmocom/core/logging.h:
https://gerrit.osmocom.org/c/libosmocore/+/32043/comment/28d50396_f79e692c
PS1, Line 400: /* Set timezone for logging of timestamps: 0 for local time, 1 for UTC. Other values are reserved for future
0 and 1? don't we have enums for that in this patch?
File src/core/logging.c:
https://gerrit.osmocom.org/c/libosmocore/+/32043/comment/8c03e462_e6ca75d7
PS1, Line 474: return 0;
I'd really return an error if the requested timestamp cannot be obtained.
https://gerrit.osmocom.org/c/libosmocore/+/32043/comment/134537ea_2bd3e264
PS1, Line 528: } else if (target->print_timestamp && get_timestamp(&tv, &tm, target) == 0) {
you are calling get_timestamp twice here every time we log something. You can move get_timestamp in front and call it once.
https://gerrit.osmocom.org/c/libosmocore/+/32043/comment/898e7f8d_7f1c16f1
PS1, Line 874: target->timezone = timezone;
this should return an int and return an error if user wants to set a timezone for which we don't have support (#ifdef HAVE_LOCALTIME_R for instance).
File src/vty/logging_vty.c:
https://gerrit.osmocom.org/c/libosmocore/+/32043/comment/6997ed90_913e13c3
PS1, Line 203: log_set_timezone(tgt, LOG_TIMEZONE_UTC);
in here, chewck the error code returned and if it failed return CMD_WARNING with a logging saying that this timezone is not supported in this system.
File tests/logging/logging_vty_test.vty:
https://gerrit.osmocom.org/c/libosmocore/+/32043/comment/e45dd7d9_932dc607
PS1, Line 75: logging_vty_test# logging timestamp ?
you could add a "logging timezone ?" test here.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32043
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I7f868b47bf8f8dfcf85e735f490ae69b18111af4
Gerrit-Change-Number: 32043
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 24 Mar 2023 12:14:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment