Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/25056 ) Change subject: Replace ul_ass_state with osmocom FSM .. Replace ul_ass_state with osmocom FSM Related: OS#2709 Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 --- M src/Makefile.am M src/encoding.cpp M src/encoding.h M src/gprs_rlcmac_sched.cpp M src/pdch.cpp M src/tbf.cpp M src/tbf.h M src/tbf_ul.cpp M src/tbf_ul.h A src/tbf_ul_ass_fsm.c A src/tbf_ul_ass_fsm.h M tests/alloc/AllocTest.err M tests/tbf/TbfTest.cpp M tests/tbf/TbfTest.err M tests/types/TypesTest.err 15 files changed, 100,530 insertions(+), 208 deletions(-) Approvals: Jenkins Builder: Verified pespin: Looks good to me, approved -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-MessageType: merged
Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/25056 ) Change subject: Replace ul_ass_state with osmocom FSM .. Patch Set 6: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Mon, 23 Aug 2021 16:27:09 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
Hello osmith, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 to look at the new patch set (#6). Change subject: Replace ul_ass_state with osmocom FSM .. Replace ul_ass_state with osmocom FSM Related: OS#2709 Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 --- M src/Makefile.am M src/encoding.cpp M src/encoding.h M src/gprs_rlcmac_sched.cpp M src/pdch.cpp M src/tbf.cpp M src/tbf.h M src/tbf_ul.cpp M src/tbf_ul.h A src/tbf_ul_ass_fsm.c A src/tbf_ul_ass_fsm.h M tests/alloc/AllocTest.err M tests/tbf/TbfTest.cpp M tests/tbf/TbfTest.err M tests/types/TypesTest.err 15 files changed, 100,530 insertions(+), 208 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/56/25056/6 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: osmith Gerrit-MessageType: newpatchset
Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/25056 ) Change subject: Replace ul_ass_state with osmocom FSM .. Patch Set 5: (4 comments) https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h File src/tbf_ul_ass_fsm.h: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@50 PS4, Line 50: assignment_fsm_timeouts > (Same for "replace dl_ass_state with osmocom FSM" patch) Done https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@62 PS4, Line 62: MS_ANR_EV_CREATE_RLCMAC_MSG > (Same for "replace dl_ass_state with osmocom FSM" patch) Done https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.c File src/tbf_ul_ass_fsm.c: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.c@219 PS4, Line 219: Plt > Pkt? Done https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.c@306 PS4, Line 306: bool tbf_ul_ass_rts(const struct gprs_rlcmac_tbf* tbf) > It's common in different osmocom projects like osmo-bts. […] Ack -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 5 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: osmith Gerrit-Comment-Date: Thu, 29 Jul 2021 13:18:34 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: osmith Comment-In-Reply-To: pespin Gerrit-MessageType: comment
Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/25056 ) Change subject: Replace ul_ass_state with osmocom FSM .. Patch Set 5: Code-Review+1 (3 comments) https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf.cpp File src/tbf.cpp: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf.cpp@967 PS4, Line 967: osmo_fsm_inst_update_id(tbf->ul_ass_fsm.fi, buf); > No need for that, the name of the FSM (which is different for the 2 types) > always show up next to th […] Ack https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h File src/tbf_ul_ass_fsm.h: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@32 PS4, Line 32: TBF_UL_ASS_EV_CREATE_RLCMAC_MSG, /* Scheduler wants to gen+Tx the Ass (rej): data=tbf_ul_ass_ev_create_rlcmac_msg_ctx */ > I prefer keeping comment in one line even if it's a few chars too long. Ack https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@52 PS4, Line 52: fi > no, the comment is fine. […] Ack -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 5 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: osmith Gerrit-Comment-Date: Thu, 29 Jul 2021 13:15:10 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: osmith Comment-In-Reply-To: pespin Gerrit-MessageType: comment
Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 to look at the new patch set (#5). Change subject: Replace ul_ass_state with osmocom FSM .. Replace ul_ass_state with osmocom FSM Related: OS#2709 Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 --- M src/Makefile.am M src/encoding.cpp M src/encoding.h M src/gprs_rlcmac_sched.cpp M src/pdch.cpp M src/tbf.cpp M src/tbf.h M src/tbf_ul.cpp M src/tbf_ul.h A src/tbf_ul_ass_fsm.c A src/tbf_ul_ass_fsm.h M tests/alloc/AllocTest.err M tests/tbf/TbfTest.cpp M tests/tbf/TbfTest.err M tests/types/TypesTest.err 15 files changed, 100,530 insertions(+), 208 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/56/25056/5 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 5 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-CC: osmith Gerrit-MessageType: newpatchset
Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/25056 ) Change subject: Replace ul_ass_state with osmocom FSM .. Patch Set 4: (5 comments) https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf.cpp File src/tbf.cpp: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf.cpp@a767 PS4, Line 767: s X2000, > why was this removed? (maybe it makes sense to do this in a separate commit > and explain it there?) Timer X2000 is moved inside the new FSM, so it's not handled by this pcu-specific mechansim anymore, but using usual osmocom FSM timer callback. https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf.cpp@967 PS4, Line 967: osmo_fsm_inst_update_id(tbf->ul_ass_fsm.fi, buf); > Why give two separate FSMs the same ID, wouldn't it be better to add some > kind of suffix that differ […] No need for that, the name of the FSM (which is different for the 2 types) always show up next to the id, so it's totally fine. Having same ID actually helps in quickly relating different FSMs to same TBF. https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h File src/tbf_ul_ass_fsm.h: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@32 PS4, Line 32: TBF_UL_ASS_EV_CREATE_RLCMAC_MSG, /* Scheduler wants to gen+Tx the Ass (rej): data=tbf_ul_ass_ev_create_rlcmac_msg_ctx */ > (line length) I prefer keeping comment in one line even if it's a few chars too long. https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@52 PS4, Line 52: fi > (Same for "replace dl_ass_state with osmocom FSM" patch) no, the comment is fine. "the_pcu" is actually a global variable which always exists :) https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.c File src/tbf_ul_ass_fsm.c: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.c@306 PS4, Line 306: bool tbf_ul_ass_rts(const struct gprs_rlcmac_tbf* tbf) > assuming rts means ready to send? would be good to add it as comment […] It's common in different osmocom projects like osmo-bts. I'm already using same sort of naming for other FSMs such as the NACC one. These functions are basically used from the scheduler to know whether a related message is waiting to be preparted and sent. -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-CC: osmith Gerrit-Comment-Date: Thu, 29 Jul 2021 11:45:16 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: osmith Gerrit-MessageType: comment
Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/25056 ) Change subject: Replace ul_ass_state with osmocom FSM .. Patch Set 4: (1 comment) https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.c File src/tbf_ul_ass_fsm.c: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.c@219 PS4, Line 219: Plt Pkt? -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-CC: osmith Gerrit-Comment-Date: Thu, 29 Jul 2021 08:12:21 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/25056 ) Change subject: Replace ul_ass_state with osmocom FSM .. Patch Set 4: (3 comments) https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h File src/tbf_ul_ass_fsm.h: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@50 PS4, Line 50: assignment_fsm_timeouts > where is assignment_fsm_timeouts defined? (I can't find it, maybe this has > been renamed? it's also u […] (Same for "replace dl_ass_state with osmocom FSM" patch) https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@52 PS4, Line 52: fi > fi is passed as parameter, so this should be "the_pcu"? (Same for "replace dl_ass_state with osmocom FSM" patch) https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@62 PS4, Line 62: MS_ANR_EV_CREATE_RLCMAC_MSG > can't find this, do you mean TBF_UL_ASS_EV_CREATE_RLCMAC_MSG? (Same for "replace dl_ass_state with osmocom FSM" patch) -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-CC: osmith Gerrit-Comment-Date: Thu, 29 Jul 2021 07:36:30 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: osmith Gerrit-MessageType: comment
Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/25056 ) Change subject: Replace ul_ass_state with osmocom FSM .. Patch Set 4: (7 comments) The only problem I see is having two FSMs with the same ID seems strange, otherwise just some cosmetic comments, rest looks good! https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf.cpp File src/tbf.cpp: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf.cpp@a767 PS4, Line 767: s X2000, why was this removed? (maybe it makes sense to do this in a separate commit and explain it there?) https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf.cpp@967 PS4, Line 967: osmo_fsm_inst_update_id(tbf->ul_ass_fsm.fi, buf); Why give two separate FSMs the same ID, wouldn't it be better to add some kind of suffix that differentiates them? https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h File src/tbf_ul_ass_fsm.h: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@32 PS4, Line 32: TBF_UL_ASS_EV_CREATE_RLCMAC_MSG, /* Scheduler wants to gen+Tx the Ass (rej): data=tbf_ul_ass_ev_create_rlcmac_msg_ctx */ (line length) https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@50 PS4, Line 50: assignment_fsm_timeouts where is assignment_fsm_timeouts defined? (I can't find it, maybe this has been renamed? it's also used two more times in other comments) https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@52 PS4, Line 52: fi fi is passed as parameter, so this should be "the_pcu"? https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.h@62 PS4, Line 62: MS_ANR_EV_CREATE_RLCMAC_MSG can't find this, do you mean TBF_UL_ASS_EV_CREATE_RLCMAC_MSG? https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.c File src/tbf_ul_ass_fsm.c: https://gerrit.osmocom.org/c/osmo-pcu/+/25056/4/src/tbf_ul_ass_fsm.c@306 PS4, Line 306: bool tbf_ul_ass_rts(const struct gprs_rlcmac_tbf* tbf) assuming rts means ready to send? would be good to add it as comment (unless this is a very common abbreviation that I just didn't see much yet) -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-CC: osmith Gerrit-Comment-Date: Thu, 29 Jul 2021 07:18:56 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 to look at the new patch set (#4). Change subject: Replace ul_ass_state with osmocom FSM .. Replace ul_ass_state with osmocom FSM Related: OS#2709 Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 --- M src/Makefile.am M src/encoding.cpp M src/encoding.h M src/gprs_rlcmac_sched.cpp M src/pdch.cpp M src/tbf.cpp M src/tbf.h M src/tbf_ul.cpp M src/tbf_ul.h A src/tbf_ul_ass_fsm.c A src/tbf_ul_ass_fsm.h M tests/alloc/AllocTest.err M tests/tbf/TbfTest.cpp M tests/tbf/TbfTest.err M tests/types/TypesTest.err 15 files changed, 100,531 insertions(+), 208 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/56/25056/4 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-MessageType: newpatchset
Change in osmo-pcu[master]: Replace ul_ass_state with osmocom FSM
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 to look at the new patch set (#3). Change subject: Replace ul_ass_state with osmocom FSM .. Replace ul_ass_state with osmocom FSM Related: OS#2709 Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 --- M src/Makefile.am M src/encoding.cpp M src/encoding.h M src/gprs_rlcmac_sched.cpp M src/pdch.cpp M src/tbf.cpp M src/tbf.h M src/tbf_ul.cpp M src/tbf_ul.h A src/tbf_ul_ass_fsm.c A src/tbf_ul_ass_fsm.h M tests/alloc/AllocTest.err M tests/tbf/TbfTest.cpp M tests/tbf/TbfTest.err M tests/types/TypesTest.err 15 files changed, 100,523 insertions(+), 205 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/56/25056/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25056 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2 Gerrit-Change-Number: 25056 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-MessageType: newpatchset
