Skip to content
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.

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

Support for nRF21540 FEM in DTM and Radio Test sample #3542

Merged
merged 5 commits into from Feb 5, 2021

Conversation

KAGA164
Copy link
Contributor

@KAGA164 KAGA164 commented Dec 11, 2020

This PR provides nrf21540 FEM support in following samples:

  • Direct Test Mode
  • Radio Test

@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label Dec 11, 2020
@KAGA164 KAGA164 removed the request for review from lemrey December 11, 2020 13:10
@KAGA164 KAGA164 force-pushed the radio_test_dtm_nrf21540_support branch from d863b85 to 9c2c22d Compare December 11, 2020 13:47
@joerchan joerchan requested a review from b-gent December 18, 2020 09:44
Comment on lines 21 to 24
#define FAST_RAMP_UP_TIME 40
/* Radio normal ramp up time in us */
#define RAMP_UP_TIME 130

Copy link
Contributor

Choose a reason for hiding this comment

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

This is rather simplistic. It depends on the PHY used, and can vary between SOCs. How important is this precision here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked mostly all our SoC PS. It seems like these values are common for all SoC. We do not need much accuracy here, it is enough that these values will not be greater than in reality. They are used to calculate a default delay between pull up FEM RX/TX pin and Radio Start task to ensure that PA/LNA is not enabled too early. User can change this delay by the Vendor-specific command.

@@ -1114,6 +1229,31 @@ SHELL_STATIC_SUBCMD_SET_CREATE(sub_transmit_pattern,
SHELL_SUBCMD_SET_END
);

#if CONFIG_NRF21540_FEM
SHELL_STATIC_SUBCMD_SET_CREATE(sub_nrf21540_antenna,
SHELL_CMD(ant_1, NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use SHELL_CMD_ARG you can specify the argument count, and don't have to do it in your command handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some try with SHELL_CMD_ARG. I would not use it here since the commands are then not displayed as I would like when using it

@@ -134,6 +134,7 @@
.. _`Thingy:91 product website (downloads)`: https://www.nordicsemi.com/Software-and-tools/Prototyping-platforms/Nordic-Thingy-91/Download#infotabs

.. _`nRF21540 DK product page`: https://www.nordicsemi.com/Products/Low-power-short-range-wireless/nRF21540
.. _`nRF21540 Front-End-Module Product Specification`: https://infocenter.nordicsemi.com/index.jsp?topic=%2Fps_nrf52833%2Fkeyfeatures_html5.html
Copy link
Contributor

Choose a reason for hiding this comment

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

This URL points to nRF52833 documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be removed. The other PR which added this link was merged.

#define NRF21540_TX_GAIN_Max 31

/**@brief Time in microseconds when RX enable GPIO pin is activated before the
* radio is ready for transmission.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be clearer to state that it is the time from RX Enable pin activation till the LNA is ready (I wouldn't use "radio" as this is just an extender chip).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, right. Changed to valid description

#define NRF21540_PG_RX_TIME_US 13

/**@brief Time in microseconds when TX enable GPIO pin is activated before the
* radio is ready for reception.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be clearer to state that it is the time from TX Enable pin activation till the PA is ready (I wouldn't use "radio" as this is just an extender chip).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, right. Changed to valid description


/**@brief Initialize nrf21540
*
* This function initialize modules needed bu nRF21540 and power-up it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* This function initialize modules needed bu nRF21540 and power-up it.
* This function initializes modules needed by the nRF21540 and powers them up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


/**@brief Choses one of two physical antenna outputs.
*
* @param[in] ant One of antenna outputs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param[in] ant One of antenna outputs.
* @param[in] ant Selected antenna output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

*/
int nrf21540_antenna_select(enum nrf21540_ant ant);

/**@brief Sets Tx gain in arbitrary units like a gain registers values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just:

Suggested change
/**@brief Sets Tx gain in arbitrary units like a gain registers values.
/**@brief Configures Tx gain of the nRF21540 in arbitrary units.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/* Radio fast ramp up time in us. */
#define FAST_RAMP_UP_TIME 40
/* Radio normal ramp up time in us */
#define RAMP_UP_TIME 130
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this only needed when FEM is used? Are these times taken from the nRF52840 PS? This should probably depend on the device as FEM may be used with any nRF device (when a shield is used). This may be aligned later when shield support is added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked mostly all our SoC PS. It seems like these values are common for all SoC

Copy link
Contributor

Choose a reason for hiding this comment

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

The ramp-up time depends on the PHY used, and have slight variances in the SoCs. The Zephyr controller has defines for all of these if we need precise values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add a include directory using:
zephyr_include_directories($ENV{ZEPHYR_BASE}/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio) and then it would be possible to use for example: hal_radio_tx_ready_delay_us_get function from zephyr controller. What do you think about it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I added a separated ramp-up time definitions for every PHY based on the value from BLE Controller. The differences between SoCs are small enough to be omitted.

const char *port_name,
gpio_pin_t pin, gpio_dt_flags_t flags)
{
/* Configure TX enable pin. */
Copy link
Contributor

Choose a reason for hiding this comment

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

This helper function looks generic, so probably this comment is incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Removed

@b-gent
Copy link
Contributor

b-gent commented Dec 21, 2020

Delegating doc review to @peknis

@b-gent b-gent requested a review from peknis December 21, 2020 09:37

This allows an increase of the transmitted power through a Power Amplifier (PA) or an increase of the sensitivity through a Low-Noise Amplifier (LNA).
Any increase in power and sensitivity results in an increased communication range.
The nRF21540 transmitted power gain and an activation delay can be configured using vendor specific commands `Vendor-Specific packet payload`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This should work as it is, as described in https://docutils.sourceforge.io/docs/user/rst/quickref.html#implicit-hyperlink-targets

In other words, to link to a section within the same document, you can just use its title, you don't need a ref target.

Any increase in power and sensitivity results in an increased communication range.
The nRF21540 transmitted power gain and an activation delay can be configured using vendor specific commands `Vendor-Specific packet payload`_.

See the `nRF21540 Front-End-Module Product Specification`_ for more details

Vendor-Specific packet payload
==============================
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to capitalize "Specific" -> Vendor-specific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


This allows an increase of the transmitted power through a Power Amplifier (PA) or an increase of the sensitivity through a Low-Noise Amplifier (LNA).
Any increase in power and sensitivity results in an increased communication range.
The nRF21540 transmitted power gain and an activation delay can be configured using vendor specific commands `Vendor-Specific packet payload`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer active voice: You can configure the transmitted power gain and activation delay in nRF21540 using vendor-specific commands, see ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -142,6 +153,12 @@ Vendor specific commands can be divided into different categories as follows:
The two most significant bits are calculated by the DTM module.
This is possible because the 6 least significant bits of all valid TX power values are unique.
The TX power can be modified only when no Transmitter Test or Receiver Test is running.
* If the Length field is set to ``3``(symbol ``NRF21540_ANTENNA_SELECT``), the Frequency field sets the nRF21540 FEM antenna.
The valid values are 0 - ANT1 enabled, ANT2 disabled or 1 - ANT1 disabled, ANT2 enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Proposing to list the valid values in a bulleted list. It would be clearer to see the values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done

@@ -142,6 +153,12 @@ Vendor specific commands can be divided into different categories as follows:
The two most significant bits are calculated by the DTM module.
This is possible because the 6 least significant bits of all valid TX power values are unique.
The TX power can be modified only when no Transmitter Test or Receiver Test is running.
* If the Length field is set to ``3``(symbol ``NRF21540_ANTENNA_SELECT``), the Frequency field sets the nRF21540 FEM antenna.
The valid values are 0 - ANT1 enabled, ANT2 disabled or 1 - ANT1 disabled, ANT2 enabled.
* If the Length field is set to ``4`` (symbol ``NRF21540_GAIN_SET``), the Frequency field sets the nRF21540 FEM TX gain value in an arbitrary units.
Copy link
Contributor

Choose a reason for hiding this comment

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

... in arbitrary units or in an arbitrary unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be "in arbitrary units". Done

@@ -168,7 +185,7 @@ The sample supports the following development kit:

.. table-from-rows:: /includes/sample_board_rows.txt
:header: heading
:rows: nrf5340dk_nrf5340_cpunet
:rows: nrf5340dk_nrf5340_cpunet, nrf21540dk_nrf52840
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a line for the added board to the sample_board_rows.txt file. It does not seem to be there yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

samples/peripheral/radio_test/README.rst Show resolved Hide resolved
@@ -56,6 +56,17 @@ The sample also requires one of the following testing devices:
The radio test can be also performed using a spectrum analyzer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer active voice: You can perform the radio test also using a spectrum analyzer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Done


This allows an increase of the transmitted power through a Power Amplifier (PA) or an increase of the sensitivity through a Low-Noise Amplifier (LNA).
Any increase in power and sensitivity results in an increased communication range.
The nRF21540 transmitted power gain, antenna and an activation delay can be configured using the user interface `User interface`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

Use active voice and fix the link:

You can configure the transmitted power gain, antenna, and activation delay in nRF21540 through the user interface, see :ref:radio_test_ui

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Done

@@ -134,6 +134,7 @@
.. _`Thingy:91 product website (downloads)`: https://www.nordicsemi.com/Software-and-tools/Prototyping-platforms/Nordic-Thingy-91/Download#infotabs

.. _`nRF21540 DK product page`: https://www.nordicsemi.com/Products/Low-power-short-range-wireless/nRF21540
.. _`nRF21540 Front-End-Module Product Specification`: https://infocenter.nordicsemi.com/index.jsp?topic=%2Fps_nrf52833%2Fkeyfeatures_html5.html
Copy link
Contributor

Choose a reason for hiding this comment

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

@KAGA164 KAGA164 force-pushed the radio_test_dtm_nrf21540_support branch 2 times, most recently from 1f44d2c to dd44c94 Compare December 21, 2020 15:13
*/
int nrf21540_init(void);

/**@brief Choses one of two physical antenna outputs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Choses -> Chooses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

*
* @note Transition from the Program State to the Transmit State takes
* @ref NRF21540_PG_TX_TIME_US microseconds and is triggered by
* the TX_EN pin. Suitable timming can be achieved by the @p
Copy link
Contributor

Choose a reason for hiding this comment

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

timming -> timing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* @ref NRF_RADIO_TASK_TXEN task when active_event occurs or triggers it
* immediately when @ref NRF21540_EXECUTE_NOW is used.
*
* @param[in] active_event event that triggers start of procedure - this event
Copy link
Contributor

Choose a reason for hiding this comment

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

An event that triggers...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* will be connected to appropriate PPI channel.
* @ref NRF21540_EXECUTE_NOW value causes start
* procedure immediately.
* @param[in] deactive_event event that triggers deactivation of the Tx - this
Copy link
Contributor

Choose a reason for hiding this comment

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

An event (change this in all places)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in all places

*
* @note Transition from the Program State to the Receive State takes
* @ref NRF21540_PG_RX_TIME_US microseconds and is triggered by
* the RX_EN pin. Suitable timming can be achieved by the @p
Copy link
Contributor

Choose a reason for hiding this comment

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

timming -> timing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

/**
* @brief Function for setting the nRF21540 activation delay.
*
* @param[in] delay activation delay in microseconds.
Copy link
Contributor

Choose a reason for hiding this comment

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

Capitalization: activation -> Activation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


This allows an increase of the transmitted power through a Power Amplifier (PA) or an increase of the sensitivity through a Low-Noise Amplifier (LNA).
Any increase in power and sensitivity results in an increased communication range.
You can configure the transmitted power gain and activation delay in nRF21540 using vendor-specific commands, see `Vendor-Specific packet payload`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to change the capitalization also here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@KAGA164 KAGA164 force-pushed the radio_test_dtm_nrf21540_support branch from dd44c94 to 1a10819 Compare December 22, 2020 15:43
Copy link
Contributor

@kapi-no kapi-no left a comment

Choose a reason for hiding this comment

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

Looking good, I suggested a few improvements.

Comment on lines 19 to 20
/**@brief Time in microseconds when RX enable GPIO pin is activated before the
* radio is ready for transmission.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still not perfectly clear to me:

Suggested change
/**@brief Time in microseconds when RX enable GPIO pin is activated before the
* radio is ready for transmission.
/**@brief Time in microseconds between RX enable GPIO pin activation and
* radio readiness for RX.

I guess radio should be ready for RX not TX (transmission).

Consider using reception instead of RX if that's your preference:
radio readiness for reception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified this description there to "Settling time from state PG to RX."

Comment on lines 24 to 25
/**@brief Time in microseconds when TX enable GPIO pin is activated before the
* radio is ready for reception.
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend using the same style as in my previous comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

*/
#define NRF21540_EXECUTE_NOW ((uint32_t) 0)

/**@brief Value used as event zero-address - for no-excute the event action.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**@brief Value used as event zero-address - for no-excute the event action.
/**@brief Value used as event zero-address - for no-execute the event action.

@@ -0,0 +1,153 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it reside in the sample source code? Shouldn't it be general-purpose module and be located in the drivers/subsys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is intended to be used only in the DTM and the Radio test sample. This driver shouldn't be used outside this samples, we have the MPSL nRF21540 driver which can be used for protocol stack

/**@brief Initialize nrf21540
*
* This function initializes modules needed by the nRF21540 and powers them up.
* The function waits @ref NRF21540_PD_PG_TIME_US for the device to be
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider expanding description little bit:

Suggested change
* The function waits @ref NRF21540_PD_PG_TIME_US for the device to be
* The function is synchronous and waits @ref NRF21540_PD_PG_TIME_US for the device to be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 20 to 23
/* Radio fast ramp up time in us. */
#define FAST_RAMP_UP_TIME 40
/* Radio normal ramp up time in us */
#define RAMP_UP_TIME 130
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving these definitions to nrf21540.h instead of duplicating it in both DTM and radio test source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to nrf21540.c

Comment on lines 447 to 453
static void nrf21540_active_delay_calculate(void)
{
uint32_t ramp_up_time = nrf_radio_modecnf0_ru_get(NRF_RADIO) ?
FAST_RAMP_UP_TIME : RAMP_UP_TIME;

nrf21540_active_delay = ramp_up_time - NRF21540_PG_TX_TIME_US;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This function could be also added to the nrf21540.h API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be. Added

Comment on lines +19 to +29
#if defined(CONFIG_HAS_HW_NRF_PPI)
#include <nrfx_ppi.h>
#define gppi_channel_t nrf_ppi_channel_t
#define gppi_channel_alloc nrfx_ppi_channel_alloc
#elif defined(CONFIG_HAS_HW_NRF_DPPIC)
#include <nrfx_dppi.h>
#define gppi_channel_t uint8_t
#define gppi_channel_alloc nrfx_dppi_channel_alloc
#else
#error "No PPI or DPPI"
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest moving this logic to the nrfx_gppi. It fits quite well in GPPI concept which is an abstraction over PPI/DPPI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be done in a separated PR and next aligned here.

Comment on lines 110 to 122
.spi_cs = {
.gpio_pin = DT_GPIO_PIN(NRF21540_SPI, cs_gpios),
.gpio_dt_flags = DT_GPIO_FLAGS(NRF21540_SPI, cs_gpios),
.delay = T_NCS_SCLK
},
.spi_cfg = {
.frequency = DT_PROP(NRF21540_SPI, clock_frequency),
.operation = (SPI_OP_MODE_MASTER | SPI_WORD_SET(8) |
SPI_TRANSFER_MSB | SPI_LINES_SINGLE),
.slave = 0,
},
.timer = NRFX_TIMER_INSTANCE(NRF21540_TIMER_INSTANCE)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can make this structure fields constant if they don't change after this initialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the timer and SPI configuration struct can be constant.


static inline nrf_radio_task_t nrf21540_radio_task_get(enum nrf21540_trx dir)
{
return dir == NRF21540_TX ? NRF_RADIO_TASK_TXEN : NRF_RADIO_TASK_RXEN;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding parentheses for clarity

Suggested change
return dir == NRF21540_TX ? NRF_RADIO_TASK_TXEN : NRF_RADIO_TASK_RXEN;
return (dir == NRF21540_TX) ? NRF_RADIO_TASK_TXEN : NRF_RADIO_TASK_RXEN;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


Vendor-Specific packet payload
nRF21540 Front-End-Module
=========================
Copy link
Contributor

@peknis peknis Jan 22, 2021

Choose a reason for hiding this comment

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

We could probably reuse the FEM support stuff from the Thread CLI sample, as we do in Thread CoAP Client sample (by include). If the text there is not good as such, we might want to make this text close to that one, and then reuse it in the Radio test sample.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could use a documentation from the Zigbee PR at this moment https://github.com/nrfconnect/sdk-nrf/pull/3688/files. There is a reference to MPSL nrf21540 driver which isn't used here but I think this should not bother us.

Copy link
Contributor

@peknis peknis Jan 27, 2021

Choose a reason for hiding this comment

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

We have the PR #3688 merged now, and it contains a generic https://github.com/nrfconnect/sdk-nrf/blob/master/doc/nrf/includes/sample_fem_support.txt. Is it generic enough to be used in these docs?

Copy link
Contributor

@peknis peknis Jan 29, 2021

Choose a reason for hiding this comment

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

If the sample_fem_support.txt is not suitable to be used in these samples, we may need to create a suitable text to one of the samples and then use it (by include) in the other one. Something like:

.. include:: samples/bluetooth/direct_test_mode/README.rst
:start-after: FEM_support_start
:end-before: FEM_support_end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this moment we can use these generic doc. I modified samples documentation

@grochu grochu modified the milestone: 1.5 Jan 25, 2021

config NRF21540_FEM
bool "nRF21540 Front-End-Module"
default y if $(dt_nodelabel_has_compat,nrf_radio_fem,$(DT_COMPAT_NORDIC_NRF21540))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does nrf21540_FEM work without DT_COMPAT_NORDIC_NRF2154? If not it should be:

depends on $(dt_nodelabel_has_compat,nrf_radio_fem,$(DT_COMPAT_NORDIC_NRF21540))
default y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Done

Comment on lines +13 to +15
target_sources_ifdef(CONFIG_NRF21540_FEM app
PRIVATE ../../bluetooth/direct_test_mode/src/fem/nrf21540.c)
zephyr_include_directories(../../bluetooth/direct_test_mode/src/fem/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could extract common code between the two test samples to a radio_test submodule, assuming we cannot unify the two samples.

Also I think these should be considered applications rather than samples at this point, and should be moved to the applications folder instead.

@KAGA164
Copy link
Contributor Author

KAGA164 commented Jan 25, 2021

@joerchan I don't fully understand your proposal. Why should these be an application ? I can agree that the nrf21540 isn't linked in the most elegant way to the radio_test sample. On the other hand, I would not like to add nRF21540 module as library or driver for common usage because it should be used in this form only in the radio_test and dtm sample. I think the solution would be to transfer these two examples to a common directory and make there a common directory for nrf21540 sources.

@joerchan
Copy link
Contributor

@KAGA164 Because a sample is meant to demonstrate how to do something as starting point, but applications are meant to be used they way they are. It's not important. but it would good to organize the code the way you describe.

@grochu
Copy link
Contributor

grochu commented Jan 25, 2021

Hi @joerchan, @KAGA164, regarding the linking of the common module controlling FEM, maybe it can be moved somewhere else, though I don't have a good proposal - maybe @joerchan you could propose something.
When it comes to sample relocation (including re-classifying them as applications) I am not a big fan of it, as they are quite different: Radio Test is a generic one and it very well can be a starting point for a more advanced test application for Radio (so it is a sample in that sense).
DTM perhaps could be called an application, but I think it makes sense to have it in the "bluetooth" folder as it implements a standard way of testing, and moving it out might confuse users.

@KAGA164 KAGA164 force-pushed the radio_test_dtm_nrf21540_support branch from 1a10819 to 81c17ec Compare January 25, 2021 15:36
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@KAGA164 KAGA164 force-pushed the radio_test_dtm_nrf21540_support branch 2 times, most recently from 6d8327e to 8a25969 Compare January 29, 2021 13:48
@KAGA164 KAGA164 force-pushed the radio_test_dtm_nrf21540_support branch from 8a25969 to b9d74b5 Compare February 1, 2021 07:59
ru-fu
ru-fu previously requested changes Feb 1, 2021
@@ -18,6 +18,10 @@

| :ref:`nRF52840 Dongle <ug_nrf52>` | PCA10059 | :ref:`nrf52840dongle_nrf52840 <nrf52840dongle_nrf52840>` | ``nrf52840dongle_nrf52840`` |

.. nrf21540dk_nrf52840
Copy link

Choose a reason for hiding this comment

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

This line is already in the file (last entry). Feel free to move, but please don't duplicate it, and don't link to the nRF52 user guide since there is no explanation about nRF21540 in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realize that nRF21540 is already added in this file. I removed my entry


Vendor-Specific packet payload
nRF21540 Front-End-Module
Copy link

Choose a reason for hiding this comment

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

Suggested change
nRF21540 Front-End-Module
nRF21540 front-end module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@KAGA164 KAGA164 force-pushed the radio_test_dtm_nrf21540_support branch from b9d74b5 to 224d998 Compare February 1, 2021 14:58
@ru-fu ru-fu dismissed their stale review February 1, 2021 16:04

Comments have been addressed

@KAGA164 KAGA164 force-pushed the radio_test_dtm_nrf21540_support branch 2 times, most recently from 41cb1ec to 3f23cb3 Compare February 3, 2021 14:20
Adds nRF21540 DK description to the documentation.

Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>
This commit adds support for nRF21540 Front-End-Module
in the Direct Test Mode sample.

Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>
This commit provides a updated documentation for
the Direct Test Mode sample with nRF21540 Front-End-Module
supports.

Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>
This commit adds support for nRF21540 Front-End-Module
in the Radio test sample.

Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>
This commit provides a updated documentation for
the Radio Test sample with nRF21540 Front-End-Module
supports.

Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>
@KAGA164 KAGA164 force-pushed the radio_test_dtm_nrf21540_support branch from 3f23cb3 to 2cd93d0 Compare February 4, 2021 15:14
@tejlmand tejlmand merged commit 83d0229 into nrfconnect:master Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
9 participants