laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/22385 )
Change subject: Introduce NACC support ...................................................................... Patch Set 3: (11 comments) https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3//COMMIT_MSG@25 PS3, Line 25: bis bits? not sure what you meant here. Siomply "the SI are received"? https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3//COMMIT_MSG@26 PS3, Line 26: the MS originating the MS The MS orignating the MS? you mean originating the CCN? https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_bssgp_rim.c File src/gprs_bssgp_rim.c: https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_bssgp_rim.c@104 PS3, Line 104: { should the 'bp' possibly be 'const' here, like all of the pointers derived from it below as local variables? https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_bssgp_rim.c@112 PS3, Line 112: possibly some OSMO_ASSERT that it is the right type of osmo_bssgp_prim? https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_bssgp_rim.c@115 PS3, Line 115: RAN the message doesnt say 'rx' or 'tx' or the like, so it's unclear whether "answered" menas we answered it, or somebody answered something we sent. https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_bssgp_rim.c@116 PS3, Line 116: nacc_cause we just added value_string arrays for those cause values, please use. https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_bssgp_rim.c@123 PS3, Line 123: B P instead of B, I guess? https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_ms.c File src/gprs_ms.c: https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/gprs_ms.c@912 PS3, Line 912: EINVAL an allocation failure is not ENOMEM? https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/nacc_fsm.h File src/nacc_fsm.h: https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/nacc_fsm.h@30 PS3, Line 30: enum nacc_fsm_event { I would appraciate some clarity in event nameing. Ideally the name should clearly state if the event is the indication that something was received (like ...RX_IND_...) or that the event requests the FSM to TX something or the like. Also, I think RX_RESOLVE_RAC_CI and SI_INFO_RECEIVED are inconsistent. If they both relate to receiving something, they both should either be RX_... or ...RECEIVED. Yes, details, but they help immensely anyone who did not write (or not remember writing) the code :) https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/nacc_fsm.c File src/nacc_fsm.c: https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/nacc_fsm.c@376 PS3, Line 376: st_tx_neighbour_data_on_enter why do we have empty onenter functions (also further below)? They just add one function call + return without benefit? https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/neigh_cache.h File src/neigh_cache.h: https://gerrit.osmocom.org/c/osmo-pcu/+/22385/3/src/neigh_cache.h@78 PS3, Line 78: char whi is this a 512 character text string and not a uint8_t of MAC_BLOCK_LEN (23 bytes)? Or are you storing all the SI in there? Then why one long buffer instread of several byte-arrays, one for each SI type? -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/22385 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id35f40d05f3e081f32fddbf1fa34cb338db452ca Gerrit-Change-Number: 22385 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Comment-Date: Fri, 22 Jan 2021 20:22:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
