ESB: Fixes and improvements #25479

maje-emb · 2025-11-07T13:21:55Z

Improved esb_suspend to suspend the protocol.
Changed esb_flush_tx to allow clearing the FIFO queue when ESB is in the IDLE state.
Fixed esb_pop_tx to use the correct pointer.
Renamed tx_fifo_remove_last to tx_fifo_remove_first

- Improved `esb_suspend` to suspend the protocol. - Changed `esb_flush_tx` to allow clearing the FIFO queue when ESB is in the IDLE state. - Fixed `esb_pop_tx` to use the correct pointer. - Renamed `tx_fifo_remove_last` to `tx_fifo_remove_first` Ref: NCSDK-36153 Signed-off-by: Marcin Jelinski <marcin.jelinski@nordicsemi.no>

Copilot

Pull Request Overview

This PR enhances the ESB (Enhanced ShockBurst) protocol implementation with critical bug fixes and improvements. The changes focus on properly suspending the protocol, fixing FIFO queue operations, and ensuring complete peripheral disconnection.

Key changes:

Enhanced esb_suspend() to properly stop radio and timer peripherals before transitioning to IDLE state
Fixed esb_pop_tx() to use the correct FIFO pointer (front instead of back)
Renamed tx_fifo_remove_last() to tx_fifo_remove_first() for clarity

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
subsys/esb/esb.c	Core ESB implementation with improved suspend logic, fixed FIFO operations, and renamed internal function
subsys/esb/esb_ppi.c	Added complete PPI endpoint cleanup to fully disconnect peripherals during suspend
subsys/esb/esb_dppi.c	Added complete DPPI publish/subscribe clearing to fully disconnect peripherals during suspend
include/esb.h	Updated documentation for `esb_flush_tx()` to clarify radio state requirements and return values

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-07T13:22:43Z

subsys/esb/esb_dppi.c

 	nrf_dppi_channels_disable(ESB_DPPIC, channels_mask);
+
+	/* Clear all publish/subscribe connections to fully disconnect peripherals */
+


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

Suggested change

Copilot · 2025-11-07T13:22:43Z

subsys/esb/esb_dppi.c

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


[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 · 2025-11-07T13:22:44Z

subsys/esb/esb_dppi.c

+	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);
+


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

Suggested change

Copilot · 2025-11-07T13:22:44Z

subsys/esb/esb_dppi.c

+	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);
+


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

Suggested change

Copilot · 2025-11-07T13:22:44Z

subsys/esb/esb.c

 {
 	if (!esb_initialized) {
 		return -EACCES;
+	} else if (esb_state != ESB_STATE_IDLE) {


[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) {

NordicBuilder · 2025-11-07T13:29:43Z

CI Information

^{To view the history of this post, click the 'edited' button above}
Build number: 1

Inputs:

Sources:

sdk-nrf: PR head: a9b1965aa83de258a383907aefe8b016ac331b38

more details

sdk-nrf:

PR head: a9b1965aa83de258a383907aefe8b016ac331b38
merge base: ad7d9b915b4d543e00b23c7b9be6a20dad9c63fd
target head (main): 40aaea40b964dd5702c7a9ce13cbd37b086abc4a
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (4)

include
│  │ esb.h
subsys
│  ├── esb
│  │  ├── esb.c
│  │  ├── esb_dppi.c
│  │  │ esb_ppi.c

Outputs:

Toolchain

Version: df3cc9d822
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:df3cc9d822_e595b21c39

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 47
✅ Integration tests
- ⚠️ test-fw-nrfconnect-proprietary_esb

Disabled integration tests

- test-fw-nrfconnect-nrf_lrcs_positioning
- desktop52_verification
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_lwm2m
- test-fw-nrfconnect-nrf-iot_samples
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-ps-main
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread-main
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-mcuboot
- test-sdk-wifi
- test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

github-actions · 2025-11-07T14:05:53Z

You can find the documentation preview for this PR here.

Preview links for modified nRF Connect SDK documents:

https://ncsdoc.z6.web.core.windows.net/PR-25479/nrf/releases_and_maturity/software_maturity.html

Szynkaa

using esb_pop_tx can cause same corruption as esb_flush_tx

Szynkaa · 2025-11-07T13:34:43Z

subsys/esb/esb_ppi.c

 				  BIT(timer_compare0_radio_disable) |
 				  BIT(radio_end_timer_start) |
 				  (IS_ENABLED(CONFIG_ESB_NEVER_DISABLE_TX) ?
 					BIT(timer_compare1_radio_txen) : 0));


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

Szynkaa · 2025-11-07T13:35:31Z

subsys/esb/esb_dppi.c

 				  BIT(timer_compare0_radio_disable) |
 				  BIT(radio_end_timer_start) |
 				  (IS_ENABLED(CONFIG_ESB_NEVER_DISABLE_TX) ?
 					BIT(timer_compare1_radio_txen) : 0));


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

Szynkaa · 2025-11-07T13:56:31Z

subsys/esb/esb_dppi.c

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


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

Szynkaa · 2025-11-07T14:00:23Z

subsys/esb/esb.c

+	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);


I don't think this is needed

Szynkaa · 2025-11-07T14:00:31Z

subsys/esb/esb.c

 	}

-	/*  Clear PPI */
+	nrf_radio_event_clear(NRF_RADIO, NRF_RADIO_EVENT_DISABLED);


I don't think this is needed

Szynkaa · 2025-11-07T14:11:30Z

subsys/esb/esb.c

+#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


esb_fem_reset also do that

Szynkaa · 2025-11-07T14:11:52Z

subsys/esb/esb.c

 {
 	if (!esb_initialized) {
 		return -EACCES;
+	} else if (esb_state != ESB_STATE_IDLE) {


maje-emb requested review from Szynkaa and doki-nordic November 7, 2025 13:21

maje-emb requested review from a team as code owners November 7, 2025 13:21

Copilot AI review requested due to automatic review settings November 7, 2025 13:21

NordicBuilder added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 7, 2025

Copilot AI reviewed Nov 7, 2025

View reviewed changes

Szynkaa reviewed Nov 7, 2025

View reviewed changes

		nrf_dppi_channels_disable(ESB_DPPIC, channels_mask);

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

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

ESB: Fixes and improvements #25479

Are you sure you want to change the base?

ESB: Fixes and improvements #25479

Uh oh!

Conversation

maje-emb commented Nov 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NordicBuilder commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

Uh oh!

github-actions bot commented Nov 7, 2025

Uh oh!

Szynkaa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NordicBuilder commented Nov 7, 2025 •

edited

Loading