pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/18903 )
Change subject: Introduce bts_shutdown FSM ...................................................................... Patch Set 1: (5 comments) https://gerrit.osmocom.org/c/osmo-bts/+/18903/1/include/osmo-bts/bts_shutdown_fsm.h File include/osmo-bts/bts_shutdown_fsm.h: https://gerrit.osmocom.org/c/osmo-bts/+/18903/1/include/osmo-bts/bts_shutdown_fsm.h@29 PS1, Line 29: ST_BTS_SHUTDOWN_NONE > Cosmetic: BTS_SHUTDOWN_ST_* and BTS_SHUTDOWN_EV_* is more usual the ST_*. Not > critical. Ack https://gerrit.osmocom.org/c/osmo-bts/+/18903/1/include/osmo-bts/bts_shutdown_fsm.h@41 PS1, Line 41: bts_shutdown > Rather move this API to bts.{c,h}, so there will be no need to include this > header everywhere. I explicitly moved it here since I prefer having everything shutdown related in one file. I know, we need to export more headers in some files, but let's start splitting stuff into different headers rather than going the gsm_data.h way with kilometric headers. https://gerrit.osmocom.org/c/osmo-bts/+/18903/1/src/common/bts_shutdown_fsm.c File src/common/bts_shutdown_fsm.c: https://gerrit.osmocom.org/c/osmo-bts/+/18903/1/src/common/bts_shutdown_fsm.c@51 PS1, Line 51: st_bts_shutdown_wait_ramp_down_compl_on_enter > Given that it's a static (private) symbol, its name can be much shorter. […] I think it's usual practice using the whole state name as function name for FSM. I'll perhaps drop the "bts_shutdown" part and leave "st_" https://gerrit.osmocom.org/c/osmo-bts/+/18903/1/src/common/bts_shutdown_fsm.c@68 PS1, Line 68: llist_for_each_entry_reverse > Would be nice to add a comment explaining why we doing reverse here. I'm simply moving this loop from the old bts_shutdown() implementation here, so no need to change anything in this patch. https://gerrit.osmocom.org/c/osmo-bts/+/18903/1/src/common/bts_shutdown_fsm.c@130 PS1, Line 130: constructor > Constructors are usually placed at the bottom of the file. I'm rather keeping all FSM internal stuff together and below this all public APIs using it. -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/18903 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I8f48f17e61c3b9b86342eaf5b8a2b1ac9758bde5 Gerrit-Change-Number: 18903 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pes...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-CC: fixeria <vyanits...@sysmocom.de> Gerrit-Comment-Date: Thu, 18 Jun 2020 14:16:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <vyanits...@sysmocom.de> Gerrit-MessageType: comment
