rtos: zephyr: use thread states from modern Zephyr versions #1823
Conversation
|
Hi @mathieuchopstm, thank you for the contributions. Can you please explain what is the reason and added value of changing current It looks that current display of On the screenshot below you can see comparison of old 
|
| # Idle threads must be handled separately: when not running, | ||
| # they are neither SLEEPING, PENDING nor QUEUED and simply | ||
| # exist in a "limbo state" outside scheduler consideration. |
There was a problem hiding this comment.
Hi @mathieuchopstm,
thank you for the contributions.
Can you please explain what is the reason and added value of changing current
idlethread behavior?It looks that current display of
idlethread can alternate betweenReadyandRunningstate which seems to be correct from behavior point of view. Also,idlethread has the lowest priority defined asK_IDLE_PRIOwhich is displayed as such, yes, it does not compete with other threads of the same priority but it does not seem wrong.On the screenshot below you can see comparison of old
idledisplay vs your suggested :
I hoped the following comment in the code would be a sufficient explanation. But since you are asking, I will write a more verbose justification in the next section:
When idle threads are not running (which, as a reminder, is not reflected by any flag in the k_thread structure), they have state equal to 0: this does not correspond to any value in STATE_NAMES and would result in state UNKNOWN being displayed.
Special handling is required to handle this situation properly. I could have reproduced the old behavior, but I wanted to reflect that idle threads are special: they don't compete with other threads for CPU time. Regardless of system configuration, an idle thread will only be scheduled if there is no other runnable thread.
In a sense, idle threads behave as if they had a priority lower than any other thread in the system, but this is not reflected in the base.prio field (which is irrelevant for idle threads anyways).
One possible implementation would be to say that since K_IDLE_PRIO = K_LOWEST_THREAD_PRIO:
if #is idle thread
if self.state == ZephyrThread.RUNNING:
return "Running; Priority %d" % self.priority + 1
else:
return "Ready; Priority %d" % self.priority + 1But (1) this relies on implementation details and (2) in my opinion it is clearer to mark idle threads explicitly.
Let me know if you want any change or if the current implementation is acceptable.
RobertRostohar
left a comment
RobertRostohar
left a comment
There was a problem hiding this comment.
@mathieuchopstm,
Thanks for your clarifications. I suggest we simplify the display for "idle" (see inline suggestions).
Please also add the Copyright line at the top.
pyocd/rtos/zephyr.py
Outdated
| self._thread_context = ZephyrThreadContext(self._target_context, self) | ||
| self._offsets = offsets | ||
| self._state = ZephyrThread.READY | ||
| self._state = ZephyrThread.QUEUED |
There was a problem hiding this comment.
| self._state = ZephyrThread.QUEUED | |
| self._state = 0 |
pyocd/rtos/zephyr.py
Outdated
| # _kernel.cpus[].idle_thread, which is the same thing). | ||
| if self._name == "idle": | ||
| if self.state == ZephyrThread.RUNNING: | ||
| return "Running; <idle thread>" |
There was a problem hiding this comment.
| return "Running; <idle thread>" | |
| return "Running" |
pyocd/rtos/zephyr.py
Outdated
| if self.state == ZephyrThread.RUNNING: | ||
| return "Running; <idle thread>" | ||
| else: | ||
| return "<idle thread>" |
There was a problem hiding this comment.
| return "<idle thread>" | |
| return "Ready" |
There was a problem hiding this comment.
Would you consider Idle as acceptable? I really dislike Ready for idle threads since they are never part of the ready queue.
There was a problem hiding this comment.
Used Ready as suggested for now.
The existing thread states were coming from a very old Zephyr version. Align to more modern definitions. Also handle the special case of idle threads which don't have a proper state when not running. Signed-off-by: Mathieu Choplain <[email protected]>
mathieuchopstm
force-pushed
the
update_zephyr_rtos_thread_states
branch
from
7d70dde to
41f18a8
Compare
3 participants
The existing thread states were coming from a very old Zephyr version.
Align to more modern definitions (found here: https://github.com/zephyrproject-rtos/zephyr/blob/8d202cb9485558b24310334ef000c171de94ff07/include/zephyr/kernel_structs.h#L51-L73)
Also handle the special case of idle threads which don't have a proper state when not running (for now, this is based on thread name matching; the more solid approach would require a deeper rework + some help from Zephyr)