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

Stack analyzer #1529

Closed
wants to merge 1 commit into from
Closed

Stack analyzer #1529

wants to merge 1 commit into from

Conversation

rakons
Copy link
Contributor

@rakons rakons commented Nov 18, 2019

A code to make stack analysis simple on our examples.

@rakons
Copy link
Contributor Author

rakons commented Nov 18, 2019

cc @grochu @kapi-no @KAGA164

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Nov 18, 2019

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

Copy link
Contributor

@grochu grochu left a comment

Choose a reason for hiding this comment

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

This is going to be a useful module for NCS users - it greatly simplifies setup of monitoring of the thread stacks.

@lemrey
Copy link
Contributor

lemrey commented Nov 19, 2019

This is very nice 🥇 I suggest letting the user pull in this functionality by simply enabling its Kconfig entry (without the addition of any code). That way we'll be able to use from any sample just by editing its project configuration.

@rakons
Copy link
Contributor Author

rakons commented Nov 20, 2019

This is very nice 🥇 I suggest letting the user pull in this functionality by simply enabling its Kconfig entry (without the addition of any code). That way we'll be able to use from any sample just by editing its project configuration.

Good point.
Maybe I will add a configuration to do it automatically - we can use a timer or workqueue for that instead of creating additional thread.
I would leave a possibility to disable automatic printing anyway, as it is possible then to implement any callback function and use the module in different ways than just printing.

@lemrey
Copy link
Contributor

lemrey commented Nov 20, 2019

I would leave a possibility to disable automatic printing

Yes, or print only when the stack usage is above a certain threshold.

@rakons
Copy link
Contributor Author

rakons commented Nov 20, 2019

Is there any way to start a timer without the need to call start function? Currently I do not see a way to implement this code without a modification of the application code when not using additional thread.

@lemrey
Copy link
Contributor

lemrey commented Nov 20, 2019

I think having a dedicated thread for this is fine. I prefer a separate thread rather than a task for the system work queue, because otherwise it will affect the stack usage of the system work queue thread :)
I have not used timers yet, but if you want to call a function automatically at startup you could do that with SYS_INIT (see /include/init.h), note that some OS components are initialized during SYS_INIT so if you rely on any of those you should set the correct priority for your task.

Copy link
Contributor

@kapi-no kapi-no left a comment

Choose a reason for hiding this comment

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

A few nitpicks

include/debug/stack_size_analyzer.h Outdated Show resolved Hide resolved
include/debug/stack_size_analyzer.h Outdated Show resolved Hide resolved
include/debug/stack_size_analyzer.h Outdated Show resolved Hide resolved
include/debug/stack_size_analyzer.rst Outdated Show resolved Hide resolved
@rakons rakons force-pushed the stack_analyzer branch 5 times, most recently from 398561b to 7b7ad85 Compare November 21, 2019 14:33
@joerchan
Copy link
Contributor

This looks very useful. Who should we put into the code-owners file as responsible for the subsys/debug? @carlescufi Any thoughts on this

@rakons
Copy link
Contributor Author

rakons commented Dec 23, 2019

@carlescufi @joerchan
I can add myself to the new files. It seems reasonable.

@ioannisg
Copy link
Contributor

ioannisg commented Jan 2, 2020

There's got to be a code-owner, at least, co-owning debug/ subsystem, IMO.

@carlescufi
Copy link
Contributor

@rakons can you add yourself as an owner for these files along with @ioannisg and also fix CI?

@joerchan
Copy link
Contributor

joerchan commented Jan 9, 2020

rakons can you add yourself as an owner for these files along with ioannisg and also fix CI?

@carlescufi AFAIK we have to wait for upmerge to complete this one.

@rakons
Copy link
Contributor Author

rakons commented Jan 9, 2020

rakons can you add yourself as an owner for these files along with ioannisg and also fix CI?

@carlescufi AFAIK we have to wait for upmerge to complete this one.

Indeed. I have to use k_thread_foreach_unlocked that will be there after upmerge.

@ru-fu ru-fu added the doc-required PR must not be merged without tech writer approval. label Jan 14, 2020
@joerchan
Copy link
Contributor

@rakons Could this be picked up again now?

@rakons
Copy link
Contributor Author

rakons commented Feb 28, 2020

@ru-fu - What documentation is missed?

Copy link
Contributor

@b-gent b-gent left a comment

Choose a reason for hiding this comment

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

I will check documentation early next week

Copy link
Contributor

@nordic-krch nordic-krch left a comment

Choose a reason for hiding this comment

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

few findings

const char *stack = (const char *)thread->stack_info.start;
unsigned int size = thread->stack_info.size;
unsigned int unused = stack_unused_space_get(stack, size);
char name[21];
Copy link
Contributor

Choose a reason for hiding this comment

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

magic number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

subsys/debug/stack_size_analyzer/Kconfig Show resolved Hide resolved
include/debug/stack_size_analyzer.h Show resolved Hide resolved
subsys/debug/stack_size_analyzer/Kconfig Show resolved Hide resolved
*
* Callback function with stack size statistics.
*
* @param name The name of the thread
Copy link
Contributor

Choose a reason for hiding this comment

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

or stringified address of the thread handle if name is not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added

subsys/debug/stack_size_analyzer/Kconfig Outdated Show resolved Hide resolved
endchoice

config STACK_SIZE_ANALYZER_AUTO
bool "Enable Stack size analyzer automatic printing"
Copy link
Contributor

Choose a reason for hiding this comment

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

since it's already in stack size analyzer menu it should be shorter. Just Enable automatic printing is sufficient.

subsys/debug/stack_size_analyzer/Kconfig Outdated Show resolved Hide resolved
@joerchan joerchan requested a review from b-gent March 2, 2020 08:46
@joerchan
Copy link
Contributor

joerchan commented Mar 2, 2020

@rakons Thank you for picking it up. Could you rebase it to fix the conflict?

@joerchan
Copy link
Contributor

joerchan commented Mar 3, 2020

Due to the need to have this functionality easily available for the Bluetooth HCI driver in upstream I suggest we move this functionality to zephyr instead.
HCI Driver uses this: https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/bluetooth/controller/hci/hci_driver.c#L403

This has already a few mentions for providing this functionality from the kernel:
zephyrproject-rtos/zephyr#23104 (comment)

If nobody disagrees then I will start to port this to upstream as @rakons has indicated he does not have the time.

We will then not merge this downstream since we would just have to remove it again in the next upmerge.

@rakons
Copy link
Contributor Author

rakons commented Mar 3, 2020

@joerchan - wait until I fix the issues reported by krch.

@b-gent
Copy link
Contributor

b-gent commented Mar 3, 2020

@joerchan I'm not sure how you want to handle this PR now, I'm adding a small commit with a language review of the RST file that also fixes some RST output issues. I'm pretty sure this will go through another doc review when getting into zephyr.

Copy link
Contributor

@nordic-krch nordic-krch left a comment

Choose a reason for hiding this comment

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

looks ok (checkpatch issues should be fixed at some point)

Stack size analyzer is simple module that helps in printing
stacks usage statistics.

Signed-off-by: Radoslaw Koppel <radoslaw.koppel@nordicsemi.no>
Signed-off-by: Bartosz Gentkowski <bartosz.gentkowski@nordicsemi.no>
@joerchan
Copy link
Contributor

joerchan commented Mar 4, 2020

@b-gent Ok. I'll wait for your RST commit and then port the changes to upstream. Then the rest will be handled in a zephyr PR.

@b-gent
Copy link
Contributor

b-gent commented Mar 4, 2020

@b-gent Ok. I'll wait for your RST commit and then port the changes to upstream. Then the rest will be handled in a zephyr PR.

the commit already went it and got squashed.

@joerchan
Copy link
Contributor

This has been merged upstream now.

@joerchan joerchan closed this Apr 28, 2020
@rakons
Copy link
Contributor Author

rakons commented Apr 29, 2020

Impossible! It was blazingly fast :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.