Attention is currently required from: fixeria, pespin. jolly has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email )
Change subject: ASCI: Add group receive and transmit mode support to RR layer ...................................................................... Patch Set 5: (8 comments) File src/host/layer23/include/osmocom/bb/mobile/gsm48_rr.h: https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/d7301427_57bd0550 PS5, Line 89: #define GSM48_RR_GST_OFF 0 > can we have this as an enum? Done https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/5038e36f_e615c5c6 PS5, Line 220: /* group call */ > Ack Done https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/82da7549_fe3f184a PS5, Line 222: int group_state; /* extension to RR state for group transmit/receive modes */ > can we have this as an enum? Done https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/9b5fec6e_e5346a10 PS5, Line 230: int uplink_tries; /* Counts number of tries to access the uplink. */ > if it's a counter, do we need negative values (int)? Done File src/host/layer23/src/mobile/gsm48_rr.c: https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/c9afe6b7_93ed519c PS5, Line 73: * GSM48_MM_EVENT_UPLINK_BUSY: Notify MM layer about uplink becoming busy. > I think you added these a few commits before. […] Only GSM48_MM_EVENT_NOTIFICATION has been added before. These comments describe the new interface to the process implemented with this patch. https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/ff0008c2_33132097 PS5, Line 134: static int gsm48_rr_render_ma(struct osmocom_ms *ms, struct gsm48_rr_cd *cd, uint16_t *ma, uint8_t *ma_len); > I bet some of this pointers can be const. It is out of the scope of this patch. The functions are not changed, they are just defined here, because they are used in code above the implementation. https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/ddb252b8_9fbff766 PS5, Line 414: if (rr->state == state && state != GSM48_RR_ST_IDLE) { > are you sure this is correct' this should be == iiuc, not !=. It is allowed to change from IDLE to IDLE. The group sub-state may have changed, but the state itself. https://gerrit.osmocom.org/c/osmocom-bb/+/34491/comment/68171800_e120be3e PS5, Line 7080: > we should start thinking about splitting this file into smaller pieces ;) Maybe. This is one layer. I changed the location of the new functions inside the gsm48_rr.c file. There is also a group now for all functions releated to uplink control. Also I split this patch into several smaller patches for easier review. -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/34491?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Ia55c182a1b4cde777a0e16dcfe0cd32e0f2e38eb Gerrit-Change-Number: 34491 Gerrit-PatchSet: 5 Gerrit-Owner: jolly <andr...@eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: fixeria <vyanits...@sysmocom.de> Gerrit-CC: pespin <pes...@sysmocom.de> Gerrit-Attention: fixeria <vyanits...@sysmocom.de> Gerrit-Attention: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Tue, 26 Sep 2023 10:37:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pes...@sysmocom.de> Comment-In-Reply-To: fixeria <vyanits...@sysmocom.de> Gerrit-MessageType: comment
