mcu-debug / rtos-views

RTOS views for microcontrollers
MIT License
24 stars 11 forks source link

ChibiOS needs to handle optional thread data better #2

Closed haneefdm closed 1 year ago

haneefdm commented 1 year ago

Currently, if there are no thread status, it would crash. I fixed (kludge) the crash just to move forward but we need to handle this better.

image

We need to either remove those columns and/or provide help for how the user can get those stats (what do they have to do in their FW).

I also do not like the top of the stack being printed for each thread. If you don't have thread names, so be it. Don't combine that with Status. that column should simply be Status.

haneefdm commented 1 year ago

@vrepetenko, can you take a look at this?

PhilippHaefele commented 1 year ago

@vrepetenko wrote following in the Cortex-Debug issue

there is a problem with stack size detection in current ChibiOS thread_t implementation, no chance to get stack size/stack start, only stack base (bottom) and free size. New ChibiOS version with stack size is coming, but how should we fill stack top , stack start now? I use SP value for stack start/top now, but sometimes SP can have zero value, is it ok?

So it seems that you are using the released version (which i used too for my tests)

According to the info above, the output looks ok to me

haneefdm commented 1 year ago

No, using SP does not make sense. There is only one SP and only makes sense for one thread. Do not try to guess is my opinion.

Please see the screenshots in the top-level readme.md file.

haneefdm commented 1 year ago

With FreeRTOS, I had to go through all combinations of optional stuff.

PhilippHaefele commented 1 year ago

One thing i found out is that they have a variable in their thread handle named sp. most likely this variable was ment and not the CPU SP register

haneefdm commented 1 year ago

One thing i found out is that they have a variable in their thread handle named sp. most likely this variable was meant

Could be. We may have to study the OS code or ask. Any OS will need to remember the tasks stack-top to eventually switch to that task

So, we combined 4 things into one column? Thread Name, Address, State and Priority. But, flags gets its own column. Not sure how I feel about this. I agree that this is too many columns

haneefdm commented 1 year ago

Here is the FreeRTOS view with expanded Help -- I thought I had it in fixed width font and it is broken right now. You won't see any HELP is none is needed

image

Even the Thread/Task name is optional in FreeRTOS

haneefdm commented 1 year ago

Warning: I am going through one major edit/refactoring

It is bleeding edge indeed

haneefdm commented 1 year ago

Done. Varialble values are no longer suffixed with '-val', '-exp', etc. Instead we create an object... Meaning to do this forever

https://github.com/mcu-debug/rtos-views/blob/a42fb075c327569e03107763173e64b3d7bae426/src/rtos/rtos-common.ts#L18

export interface VarObjVal {
    val: string;                    // The value
    ref: number;                    // Variable reference. Technically, this is not a number, it is handle
    exp: string | undefined;        // This is the expression that represents the value
}

export type RTOSStrToValueMap = { [key: string]: VarObjVal };
vrepetenko commented 1 year ago

@vrepetenko, can you take a look at this?

sure, it is in todo list

vrepetenko commented 1 year ago

Currently, if there are no thread status, it would crash. I fixed (kludge) the crash just to move forward but we need to handle this better.

image

We need to either remove those columns and/or provide help for how the user can get those stats (what do they have to do in their FW).

I also do not like the top of the stack being printed for each thread. If you don't have thread names, so be it. Don't combine that with Status. that column should simply be Status.

Thread name, address, state and priority combined in one column to make table more compact, otherwise it looks weird on normal screen size.

vrepetenko commented 1 year ago

Now there is help information:

Screenshot 2022-10-15 at 12 39 07
PhilippHaefele commented 1 year ago

Thread name, address, state and priority combined in one column to make table more compact, otherwise it looks weird on normal screen size.

I personally do not see a need to always show the thread address. Either name or thread address as a fallback would be enough.

To further improve size, you can combine Stack stuff into a percentage column, as at least i do not see a big value in listing the start and end address. It's much more interesting how much is currently (and assumed 'peak' by looking for not default stack pattern) is used/free. That's why i introduced the percentage column type and added it to the uC/OS-II view (see a screenshot in the README.md)

After that there should be enough/more space for a separate state and Prio column

vrepetenko commented 1 year ago

Thread name, address, state and priority combined in one column to make table more compact, otherwise it looks weird on normal screen size.

I personally do not see a need to always show the thread address. Either name or thread address as a fallback would be enough.

To further improve size, you can combine Stack stuff into a percentage column, as at least i do not see a big value in listing the start and end address. It's much more interesting how much is currently (and assumed 'peak' by looking for not default stack pattern) is used/free. That's why i introduced the percentage column type and added it to the uC/OS-II view (see a screenshot in the README.md)

After that there should be enough/more space for a separate state and Prio column

I would suggest to keep thread address, it is really useful to quick create watch like this: (thread_t*) [thread address].

There is no way to show percentage column now, it is not possible to get thread stack size, only SP, wabase (stack bottom) and peak usage - by scanning 0x55 from wabase to SP.

Question: is it possible to get MCU registers (to get real SP for current thread)?

haneefdm commented 1 year ago

Question: is it possible to get MCU registers (to get real SP for current thread)?

Majority of the time evaluating $sp gets you the current SP right? But it could be wrong. When the program is paused, execution can be anywhere including in the kernel, about to context switch and SP could be in no thread. Current thread is just a notion kept by the Kernel.

Note: I do not like us to get down to the register level here. I want this extension to be processor agnostic. There will be RISC-V clients.

I recommend you look at the following (See line 478) https://openocd.org/doc/doxygen/html/chibios_8c_source.html

On the surface, it appears to be the p_ctx field of the data structure. But this data structure is available only if the user enabled debug. And, still might not be right for current thread.

vrepetenko commented 1 year ago

yes, I know ch_debug structure, you can use it in plugins/code with restricted symbols reading.

haneefdm commented 1 year ago

Now, I am beginning to wonder if the StackTop for any RTOS is correct. I have to run some experiments. There is no way any kernel can keep this up-to-date while the current thread is running. At best the StackTop for FreeRTOS for instance would be the top the last time it got kicked off.

haneefdm commented 1 year ago

Yup. For FreeRTOS, the Stack Top is definitely wrong. For current, I have a couple of options

  1. Depending on arch use $sp. For now, this works for both ARM, ARC, RISC-V. Even x86!! But not Xtensa -- it is $a1. Anyways, these are the architectures I run into.
  2. If we know the stack range, it should be within that range and we are done
  3. Else we may still be okay to use the $sp as if it is in Kernel code, there shouldn't be (mostly) a current-thread. The window of vulnerability is tiny
  4. We can figure this out if this becomes a problem that the stack top should be within a certain range of a user-specified limit or we would not use it.

Just thinking out loud

I will make a common function to get the current stack pointer that we all can use.

PhilippHaefele commented 1 year ago

Yup. For FreeRTOS, the Stack Top is definitely wrong. For current, I have a couple of options

  1. Depending on arch use $sp. For now, this works for both ARM, ARC, RISC-V. Even x86!! But not Xtensa -- it is $a1. Anyways, these are the architectures I run into.

  2. If we know the stack range, it should be within that range and we are done

  3. Else we may still be okay to use the $sp as if it is in Kernel code, there shouldn't be (mostly) a current-thread. The window of vulnerability is tiny

  4. We can figure this out if this becomes a problem that the stack top should be within a certain range of a user-specified limit or we would not use it.

Just thinking out loud

I will make a common function to get the current stack pointer that we all can use.

I would be very nice if we could get the actual stack usage of the current running thread, as it improves debugging stack issues a lot.

One more thing to consider is if the current execution is a interrupt/exception (mostly your point 3, but not sure what you mean exactly with kernel code), as they mostly use a dedicated system stack (initial stack pointer before start of OS). At least it's like that for uC/OS-II on Cortex-M processors.

When we implemented a uC/OS-II J-Link thread aware plugin we did this by checking the OSIntNesting variable of uC/OS and the NVIC VECTACTIVE bit. So we maybe also need a common function to test that that one.

Just my thoughts on this topic

@haneefdm Did you do anything regarding this topic until now?

Best regards

haneefdm commented 1 year ago

Unfortunately, I have not had any time to look at this. By Kernel code meaning when you in the scheduler when it is about to swap out one thread with another. But this could include interrupt/exceptions

VECTACTIVE is a good idea but it is very ARM specific. I am expecting non-ARM clients as well. I will keep this in mind.

vrepetenko commented 1 year ago

Hi, @PhilippHaefele !

Do you run J-Link plugin on macOs without signing plugin lib?

When we implemented a uC/OS-II J-Link thread aware plugin we did this by checking the OSIntNesting variable of uC/OS and the NVIC VECTACTIVE bit. So we maybe also need a common function to test that that one.

PhilippHaefele commented 1 year ago

Hi, @PhilippHaefele !

Do you run J-Link plugin on macOs without signing plugin lib?

When we implemented a uC/OS-II J-Link thread aware plugin we did this by checking the OSIntNesting variable of uC/OS and the NVIC VECTACTIVE bit. So we maybe also need a common function to test that that one.

Hi,

sorry but i do not use it on MacOS, so i didn't have seen this issue.

Best regards

Philipp

PhilippHaefele commented 1 year ago

@haneefdm @vrepetenko Are there any points open regarding ChibiOS?

I will split out the SP thing of the current running task into a separate issue

vrepetenko commented 1 year ago

@haneefdm @vrepetenko Are there any points open regarding ChibiOS?

I will split out the SP thing of the current running task into a separate issue

Do not remember any but I am working on new version now:

Screenshot 2023-02-21 at 23 13 33

and would like to discuss separation of UI(View) and RTOS Data Model.

PhilippHaefele commented 1 year ago

Ok, so i will close this one now.

Your approach looks and reads reasonable. Please create another issue for discussion of this if needed.

haneefdm commented 1 year ago

I will create a new issue based on @vrepetenko comment

A couple of things:

It is one of those things. I started out small, I didn't know much about WebViews back then and followed examples to generate the entire HTML/CSS on the fly. But with memory view, I learned a LOT, and I used React

My worry is that React is not that easy or simple for most people. It also has quite a few UI bugs/quirks that they can't fix for legacy reasons. Using React (or something else) means that the view(s) is totally separated from the model(s)

haneefdm commented 1 year ago

Submitted new issue https://github.com/mcu-debug/rtos-views/issues/29