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

sample: Add HCI interface to DTM #12028

Merged
merged 1 commit into from
Oct 12, 2023
Merged

Conversation

dchat-nordic
Copy link
Contributor

Add HCI interface to Direct Test Mode sample.

Jira: NCSDK-22490

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Aug 14, 2023
@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.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Aug 14, 2023

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
test-fw-nrfconnect-ble X
test-fw-nrfconnect-ble_samples X
test-fw-nrfconnect-fem X
test-fw-nrfconnect-rs X

Detailed information of selected test modules

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

@dchat-nordic dchat-nordic marked this pull request as ready for review August 25, 2023 10:41
@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. and removed changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Aug 25, 2023
@dchat-nordic dchat-nordic removed the DNM label Aug 28, 2023
@@ -50,8 +50,10 @@ The DTM sample includes two parts:

You can find the source code of both parts in :file:`samples/bluetooth/direct_test_mode/src`.

The DTM sample contains a driver for a 2-wire UART interface.
The driver maps two-octet commands and events to the DTM library, as specified by the Bluetooth Low Energy DTM specification.
The DTM sample contains a driver for a 2-wire UART interface and an experimental HCI uart interface.
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
The DTM sample contains a driver for a 2-wire UART interface and an experimental HCI uart interface.
The DTM sample contains a driver for a 2-wire UART interface and an experimental HCI UART interface.

The DTM sample contains a driver for a 2-wire UART interface.
The driver maps two-octet commands and events to the DTM library, as specified by the Bluetooth Low Energy DTM specification.
The DTM sample contains a driver for a 2-wire UART interface and an experimental HCI uart interface.
Unless otherwise stated all following commands and references describe 2-wire interface.
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
Unless otherwise stated all following commands and references describe 2-wire interface.
Unless otherwise stated, all following commands and references describe the 2-wire interface.

The DTM sample contains a driver for a 2-wire UART interface and an experimental HCI uart interface.
Unless otherwise stated all following commands and references describe 2-wire interface.
The 2-wire driver maps two-octet commands and events to the DTM library, as specified by the Bluetooth Low Energy DTM specification.
The HCI driver accepts HCI commands in H4 format and implements minimal set of HCI commands usually sufficient to run DTM testing.
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
The HCI driver accepts HCI commands in H4 format and implements minimal set of HCI commands usually sufficient to run DTM testing.
The HCI driver accepts HCI commands in H4 format and implements a minimal set of HCI commands usually sufficient to run DTM testing.

Experimental HCI interface
==========================

To build the sample with HCI interface the following command can be used:
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
To build the sample with HCI interface the following command can be used:
To build the sample with an HCI interface, use the following command:

@@ -455,7 +469,7 @@ The Bluetooth Low Energy DTM UART interface standard specifies the following con
.. note::
The default bit rate of the DTM UART driver is 19200 bps, which is supported by most certified testers.

You must send all commands as two-byte HEX numbers.
Using 2-wire interface you must send all commands as two-byte HEX numbers.
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
Using 2-wire interface you must send all commands as two-byte HEX numbers.
When using a 2-wire interface, you must send all commands as two-byte HEX numbers.

};

typedef void (*dtm_iq_report_callback_t)(struct dtm_iq_data *data);

/** @brief Function for initializing DTM module.
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually have the function brief descriptions in imperative format, so suggesting:
"Initialize the DTM module."

* This function initializes the DTM module and registers the IQ sampling callback.
* If the callback is not needed, the pointer can be NULL.
*
* @param[in] iq_callback Function pointer to the iq report callback, can be NULL.
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] iq_callback Function pointer to the iq report callback, can be NULL.
* @param[in] iq_callback Function pointer to the IQ report callback, can be NULL.

*/
int dtm_uart_wait_init(void);

/** @brief Wait for uart poll cycle.
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 Wait for uart poll cycle.
/** @brief Wait for UART poll cycle.


/** @brief Wait for uart poll cycle.
*
* Wait for half of the uart period used by the DTM.
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
* Wait for half of the uart period used by the DTM.
* Wait for half of the UART period used by the DTM.

The DTM sample contains a driver for a 2-wire UART interface.
The driver maps two-octet commands and events to the DTM library, as specified by the Bluetooth Low Energy DTM specification.
The DTM sample contains a driver for a 2-wire UART interface and an experimental HCI UART interface.
Unless otherwise stated all following commands and references describe the 2-wire interface.
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
Unless otherwise stated all following commands and references describe the 2-wire interface.
Unless otherwise stated, all following commands and references describe the 2-wire interface.

ipc = <&ipc0>;
ept-name = "remote shell";
current-speed = <19200>;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new line at the end of file

int hci_uart_write(uint8_t type, const uint8_t *hdr, size_t hdr_len,
const uint8_t *pld, size_t len);

#endif /* HCI_UART_H_ */
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new line at the end of file


net_buf_put(&hci_tx_queue, buf);
return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new line at the end of file


CONFIG_NET_BUF=y

CONFIG_BOARD_ENABLE_CPUNET=y
Copy link
Contributor

Choose a reason for hiding this comment

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

No new line at the end of file

#include "../../src/transport/hci_uart.h"
#include "../../rpc/dtm_serialization.h"

LOG_MODULE_REGISTER(serialization_layer,4 );
Copy link
Contributor

Choose a reason for hiding this comment

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

4 ?

return 0;
}

static int serialization_init(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is needed. Maybe main is the best place to execute this

samples/bluetooth/direct_test_mode/src/dtm.c Show resolved Hide resolved
Copy link
Contributor

@KAGA164 KAGA164 left a comment

Choose a reason for hiding this comment

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

I have additional questions to the design. We can take them offline

@@ -54,8 +54,36 @@ config DTM_TRANSPORT_TWOWIRE
help
Use the Two Wire transport interface as the DTM transport layer.

config DTM_TRANSPORT_HCI
bool "DTM over HCI UART"
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
bool "DTM over HCI UART"
bool "DTM over HCI UART [EXPERIMENTAL]"

samples/bluetooth/direct_test_mode/Kconfig Show resolved Hide resolved
#include <nrf_rpc/nrf_rpc_ipc.h>
#include <nrf_rpc_cbor.h>

#include "../../src/transport/hci_uart.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use target_include_directory(app PRIVATE .....) in sample CMake to avoid ugly relative paths


case UART_RX_BUF_REQUEST:
LOG_DBG("Uart rx buf request");
uart_rx_buf_rsp(dev, uart_buf(), 128);
Copy link
Contributor

Choose a reason for hiding this comment

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

128 magic number?

return err;
}

uart_rx_enable(hci_uart_dev, uart_buf(), 128, 10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic numbers?

return;

error_exit:
__ASSERT_NO_MSG(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change it in all places

Suggested change
__ASSERT_NO_MSG(0);
__ASSERT_NO_MSG(false);


static int hci_rx_test(uint16_t opcode, const uint8_t *data)
{
struct bt_hci_cp_le_rx_test *params_v1 = (struct bt_hci_cp_le_rx_test *)data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe union here would be a good idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to union.


static int hci_tx_test(uint16_t opcode, const uint8_t *data)
{
struct bt_hci_cp_le_tx_test *params_v1 = (struct bt_hci_cp_le_tx_test *)data;
Copy link
Contributor

Choose a reason for hiding this comment

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

The same here about union

@@ -981,6 +1098,8 @@ static bool check_pdu(const struct dtm_pdu *pdu)
((dtm_inst.cte_info.slot == DTM_CTE_SLOT_1US) ? 2 : 4);
cte_sample_cnt = NRF_RADIO->DFEPACKET.AMOUNT;

report_iq();
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that this function could be called only in cases when iq sample callback is not NULL. The same about starting RSSI measurement. Is it make sense?

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 condition of callback not null.

struct nrf_rpc_cbor_ctx ctx;

LOG_DBG("Call to hci_uart_write");
NRF_RPC_CBOR_ALLOC(&hci_group, ctx, 1024+64);
Copy link
Contributor

Choose a reason for hiding this comment

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

1024+64 ?

samples/bluetooth/direct_test_mode/CMakeLists.txt Outdated Show resolved Hide resolved
samples/bluetooth/direct_test_mode/src/transport/dtm_hci.c Outdated Show resolved Hide resolved
@@ -50,8 +50,10 @@ The DTM sample includes two parts:

You can find the source code of both parts in :file:`samples/bluetooth/direct_test_mode/src`.

The DTM sample contains a driver for a 2-wire UART interface.
The driver maps two-octet commands and events to the DTM library, as specified by the Bluetooth Low Energy DTM specification.
The DTM sample contains a driver for a 2-wire UART interface and an experimental HCI UART interface.
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
The DTM sample contains a driver for a 2-wire UART interface and an experimental HCI UART interface.
This sample contains a driver for a 2-wire UART interface and an experimental HCI UART interface.

@@ -60,8 +62,9 @@ The implementation is self-contained and requires no Bluetooth Low Energy protoc
The MPU is initialized in the standard way.
The DTM library function :c:func:`dtm_init` configures all interrupts, timers, and the radio.

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 DTM sample may be extended with other interface implementations, such as an HCI interface, USB, or another interface required by the Upper Tester.
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
The DTM sample may be extended with other interface implementations, such as an HCI interface, USB, or another interface required by the Upper Tester.
This sample 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 an appropriate interface implementation in the :file:`transport` directory.
The transports shall conform to the interface specified in the :file:`transport/dtm_transport.h` file.
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
The transports shall conform to the interface specified in the :file:`transport/dtm_transport.h` file.
The transports must conform to the interface specified in the :file:`transport/dtm_transport.h` file.

@@ -316,9 +319,13 @@ The DTM-to-Serial adaptation layer

The :file:`dtm_uart_twowire.c` file is an implementation of the UART interface specified in the `Bluetooth Core Specification`_, Volume 6, Part F, Chapter 3.

The :file:`dtm_hci.c` and :file:`hci_uart.c` files are an implementation of the HCI UART interface specified in the `Bluetooth Core Specification`_, Volume 4, Part A (the flow control can be configured by an overlay file).


Copy link
Contributor

Choose a reason for hiding this comment

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

Remove extra empty line.

The default selection of UART pins is defined in :file:`zephyr/boards/arm/board_name/board_name.dts`.
You can change the defaults using the symbols ``tx-pin`` and ``rx-pin`` in the DTS overlay file of the child image at the project level.
The overlay files for the :ref:`nrf5340_remote_shell` child image are located in the :file:`child_image/remote_shell` directory.
The HCI interface allows for a custom `remote_hci` image to be used with |nRF5340DKnoref|.
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
The HCI interface allows for a custom `remote_hci` image to be used with |nRF5340DKnoref|.
The HCI interface allows for a custom ``remote_hci`` image to be used with |nRF5340DKnoref|.


.. code-block:: console

west build samples/bluetooth/direct_test_mode -b board_name -- --DEXTRA_CONF_FILE=overlay-hci.conf
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
west build samples/bluetooth/direct_test_mode -b board_name -- --DEXTRA_CONF_FILE=overlay-hci.conf
west build samples/bluetooth/direct_test_mode -b board_name -- --DEXTRA_CONF_FILE=overlay-hci.conf


.. code-block:: console

west build samples/bluetooth/direct_test_mode -b board_name -- --DEXTRA_CONF_FILE=overlay-hci-nrf53.conf
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
west build samples/bluetooth/direct_test_mode -b board_name -- --DEXTRA_CONF_FILE=overlay-hci-nrf53.conf
west build samples/bluetooth/direct_test_mode -b board_name -- --DEXTRA_CONF_FILE=overlay-hci-nrf53.conf

/** @brief Function for initializing DTM module.
/** @brief DTM Packet status for IQ Sample report. */
enum dtm_packet_status {
/** Packet received with proper CRC */
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
/** Packet received with proper CRC */
/** Packet received with proper CRC. */

if DTM_TRANSPORT_HCI

config DTM_HCI_QUEUE_COUNT
int "Count of HCI rx/tx queues"
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
int "Count of HCI rx/tx queues"
int "Count of HCI RX/TX queues"

Capitalize these in all places.

Add HCI interface to Direct Test Mode sample.

Jira: NCSDK-22490

Signed-off-by: Dominik Chat <dominik.chat@nordicsemi.no>
@KAGA164 KAGA164 removed the DNM label Oct 12, 2023
@rakons rakons added this to the 2.5.0 milestone Oct 12, 2023
@cvinayak cvinayak merged commit c790a93 into nrfconnect:main Oct 12, 2023
17 of 18 checks passed
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
Development

Successfully merging this pull request may close these issues.

7 participants