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

[osmith]

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 <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: Wed, 11 Sep 2019 07:59:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge at gnumonks.org>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190911/569e6415/attachment.htm>