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

Use of RTX layer in mbed_error #9125

Closed
embeddedteam103 opened this issue Dec 17, 2018 · 9 comments
Closed

Use of RTX layer in mbed_error #9125

embeddedteam103 opened this issue Dec 17, 2018 · 9 comments

Comments

@embeddedteam103
Copy link
Contributor

Description

As I mentioned in ARM-software/CMSIS_5#479 we work with mbed-os with a different implementation of the cmsis-os abstraction layer based on the closed source threadx rtos.
There are cases where internal details of rtx are used in mbed.

One of the newer ones is with the introduction of mbed_error in one of the recent releases.
It uses information of the current thread:

#ifdef MBED_CONF_RTOS_PRESENT

Note that AFAICT this information cannot be retrieved from the cmsis abstraction layer and the only solution for this bug is an extension of the cmsis api.
Do you think such an extension is viable?

Issue request type

[ ] Question
[ ] Enhancement
[x] Bug
@embeddedteam103 embeddedteam103 changed the title Use of RTX implementation details in mbed_error Use of RTX layer in mbed_error Dec 17, 2018
@0xc0170
Copy link
Contributor

0xc0170 commented Dec 17, 2018

@SenRamakri please review. Was this usage reported back to CMSIS_5 as a issue/pull request for adding this functionality ?

I do not see this functionality covered in cmsis rtos v2.

@kjbracey
Copy link
Contributor

As of a very recent patch release of CMSIS-RTOS 2 / RTX, osThreadGetId is now documented callable from interrupt.

I know this, because it came up in #8961

Documentation doesn't specify what it does from exception, but it seems that RTX's implementation does return the OS's current thread, even when called from exception context. (At some point I thought it returned 0). So osThreadGetId() here might do.

The remaining points are:

  • can CMSIS-RTOS document that it returns current thread, so is good for this purpose (or profiling)?
  • can it be called from thread context with ISRs disabled, which might happen here?

The latter is never specified for any CMSIS-RTOS call, but generally it seems that if a call is documented as callable from interrupt, thread context with ISRs disabled also works, at least for RTX.

If you haven't hit it already, you will soon hit the osKernelSuspend in mbed_rtx_idle.cpp. I can't see how to do that without a race with only CMSIS-RTOS API. (See #6534 for discussion - don't think an issue was raised anywhere).

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 17, 2018

@JonatanAntoni please review

@embeddedteam103
Copy link
Contributor Author

If you haven't hit it already, you will soon hit the osKernelSuspend in mbed_rtx_idle.cpp. I can't see how to do that without a race with only CMSIS-RTOS API. (See #6534 for discussion - don't think an issue was raised anywhere).

The idle thread is actually something we haven't implemented in our cmsis wrapper at all (due to differing functionality of suspend/resume in threadx) and we simply rolled our own thread_idle implementation.

We are actually planning to move to the standard one in some point.
I will look at this issue to see how it would work in threadx when we finally decide to prioritze this.

@JonatanAntoni
Copy link
Member

JonatanAntoni commented Dec 17, 2018

Hi @kjbracey-arm,

The remaining points are:
can CMSIS-RTOS document that it returns current thread, so is good for this purpose (or profiling)?
can it be called from thread context with ISRs disabled, which might happen here?

The current documentation of osThreadGetId() states

[..] returns the thread object ID of the currently running thread.

So, yes, for RTX it simply returns osRtxInfo.thread.run.curr, which is the currently scheduled thread.

And yes, it can be called from ISR context since a while (or with interrupts disabled/masked).
Please be aware of possible "race conditions" when relying on this information from ISR context.

Low-power (or tickless) operation is tricky and might be implemented very differently. Hence we never tried to abstract this functionality using CMSIS-RTOS2 APIs. The API initial intention of this API is not to fully decouple user applications from the actual RTOS implementation but allow generic middle-ware components (like com stacks for instance).

Cheers,
Jonatan

@kjbracey
Copy link
Contributor

returns the thread object ID of the currently running thread.

To me that is not 100% concrete - if called from exception context, arguably that thread isn't currently "running" - it's been preempted by the exception - so I can imagine another CMSIS-RTOS implementer doing something different. Another possible definition would be to return NULL if not called from thread context, and that would make more sense if, say, using it to implement std::this_thread::get_id(); if you were permitting that to be called from actual exception context (as opposed to just ISRs disabled), it wouldn't make much sense given its name to return the current foreground thread.

(Been spending far too much time pinning down some of this "current thread" stuff in RTOS awareness in pyOCD recently.)

And yes, it can be called from ISR context since a while (or with interrupts disabled/masked).

In the past I do recall there were calls that were callable from ISR, but faulted if called from thread context with ISRs disabled. I know in RTX 4 either semaphore-post or flag-set blew up. Is this a general rule now that "callable from ISR" context now permits that case? Is this stated?

@ciarmcom
Copy link
Member

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-341

@hugueskamba
Copy link
Collaborator

As there is no direct CMSIS equivalent for most RTX functionalities used in mbed_error.c, it is not worth changing only the ones available (osThreadGetId(), osThreadGetStackSize(), osThreadGetStackSpace()) and still have some RTX references.
I discussed this with @BartSX

@adbridge
Copy link
Contributor

adbridge commented Jun 7, 2021

As there has been no movement on this issue for the last 2 years I am closing it. Please re-open if it is still a valid issue.

@adbridge adbridge closed this as completed Jun 7, 2021
@adbridge adbridge removed this from Untriaged in Issue Severity Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants