Change in ...osmo-pcu[master]: Forward ETWS Primary Notification to MS

[laforge]

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 <osmith at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: laforge <laforge at gnumonks.org>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Tue, 10 Sep 2019 12:39:34 +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/20190910/0bb7ad50/attachment.htm>