osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/15459 )
Change subject: Forward ETWS Primary Notification to MS ...................................................................... Patch Set 3: (4 comments) Thanks for the review, updated the patch. One thing I am not sure about is this line from 3GPP TS 44.060 11.2.47: > This message may be segmented across more than two RLC/MAC control blocks by > using extended RLC/MAC control message segmentation. Does this mean, we need to support splitting long messages into multiple msgb buffers (and sending multiple buffers to each UE then)? https://gerrit.osmocom.org/#/c/15459/3/src/bts.h File src/bts.h: https://gerrit.osmocom.org/#/c/15459/3/src/bts.h@171 PS3, Line 171: uint32_t app_info_todo; > but nobody ever reads/checks it It is used in gprs_rlcmac_sched.cpp, function sched_app_info(): after decrementing, if it is zero, free bts_data->app_info and log that the message was sent to all MS with active TBF. If you like, I can describe this in the surrounding comments. (I've left it out, because the commenting seems to be quite verbose already for the two variables.) https://gerrit.osmocom.org/#/c/15459/3/src/gprs_ms.h File src/gprs_ms.h: https://gerrit.osmocom.org/#/c/15459/3/src/gprs_ms.h@43 PS3, Line 43: struct gprs_rlcmac_ms { : bool app_info_send; : }; > why an extra struct around a single member? Otherwise I would have needed to add a setter and getter. As I understood from the IRC discussion, it is preferred to not introduce more C++ style code in osmo-pcu where possible. This ms_data is now similar to bts_data in bts.h. > Update: I now understand it actually determines if the app_info still has to > be sent. Let's call it app_info_pending? Yes, that is a better name! Renamed it. Also I've renamed bts_data->app_info_todo to bts_data->app_info_pending, as it is the count of pending messages. https://gerrit.osmocom.org/#/c/15459/3/src/gprs_rlcmac.cpp File src/gprs_rlcmac.cpp: https://gerrit.osmocom.org/#/c/15459/3/src/gprs_rlcmac.cpp@59 PS3, Line 59: word = 0x00; /* 0-1: page mode: normal */ : word |= (0x0F & req->application_type) << 6; /* 2-5: application type */ : word |= (0xC0 & req->data[0]) >> 6; /* 6-7: first two data bits */ : msgb_put_u8(msg, word); : : for (i=0; i < req->len - 1; i++) { : word = req->data[i] << 2; /* 0-6: last six data bits from current byte */ : word |= (0xC0 & req->data[i + 1]) >> 6; /* 7-8: first two data bits from next byte */ : msgb_put_u8(msg, word); : } : : word = (0xC0 & req->data[req->len -1]) << 2; /* 0-6: last six data bits from last byte (rest is padding) */ : msgb_put_u8(msg, word); > I would have suggested to use the osmocom/core/bitvec.h infrastructure for > constructing the payload. […] Done. https://gerrit.osmocom.org/#/c/15459/3/src/gprs_rlcmac_sched.cpp File src/gprs_rlcmac_sched.cpp: https://gerrit.osmocom.org/#/c/15459/3/src/gprs_rlcmac_sched.cpp@140 PS3, Line 140: } : else > style Done -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/15459 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Ie35959f833f46bde5f2126314b6f96763f863b36 Gerrit-Change-Number: 15459 Gerrit-PatchSet: 3 Gerrit-Owner: osmith <osm...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <axilira...@gmail.com> Gerrit-Reviewer: laforge <lafo...@gnumonks.org> Gerrit-Reviewer: osmith <osm...@sysmocom.de> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Wed, 11 Sep 2019 07:59:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <lafo...@gnumonks.org> Gerrit-MessageType: comment
