Attention is currently required from: laforge, pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/30743 ) Change subject: {trxcon,virt_phy}: shared GPRS L1 (MAC) implementation ...................................................................... Patch Set 11: (5 comments) File include/l1ctl_proto.h: https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/4395d2b1_3a6490a1 PS10, Line 367: uint8_t padding[3];

I still don't see the point in adding these padding bytes here. […] I already tried to explain and additionally brought this topic up in the IRC. Let's please avoid spending even more time discussing this. Padding is already present in existing L1CTL messages, so it will be in the new ones too.

File src/host/virt_phy/src/virt_prim_pdch.c: https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/942ed7ba_34ff2bc0 PS10, Line 34: void l1ctl_rx_gprs_uldl_tbf_cfg_req(struct l1_model_ms *ms, struct msgb *msg)

So you send both L1CTL messages to this function, to then split it again to 2 differents functions b […] I think it's fine to have a single function for handling both L1CTL messages, because a) we want to check `ms->gprs` against NULL, and b) we need to `pull()` the `l1ctl_hdr`. This is common for both messages and having two separate functions would be code duplication. The two different functions below are "external" ones from l1gprs. So I disagree that it's "totally unnecesary" and prefer to do it this way. It's not like I am mixing up all GPRS related L1CTL messages here, just the two related ones.

https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/7f2ed3a0_7fad25de PS10, Line 69: if (OSMO_UNLIKELY(fn_sched != req.hdr.fn)) {

AFAIU this should be OK if req.hdr.fn < fn_sched (taking wraparound into account). No. The frame number must match, otherwise we're sending stuff earlier or later then requested, what is unacceptable in GPRS. We're violating this and not returning early below, but this is rather a temporary hack.

https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/67a04d89_0da714d1 PS10, Line 73: /* FIXME: return; */

yes, you must return here and not schedule it. Well, at the moment we always hit this block. So if we return, all UL blocks will be discarded. The root problem needs to be fixed first. Either virtphy is scheduling stuff in advance, or the upper layers are sending block too late. For now it's a FIXME.

File src/shared/l1gprs.c: https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/ac1f7c60_60c68060 PS10, Line 83: static struct l1gprs_tbf *l1gprs_find_tbf(struct l1gprs_state *gprs,

In the end you find based on uplink and downlink, so yeah having 2 llists makes sense imho. What do we win by having two different lists?

-- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/30743 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: I9567d64f9d00262e36147e8d7e541e5e246bda5f Gerrit-Change-Number: 30743 Gerrit-PatchSet: 11 Gerrit-Owner: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge@osmocom.org> Gerrit-CC: pespin <pespin@sysmocom.de> Gerrit-Attention: laforge <laforge@osmocom.org> Gerrit-Attention: pespin <pespin@sysmocom.de> Gerrit-Comment-Date: Thu, 16 Mar 2023 14:28:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin@sysmocom.de> Gerrit-MessageType: comment