Skip to content

Conversation

@SebastianBoe
Copy link
Contributor

Invalid configurations should be detected during configuration instead
of during compilation whenever possible.

This patch replaces a BUILD_ASSERT on CONFIG_SYSTEM_WORKQUEUE_PRIORITY
with what is intended to be an equivalent Kconfig restriction.

Signed-off-by: Sebastian Bøe [email protected]

@SebastianBoe
Copy link
Contributor Author

SebastianBoe commented Jun 14, 2018

This patch is in a similar vein to #8338

kernel/Kconfig Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidental spaces instead of tabs.

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

@SebastianBoe
Copy link
Contributor Author

SebastianBoe commented Jun 14, 2018

Testing shows that it still defaults to -1 for BT samples and that trying to set it to a non-negative number errors-out at Kconfig as desired:

sebo@mach:~/zephyr/samples/bluetooth/mesh$ cb
-- Found PythonInterp: /usr/bin/python3 (found suitable version "3.5.2", minimum required is "3.4") 
-- Selected BOARD nrf52840_pca10056
Zephyr version: 1.12.99
Parsing Kconfig tree in /home/sebo/zephyr/Kconfig
Using /home/sebo/zephyr/boards/arm/nrf52840_pca10056/nrf52840_pca10056_defconfig as base
Merging /home/sebo/zephyr/samples/bluetooth/mesh/prj.conf
warning: SYSTEM_WORKQUEUE_PRIORITY (defined at kernel/Kconfig:328) was assigned the value "0" but got the value "-1". Check its dependencies in the 'menuconfig' interface (see the Application Development Primer section of the manual), or in the Kconfig reference at http://docs.zephyrproject.org/reference/kconfig/CONFIG_SYSTEM_WORKQUEUE_PRIORITY.html (which is updated regularly from the master branch)
warning: BT_MESH_LPN_RECV_DELAY (defined at subsys/bluetooth/host/mesh/Kconfig:289) was assigned the value "40" but got the value "". Check its dependencies in the 'menuconfig' interface (see the Application Development Primer section of the manual), or in the Kconfig reference at http://docs.zephyrproject.org/reference/kconfig/CONFIG_BT_MESH_LPN_RECV_DELAY.html (which is updated regularly from the master branch)
warning: BT_MESH_LPN_SCAN_LATENCY (defined at subsys/bluetooth/host/mesh/Kconfig:322) was assigned the value "30" but got the value "". Check its dependencies in the 'menuconfig' interface (see the Application Development Primer section of the manual), or in the Kconfig reference at http://docs.zephyrproject.org/reference/kconfig/CONFIG_BT_MESH_LPN_SCAN_LATENCY.html (which is updated regularly from the master branch)
warning: BT_L2CAP_RX_MTU (defined at subsys/bluetooth/host/Kconfig:165) was assigned the value "69" but got the value "". Check its dependencies in the 'menuconfig' interface (see the Application Development Primer section of the manual), or in the Kconfig reference at http://docs.zephyrproject.org/reference/kconfig/CONFIG_BT_L2CAP_RX_MTU.html (which is updated regularly from the master branch)
warning: BT_CTLR_LE_ENC (defined at subsys/bluetooth/controller/Kconfig:197) was assigned the value "n" but got the value "y". Check its dependencies in the 'menuconfig' interface (see the Application Development Primer section of the manual), or in the Kconfig reference at http://docs.zephyrproject.org/reference/kconfig/CONFIG_BT_CTLR_LE_ENC.html (which is updated regularly from the master branch)
warning: user value 0 on the int symbol SYSTEM_WORKQUEUE_PRIORITY (defined at kernel/Kconfig:328) ignored due to being outside the active range ([-256, -1]) -- falling back on defaults
Error: Aborting due to non-whitelisted Kconfig warning 'warning: user value 0 on the int symbol SYSTEM_WORKQUEUE_PRIORITY (defined at kernel/Kconfig:328) ignored due to being outside the active range ([-256, -1]) -- falling back on defaults'.
Note: If this warning doesn't point to an actual problem, you can add it to the whitelist at the top of /home/sebo/zephyr/scripts/kconfig/kconfig.py.
CMake Error at /home/sebo/zephyr/cmake/kconfig.cmake:119 (message):
  command failed with return code: 1
Call Stack (most recent call first):
  /home/sebo/zephyr/cmake/app/boilerplate.cmake:241 (include)
  CMakeLists.txt:3 (include)


-- Configuring incomplete, errors occurred!
sebo@mach:~/zephyr/samples/bluetooth/mesh$ 

@SebastianBoe SebastianBoe force-pushed the move_more_asserts_into_kconfig branch from 8386708 to 47d02d5 Compare June 14, 2018 08:13
kernel/Kconfig Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This default is outside the range and will get clamped to -1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, wait, there's an if BT on the range. Maybe this can't apply in that case.

Copy link
Contributor Author

@SebastianBoe SebastianBoe Jun 14, 2018

Choose a reason for hiding this comment

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

I imagine that BT implies COOP_ENABLED because there was an assertion on this value being negative in BT (So this is safe).

Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Alright, as long as the default 0 and the range can't apply at the same time, this should be safe.

@codecov-io
Copy link

codecov-io commented Jun 14, 2018

Codecov Report

Merging #8377 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8377   +/-   ##
=======================================
  Coverage   64.61%   64.61%           
=======================================
  Files         423      423           
  Lines       40293    40293           
  Branches     6801     6801           
=======================================
  Hits        26034    26034           
  Misses      11126    11126           
  Partials     3133     3133

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a55c72d...cca1ca7. Read the comment docs.

@carlescufi carlescufi requested a review from jhedberg June 14, 2018 10:28
Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

I dislike having Bluetooth dependencies in the Kernel Kconfig. I need to be able to use the kernel standalone without pulling Bluetooth code or Kconfigs. Lets not have cross-pollution of kernel and subsystem configurations.

Invalid configurations should be detected during configuration instead
of during compilation whenever possible.

This patch replaces a BUILD_ASSERT on CONFIG_SYSTEM_WORKQUEUE_PRIORITY
with what is intended to be an equivalent Kconfig restriction.

Signed-off-by: Sebastian Bøe <[email protected]>
@SebastianBoe SebastianBoe force-pushed the move_more_asserts_into_kconfig branch from 47d02d5 to cca1ca7 Compare June 14, 2018 11:22
@SebastianBoe
Copy link
Contributor Author

I dislike having Bluetooth dependencies in the Kernel Kconfig.

I agree.

It is now re-written to have BT declare their kernel requirements, instead of the kernel configuring itself based on it's users requirements.

@jukkar
Copy link
Member

jukkar commented Jun 14, 2018

There is actually same requirement for networking code. Also for networking, the main thread priority (CONFIG_MAIN_THREAD_PRIORITY) should be COOP (so < 0). I can prepare a patch for that.

@nashif nashif merged commit d62117e into zephyrproject-rtos:master Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants