This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
fixeria gerrit-no-reply at lists.osmocom.orgfixeria 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? Yes, as soon as I find time for that... 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: […] Actually M_CHOICE does parsing for us, check out 'case CSN_CHOICE' in csnStreamDecoder(). The definition of IA_EGPRS_t looks wrong to me. None of the other definitions is using M_CHOICE like that. 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. Lifetime of this structure is limited by the execution scope of this function, so I don't think we really need to allocate it on heap just because we have an API for that. Moreover, this is not the only place where a bitvector is allocated on stack, there is at least one such allocation in libosmocore and one more in osmo-pcu. 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. […] You mean other decoding functions in this file or csnStreamDecoder()? The later does not zero-initialize the structure, and that's nice because we can catch bugs with AddressSanitizer or Valgrind. As you probably know, I am against using zero-initialization without the real need for that. 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. Why is it better? What would we benefit? 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 th […] Since we're on the BTS/PCU side, we never need to encode RACH blocks on practice. So I think it's worthless to add encoding API just for a unit test. I am happy with decoding results. -- 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 14:12:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin at sysmocom.de> Gerrit-MessageType: comment -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200404/db983682/attachment.htm>