samples: Extract DTM api in sample #11789
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dchat-nordic
requested review from
alwa-nordic,
carlescufi and
KAGA164
as code owners
github-actions
bot
added
the
changelog-entry-required
Test specificationCI/Jenkins/NRF
CI/Jenkins/integration
Detailed information of selected test modules Note: This message is automatically posted and updated by the CI |
dchat-nordic
force-pushed
the
hci_dtm
branch
2 times, most recently
from
026a2e9
to
7d5265a
Compare
github-actions
bot
added
doc-required
|
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. |
dchat-nordic
force-pushed
the
hci_dtm
branch
2 times, most recently
from
c4beb97
to
28a52dd
Compare
annakielar
suggested changes
Jul 20, 2023
| @@ -228,6 +228,10 @@ Bluetooth samples | |||
|
|
|||
| * Support for the nRF52840 DK. | |||
|
|
|||
| * Updated: | |||
|
|
|||
| * Extracted the DTM radio API from transport layer. | |||
There was a problem hiding this comment.
Suggested change
| * Extracted the DTM radio API from transport layer. | |
| * Extracted the DTM radio API from the transport layer. |
Comment on lines
74
to
89
| * Initialization function: | ||
| * ``dtm_init`` | ||
| * Setup functions: | ||
| * ``dtm_setup_reset`` | ||
| * ``dtm_setup_set_phy`` | ||
| * ``dtm_setup_set_modulation`` | ||
| * ``dtm_setup_read_features`` | ||
| * ``dtm_setup_read_max_supported_value`` | ||
| * ``dtm_setup_set_cte_mode`` | ||
| * ``dtm_setup_set_cte_slot`` | ||
| * ``dtm_setup_set_antenna_params`` | ||
| * ``dtm_setup_set_transmit_power`` | ||
| * Test functions: | ||
| * ``dtm_test_receive`` | ||
| * ``dtm_test_transmit`` | ||
| * ``dtm_test_end`` |
There was a problem hiding this comment.
Suggested change
| * Initialization function: | |
| * ``dtm_init`` | |
| * Setup functions: | |
| * ``dtm_setup_reset`` | |
| * ``dtm_setup_set_phy`` | |
| * ``dtm_setup_set_modulation`` | |
| * ``dtm_setup_read_features`` | |
| * ``dtm_setup_read_max_supported_value`` | |
| * ``dtm_setup_set_cte_mode`` | |
| * ``dtm_setup_set_cte_slot`` | |
| * ``dtm_setup_set_antenna_params`` | |
| * ``dtm_setup_set_transmit_power`` | |
| * Test functions: | |
| * ``dtm_test_receive`` | |
| * ``dtm_test_transmit`` | |
| * ``dtm_test_end`` | |
| * The ``dtm_init`` initialization function | |
| * Setup functions: | |
| * ``dtm_setup_reset`` | |
| * ``dtm_setup_set_phy`` | |
| * ``dtm_setup_set_modulation`` | |
| * ``dtm_setup_read_features`` | |
| * ``dtm_setup_read_max_supported_value`` | |
| * ``dtm_setup_set_cte_mode`` | |
| * ``dtm_setup_set_cte_slot`` | |
| * ``dtm_setup_set_antenna_params`` | |
| * ``dtm_setup_set_transmit_power`` | |
| * Test functions: | |
| * ``dtm_test_receive`` | |
| * ``dtm_test_transmit`` | |
| * ``dtm_test_end`` |
There was a problem hiding this comment.
please add missing empty lines before and after sub-lists, and remember to use :c:func:.
|
|
||
| In the ``dtm_cmd_put`` interface, DTM commands are accepted in the 2-byte format. | ||
| Parameters such as ``CMD code``, ``Frequency``, ``Length``, or ``Packet Type`` are encoded within this command. | ||
| The setup functions implement all the DTM setup required by the Bluetooth Low Energy standard. |
There was a problem hiding this comment.
Suggested change
| The setup functions implement all the DTM setup required by the Bluetooth Low Energy standard. | |
| The setup functions implement the DTM setup required by the Bluetooth Low Energy standard. |
| @@ -69,20 +69,32 @@ For this reason, a coaxial connection between the DUT and the Lower Tester is em | |||
| DTM module interface | |||
| ==================== | |||
|
|
|||
| The DTM function ``dtm_cmd_put`` implements the four commands defined by the Bluetooth Low Energy standard: | |||
| The DTM module interface can be divided into three parts: | |||
There was a problem hiding this comment.
Suggested change
| The DTM module interface can be divided into three parts: | |
| The DTM module interface can be divided into the following three parts: |
Comment on lines
95
to
97
| The sample is required to parse the encoding format of the command (ie. UART, HCI, etc.) and use the DTM module interface accordingly. | ||
|
|
||
| The sample is also required to interpret the return values of DTM module interface functions and respond to the tester in the correct format. |
There was a problem hiding this comment.
Suggested change
| The sample is required to parse the encoding format of the command (ie. UART, HCI, etc.) and use the DTM module interface accordingly. | |
| The sample is also required to interpret the return values of DTM module interface functions and respond to the tester in the correct format. | |
| The sample is required to perform the following tasks: | |
| * To parse the relevant encoding format - UART or HCI - for the command, and to use the DTM module interface accordingly. | |
| * To interpret the return values of interface functions used in the DTM module, and to respond to the tester in the correct format. |
Please check if rephrased correctly.
KAGA164
suggested changes
Jul 21, 2023
| dtm_inst.event = LE_TEST_STATUS_EVENT_ERROR; | ||
| return DTM_ERROR_ILLEGAL_CONFIGURATION; | ||
| if (count > dtm_hw_radio_antenna_number_get()) { | ||
| return -EINVAL; |
There was a problem hiding this comment.
Have you checked this with HCI cammand specification against different return codes?
There was a problem hiding this comment.
Yes. I will change this instance to another code, but the transport part of the sample is responsible for translation of error codes. So the return codes of dtm.c don't need to be exact same as Bluetooth specification, but just need to be precise to translate later in transport module.
dchat-nordic
force-pushed
the
hci_dtm
branch
2 times, most recently
from
33829ba
to
4774f61
Compare
annakielar
suggested changes
Jul 26, 2023
| * ``RECEIVER_TEST`` | ||
| * ``TRANSMITTER_TEST`` | ||
| * ``TEST_END`` | ||
| * The :c:func:`dtm_init`` Initialization function |
There was a problem hiding this comment.
Suggested change
| * The :c:func:`dtm_init`` Initialization function | |
| * The :c:func:`dtm_init` initialization function |
| * | ||
| * This function polls for Two Wire UART command. | ||
| * | ||
| * @return 16 bit command. |
There was a problem hiding this comment.
Suggested change
| * @return 16 bit command. | |
| * @return 16-bit command. |
|
|
||
| The :file:`main.c` file may be replaced with other interface implementations, such as an HCI interface, USB, or another interface required by the Upper Tester. | ||
| The :file:`mai.c` file may be extended with other interface implementations, such as an HCI interface, USB, or another interface required by the Upper Tester. |
There was a problem hiding this comment.
Suggested change
| The :file:`mai.c` file may be extended with other interface implementations, such as an HCI interface, USB, or another interface required by the Upper Tester. | |
| The :file:`main.c` file may be extended with other interface implementations, such as an HCI interface, USB, or another interface required by the Upper Tester. |
annakielar
approved these changes
Jul 26, 2023
grochu
reviewed
Jul 31, 2023
| @@ -1,101 +1,33 @@ | |||
| /* | |||
| * Copyright (c) 2020 Nordic Semiconductor ASA | |||
| * Copyright (c) 2023 Nordic Semiconductor ASA | |||
There was a problem hiding this comment.
Not necessary - typically this header contains the original year of creation of the file.
grochu
reviewed
Aug 1, 2023
| /* Set front-end module (FEM) gain value. */ | ||
| FEM_GAIN_SET = 4, | ||
|
|
||
| /* Set radio ramp-up time. */ |
There was a problem hiding this comment.
I think it should be FEM ramp-up time that this command sets.
|
|
||
| #define DTM_UART DT_CHOSEN(ncs_dtm_uart) | ||
|
|
||
| /* The DTM maximum wait time for the UART command second byte. */ |
There was a problem hiding this comment.
Could you specify the unit? I know it is probably moved directly from the original code, but it would be nice to make it precise.
|
|
||
| case LE_PHY_LE_CODED_S2_MIN_RANGE ... LE_PHY_LE_CODED_S2_MAX_RANGE: | ||
| return dtm_setup_set_phy(DTM_PHY_CODED_S2); | ||
| } |
There was a problem hiding this comment.
Please add the default case (per coding style). Also in other places in this file.
dchat-nordic
force-pushed
the
hci_dtm
branch
3 times, most recently
from
077490c
to
49b9b80
Compare
KAGA164
suggested changes
Aug 2, 2023
| return -EINVAL; | ||
| } | ||
|
|
||
| return -EINVAL; |
There was a problem hiding this comment.
Seems to be no needed. Maybe __ASSERT_NO)MSG(false) instead of since it should go outside switch case.
The same in other cases
There was a problem hiding this comment.
Removed unnecessary returns.
samples/bluetooth/direct_test_mode/src/transport/dtm_uart_twowire.c
Outdated
Show resolved
Hide resolved
samples/bluetooth/direct_test_mode/src/transport/dtm_uart_twowire.c
Outdated
Show resolved
Hide resolved
samples/bluetooth/direct_test_mode/src/transport/dtm_uart_twowire.c
Outdated
Show resolved
Hide resolved
samples/bluetooth/direct_test_mode/src/transport/dtm_uart_twowire.c
Outdated
Show resolved
Hide resolved
samples/bluetooth/direct_test_mode/src/transport/dtm_uart_twowire.c
Outdated
Show resolved
Hide resolved
samples/bluetooth/direct_test_mode/src/transport/dtm_uart_twowire.c
Outdated
Show resolved
Hide resolved
samples/bluetooth/direct_test_mode/src/transport/dtm_uart_twowire.h
Outdated
Show resolved
Hide resolved
annakielar
suggested changes
Aug 3, 2023
|
|
||
| The :file:`main.c` file may be replaced with other interface implementations, such as an HCI interface, USB, or another interface required by the Upper Tester. | ||
| The :file:`main.c` file may be extended with other interface implementations, such as an HCI interface, USB, or another interface required by the Upper Tester. | ||
| The extension should be done by adding appropriate interface implementation in the `transport` directory. |
There was a problem hiding this comment.
Suggested change
| The extension should be done by adding appropriate interface implementation in the `transport` directory. | |
| The extension should be done by adding an appropriate interface implementation in the :file:`transport` directory. |
maje-emb
reviewed
Aug 3, 2023
samples/bluetooth/direct_test_mode/src/transport/dtm_uart_twowire.c
Outdated
Show resolved
Hide resolved
samples/bluetooth/direct_test_mode/src/transport/dtm_uart_twowire.c
Outdated
Show resolved
Hide resolved
grochu
reviewed
Aug 3, 2023
There was a problem hiding this comment.
@dchat-nordic, could you please also fix a TYPO in line 300 in this file ("Anritsu MT885" -> "Anritsu MT8852")? Just spotted it when reading the docs and don't want to open a separate PR for that :)
dchat-nordic
force-pushed
the
hci_dtm
branch
4 times, most recently
from
e70645f
to
7a813de
Compare
grochu
approved these changes
Aug 7, 2023
KAGA164
suggested changes
Aug 7, 2023
| .data_len_ext = true, | ||
| .phy_2m = true, | ||
| .stable_mod = false, | ||
| #if (RADIO_MODE_MODE_Ble_LR125Kbit || RADIO_MODE_MODE_Ble_LR500Kbit) |
There was a problem hiding this comment.
Suggested change
| #if (RADIO_MODE_MODE_Ble_LR125Kbit || RADIO_MODE_MODE_Ble_LR500Kbit) | |
| #if (defined(RADIO_MODE_MODE_Ble_LR125Kbit || defined(RADIO_MODE_MODE_Ble_LR500Kbit)) |
Some radio mode values could be 0 then the #if will evaluate to false
There was a problem hiding this comment.
Or maybe even better:
Suggested change
| #if (RADIO_MODE_MODE_Ble_LR125Kbit || RADIO_MODE_MODE_Ble_LR500Kbit) | |
| #if CONFIG_HAS_HW_NRF_RADIO_BLE_CODED |
| #include "dtm_uart_twowire.h" | ||
| #include "dtm.h" | ||
|
|
||
| LOG_MODULE_REGISTER(dtm_tw_tr); |
There was a problem hiding this comment.
Please consider making log level configurable through Kconfig like for other modules
annakielar
suggested changes
Aug 7, 2023
| @@ -281,7 +297,7 @@ Vendor-specific commands can be divided into different categories as follows: | |||
|
|
|||
| .. note:: | |||
| Front-end module configuration parameters, such as ``antenna output``, ``gain``, and ``active delay``, are not set to their default values after the DTM reset command. | |||
| Testers, for example Anritsu MT885, issue a reset command in the beginning of every test. | |||
| Testers, for example Anritsu MT8852, issue a reset command in the beginning of every test. | |||
There was a problem hiding this comment.
Suggested change
| Testers, for example Anritsu MT8852, issue a reset command in the beginning of every test. | |
| Testers, for example Anritsu MT8852, issue a reset command at the beginning of every test. |
| @@ -69,20 +70,35 @@ For this reason, a coaxial connection between the DUT and the Lower Tester is em | |||
| DTM module interface | |||
| ==================== | |||
|
|
|||
| The DTM function ``dtm_cmd_put`` implements the four commands defined by the Bluetooth Low Energy standard: | |||
| The DTM module interface can be divided into following three parts: | |||
There was a problem hiding this comment.
Suggested change
| The DTM module interface can be divided into following three parts: | |
| The DTM module interface can be divided into the following three parts: |
| * :c:func:`dtm_test_end` | ||
|
|
||
| The setup functions implement the DTM setup required by the Bluetooth Low Energy standard. | ||
|
|
There was a problem hiding this comment.
I recommend removing this empty line to have both similar sentences in one paragraph.
| @@ -299,7 +315,7 @@ Vendor-specific commands can be divided into different categories as follows: | |||
| The DTM-to-Serial adaptation layer | |||
| ================================== | |||
|
|
|||
| The :file:`main.c` file is an implementation of the UART interface specified in the `Bluetooth Core Specification`_: Vol. 6, Part F, Chap. 3. | |||
| The :file:`main_uart.c` file is an implementation of the UART interface specified in the `Bluetooth Core Specification`_: Vol. 6, Part F, Chap. 3. | |||
There was a problem hiding this comment.
Suggested change
| The :file:`main_uart.c` file is an implementation of the UART interface specified in the `Bluetooth Core Specification`_: Vol. 6, Part F, Chap. 3. | |
| The :file:`main_uart.c` file is an implementation of the UART interface specified in the `Bluetooth Core Specification`_, Volume 6, Part F, Chapter 3. |
| LE_TEST_SUPPORTED_TX_TIME_MIN_RANGE = 0x04, | ||
| /** @brief DTM maximum supported values */ | ||
| enum dtm_max_supported { | ||
| /** Max supported TX octets */ |
There was a problem hiding this comment.
Suggested change
| /** Max supported TX octets */ | |
| /** Maximum supported TX octets. */ |
Please use full forms instead of max and min - please correct all occurrences in this file. Please also add periods at the end of all doxygen comments.
| /** @brief Read the maximum supported parameter value by DTM. | ||
| * | ||
| * @param[in] parameter Value to be read. | ||
| * @param[out] max_val The pointer to the maximum value. |
There was a problem hiding this comment.
Suggested change
| * @param[out] max_val The pointer to the maximum value. | |
| * @param[out] max_val The pointer to the maximum value. |
All params' descriptions per function should be aligned to the same vertical line.
Comment on lines
231
to
234
| * @param[in] count The antenna count. | ||
| * @param[in] pattern The antenna switching pattern. | ||
| * @param[in] pattern_len The length of the pattern. |
There was a problem hiding this comment.
Suggested change
| * @param[in] count The antenna count. | |
| * @param[in] pattern The antenna switching pattern. | |
| * @param[in] pattern_len The length of the pattern. | |
| * @param[in] count The antenna count. | |
| * @param[in] pattern The antenna switching pattern. | |
| * @param[in] pattern_len The length of the pattern. |
Comment on lines
241
to
244
| * @param[in] power The transmit power request. | ||
| * @param[in] val TX power value (in dBM) in case of DTM_TX_POWER_REQUEST_VAL request. | ||
| * @param[in] channel The channel to adjust power (set to 0 if unknown). |
There was a problem hiding this comment.
Suggested change
| * @param[in] power The transmit power request. | |
| * @param[in] val TX power value (in dBM) in case of DTM_TX_POWER_REQUEST_VAL request. | |
| * @param[in] channel The channel to adjust power (set to 0 if unknown). | |
| * @param[in] power The transmit power request. | |
| * @param[in] val TX power value (in dBM) in case of DTM_TX_POWER_REQUEST_VAL request. | |
| * @param[in] channel The channel to adjust power (set to 0 if unknown). |
Comment on lines
260
to
262
| * @param[in] channel The transmission channel. | ||
| * @param[in] length The packet length. | ||
| * @param[in] pkt The packet type. |
There was a problem hiding this comment.
Suggested change
| * @param[in] channel The transmission channel. | |
| * @param[in] length The packet length. | |
| * @param[in] pkt The packet type. | |
| * @param[in] channel The transmission channel. | |
| * @param[in] length The packet length. | |
| * @param[in] pkt The packet type. |
| /** @brief Set the transmit power for DTM. | ||
| * | ||
| * @param[in] power The transmit power request. | ||
| * @param[in] val TX power value (in dBM) in case of DTM_TX_POWER_REQUEST_VAL request. |
There was a problem hiding this comment.
Suggested change
| * @param[in] val TX power value (in dBM) in case of DTM_TX_POWER_REQUEST_VAL request. | |
| * @param[in] val TX power value (in dBm) in case of DTM_TX_POWER_REQUEST_VAL request. |
Please correct all occurrences.
annakielar
approved these changes
Aug 7, 2023
KAGA164
suggested changes
Aug 8, 2023
samples/bluetooth/direct_test_mode/src/transport/dtm_uart_twowire.c
Outdated
Show resolved
Hide resolved
Extract DTM radio part from transport interface in DTM sample. Jira: NCSDK-22497 Signed-off-by: Dominik Chat <dominik.chat@nordicsemi.no>
maje-emb
approved these changes
Aug 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extract DTM radio part from transport interface in DTM sample.
Jira: NCSDK-22497