Change in osmo-bsc[master]: power_control: implement BCCH carrier power reduction operation

neels gerrit-no-reply at lists.osmocom.org
Fri Jul 2 15:52:07 UTC 2021


neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/24777 )

Change subject: power_control: implement BCCH carrier power reduction operation
......................................................................


Patch Set 3:

(15 comments)

https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3//COMMIT_MSG 
Commit Message:

https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3//COMMIT_MSG@14 
PS3, Line 14: However, in the recent 3GPP TS 45.008 there is a feature called
(could be nice to mention the spec version nr)


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3//COMMIT_MSG@30 
PS3, Line 30: A value greater than zero would make osmo-bts reduce the power
what do you mean, "would"?


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/doc/manuals/chapters/power_control.adoc 
File doc/manuals/chapters/power_control.adoc:

https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/doc/manuals/chapters/power_control.adoc@334 
PS3, Line 334: a BTS shall maintain discontinuous Downlink transmission at full power in order to
(are you sure that you don't mean "continuous")


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/doc/manuals/chapters/power_control.adoc@337 
PS3, Line 337: version 13.0.0 (2015-11) - "BCCH carrier power reduction operation".
"in version"


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/doc/manuals/chapters/power_control.adoc@348 
PS3, Line 348: and the simulation results can be found in 3GPP TR 45.926.
("TS"?)


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/doc/manuals/chapters/power_control.adoc@352 
PS3, Line 352: At the moment of writing this manual, the only BTS model that can instructed to
(the usual expression is "at the time of writing")

"that can be instructed"


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/doc/manuals/chapters/power_control.adoc@354 
PS3, Line 354: can be added on request
sounds like it doesn't belong in this manual?

if you need to say this, maybe rather say "may be added in the future", otherwise it sounds like osmocom is offering a free service of implementing things on request


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/doc/manuals/chapters/power_control.adoc@358 
PS3, Line 358: `show bts` command in the VTY interface.
maybe "in OsmoBSC's VTY interface"


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/doc/manuals/chapters/power_control.adoc@363 
PS3, Line 363: There is currently no logic in osmo-bsc for automatic activation and deactivation
(technically "osmo-bsc" is the binary and here it should be OsmoBSC ...)


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/doc/manuals/chapters/power_control.adoc@407 
PS3, Line 407: 3652121201381481804
maybe rather make it "1" ? otherwise it looks like a meaningful important number


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/src/osmo-bsc/bsc_ctrl_commands.c 
File src/osmo-bsc/bsc_ctrl_commands.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/src/osmo-bsc/bsc_ctrl_commands.c@555 
PS3, Line 555: 	const int red = atoi(value);
atoi() is not sufficient to verify proper input. The value could be *anything*, supplied by an external user. atoi() would return some int value on error, e.g. if the user supplies value = "potatoe" or "1.234".

A proper input value checking is found at e.g.
https://git.osmocom.org/libosmocore/tree/src/gsm/gsm23003.c?id=d9825c0a2cfeab682f29167b11aaa154b8a36bae#n498

	errno = 0;
	_mnc = strtol(mnc_str, &endptr, 10);
	if (errno)
		return -errno;
	else if (*endptr)
		return -EINVAL;
	if (_mnc < 0 || _mnc > 999)
		return -ERANGE;

(slightly tweaked the example here to return -errno)


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/src/osmo-bsc/bsc_ctrl_commands.c@584 
PS3, Line 584: 	const int red = atoi(cmd->value);
(here atoi() is fine because the verify function should have verified the value already)


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/src/osmo-bsc/bts_osmobts.c 
File src/osmo-bsc/bts_osmobts.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/src/osmo-bsc/bts_osmobts.c@61 
PS3, Line 61: 	dh = (struct abis_rsl_dchan_hdr *) msgb_put(msg, sizeof(*dh));
(usually no spaces in type cast)


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/src/osmo-bsc/bts_osmobts.c@65 
PS3, Line 65: 	dh->chan_nr = RSL_CHAN_BCCH;
in the commit log you're saying this is abusing the message?
maybe some comment here would be nice to indicate that it is an osmocom specific coding?


https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/tests/osmo-bsc.vty 
File tests/osmo-bsc.vty:

https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/tests/osmo-bsc.vty@49 
PS3, Line 49:   c0-power-reduction             BCCH carrier power reduction operation
i would recommend also adding tests for the parameters in the way done below, e.g. 'bts 0 c0-power-reduction ?' to verify that the argument doc strings are matched up correctly



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/24777
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I047fce33d4d3e4c569dd006ba17858467a2f4783
Gerrit-Change-Number: 24777
Gerrit-PatchSet: 3
Gerrit-Owner: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Fri, 02 Jul 2021 15:52:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210702/355d9478/attachment.htm>


More information about the gerrit-log mailing list