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

Add ePendingReady state for task in a pending ready list #696

Closed

Conversation

chinglee-iot
Copy link
Member

@chinglee-iot chinglee-iot commented Jun 26, 2023

Add ePendingReady state.
The unit test is updated in another PR.

Description

  • Add ePendingReady state for task in a pending ready list.

Test Steps

Run the unit test case test_eTaskGetState_success_not_current_tcb_pending_ready.

  • Before this PR, eReady is expected.
  • After this PR, ePendingReady should be expected.

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

* Add ePendingReady state for task in a pending ready list
@chinglee-iot chinglee-iot requested a review from a team as a code owner June 26, 2023 05:16
@chinglee-iot chinglee-iot requested review from ActoryOu and aggarg June 26, 2023 05:16
@chinglee-iot
Copy link
Member Author

The uxTaskGetSystemState needs more consideration. uxTaskGetSystemState also suspends the scheduler.
If a blocked or suspended task is readied by an ISR while the running task suspends the scheduler, the task's state will still be eBlocked or eSuspended with current logic. vTaskList also make use of this information.

The following approach may fix the problem:

  • Get the system state in critical section instead of suspending scheduler. This ensures that no task will be added in pending ready list when getting system state.
  • Handle tasks already in pending ready separately in uxTaskGetSystemState.

@jefftenney
Copy link
Contributor

jefftenney commented Jun 28, 2023

(Edit: Deleted a comment that overlooked the inconsistency between uxTaskGetSystemState() and eTaskGetState(). It seems the inconsistency was introduced in #679.)

Is there a benefit to distinguishing ePendingReady from eReady in the API? The pending-ready status is a scheduler implementation detail not currently exposed to the API. From the user's perspective, maybe a task in the pending-ready list really is eReady.

@RichardBarry
Copy link
Contributor

Reading Jeff's comment, I agree it could be confusing to expose internal implementation detail. The PR is responding to this scenario: #577, so the question is, is that a valid scenario. Is it valid to call eTaskGetState() when the scheduler is suspended when the generalised rule of thumb is not to call API functions from a critical section or with the scheduler suspended.

The head revision already returns eReady for this scenario.

@chinglee-iot
Copy link
Member Author

Agreed with Jeff's and Richard's comment. Let's close this PR and create another for minor update with eReady comment.

@jefftenney
Copy link
Contributor

@chinglee-iot Prior to #679, eTaskGetState() would return eBlocked or eSuspended under the conditions being discussed. After #679, it returns eReady. However, #679 did not make the corresponding change to uxTaskGetSystemState(), which still returns eBlocked or eSuspended. Thus #679 introduced a minor inconsistency. Addressing that inconsistency would probably make a good PR.

In my opinion returning eReady is ideal behavior, even considering that a developer making this API call with the scheduler suspended is suspect, as Richard says. So I think changing uxTaskGetSystemState() to match the current eTaskGetState() behavior is best.

@chinglee-iot
Copy link
Member Author

@jefftenney I will create another PR for the uxTaskGetSystemState() function. Let's discuss in the new PR.

@chinglee-iot
Copy link
Member Author

We created another PR for uxTaskGetSystemState() function. #702

@chinglee-iot chinglee-iot deleted the add-ePendingReady-state branch July 26, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants