Attention is currently required from: laforge, pespin.
5 comments:
File include/l1ctl_proto.h:
Patch Set #10, 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:
Patch Set #10, 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.
Patch Set #10, 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.
Patch Set #10, 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:
Patch Set #10, 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 change 30743. To unsubscribe, or for help writing mail filters, visit settings.