This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
laforge gerrit-no-reply at lists.osmocom.orglaforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/22888 ) Change subject: l1sap: add logging for ACCH repetition ...................................................................... Patch Set 1: (2 comments) I agree with previous feedback. This is tons of logging which nobody ever will want to enable during production. And even during testing, it is logging partially redundant information, as there is not one log line per frame/block, but anywhre from 1..N log lines per FACCH/SACCH block. If we should add some logging, I think it would be best to * consolidate logging into one line per MAC block (== function call?) * consider logging messages on state change (enable/disable) at INFO level and messages about function calls without state change at DEBUG level * finally, make sure that the FACCH repetition state (permission by BSC, current state, thresholds, ...) can be introspected via VTY in "show lchan" https://gerrit.osmocom.org/c/osmo-bts/+/22888/1/src/common/l1sap.c File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/22888/1/src/common/l1sap.c@994 PS1, Line 994: LOGPLCHAN(lchan, DL1C, LOGL_DEBUG, "DL-FACCH repetition: disabled by BSC\n"); > This looks redundant to me, if it's disabled that I would not expect any logging. I think it's useful information, but I think that should be something like part of a "show" command in the VTY (like show lchan). It's not like the BSC would enable/disable it all the time on a given lchan? I think given that this function is called quite frequently (at every facch downlink block?), we should be careful not to log redundant informatoin all the time. https://gerrit.osmocom.org/c/osmo-bts/+/22888/1/src/common/l1sap.c@1001 PS1, Line 1001: LOGPLCHAN(lchan, DL1C, LOGL_DEBUG, "DL-FACCH repetition: inactive\n"); it it was disabled above, then you would never reach here, as there is a 'return' above. If you want to indicate the previous state and the new state durign a decision making process, then the log text should clearly indicate this. Having two lines printed "foobar active" and "foobar active" further below is very hard to understand. I think if at all, there should be one summary log line expressing what happesn (if there was any change). So basically, save the old value, then go through the decision making, compare it with the new value and _if_ it has changed, print a line about the change and ideally also the reason (e.g. by setting a cause_string or log_string variable in the code which is then used by the log statement at the end of the function. -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/22888 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I59d11fd03be3d29fb8a4279d9945b03006764c0e Gerrit-Change-Number: 22888 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pmaier at sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin at sysmocom.de> Gerrit-CC: fixeria <vyanitskiy at sysmocom.de> Gerrit-CC: laforge <laforge at osmocom.org> Gerrit-Comment-Date: Sat, 13 Feb 2021 08:04:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <vyanitskiy at sysmocom.de> Gerrit-MessageType: comment -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210213/d64f0e28/attachment.htm>