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

bluetooth: shell: Fix includes #80105

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

Conversation

rettichschnidi
Copy link
Contributor

This allows to build the shell with BT_CTLR_DTM and/or BT_CTLR_ADV_EXT enabled.

The issues has been introduced by commit
bf897cf (Bluetooth: Shell: Restructure shell files).

Question:

  • For those two settings, would it be okay (tolerated? appreciated?) to add an an extra build in CI "just" to test whether enabling those flags does not break the build?

This allows to build the shell with BT_CTLR_DTM and/or BT_CTLR_ADV_EXT
enabled.

The issues has been introduced by commit
bf897cf (Bluetooth: Shell: Restructure
shell files).

Signed-off-by: Reto Schneider <reto.schneider@husqvarnagroup.com>
Copy link
Contributor

@cvinayak cvinayak left a comment

Choose a reason for hiding this comment

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

Question:

* For those two settings, would it be okay (tolerated? appreciated?) to add an an extra build in CI "just" to test whether enabling those flags does not break the build?

Please add a new tests target in testcases.yaml file with added extra_configs with CONFIG_BT_CTLR_DTM for nrf52840dk/nrf52840 board.

Using which board did you discover the issue?

@@ -53,7 +53,7 @@ int cmd_ll_addr_read(const struct shell *sh, size_t argc, char *argv[])
}

#if defined(CONFIG_BT_CTLR_DTM)
#include "../controller/ll_sw/ll_test.h"
#include "../ll_test.h"
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
#include "../ll_test.h"
#include "controller/ll_sw/ll_test.h"

@@ -123,7 +123,7 @@ int cmd_test_end(const struct shell *sh, size_t argc, char *argv[])
#endif /* CONFIG_BT_CTLR_DTM */

#if defined(CONFIG_BT_CTLR_ADV_EXT)
#include "controller/ll_sw/lll.h"
#include "../lll.h"
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
#include "../lll.h"
#include "controller/ll_sw/lll.h"

@cvinayak cvinayak added bug The issue is a bug, or the PR is fixing a bug Regression Something, which was working, does not anymore labels Oct 21, 2024
@cvinayak cvinayak requested a review from Thalley October 21, 2024 04:11
@rettichschnidi
Copy link
Contributor Author

rettichschnidi commented Oct 21, 2024

Using which board did you discover the issue?

An nRF52840 (or maybe nRF54L15) based out-of-tree board.

@rettichschnidi
Copy link
Contributor Author

Please add a new tests target in testcases.yaml file with added extra_configs with CONFIG_BT_CTLR_DTM for nrf52840dk/nrf52840 board.

Any proposal on which testcases.yaml is should add this to? There are many:

$ find tests/bluetooth/controller/ -name testcase.yaml
tests/bluetooth/controller/ctrl_api/testcase.yaml
tests/bluetooth/controller/ctrl_chmu/testcase.yaml
tests/bluetooth/controller/ctrl_cis_create/testcase.yaml
tests/bluetooth/controller/ctrl_cis_terminate/testcase.yaml
tests/bluetooth/controller/ctrl_collision/testcase.yaml
tests/bluetooth/controller/ctrl_conn_update/testcase.yaml
tests/bluetooth/controller/ctrl_cte_req/testcase.yaml
tests/bluetooth/controller/ctrl_data_length_update/testcase.yaml
tests/bluetooth/controller/ctrl_encrypt/testcase.yaml
tests/bluetooth/controller/ctrl_feature_exchange/testcase.yaml
tests/bluetooth/controller/ctrl_hci/testcase.yaml
tests/bluetooth/controller/ctrl_invalid/testcase.yaml
tests/bluetooth/controller/ctrl_le_ping/testcase.yaml
tests/bluetooth/controller/ctrl_min_used_chans/testcase.yaml
tests/bluetooth/controller/ctrl_phy_update/testcase.yaml
tests/bluetooth/controller/ctrl_terminate/testcase.yaml
tests/bluetooth/controller/ctrl_tx_buffer_alloc/testcase.yaml
tests/bluetooth/controller/ctrl_tx_queue/testcase.yaml
tests/bluetooth/controller/ctrl_unsupported/testcase.yaml
tests/bluetooth/controller/ctrl_version/testcase.yaml
tests/bluetooth/controller/ctrl_sca_update/testcase.yaml
tests/bluetooth/controller/ctrl_isoal/testcase.yaml
tests/bluetooth/controller/ctrl_sw_privacy/testcase.yaml
tests/bluetooth/controller/ctrl_sw_privacy_unit/testcase.yaml
tests/bluetooth/controller/ctrl_user_ext/testcase.yaml
tests/bluetooth/controller/ll_settings/testcase.yaml

Or did you mean to add a directory tests/boards/nrf/bluetooth and put a new testcase.yaml in it?

@cvinayak
Copy link
Contributor

Please add a new tests target in testcases.yaml file with added extra_configs with CONFIG_BT_CTLR_DTM for nrf52840dk/nrf52840 board.

Any proposal on which testcases.yaml is should add this to? There are many:

$ find tests/bluetooth/controller/ -name testcase.yaml
tests/bluetooth/controller/ctrl_api/testcase.yaml
tests/bluetooth/controller/ctrl_chmu/testcase.yaml
tests/bluetooth/controller/ctrl_cis_create/testcase.yaml
tests/bluetooth/controller/ctrl_cis_terminate/testcase.yaml
tests/bluetooth/controller/ctrl_collision/testcase.yaml
tests/bluetooth/controller/ctrl_conn_update/testcase.yaml
tests/bluetooth/controller/ctrl_cte_req/testcase.yaml
tests/bluetooth/controller/ctrl_data_length_update/testcase.yaml
tests/bluetooth/controller/ctrl_encrypt/testcase.yaml
tests/bluetooth/controller/ctrl_feature_exchange/testcase.yaml
tests/bluetooth/controller/ctrl_hci/testcase.yaml
tests/bluetooth/controller/ctrl_invalid/testcase.yaml
tests/bluetooth/controller/ctrl_le_ping/testcase.yaml
tests/bluetooth/controller/ctrl_min_used_chans/testcase.yaml
tests/bluetooth/controller/ctrl_phy_update/testcase.yaml
tests/bluetooth/controller/ctrl_terminate/testcase.yaml
tests/bluetooth/controller/ctrl_tx_buffer_alloc/testcase.yaml
tests/bluetooth/controller/ctrl_tx_queue/testcase.yaml
tests/bluetooth/controller/ctrl_unsupported/testcase.yaml
tests/bluetooth/controller/ctrl_version/testcase.yaml
tests/bluetooth/controller/ctrl_sca_update/testcase.yaml
tests/bluetooth/controller/ctrl_isoal/testcase.yaml
tests/bluetooth/controller/ctrl_sw_privacy/testcase.yaml
tests/bluetooth/controller/ctrl_sw_privacy_unit/testcase.yaml
tests/bluetooth/controller/ctrl_user_ext/testcase.yaml
tests/bluetooth/controller/ll_settings/testcase.yaml

Or did you mean to add a directory tests/boards/nrf/bluetooth and put a new testcase.yaml in it?

This one: https://github.com/zephyrproject-rtos/zephyr/blob/9589e375ab7098a78b72121bb63c8d73d9d54095/tests/bluetooth/shell/testcase.yaml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth Controller area: Bluetooth bug The issue is a bug, or the PR is fixing a bug Regression Something, which was working, does not anymore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants