nuttx icon indicating copy to clipboard operation
nuttx copied to clipboard

arch/delays: Set invalid default value for BOARD_LOOPSPERMSEC

Open linguini1 opened this issue 3 months ago • 19 comments

Summary

This commit sets the default Kconfig value for CONFIG_BOARD_LOOPSPERMSEC as -1, which is invalid. It statically asserts at compile time that the value is greater than or equal to 0 so that the user is notified if their board/configuration does not yet have a calibrated value for this configuration option. The user is directed to set a non-negative value and then later calibrate with calib_udelay.

QEMU ARMv8-A configurations were given a value produced by calib_udelay when run on the qemu-armv8a: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_LOOPSPERMSEC by preventing them from compiling. It causes files with the assertion to fail compilation. I have restricted these files to only those which directly use the macro for some implementation of something.

FYI, I have used static_assert to achieve the compilation error here since it is defined in the NuttX implementation of assert.h and it is used elsewhere within the codebase, so I assume it is safe for the toolchains used in NuttX.

Testing

Testing was performed by compiling the raspberrypi-4b:nsh configuration with different values. Testing was done using the gcc compiler.

Compiling with CONFIG_BOARD_LOOPSPERMSEC set to 106645:

$ make -j
LD: nuttx
Memory region         Used Size  Region Size  %age Used
CP: nuttx.hex
CP: nuttx.bin
Generating config.txt

Compiling with CONFIG_BOARD_LOOPSPERMSEC set to the default value of -1:

$ make -j
Create version.h
LN: platform/board to /home/linguini/coding/nuttx-space/apps/platform/dummy
Register: leds
Register: dd
Register: getprime
Register: ostest
Register: sh
Register: nsh
Register: hello
CC:  group/group_waiter.c In file included from /home/linguini/coding/nuttx-space/nuttx/include/nuttx/mutex.h:30,
                 from /home/linguini/coding/nuttx-space/nuttx/include/pthread.h:32,
                 from /home/linguini/coding/nuttx-space/nuttx/include/nuttx/sched.h:37,
                 from /home/linguini/coding/nuttx-space/nuttx/include/nuttx/arch.h:89,
                 from clock/delay.c:53:
clock/delay.c:63:1: error: static assertion failed: "Please set a non-negative value for CONFIG_BOARD_LOOPSPERMSEC to pass compilation. It is recommended that after your initial build, you use the example \'calib_udelay\' to get a precise value for this option. Please search the NuttX documentation for calib_udelay for more information."
   63 | static_assert(
      | ^~~~~~~~~~~~~
make[1]: *** [Makefile:64: delay.o] Error 1
make[1]: *** Waiting for unfinished jobs....
CC:  timers/oneshot.c make: *** [tools/LibTargets.mk:71: sched/libsched.a] Error 2
make: *** Waiting for unfinished jobs....

linguini1 avatar Sep 12 '25 15:09 linguini1

Looks like there are some defconfigs using the default value. Good to catch.

linguini1 avatar Sep 12 '25 16:09 linguini1

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? :-)

cederom avatar Sep 12 '25 18:09 cederom

@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?

acassis avatar Sep 12 '25 18:09 acassis

@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.

That's fine, I plan to make one for BOARD_LOOPSPERMSEC but it's better to redirect to calib_udelay for now, I agree.

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?

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.

linguini1 avatar Sep 12 '25 20:09 linguini1

MSVC failure appears unrelated.

linguini1 avatar Sep 12 '25 20:09 linguini1

MSVC failure appears unrelated.

@simbit18 you are the MS guy, any idea?

acassis avatar Sep 12 '25 21:09 acassis

@acassis @jerpelea do you think this change would be considered a "breaking change"?

linguini1 avatar Sep 12 '25 21:09 linguini1

@acassis @jerpelea do you think this change would be considered a "breaking change"?

No, it is not.

acassis avatar Sep 12 '25 21:09 acassis

Anyone who can calibrate for fvp-armv8r, avaota-a1 and imx8qm-mek?

As a side note, it would be great if the build logs displayed a summary of what failed at the bottom instead of having to scroll through the error output.

linguini1 avatar Sep 13 '25 16:09 linguini1

Anyone who can calibrate for fvp-armv8r, avaota-a1 and imx8qm-mek?

According to codeowners, maybe @qinwei2004, @hujun260 can help with fvp-armv8r, @lupyuen can help with avaota-a1 and @qinwei2004 or @acassis can help iwth imx8qm-mek?

linguini1 avatar Sep 13 '25 18:09 linguini1

Anyone who can calibrate for fvp-armv8r, avaota-a1 and imx8qm-mek?

According to codeowners, maybe @qinwei2004, @hujun260 can help with fvp-armv8r, @lupyuen can help with avaota-a1 and @qinwei2004 or @acassis can help iwth imx8qm-mek?

@linguini1 I don't have imx8qm-mek (first time I heard about this i.MX 8QuadMax MEK). This board is very expensive (more than U$ 1K), it should be nice if we had a low cost board powered by MCIMX8QM.

acassis avatar Sep 13 '25 21:09 acassis

I don't have imx8qm-mek (first time I heard about this i.MX 8QuadMax MEK). This board is very expensive (more than U$ 1K), it should be nice if we had a low cost board powered by MCIMX8QM.

Ah, sorry! I guess the code-owners file might not be super representative. Hopefully someone else has one they can calibrate for.

linguini1 avatar Sep 13 '25 22:09 linguini1

@acassis is there a defined way to document Kconfig options, such as this important constant? I think it would be good to be able to reference it from other places in the RST docs (like a global macro symbol that links to its documentation location). I want to be able to indicate that drivers should not assume this value is defined, and should avoid using it if at all possible. I saw a few drivers and board bring up code that rely on this value where they could instead use up_udelay.

linguini1 avatar Sep 29 '25 21:09 linguini1

@acassis is there a defined way to document Kconfig options, such as this important constant? I think it would be good to be able to reference it from other places in the RST docs (like a global macro symbol that links to its documentation location). I want to be able to indicate that drivers should not assume this value is defined, and should avoid using it if at all possible. I saw a few drivers and board bring up code that rely on this value where they could instead use up_udelay.

it's better to call up_udelay and remove BOARD_LOOPSPERMSEC directly.

xiaoxiang781216 avatar Sep 30 '25 02:09 xiaoxiang781216

it's better to call up_udelay and remove BOARD_LOOPSPERMSEC directly.

I agree completely, hence why I want to document that drivers shouldn't be relying on the LOOPSPERMSEC macro ever, and if they can, they should avoid using up_udelay as well since the default implementation isn't very robust.

linguini1 avatar Sep 30 '25 15:09 linguini1

@acassis is there a defined way to document Kconfig options, such as this important constant? I think it would be good to be able to reference it from other places in the RST docs (like a global macro symbol that links to its documentation location). I want to be able to indicate that drivers should not assume this value is defined, and should avoid using it if at all possible. I saw a few drivers and board bring up code that rely on this value where they could instead use up_udelay.

You can create a Documentation/ to it, but a better idea to avoid user to not setting it is raising a #warn when this symbol has the default value 0

acassis avatar Sep 30 '25 20:09 acassis

The code which redirected the ndelay&udelay to oneshot driver ended up using systick to implement udelay, which caused e.g. udelay(1) to sleep 10000-20000 microseconds instead of busylooping for one microsecond. This just blew up many places where a short bysyloop was needed, in device drivers

Just when you revert the fixes, please make sure that ndelay/udelay never end up to a systick based delay!

Matteo Golin kirjoitti keskiviikko 22. lokakuuta 2025:

@linguini1 commented on this pull request.

@@ -12,6 +12,7 @@ CONFIG_ARCH_BOARD_SIM=y CONFIG_ARCH_CHIP="sim" CONFIG_ARCH_SIM=y CONFIG_BOARD_LATE_INITIALIZE=y +CONFIG_BOARD_LOOPSPERMSEC=0

Xiang can probably answer better, but my understanding is that once the CLOCKDEVICE logic is merged in upstream, that can replace the delay method used by arch_alarm.c and make it more accurate (not using busy-wait, not using system-ticks). Unfortunately I have been to busy to go back and re-watch the talk, but I plan to sometime soon.

-- Reply to this email directly or view it on GitHub: https://github.com/apache/nuttx/pull/17011#discussion_r2452619791 You are receiving this because you were mentioned.

Message ID: @.***

jlaitine avatar Oct 22 '25 19:10 jlaitine

The code which redirected the ndelay&udelay to oneshot driver ended up using systick to implement udelay, which caused e.g. udelay(1) to sleep 10000-20000 microseconds instead of busylooping for one microsecond. This just blew up many places where a short bysyloop was needed, in device drivers

That I can understand, which is definitely suboptimal. However, drivers shouldn't have been relying on up_udelay being accurate either. If the busy delay gets interrupted for 1us because of a context switch, you end up sleeping for 2+us as well. I don't think these implementations differ in that way. But with systick, if you sleep for 1us and get interrupted due to context switch for 10us, when CPU is regained you'll see that >2us has passed and stop immediately. With busy wait, you'll continue the remaining iterations for 1us and total sleep time ends up being 11us. The only time busy wait is better is for sub-systick delays where context switching doesn't happen or very rarely happens.

I personally think neither of these methods are very good, but I do think that the sim architecture needs to adhere to delays.

linguini1 avatar Oct 22 '25 19:10 linguini1

Thank you @jlaitine for the hints here! :-) @linguini1 looks like you found a hard nutt(x) to crack :D :D

cederom avatar Oct 22 '25 21:10 cederom