laforge 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) 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; I'm not quite sure I understand the purpose of this. It seems we firt count the number of UEs with active TBF, then print that number, then decrement it but nobody ever reads/checks it. So why not simply iterate only once over all UEs and have a local variable that counts how many we sent (and then print that)? 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? * i guess it should be called 'sent' if it wants to indicate whether transmission already happened or not. Update: I now understand it actually determines if the app_info still has to be sent. Let's call it app_info_pending? 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. See e.g. http://git.osmocom.org/osmo-bts/commit/?id=f53fde91a36eff2601df9811fddee97b8f89d6ee how it's done in the BTS. 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 -- 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: Tue, 10 Sep 2019 12:39:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
