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

kernel: banner: cleanup implementation #51533

Merged
merged 2 commits into from
Oct 28, 2022
Merged

kernel: banner: cleanup implementation #51533

merged 2 commits into from
Oct 28, 2022

Conversation

JordanYates
Copy link
Collaborator

Cleanup the mess of duplicate function definitions, unnecessary variables and duplicate strings. All banner strings are now constant in ROM. Also fixes a double space between the end of the version string and the trailing *** when there is no boot delay.

Jordan Yates added 2 commits October 22, 2022 18:17
The BOOT_DELAY option does nothing in code if MULTITHREADING is not
enabled. Move the dependency to Kconfig instead.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
Cleanup the mess of duplicate function definitions, unnecessary
variables and duplicate strings. All banner strings are now constant in
ROM. Also fixes a double space between the end of the version string and
the trailing `***` when there is no boot delay.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
@@ -331,6 +331,7 @@ config BOOT_BANNER

config BOOT_DELAY
int "Boot delay in milliseconds"
depends on MULTITHREADING
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I can see it doesn't actually depend on MULTITHREADING. Probably it's worth to drop this restriction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding is that this delay is so that other threads can run and perform setup operations. Without MULTITHREADING what reason is there for the delay?

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense for certain tests. Please leave it without any dependency on multithreading.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But the previous implementation did not delay at all when MULTITHREADING was disabled. Dropping that dependency would be a change in behaviour. This PR is only meant to cleanup the implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we have that limitation. I'd say a clean-up could also simplify this by removing that condition.

Copy link
Member

Choose a reason for hiding this comment

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

I am with @JordanYates, do not change behaviour in a cleanup PR. This keeps biting us.. If there is something to improve or fix, please do that in a different PR. In this case, I remember the issue and it was added for a reason: #14454

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, fair enough :)

@nashif nashif merged commit d1d20c0 into zephyrproject-rtos:main Oct 28, 2022
@JordanYates JordanYates deleted the 221022_boot_banner branch October 28, 2022 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants