andrzej-kaczmarek commented on code in PR #1974:
URL: https://github.com/apache/mynewt-nimble/pull/1974#discussion_r1973751179
##########
nimble/drivers/nrf5x/src/ble_phy.c:
##########
@@ -1167,8 +1224,31 @@ ble_phy_tx_end_isr(void)
phy_ppi_wfr_disable();
phy_ppi_timer0_compare0_to_radio_txen_disable();
phy_ppi_rtc0_compare0_to_timer0_start_disable();
- assert(transition == BLE_PHY_TRANSITION_NONE);
+
+ return;
}
+
+ wfr_usecs = g_ble_phy_data.wfr_usecs;
+ tifs_usecs = g_ble_phy_data.tifs_usecs;
+ tifs_anchor = g_ble_phy_data.tifs_anchor;
+ cur_phy_mode = g_ble_phy_data.phy_cur_phy_mode;
+ next_phy_mode = cur_phy_mode;
+
+ if (transition == PHY_TRANS_TO_RX) {
+#if MYNEWT_VAL(BLE_LL_PHY)
+ ble_phy_mode_apply(g_ble_phy_data.phy_rx_phy_mode);
+#endif
+ /* Packet pointer needs to be reset. */
+ ble_phy_rx_xcvr_setup();
+ next_phy_mode = g_ble_phy_data.phy_rx_phy_mode;
+ }
+
+ ble_phy_transition(transition, tifs_anchor, tifs_usecs, wfr_usecs,
+ BLE_PHY_STATE_TX, cur_phy_mode, next_phy_mode);
+
+ /* Return to default values for the next transition */
+ g_ble_phy_data.phy_transition = PHY_TRANS_NONE;
+ g_ble_phy_data.tifs_anchor = PHY_TRANS_ANCHOR_END;
Review Comment:
you cannot reset parameters here because it will overwrite transition that
was set in txend_cb.
if you want to reset to default parameters just in case code doesn't set
transition, this should be done after reading parameters for current transition
and before calling txend_cb.
##########
nimble/drivers/nrf5x/src/ble_phy.c:
##########
@@ -1167,8 +1224,31 @@ ble_phy_tx_end_isr(void)
phy_ppi_wfr_disable();
phy_ppi_timer0_compare0_to_radio_txen_disable();
phy_ppi_rtc0_compare0_to_timer0_start_disable();
- assert(transition == BLE_PHY_TRANSITION_NONE);
+
+ return;
}
+
+ wfr_usecs = g_ble_phy_data.wfr_usecs;
+ tifs_usecs = g_ble_phy_data.tifs_usecs;
+ tifs_anchor = g_ble_phy_data.tifs_anchor;
+ cur_phy_mode = g_ble_phy_data.phy_cur_phy_mode;
+ next_phy_mode = cur_phy_mode;
+
+ if (transition == PHY_TRANS_TO_RX) {
+#if MYNEWT_VAL(BLE_LL_PHY)
+ ble_phy_mode_apply(g_ble_phy_data.phy_rx_phy_mode);
+#endif
+ /* Packet pointer needs to be reset. */
+ ble_phy_rx_xcvr_setup();
+ next_phy_mode = g_ble_phy_data.phy_rx_phy_mode;
+ }
Review Comment:
this should be done as part of transition_to_rx code
##########
nimble/drivers/nrf5x/src/ble_phy.c:
##########
@@ -1065,99 +1205,16 @@ ble_phy_tx_end_isr(void)
}
#endif
-#if MYNEWT_VAL(BLE_PHY_VARIABLE_TIFS)
- tifs = g_ble_phy_data.tifs;
- g_ble_phy_data.tifs = BLE_LL_IFS;
-#else
- tifs = BLE_LL_IFS;
-#endif
- transition = g_ble_phy_data.phy_transition;
-
if (g_ble_phy_data.txend_cb) {
g_ble_phy_data.txend_cb(g_ble_phy_data.txend_arg);
}
- if (transition == BLE_PHY_TRANSITION_TX_RX) {
-#if MYNEWT_VAL(BLE_LL_PHY)
- ble_phy_mode_apply(g_ble_phy_data.phy_rx_phy_mode);
-#endif
-
- /* Packet pointer needs to be reset. */
- ble_phy_rx_xcvr_setup();
-
- ble_phy_wfr_enable(BLE_PHY_WFR_ENABLE_TXRX, tx_phy_mode, 0);
-
- /* Schedule RX exactly T_IFS after TX end captured in CC[2] */
- rx_time = NRF_TIMER0->CC[2] + tifs;
- /* Adjust for delay between EVENT_END and actual TX end time */
- rx_time += g_ble_phy_t_txenddelay[tx_phy_mode];
- /* Start listening a bit earlier due to allowed active clock accuracy
*/
- rx_time -= 2;
-
-#if PHY_USE_FEM_LNA
- fem_time = rx_time - MYNEWT_VAL(BLE_FEM_LNA_TURN_ON_US);
- nrf_timer_cc_set(NRF_TIMER0, 2, fem_time);
- NRF_TIMER0->EVENTS_COMPARE[2] = 0;
- phy_fem_enable_lna();
-#endif
-
- radio_time = rx_time - BLE_PHY_T_RXENFAST;
- nrf_timer_cc_set(NRF_TIMER0, 0, radio_time);
- NRF_TIMER0->EVENTS_COMPARE[0] = 0;
- phy_ppi_timer0_compare0_to_radio_rxen_enable();
-
- /* In case TIMER0 did already count past CC[0] and/or CC[2], radio
- * and/or LNA may not be enabled. In any case we won't be stuck since
- * wfr will cancel rx if needed.
- *
- * FIXME failing to enable LNA may result in unexpected RSSI drop in
- * case we still rxd something, so perhaps we could check it here
- */
- } else if (transition == BLE_PHY_TRANSITION_TX_TX) {
- if (g_ble_phy_data.txtx_time_anchor) {
- /* Calculate TX anchor relative to current TX end */
-
- /* TX end timestamp is captured in CC[2] */
- tx_time = NRF_TIMER0->CC[2];
- /* Adjust for delay between EVENT_END and actual TX end time */
- tx_time += g_ble_phy_t_txenddelay[tx_phy_mode];
- } else {
- /* Calculate TX anchor relative to current TX start */
-
- /* AA timestamp is captured in CC[1] */
- tx_time = NRF_TIMER0->CC[1];
- /* Adjust for delay between EVENT_ADDRESS and actual AA time ota */
- tx_time += g_ble_phy_t_txaddrdelay[tx_phy_mode];
- /* Adjust by sync word length to get TX start time */
- tx_time -= ble_ll_pdu_syncword_us(tx_phy_mode);
- }
-
- tx_time += g_ble_phy_data.txtx_time_us;
-
-#if PHY_USE_FEM_PA
- fem_time = tx_time - MYNEWT_VAL(BLE_FEM_PA_TURN_ON_US);
-#endif
-
- /* Adjust for delay between EVENT_READY and actual TX start time */
- tx_time -= g_ble_phy_t_txdelay[g_ble_phy_data.phy_cur_phy_mode];
-
- radio_time = tx_time - BLE_PHY_T_TXENFAST;
- nrf_timer_cc_set(NRF_TIMER0, 0, radio_time);
- NRF_TIMER0->EVENTS_COMPARE[0] = 0;
- phy_ppi_timer0_compare0_to_radio_txen_enable();
-
-#if PHY_USE_FEM_PA
- nrf_timer_cc_set(NRF_TIMER0, 2, fem_time);
- NRF_TIMER0->EVENTS_COMPARE[2] = 0;
- phy_fem_enable_pa();
-#endif
+ transition = g_ble_phy_data.phy_transition;
Review Comment:
txend_cb will setup next transition so you need to read all parameters for
current transition before that.
##########
nimble/drivers/nrf5x/src/ble_phy.c:
##########
@@ -1024,26 +1022,168 @@ ble_phy_rx_xcvr_setup(void)
RADIO_INTENSET_DISABLED_Msk);
}
+static uint32_t
+ble_phy_transition_anchor_get(uint8_t tifs_anchor, uint8_t phy_state, uint8_t
phy_mode)
+{
+ uint32_t time;
+
+ if (tifs_anchor == PHY_TRANS_ANCHOR_END) {
+ /* TX end timestamp is captured in CC[2] */
+ time = NRF_TIMER0->CC[2];
+
+ /* Adjust for delay between EVENT_END and actual TX/RX end time */
+ time += (phy_state == BLE_PHY_STATE_TX)
+ ? g_ble_phy_t_txenddelay[phy_mode]
+ : -g_ble_phy_t_rxenddelay[phy_mode];
+
+ } else {
+ /* RX end timestamp is captured in CC[2] */
+ time = NRF_TIMER0->CC[1];
+
+ /* Adjust for delay between EVENT_ADDRESS and actual AA time ota */
+ time += (phy_state == BLE_PHY_STATE_TX)
+ ? g_ble_phy_t_txaddrdelay[phy_mode]
+ : -g_ble_phy_t_rxaddrdelay[phy_mode];
+
+ /* Adjust by sync word length to get TX/RX start time */
+ time -= ble_ll_pdu_syncword_us(phy_mode);
+ }
+
+ return time;
+}
+
+static void
+ble_phy_transition_fem(uint8_t transition, uint32_t anchor_time)
+{
+#if PHY_USE_FEM
+ /* Note: CC[2] is used for end timestamp which we do not need here. */
+ if (transition == PHY_TRANS_TO_TX) {
+#if PHY_USE_FEM_PA
+ fem_time = anchor_time - MYNEWT_VAL(BLE_FEM_PA_TURN_ON_US);
+ nrf_timer_cc_set(NRF_TIMER0, 2, fem_time);
+ NRF_TIMER0->EVENTS_COMPARE[2] = 0;
+ phy_fem_enable_pa();
+#endif
+ } else {
+#if PHY_USE_FEM_LNA
+ fem_time = anchor_time - MYNEWT_VAL(BLE_FEM_LNA_TURN_ON_US);
+ nrf_timer_cc_set(NRF_TIMER0, 2, fem_time);
+ NRF_TIMER0->EVENTS_COMPARE[2] = 0;
+ phy_fem_enable_lna();
+#endif
+ }
+#endif
+}
+
+static int
+ble_phy_transition(uint8_t transition, uint8_t tifs_anchor, uint16_t
tifs_usecs,
+ uint16_t wfr_usecs, uint8_t phy_state, uint8_t cur_phy_mode,
+ uint8_t next_phy_mode)
+{
+ uint32_t anchor_time;
+ uint32_t radio_time;
+ uint32_t start_time;
+ uint32_t wfr_time;
+ bool is_late;
+
+ if ((phy_state != BLE_PHY_STATE_TX && phy_state != BLE_PHY_STATE_RX) ||
+ transition == PHY_TRANS_NONE) {
+ ble_phy_disable();
+
+ return 1;
+ }
+
+ anchor_time = ble_phy_transition_anchor_get(tifs_anchor, phy_state,
cur_phy_mode);
+ start_time = anchor_time + tifs_usecs;
+ radio_time = start_time;
+
+ ble_phy_transition_fem(transition, start_time);
+
+ if (transition == PHY_TRANS_TO_TX) {
Review Comment:
I'd split this into smaller blocks, i.e.
```
if (transition == PHY_TRANS_TO_TX) {
ble_transition_to_tx(...);
} else if (transition == PHY_TRANS_TO_RX) {
ble_transition_to_rx(...);
} else {
ble_transition_to_none(...);
}
```
the `ble_transition_to_...` functions could combine also FEM code so there's
no multiple ifs for the same conditions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
