Skip to content
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

Fix RP2040 build without configASSERT defined #1170

Closed

Conversation

jackwilsdon
Copy link

@jackwilsdon jackwilsdon commented Oct 31, 2024

Description

Fixes building the RP2040 port without configASSERT defined. It seems like no other ports use configASSERT in portmacro.h, so this isn't an issue anywhere else. Normally FreeRTOS.h defines configASSERT, but that can't be included in portmacro.h.

This may be a bit easier to review with whitespace changes ignored: https://github.com/FreeRTOS/FreeRTOS-Kernel/pull/1170/files?w=1

Test Steps

Build the RP2040 port without configASSERT defined.

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jackwilsdon jackwilsdon requested a review from a team as a code owner October 31, 2024 14:39
Copy link

sonarcloud bot commented Oct 31, 2024

@kar-rahul-aws kar-rahul-aws self-requested a review October 31, 2024 16:40
@ActoryOu
Copy link
Member

ActoryOu commented Nov 1, 2024

Hello @jackwilsdon,
Could you help share the compile error log when configASSERT is not defined without this PR?
The fix here is inside a inline function. It should be no issue If the caller also includes the FreeRTOS.h (before portmacro.h).

Thank you.

@jackwilsdon
Copy link
Author

jackwilsdon commented Nov 1, 2024

FreeRTOS.h imports portable.h (which imports portmacro.h) before it defines a default for configASSERT.

In file included from vendor/FreeRTOS-Kernel/include/portable.h:53,
                 from vendor/FreeRTOS-Kernel/include/FreeRTOS.h:107,
                 from src/main.c:1:
vendor/FreeRTOS-Kernel/portable/ThirdParty/GCC/RP2040/include/portmacro.h: In function 'vPortRecursiveLock':
vendor/FreeRTOS-Kernel/portable/ThirdParty/GCC/RP2040/include/portmacro.h:216:5: warning: implicit declaration of function 'configASSERT' [-Wimplicit-function-declaration]
  216 |     configASSERT( ulLockNum < portRTOS_SPINLOCK_COUNT );
      |     ^~~~~~~~~~~~
/usr/lib/gcc/arm-none-eabi/13.2.1/../../../arm-none-eabi/bin/ld: vendor/FreeRTOS-Kernel/tasks.c.obj: in function `vPortRecursiveLock':
vendor/FreeRTOS-Kernel/portable/ThirdParty/GCC/RP2040/include/portmacro.h:216:(.text.prvCheckForRunStateChange+0x30): undefined reference to `configASSERT'

@ActoryOu
Copy link
Member

ActoryOu commented Nov 1, 2024

Hi @jackwilsdon,
May I know the configASSERT definition in your FreeRTOSConfig.h?
If there is no definition, would it be easier to define it to nothing like below if you don't need it?

#define configASSERT( x )

EDIT: FreeRTOS.h includes FreeRTOSConfig.h at line 58. The configASSERT is defined before including portable.h.
EDIT2: Sorry that I missed the default setting part below. Let me bring the discussion back to the team then reply you. But the method I provided here should help you solve this as workaround.

Thank you.

@jackwilsdon
Copy link
Author

jackwilsdon commented Nov 1, 2024

I'm already defining an empty configASSERT as a workaround (which does fix the issue) - it'd be nice if it was consistent with other ports though.

@ActoryOu
Copy link
Member

ActoryOu commented Nov 4, 2024

Hi @jackwilsdon,
Thanks for your patient! We had a discussion about this topic.
We'll create a separate PR to move configASSERT definition above before including portable.h.
Then we don't need to handle this in the port layer anymore!
Will make a note here once the PR is created.

Thank you.

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.

3 participants