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 <axilira...@gmail.com> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <axilira...@gmail.com> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Sat, 04 Apr 2020 12:46:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
