Change in osmo-pcu[master]: RLC/MAC: implement decoding of [EGPRS] Packet Channel Request

[pespin]

pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/17718 )

Change subject: RLC/MAC: implement decoding of [EGPRS] Packet Channel Request
......................................................................


Patch Set 1:

(5 comments)

Are you planning to submit similar to wireshark?

https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c 
File src/gsm_rlcmac.c:

https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@5337 
PS1, Line 5337:   M_CHOICE       (PacketChannelRequest_11b_t, Type,
I think you need to first parse Type, or are you doing it somewhere else? See for instance:
CSN_DESCR_BEGIN(IA_EGPRS_t)
  M_UINT       (IA_EGPRS_t,  UnionType ,  1 ),
  M_CHOICE     (IA_EGPRS_t, UnionType, IA_EGPRS_Choice, ElementsOf(IA_EGPRS_Choice)),
CSN_DESCR_END  (IA_EGPRS_t)


https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@6319 
PS1, Line 6319:   struct bitvec bv = {
If we have an API to allocate better use that, in order not to assume implementation details.


https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@6325 
PS1, Line 6325:   switch (req->type) {
In decode functions afaik the data structure is memzeroes by default. I'd rather pass this type as a differnet param if needed, and assign it here.


https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@6333 
PS1, Line 6333:         descr = CSNDESCR(EGPRS_PacketChannelRequest_t);
Better have one function for each initial csnStreamInit like we do with others.


https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/tests/rlcmac/RLCMACTest.cpp 
File tests/rlcmac/RLCMACTest.cpp:

https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/tests/rlcmac/RLCMACTest.cpp@481 
PS1, Line 481: 	static const uint16_t GPRSPktCh11bReqs[] = {
Did you think about implementing encoding and using the struct and pass it to the encoder then to the decoder?



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/17718
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I96df3352856933c9140177b2801a2c71f4134183
Gerrit-Change-Number: 17718
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Sat, 04 Apr 2020 12:46:09 +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/20200404/2235960a/attachment.htm>