xiaoxiang781216 commented on code in PR #17011:
URL: https://github.com/apache/nuttx/pull/17011#discussion_r2691718401
##########
boards/arm/mps/mps2-an500/include/board.h:
##########
@@ -33,6 +33,12 @@
* Pre-processor Definitions
****************************************************************************/
+#warning "CONFIG_BOARD_LOOPSPERMSEC is set to 0 to prevent CI build errors." \
Review Comment:
> That misses the whole point of this PR. There is no default value for
`BOARD_LOOPSPERMSEC` so that an error is thrown (for undefined symbol) when the
user forgets to set the value.
set default value to zero and static_assert(or DEBUGASSERT)
BOARD_LOOPSPERMSEC != 0 can report the same error too.
> This prevents missing values on new board ports and for users making new
configurations. I think this part of it is fine; no default value -> error when
undefined.
>
zero value->error also fine.
> The problem arising from this is that there ~10 boards with no
LOOPSPERMSEC value and where I do not have the ability to calibrate one (as you
suggest in 2).
these boards don't set LOOPSPERMSEC because all of them use arch_alarm, and
arch_alarm doesn't use LOOPSPERMSEC in most case(except delay is called before
g_oneshot_lower set, which doesn't happen at all).
> These boards would block this PR from being merged because the undefined
symbol errors will break the CI. This is also why your 1st suggestion isn't
possible for these boards (I don't want to break CI permanently). The error
behaviour is intended for new boards, but these legacy boards can't be fixed
without users to calibrate a value. In these specific cases, I have added a
warning to inform any users of the issue but not break the CI process.
>
I suggest you move the warning/error to the code which really use it, which
can help you:
1. arch_alarm/arch_timer change to runtime check since it's rare used with
arch_alarm/arch_timer
2. use static_assert for other cases, since LOOPSPERMSEC is always used
I will create a patch for item 1.
> I put the warning in board.h since the warning is specific to these boards
who do not have a value and for which no one owns the board to provide me with
one. If I am understanding correctly, you would prefer the warning go in the
location where `CONFIG_BOARD_LOOPSPERMSEC` is used and be raised when
`CONFIG_BOARD_LOOPSPERMSEC=0`. I am suggesting not to do that because some
boards can have `CONFIG_BOARD_LOOPSPERMSEC=0` as proper operation.
I only want the warnings to affect those ~10 boards which I described.
However, if it will block this PR from being merged, I could move this warning
to the delay implementation C files like you suggest and raise it when
`CONFIG_BOARD_LOOPSPERMSEC=0`. Just this will result in the warning being
raised for some boards where `CONFIG_BOARD_LOOPSPERMSEC=0` on purpose.
As I explain before, all these are fake alarm since
CONFIG_BOARD_LOOPSPERMSEC is never used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
