Skip to content

printk: Add tests for print format #44193

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

Merged
merged 2 commits into from
Mar 29, 2022

Conversation

yperess
Copy link
Collaborator

@yperess yperess commented Mar 25, 2022

Add tests to verify that the various PRI\w\d+ macros map to the
correct U?INT\d+_C macros.

Signed-off-by: Yuval Peress [email protected]

Fixes #44199

@yperess yperess requested a review from MaureenHelm March 25, 2022 06:23
@yperess yperess requested a review from nashif as a code owner March 25, 2022 06:23
@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label Mar 25, 2022
@yperess
Copy link
Collaborator Author

yperess commented Mar 25, 2022

This is a PR that's building on @MaureenHelm 's find at #41735 (review). It's still a work in progress, but basically I'm finding that in the Kinetis K6X a value such as 8U is an unsigned long instead of the expected unsigned int. I'm still not sure what the right fix is.

@yperess yperess marked this pull request as draft March 25, 2022 06:28
@github-actions github-actions bot added the area: API Changes to public APIs label Mar 25, 2022
@stephanosio stephanosio added bug The issue is a bug, or the PR is fixing a bug area: Portability Standard compliant code, toolchain abstraction labels Mar 25, 2022
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

the changes to zephyr_stdint.h should be a separate commit (you can cherry-pick the existing commit 2f6c982).

By the way, during local testing, I saw a bunch of timeouts from twister even though the actual QEMU output was correct.

Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

toolchain: Add missing U?INT(32|64)_C implementations

These implementations were missing/incorrect for several architectures
that need the zephyr_stdint.h.

Signed-off-by: Yuval Peress <[email protected]>

As noted in #44199 and 2f6c982, these macros were not missing -- they are always internally defined by the toolchain/compiler.

The problem is that Zephyr defines its own stdint types and, unless we re-define/override these integer constant macros as well, they are not going to correspond.

Please update the commit message accordingly.

@yperess yperess marked this pull request as ready for review March 25, 2022 21:50
@yperess yperess marked this pull request as draft March 25, 2022 21:51
This commit overrides the toolchain internal `__INTN_C(value)` integer
constant macros to match the types defined by the `zephyr_stdint.h`
header.

For more details, refer to the GitHub issue zephyrproject-rtos#44199.

Signed-off-by: Stephanos Ioannidis <[email protected]>
@yperess
Copy link
Collaborator Author

yperess commented Mar 25, 2022

toolchain: Add missing U?INT(32|64)_C implementations

These implementations were missing/incorrect for several architectures
that need the zephyr_stdint.h.

Signed-off-by: Yuval Peress <[email protected]>

As noted in #44199 and 2f6c982, these macros were not missing -- they are always internally defined by the toolchain/compiler.

The problem is that Zephyr defines its own stdint types and, unless we re-define/override these integer constant macros as well, they are not going to correspond.

Please update the commit message accordingly.

Sorry, finally figured out how to cherry pick from the other repo without adding another remote to my git :) I'm still seeing the timeouts so I'll figure that out and remove the WIP label then

@yperess yperess marked this pull request as ready for review March 26, 2022 15:58
@yperess yperess requested a review from stephanosio March 26, 2022 15:58
@stephanosio
Copy link
Member

@yperess By the way, what was causing the timeout earlier?

@yperess
Copy link
Collaborator Author

yperess commented Mar 26, 2022

@yperess By the way, what was causing the timeout earlier?

A copy paste error in the expected console output. I didn't change the numbers to octal base for PRIo* ☺️

Add tests to verify that the different print formats and constant
macros match.

Fixes zephyrproject-rtos#44199

Signed-off-by: Yuval Peress <[email protected]>
@mbolivar-nordic mbolivar-nordic merged commit fab5bb9 into zephyrproject-rtos:main Mar 29, 2022
@yperess yperess deleted the print_format branch April 4, 2022 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Portability Standard compliant code, toolchain abstraction area: Tests Issues related to a particular existing or missing test bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(U)INT{32,64}_C macro constants do not match the Zephyr stdint types
4 participants