Skip to content

Conversation

@cvinayak
Copy link
Contributor

Replace use of select with depends on in the LL_SW_SPLIT Kconfig options.

@cvinayak cvinayak requested a review from Copilot October 26, 2025 05:35
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the Bluetooth controller's Kconfig file by replacing select statements with depends on clauses where appropriate. This change aligns with Kconfig best practices where features should explicitly declare their dependencies rather than silently enabling them.

Key Changes:

  • Converted several select statements to depends on to make dependencies explicit
  • Added explicit depends on clauses for features that previously used implicit enabling
  • Named several previously unnamed choice blocks for better clarity

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

config BT_CTLR_RX_PDU_META
prompt "RX pdu meta data"
bool
bool"RX pdu meta data"
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

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

Missing space between bool and the string literal. Should be bool \"RX pdu meta data\".

Copilot uses AI. Check for mistakes.
Comment on lines +1006 to +1010
depends on BT_CTLR_RADIO_ENABLE_FAST && \
(SOC_COMPATIBLE_NRF52X || \
SOC_COMPATIBLE_NRF53X || \
SOC_COMPATIBLE_NRF54LX)
default y if SOC_COMPATIBLE_NRF54LX
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

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

The dependency chain is broken. BT_CTLR_RADIO_ENABLE_FAST now depends on this option (line 1006), but this option defaults to y when SOC_COMPATIBLE_NRF54LX (line 1010). However, looking at lines 983-985, BT_CTLR_RADIO_ENABLE_FAST only depends on SOC types without depending on BT_CTLR_SW_SWITCH_SINGLE_TIMER. This creates a situation where BT_CTLR_SW_SWITCH_SINGLE_TIMER depends on BT_CTLR_RADIO_ENABLE_FAST, which may not match the original select-based logic where enabling radio fast mode would automatically enable single timer switching on NRF54LX.

Copilot uses AI. Check for mistakes.
@cvinayak
Copy link
Contributor Author

Please analyse the pull request, give a summary, any possible regression failures or drawbacks of it?

I reviewed the pull request metadata you provided (title: "Bluetooth: Controller: Replace Kconfig select with depends") and analyzed the likely changes, their rationale, and the ways they can affect builds and users. I did not run CI or read the patch itself, so the analysis below is based on the standard Kconfig semantics in Zephyr/Linux and the implications of replacing Kconfig "select" with "depends on".

Summary

  • What the PR does (inferred): replaces Kconfig use of "select" with "depends on" for one or more Bluetooth controller-related Kconfig symbols.
  • Why this is usually done: "select" forces the selected symbol to become visible/enabled regardless of its own dependencies and can create hidden/unsatisfied dependency chains and subtle configuration bugs. Replacing "select" with "depends on" makes the relationship explicit: the selected symbol will only be available (and possibly set) when its dependencies are satisfied. This follows Kconfig best practices (avoid select for enabling symbols that themselves have dependencies).
  • Expected positive outcomes: eliminates implicit enabling that could leave the tree in inconsistent config states, prevents accidental enabling of options whose prerequisites are not satisfied, and reveals underlying dependency problems so they can be fixed explicitly.

Possible regression failures, breakages, and drawbacks

  1. Build failures for consumers that relied on implicit enabling

    • If other Kconfig symbols (or sample/prj.conf) relied on the previous "select" behavior to automatically enable a needed option, replacing it with "depends on" will no longer auto-enable that option. That will cause compile-time failures (undefined symbols, missing compile flags) until the dependent option is explicitly enabled in the appropriate Kconfig/default or prj.conf.
    • Impact vectors:
      • Existing board/samples that previously built without changes may now fail to build.
      • Downstream projects which depended on the implicit selection will break until they explicitly enable the dependency.
  2. Configuration visibility / menu changes

    • The symbol that used to be auto-enabled may now be hidden unless its dependency is enabled. Users doing menuconfig might not find the option in the same place, or the option might be disabled/hidden, causing confusion.
  3. Exposed or re-introduced circular dependency issues

    • "select" sometimes masked a circular/unsatisfied dependency by force-enabling a symbol. Replacing it with "depends on" may reveal those cycles and produce unsatisfiable configurations. That is good in principle, but it will cause immediate build/test failures until the cycle is resolved.
  4. Runtime behavior only indirectly affected

    • The change itself is a Kconfig/configuration change and does not alter runtime code. Still, if an option stops being enabled, related runtime features will be absent (e.g., controller features, logging, or optimizations), which could appear as regressions in functionality or protocol behavior. These are not code regressions but configuration regressions.
  5. User impact / migration pain

    • Downstream users and CI that depended on the old implicit enabling will need to update their prj.conf files or Kconfig defaults. Without release notes or migration guidance, this will cause user frustration and spurious breakage.
  6. Potential missing default fallback

    • If no default or other Kconfig path enables the needed option, the functionality may be entirely disabled until someone adds an explicit enable. Tests assuming that functionality will fail.

Concrete examples of the types of failures you might observe

  • Missing header macros or #if CONFIG_FOO guards failing because CONFIG_BAR was no longer defined.
  • Linker errors because code that was compiled out (by CONFIG change) removes symbol definitions.
  • Sample apps that previously use Bluetooth controller features fail to start or error at initialization.

How to detect and validate regressions (practical tests)

  • Search for other uses of "select" in the Bluetooth controller Kconfig tree to ensure there are no remaining problematic selects:
    • grep -R "select .*" subsys/bluetooth controller/ (adjust path as appropriate)
  • Build the Bluetooth sample apps and controller tests across supported boards:
    • west build -b samples/bluetooth/ - for each important board (nRF, qemu_x86, etc.)
  • Run Zephyr CI / sanitycheck for relevant tests:
    • sanitycheck -p -T tests/bluetooth --disable-capability-checks (or the project's recommended command)
  • Run CI test suites that exercise the controller (controllers unit tests, host-controller integration tests).
  • Run menuconfig or kconfig tool to inspect that the symbol is visible only when its dependency is enabled:
    • west build -t menuconfig or scripts/kconfig/kconfig.py --diff

Mitigations and recommendations for the PR / maintainers

  • Add migration notes in the PR description and CHANGELOG explaining which Kconfig options changed behavior and what users must add to prj.conf to preserve previous behavior.
  • If the old "select" was effectively equivalent to "imply", consider explicitly either:
    • Add a Kconfig "default" for the dependency in the Kconfig location that makes sense (so the option is enabled when appropriate), or
    • Update the dependent Kconfig so that the option is only relevant when it can be satisfied (i.e., properly gated behind a parent option).
  • Add compile-time Kconfig checks or helpful error messages:
    • Use "warning" or sanity checks in Kconfig to indicate the dependency must be enabled if the feature is required.
  • Add or update unit/integration tests that previously passed due to the implicit enable to explicitly set the required configs in their test prj.conf files.
  • Run a repo-wide config build or targeted CI matrix that includes common downstream configurations to catch regressions early.

Checklist for reviewers / CI to run before merging

  • Verify the PR includes updates to any affected sample prj.conf files and tests.
  • Run builds for major Bluetooth boards (nRF series, any SoC using the controller) and for qemu_x86 (if supported).
  • Run Bluetooth subsystem test suite and relevant controller tests.
  • Confirm there are no remaining "select" usages that cause similar problems.
  • Confirm documentation/migration guidance is present.

Summary recommendation

  • The change aligns with Kconfig best practices and is generally desirable because it makes dependencies explicit and avoids hidden misconfigurations.
  • However, because it changes behavior (no longer implicitly enabling symbols), it can cause immediate build/test regressions for anyone who relied on that behavior. Carefully run CI and update sample/test configs and release notes; consider adding explicit defaults or messages to minimize downstream breakage.

If you want, I can:

  • Inspect the actual diff for this PR to point to exact symbols changed and the exact impact (I can do that if you provide the diff or allow me to fetch it).
  • Produce a suggested short migration snippet (prj.conf changes) and Kconfig warning text to include in the PR description.
  • Generate a focused test matrix for sanitycheck (list of boards/tests to run) tailored to the exact symbols changed.

Which would you like me to do next?

Replace use of `select` with `depends on` in the LL_SW_SPLIT
Kconfig options.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
@cvinayak cvinayak force-pushed the github_bt_ctlr_select_to_depends branch from c3837b0 to f17dcb1 Compare October 26, 2025 19:38
@sonarqubecloud
Copy link

@cvinayak cvinayak marked this pull request as ready for review October 26, 2025 20:07
@cvinayak cvinayak added the Enhancement Changes/Updates/Additions to existing features label Oct 27, 2025
@cvinayak cvinayak added this to the v4.4.0 milestone Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Controller area: Bluetooth Enhancement Changes/Updates/Additions to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants