Change in osmo-pcu[master]: Introduce NACC support

[laforge]

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 <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Fri, 22 Jan 2021 20:22:05 +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/20210122/5f7d9ed5/attachment.htm>