Skip to content

Conversation

@DNedic
Copy link
Contributor

@DNedic DNedic commented Nov 9, 2024

This caused failed builds due to the missing DT_SIZE_K(x) macro.

@zephyrbot zephyrbot added size: XS A PR changing only a single line of code platform: STM32 ST Micro STM32 labels Nov 9, 2024
@github-actions
Copy link

github-actions bot commented Nov 9, 2024

Hello @DNedic, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@DNedic DNedic force-pushed the soc/fix_stm32h562_includes branch from adbaebc to 3fcfe9f Compare November 9, 2024 23:14
This caused failed builds due to the missing DT_SIZE_K(x) macro.

Signed-off-by: Djordje Nedic <[email protected]>
@DNedic DNedic force-pushed the soc/fix_stm32h562_includes branch from 3fcfe9f to 77470c8 Compare November 11, 2024 17:56
Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Sorry but this is not in line with how mem.h is included in other .dtsi files.
Usually they are included in soc packages variant files stm32h563Xi.dtsi for instance (files which actually make use of memory size macros).

In h562 case, we're missing this file, so actually it's not supposed to be compiled.
If you're facing compilation issues, I guess you've defined one out of tree, and this is the file that should include mem.h but maybe you can pushed it in current PR as well ?

@DNedic
Copy link
Contributor Author

DNedic commented Nov 14, 2024

Hi @erwango , the issue is that this file uses DT_SIZE_K() here itself.

@erwango
Copy link
Member

erwango commented Nov 14, 2024

Hi @erwango , the issue is that this file uses DT_SIZE_K() here itself.

Ok, in that case that's valid indeed, even if this file isn't supposed to be build by itself (out of stm32h562Xi.dtsi like file).

@DNedic
Copy link
Contributor Author

DNedic commented Nov 14, 2024

Upon further inspection, it appears that mem.h is needed in st/h5/stm32h5.dtsi as well, let's not merge this yet.

@mmahadevan108
Copy link
Contributor

@DNedic , can you add a Zephyr Issue with details about the Build failure seen and link the Issue to this PR.

@erwango
Copy link
Member

erwango commented Nov 14, 2024

Upon further inspection, it appears that mem.h is needed in st/h5/stm32h5.dtsi as well, let's not merge this yet.

Yes, and this is probably true for other series as well. But as mentioned, this file isn't supposed to be built outside of a package variant file (which provides flash and ram size and includes mem.h).
This being said, it would actually make sense to move mem.h inclusion in all series from these files to series root files (such as stm32h5.dtsi as you mentioned), this is a bigger change, and there used to be some people arguing against this, but this would be worth trying again. In any case, solution has to remain consistent across family.

@DNedic
Copy link
Contributor Author

DNedic commented Nov 14, 2024

I see, I might not be 100% aware of the context here, but at least in C and C++ it is strongly discouraged for anything to depend on the include order, and every source file or header must include everything it needs. In this case you would have to include mem.h before including everything else in your board file. If this is general practice than it is fine, but it does strike me as odd.

@nashif nashif merged commit 5c4f7d9 into zephyrproject-rtos:main Nov 16, 2024
23 checks passed
@github-actions
Copy link

Hi @DNedic!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform: STM32 ST Micro STM32 size: XS A PR changing only a single line of code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants