Skip to content

ESB: Fixes and improvements #25479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ESB: Fixes and improvements #25479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions include/esb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,8 +438,13 @@ int esb_stop_rx(void);
*
* This function clears the TX FIFO buffer.
*
* @retval 0 If successful.
* Otherwise, a (negative) error code is returned.
* @note The radio must not be in transmission state for this operation to succeed.
* This requirement prevents erroneous operations on the FIFO queue that could
* occur if the buffer is cleared while the radio is transmitting.
*
* @retval 0 If successful (FIFO cleared).
* @retval -EACCES If ESB is not initialized.
* @retval -EBUSY If radio is transmitting.
*/
int esb_flush_tx(void);

Expand Down
74 changes: 43 additions & 31 deletions subsys/esb/esb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,7 +1072,7 @@ static void initialize_fifos(void)
}
}

static void tx_fifo_remove_last(void)
static void tx_fifo_remove_first(void)
{
if (tx_fifo.count == 0) {
return;
Expand Down Expand Up @@ -1332,7 +1332,7 @@ static void on_timer_compare1_tx_noack(void)
esb_ppi_for_wait_for_rx_clear();

interrupt_flags |= INT_TX_SUCCESS_MSK;
tx_fifo_remove_last();
tx_fifo_remove_first();

if (tx_fifo.count == 0) {
esb_state = ESB_STATE_PTX_TXIDLE;
Expand All @@ -1349,7 +1349,7 @@ static void on_radio_disabled_tx_noack(void)
esb_ppi_for_txrx_clear(false, false);

interrupt_flags |= INT_TX_SUCCESS_MSK;
tx_fifo_remove_last();
tx_fifo_remove_first();

if (tx_fifo.count == 0) {
esb_state = ESB_STATE_IDLE;
Expand Down Expand Up @@ -1440,7 +1440,7 @@ static void on_radio_disabled_tx_wait_for_ack(void)
interrupt_flags |= INT_TX_SUCCESS_MSK;
last_tx_attempts = esb_cfg.retransmit_count - retransmits_remaining + 1;

tx_fifo_remove_last();
tx_fifo_remove_first();

if ((esb_cfg.protocol != ESB_PROTOCOL_ESB) && (rx_pdu->type.dpl_pdu.length > 0)) {
if (rx_fifo_push_rfbuf(
Expand Down Expand Up @@ -2100,15 +2100,45 @@ int esb_init(const struct esb_config *config)

int esb_suspend(void)
{
if (esb_state != ESB_STATE_IDLE) {
return -EBUSY;
if (esb_state == ESB_STATE_IDLE) {
return -EALREADY;
}
on_radio_disabled = NULL;

/* Stop radio */
nrf_radio_shorts_disable(NRF_RADIO, 0xFFFFFFFF);
nrf_radio_int_disable(NRF_RADIO, 0xFFFFFFFF);

nrf_radio_event_clear(NRF_RADIO, NRF_RADIO_EVENT_DISABLED);
nrf_radio_task_trigger(NRF_RADIO, NRF_RADIO_TASK_DISABLE);

while (!nrf_radio_event_check(NRF_RADIO, NRF_RADIO_EVENT_DISABLED)) {
/* wait for register to settle */
}

/* Clear PPI */
nrf_radio_event_clear(NRF_RADIO, NRF_RADIO_EVENT_DISABLED);
Copy link
Member

@Szynkaa Szynkaa Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed


/* Stop timer */
nrf_timer_shorts_disable(esb_timer.p_reg, 0xFFFFFFFF);
nrf_timer_int_disable(esb_timer.p_reg, 0xFFFFFFFF);

nrf_timer_event_clear(esb_timer.p_reg, NRF_TIMER_EVENT_COMPARE0);
nrf_timer_event_clear(esb_timer.p_reg, NRF_TIMER_EVENT_COMPARE1);
nrf_timer_event_clear(esb_timer.p_reg, NRF_TIMER_EVENT_COMPARE2);
nrf_timer_event_clear(esb_timer.p_reg, NRF_TIMER_EVENT_COMPARE3);
Comment on lines +2125 to +2128
Copy link
Member

@Szynkaa Szynkaa Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed


#if NRF_TIMER_HAS_SHUTDOWN
nrf_timer_task_trigger(esb_timer.p_reg, NRF_TIMER_TASK_SHUTDOWN);
#else
nrf_timer_task_trigger(esb_timer.p_reg, NRF_TIMER_TASK_STOP);
nrf_timer_task_trigger(esb_timer.p_reg, NRF_TIMER_TASK_CLEAR);
#endif
Comment on lines +2130 to +2135
Copy link
Member

@Szynkaa Szynkaa Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

esb_fem_reset also do that


esb_ppi_disable_all();
esb_fem_reset();

esb_state = ESB_STATE_IDLE;
errata_216_off();
esb_state = ESB_STATE_IDLE;

return 0;
}
Expand Down Expand Up @@ -2313,33 +2343,15 @@ int esb_stop_rx(void)
return -EINVAL;
}

on_radio_disabled = NULL;

esb_ppi_for_txrx_clear(true, false);
esb_fem_reset();

nrf_radio_shorts_disable(NRF_RADIO, 0xFFFFFFFF);
nrf_radio_int_disable(NRF_RADIO, 0xFFFFFFFF);

nrf_radio_event_clear(NRF_RADIO, NRF_RADIO_EVENT_DISABLED);
nrf_radio_task_trigger(NRF_RADIO, NRF_RADIO_TASK_DISABLE);

while (!nrf_radio_event_check(NRF_RADIO, NRF_RADIO_EVENT_DISABLED)) {
/* wait for register to settle */
}

nrf_radio_event_clear(NRF_RADIO, NRF_RADIO_EVENT_DISABLED);

esb_state = ESB_STATE_IDLE;
errata_216_off();

return 0;
return esb_suspend();
}

int esb_flush_tx(void)
{
if (!esb_initialized) {
return -EACCES;
} else if (esb_state != ESB_STATE_IDLE) {
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Use separate if statements instead of else-if for better clarity and to avoid unnecessary coupling between unrelated error conditions.

Suggested change
} else if (esb_state != ESB_STATE_IDLE) {
}
if (esb_state != ESB_STATE_IDLE) {

Copilot uses AI. Check for mistakes.
Copy link
Member

@Szynkaa Szynkaa Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

return -EBUSY;
}

unsigned int key = irq_lock();
Expand Down Expand Up @@ -2373,8 +2385,8 @@ int esb_pop_tx(void)

unsigned int key = irq_lock();

if (++tx_fifo.back >= CONFIG_ESB_TX_FIFO_SIZE) {
tx_fifo.back = 0;
if (++tx_fifo.front >= CONFIG_ESB_TX_FIFO_SIZE) {
tx_fifo.front = 0;
}
tx_fifo.count--;

Expand Down
27 changes: 27 additions & 0 deletions subsys/esb/esb_dppi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,33 @@
BIT(timer_compare1_radio_txen) : 0));
Copy link
Member

@Szynkaa Szynkaa Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this 2 channels are swapped, radio_end_timer_start depends on CONFIG_ESB_NEVER_DISABLE_TX


nrf_dppi_channels_disable(ESB_DPPIC, channels_mask);

/* Clear all publish/subscribe connections to fully disconnect peripherals */

Check failure on line 299 in subsys/esb/esb_dppi.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

TRAILING_WHITESPACE 

subsys/esb/esb_dppi.c:299  trailing whitespace
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Remove this empty line to maintain consistent code style.

Suggested change

Copilot uses AI. Check for mistakes.
/* Clear EGU */
nrf_egu_publish_clear(ESB_EGU, ESB_EGU_EVENT);
nrf_egu_publish_clear(ESB_EGU, ESB_EGU_DPPI_EVENT);
nrf_egu_subscribe_clear(ESB_EGU, ESB_EGU_TASK);
nrf_egu_subscribe_clear(ESB_EGU, ESB_EGU_DPPI_TASK);

Check failure on line 305 in subsys/esb/esb_dppi.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

TRAILING_WHITESPACE 

subsys/esb/esb_dppi.c:305  trailing whitespace
/* Clear Radio */
nrf_radio_publish_clear(NRF_RADIO, NRF_RADIO_EVENT_ADDRESS);
nrf_radio_publish_clear(NRF_RADIO, NRF_RADIO_EVENT_DISABLED);
Comment on lines +305 to +308
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Remove this empty line to maintain consistent code style.

Suggested change
/* Clear Radio */
nrf_radio_publish_clear(NRF_RADIO, NRF_RADIO_EVENT_ADDRESS);
nrf_radio_publish_clear(NRF_RADIO, NRF_RADIO_EVENT_DISABLED);
/* Clear Radio */
nrf_radio_publish_clear(NRF_RADIO, NRF_RADIO_EVENT_ADDRESS);
nrf_radio_publish_clear(NRF_RADIO, NRF_RADIO_EVENT_DISABLED);
nrf_radio_publish_clear(NRF_RADIO, NRF_RADIO_EVENT_DISABLED);

Copilot uses AI. Check for mistakes.
Copy link
Member

@Szynkaa Szynkaa Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we cannot clear publish for NRF_RADIO_EVENT_DISABLED here, because it comes from ppi_init
also clear for ESB_RADIO_EVENT_END is problematic because on nrf54X it alias for NRF_RADIO_EVENT_PHYEND and it's set in ppi_init for CONFIG_ESB_FAST_SWITCHING

nrf_radio_publish_clear(NRF_RADIO, ESB_RADIO_EVENT_END);
nrf_radio_subscribe_clear(NRF_RADIO, NRF_RADIO_TASK_RXEN);
nrf_radio_subscribe_clear(NRF_RADIO, NRF_RADIO_TASK_TXEN);
nrf_radio_subscribe_clear(NRF_RADIO, NRF_RADIO_TASK_DISABLE);

Check failure on line 313 in subsys/esb/esb_dppi.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

TRAILING_WHITESPACE 

subsys/esb/esb_dppi.c:313  trailing whitespace
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Remove this empty line to maintain consistent code style.

Suggested change

Copilot uses AI. Check for mistakes.
/* Clear Timer */
nrf_timer_publish_clear(ESB_NRF_TIMER_INSTANCE, NRF_TIMER_EVENT_COMPARE0);
nrf_timer_publish_clear(ESB_NRF_TIMER_INSTANCE, NRF_TIMER_EVENT_COMPARE1);
nrf_timer_subscribe_clear(ESB_NRF_TIMER_INSTANCE, NRF_TIMER_TASK_START);
nrf_timer_subscribe_clear(ESB_NRF_TIMER_INSTANCE, NRF_TIMER_TASK_STOP);

Check failure on line 319 in subsys/esb/esb_dppi.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

TRAILING_WHITESPACE 

subsys/esb/esb_dppi.c:319  trailing whitespace
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Remove this empty line to maintain consistent code style.

Suggested change

Copilot uses AI. Check for mistakes.
/* Clear DPPI */
nrf_dppi_subscribe_clear(ESB_DPPIC,
nrf_dppi_group_disable_task_get((uint8_t)ramp_up_dppi_group));
nrf_dppi_channels_remove_from_group(ESB_DPPIC, BIT(egu_ramp_up), ramp_up_dppi_group);
}

void esb_ppi_deinit(void)
Expand Down
15 changes: 15 additions & 0 deletions subsys/esb/esb_ppi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,21 @@ void esb_ppi_disable_all(void)
BIT(timer_compare1_radio_txen) : 0));
Copy link
Member

@Szynkaa Szynkaa Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this 2 channels are swapped, radio_end_timer_start depends on CONFIG_ESB_NEVER_DISABLE_TX and timer_compare1_radio_txen is used as retx trigger

same thing is below in clearing endpoint


nrf_ppi_channels_disable(NRF_PPI, channels_mask);

/* Clear all PPI endpoints to fully disconnect peripherals */
nrf_ppi_channel_and_fork_endpoint_setup(NRF_PPI, egu_ramp_up, 0, 0, 0);
nrf_ppi_channel_endpoint_setup(NRF_PPI, disabled_egu, 0, 0);
nrf_ppi_channel_endpoint_setup(NRF_PPI, egu_timer_start, 0, 0);
nrf_ppi_channel_endpoint_setup(NRF_PPI, radio_address_timer_stop, 0, 0);
nrf_ppi_channel_endpoint_setup(NRF_PPI, timer_compare0_radio_disable, 0, 0);
nrf_ppi_channel_endpoint_setup(NRF_PPI, radio_end_timer_start, 0, 0);

if (IS_ENABLED(CONFIG_ESB_NEVER_DISABLE_TX)) {
nrf_ppi_channel_endpoint_setup(NRF_PPI, timer_compare1_radio_txen, 0, 0);
}

/* Remove channel from group */
nrf_ppi_channel_remove_from_group(NRF_PPI, egu_ramp_up, ramp_up_ppi_group);
}

void esb_ppi_deinit(void)
Expand Down
Loading