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

Reply via email to