<p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If we should add some logging, I think it would be best to</p><ul><li>consolidate logging into one line per MAC block (== function call?)</li><li>consider logging messages on state change (enable/disable) at INFO level and messages about function calls without state change at DEBUG level</li><li>finally, make sure that the FACCH repetition state (permission by BSC, current state, thresholds, ...) can be introspected via VTY in "show lchan"</li><li></li></ul><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/22888">View Change</a></p><p>2 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/22888/1/src/common/l1sap.c">File src/common/l1sap.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/22888/1/src/common/l1sap.c@994">Patch Set #1, Line 994:</a> <code style="font-family:monospace,monospace">         LOGPLCHAN(lchan, DL1C, LOGL_DEBUG, "DL-FACCH repetition: disabled by BSC\n");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This looks redundant to me, if it's disabled that I would not expect any logging.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/22888/1/src/common/l1sap.c@1001">Patch Set #1, Line 1001:</a> <code style="font-family:monospace,monospace">            LOGPLCHAN(lchan, DL1C, LOGL_DEBUG, "DL-FACCH repetition: inactive\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">it it was disabled above, then you would never reach here, as there is a 'return' above.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think if at all, there should be one summary log line expressing what happesn (if there was any change).</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bts/+/22888">change 22888</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-bts/+/22888"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bts </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I59d11fd03be3d29fb8a4279d9945b03006764c0e </div>
<div style="display:none"> Gerrit-Change-Number: 22888 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Sat, 13 Feb 2021 08:04:26 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>