-
Notifications
You must be signed in to change notification settings - Fork 1.4k
arch: Remove default value for BOARD_LOOPSPERMSEC #17011
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
base: master
Are you sure you want to change the base?
Conversation
1c5b113 to
e0242ab
Compare
|
Looks like there are some defconfigs using the default value. Good to catch. |
|
Looks like we need to fix the boards where -1 / default is used, just to pass build here and have correct timings on these boards, then update the res of the boards depending on what we have at hand? :-) On the other hand we may keep this default and put some pressure / attention to the proper timings calibration when using different boards. I am wondering what impacts the value and if the differences are big - is it build specific? For instance when using different compiler, specific optimization level, many irqs, threads, etc :-) I mean this 5000 default was set because any default may be wrong when this value is not calibrated on a final firmware build? People just did not know that? In that case we may leave as-is and just add Pre-Flight-Check-List to the documentation with list of important things to know / check / verify when creating / building NuttX based projects? :-) |
|
@linguini1 instead of suggesting searching for CONFIG_BOARD_LOOPSPERMSEC I suggest search for calib_udelay because there is not a page dedicated to CONFIG_BOARD_LOOPSPERMSEC and when searching for it returns many pages. Searching for calib_udelay returned two pages, but I think we need to move calib_udelay from apps/examples/ to apps/system/ because it is a system tool used to help the system to work correctly. And it also will avoid calib_udelay returning in the apps examples pages. What do you think? |
That's fine, I plan to make one for BOARD_LOOPSPERMSEC but it's better to redirect to calib_udelay for now, I agree.
I don't mind moving it to the system directory but that's outside the scope of this PR. I agree it makes more sense, but I might do it later. Right now I just want to prevent users from making a frustrating mistake. |
e0242ab to
c96fe68
Compare
|
MSVC failure appears unrelated. |
c96fe68 to
20d1c70
Compare
@simbit18 you are the MS guy, any idea? |
With these new commits it should be ready to merge, provided that the CI passes! |
a81afb9 to
e10bfca
Compare
|
Ugh, another problem revealed now? If there are build errors then CI will fail for all builds after merge? o_O |
Yeah, I'm travelling right now but I aim to fix this. The other sim configs are able to avoid the use of LOOPSPERMSEC so it should be a trivial change for this one too. Hopefully the last issue left! |
|
Safe travels @linguini1 :-) |
This commit removes the default Kconfig value for CONFIG_BOARD_LOOPSPERMSEC. This will result in a compile time error for any users who have not configured this value for their board. QEMU ARMv8-A configurations were given a value obtained by running `calib_udelay` on the NSH configuration. Signed-off-by: Matteo Golin <[email protected]>
Propagated correct CONFIG_BOARDS_LOOPSPERMSEC value to remaining configurations for this board. Signed-off-by: Matteo Golin <[email protected]>
Warning that CONFIG_BOARD_LOOPSPERMSEC must be set by the user for correct delay operation, and message that upstream patch would be appreciated. Fixed typo in board.h header guard as well. Signed-off-by: Matteo Golin <[email protected]>
Warning that CONFIG_BOARD_LOOPSPERMSEC must be set by the user for correct delay operation, and message that upstream patch would be appreciated. Signed-off-by: Matteo Golin <[email protected]>
Warning that CONFIG_BOARD_LOOPSPERMSEC must be set by the user for correct delay operation, and message that upstream patch would be appreciated. Signed-off-by: Matteo Golin <[email protected]>
Warning that CONFIG_BOARD_LOOPSPERMSEC must be set by the user for correct delay operation, and message that upstream patch would be appreciated. Signed-off-by: Matteo Golin <[email protected]>
Warning that CONFIG_BOARD_LOOPSPERMSEC must be set by the user for correct delay operation, and message that upstream patch would be appreciated. Signed-off-by: Matteo Golin <[email protected]>
Warning that CONFIG_BOARD_LOOPSPERMSEC must be set by the user for correct delay operation, and message that upstream patch would be appreciated. Signed-off-by: Matteo Golin <[email protected]>
Warning that CONFIG_BOARD_LOOPSPERMSEC must be set by the user for correct delay operation, and message that upstream patch would be appreciated. Signed-off-by: Matteo Golin <[email protected]>
Warning that CONFIG_BOARD_LOOPSPERMSEC must be set by the user for correct delay operation, and message that upstream patch would be appreciated. Signed-off-by: Matteo Golin <[email protected]>
Warning that CONFIG_BOARD_LOOPSPERMSEC must be set by the user for correct delay operation, and message that upstream patch would be appreciated. Signed-off-by: Matteo Golin <[email protected]>
c1e7802 to
f9934fe
Compare
In Kconfig, it is valid for configuration variables to have no value. However, the replace logic in this cmake file will throw an error that at least four arguments are required if 'Value' is empty. This commit avoids performing the replacement logic if 'Value' is empty to prevent an error on valid, empty configuration variables. Signed-off-by: Matteo Golin <[email protected]>
The simulator cannot have a value of BOARD_LOOPSPERMSEC since it will vary depending on the host machine running the simulator. Since the simulator uses ALARM_ARCH, a value of 0 can be used since a more accurate, timer-based method will take over as soon as `clock_initialize()` is called in the boot process. Signed-off-by: Matteo Golin <[email protected]>
03d0d78 to
54eeedd
Compare
Gave proper value for CONFIG_BOARDLOOPSPERMSEC to avoid build error. Signed-off-by: Matteo Golin <[email protected]>
|
Can anyone tell me how I might run |
I tried to adapt that to armv7 by using |
Summary
This commit removes the default Kconfig value for CONFIG_BOARD_LOOPSPERMSEC.
This will result in a compile time error for any users who have not configured
this value for their board.
QEMU ARMv8-A configurations were given a value obtained by running
calib_udelayon the NSH configuration.Closes #17004.
Impact
Users will no longer waste time debugging issues that result from having an incorrect value of this configuration option, causing delay/timing issues.
This will affect any user-created defconfig (or NuttX provided defconfig)
without a value set for
CONFIG_BOARD_LOOPSPERMSECby preventing them fromcompiling. It causes files that use
CONFIG_BOARD_LOOPSPERMSECwhile it isundefined to fail compilation.
Testing
Compiling the configuration
nucleo-g474re:lpuartnsh(currently has no valueset for
CONFIG_BOARD_LOOPSPERMSEC), we see the compile error as desired:When we set the parameter to a dummy value of 0, compilation passes:
Since some boards do not have active users who can provide us with a correct
value, I have set those board configurations to use a value of
CONFIG_BOARD_LOOPSPERMSEC=0to allow this PR to be merged without breaking theCI runs due to build errors. However, these boards have a warning in the
board.hfile now to ask users to use a proper value and please submit thepatch to the upstream repository. Here is what that looks like at compile time
for one of the stm32 boards: