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 Critical Section HAL API specification #5346

Merged
merged 8 commits into from Jan 16, 2018

Conversation

Projects
None yet
10 participants
@scartmell-arm
Contributor

scartmell-arm commented Oct 19, 2017

Description

As discussed in NRF51_DK: core_util_are_interrupts_enabled wrong behavior when inside critical section #5198 there is a need for a HAL API specification for platform-specific Critical Section behaviors.

Specifically, the purpose of this pull request is to:

  • Provide a reliable method of detecting if the current program is running within a critical section.

    The current method of checking if the program is in a critical section is to call core_util_interrupts_enabled(), under most circumstances this works as by default entering a critical section disables interrupts. Some platforms do not disable all interrupts when entering a critical section, causing this to fail.

    This PR provides a method core_util_in_critical_section() which specifically checks the state of the program.

  • Move platform-specific implementations for critical sections from the User Facing API into the HAL.

    Added the platform specific HAL functions hal_critical_section_enter and hal_critical_section_exit which are called from the platform independent API core_util_critical_section_enter and core_util_critical_section_exit.

    Added a weak default implementation for HAL API. The default implementation
    matches the previous behavior stored in the User facing API:

    • The first call to enter a Critical Section stores the state of interrupts
      before disabling and each successive call re-disables interrupts.
    • The last call (non-nested) will restore the IRQ state that was set on the
      enter to the critical section. Nested calls are ignored.

Porting Guide

Implement the API declared in mbed-os/hal/critical_section_api.h. If the platform you are porting requires custom critical section behavior that is incompatible with the default (disabling all interrupts).

The functions defined in the header hal_critical_section_enter and hal_critical_section_exit are called on the first entry to, and the last exit from the critical section respectively, any nested calls to enter critical sections do not call the HAL API functions.

You must define the state changes that need to happen when entering a critical section. You should store this state in the critical section translation unit before setting to a state that is safe for the critical section. When hal_critical_section_exit is called the saved state should be restored.

Status

IN DEVELOPMENT

TODO:

  • Porting Guide
  • Test Plan
  • Ported All Boards:
    • NRF51

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:feature-hal-spec-critical-section branch Oct 19, 2017

hal/critical_section_api.h Outdated
* critical section or a nested call. The first call should
* store the state prior to entering the critical section.
*/
void hal_critical_section_enter(const bool in_nested_critical_section);

This comment has been minimized.

@bulislaw

bulislaw Oct 20, 2017

Member

Do we need the parameter? Mbed core is keeping count anyway we may as well call it once? What do you think? I would prefer to have complicated drivers and simple HAL implementation @0xc0170 @c1728p9

This comment has been minimized.

@scartmell-arm

scartmell-arm Oct 23, 2017

Contributor

Yeah, that makes more sense. I did it that way because the original implementation let you call enter multiple times and it would re-disable interrupts. But, that doesn't make much sense as it will assert if they've been re-enabled during the critical section.

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:feature-hal-spec-critical-section branch 2 times, most recently Oct 23, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 24, 2017

@bulislaw are you happy with this now ?

hal/critical_section_api.h Outdated
/** Mark the start of a critical section
*
* This function is called directly by core_util_critical_section_enter on

This comment has been minimized.

@bulislaw

bulislaw Oct 24, 2017

Member

That's a bit sparse, can we add some info about multiple calls - as far as I understand that this function will be only called once (no need to handle enter-...-enter-exit). The same for exit. Are there any other assumptions or limitations we should mention?

hal/mbed_critical_section_api.c Outdated
MBED_WEAK void hal_critical_section_exit()
{
// FIXME

This comment has been minimized.

@bulislaw

bulislaw Oct 24, 2017

Member

Why is that fix me? if we don't know lets get rid of that.

This comment has been minimized.

@scartmell-arm

scartmell-arm Oct 24, 2017

Contributor

It was in the original code, it was added here: Don't call __disable_irq for uVisor. It seems like it still needs resolving at some point so I didn't remove it.

platform/mbed_critical.c Outdated
interrupt_enable_counter--;
critical_section_reentrancy_counter--;

This comment has been minimized.

@bulislaw

bulislaw Oct 24, 2017

Member

Shall we assert at underflowing?

This comment has been minimized.

@scartmell-arm

scartmell-arm Oct 24, 2017

Contributor

I suppose there's no harm in adding an assert, but it shouldn't underflow because it returns early if the counter is already 0.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 24, 2017

Ah and also can we unify the name of mbed_critical_section_api.c to match the header.

@scartmell-arm

This comment has been minimized.

Contributor

scartmell-arm commented Oct 24, 2017

Ah and also can we unify the name of mbed_critical_section_api.c to match the header.

That seems reasonable. But, all of the current HAL files follow that naming scheme and I don't see the distinction from them e.g
mbed_flash_api.c - flash_api.h
mbed_lp_ticker_api.c - lp_ticker_api.h

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 24, 2017

Ah ok fair enough.

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:feature-hal-spec-critical-section branch 2 times, most recently Oct 24, 2017

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 25, 2017

There are some issues with doxy can you please fix them.

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:feature-hal-spec-critical-section branch 2 times, most recently Oct 25, 2017

targets/TARGET_NORDIC/TARGET_NRF5/critical_section_api.c Outdated
void hal_critical_section_enter(void)
{
sd_nvic_critical_region_enter(&_sd_state);

This comment has been minimized.

@0xc0170

0xc0170 Oct 25, 2017

Member

4 spaces indentation

hal/critical_section_api.h Outdated
* function so it can be restored when exiting the critical section.
*
*/
void hal_critical_section_enter(void);

This comment has been minimized.

@0xc0170

0xc0170 Oct 25, 2017

Member

Is it documented anywhere that we are providing default implementation (reference where it can be found) ? I can not spot it, we shall mention it, so during porting, they do not reimplement it all over again

This comment has been minimized.

@scartmell-arm

scartmell-arm Oct 25, 2017

Contributor

There's a brief mention of it in the porting guide, which is in the PR description but I haven't added to the handbook yet. I can make it a bit clearer and point it to the default implementation.

This comment has been minimized.

@0xc0170

0xc0170 Oct 25, 2017

Member

What do you think about at least having a note here in API docs, that we are providing default implementation that can be overriden?

This comment has been minimized.

@scartmell-arm

scartmell-arm Oct 25, 2017

Contributor

Sure, I can make that clearer

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:feature-hal-spec-critical-section branch Oct 25, 2017

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 25, 2017

Please not that this PR is just the code, the other part will be the PG section.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 26, 2017

Is that reviewed now?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 27, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 27, 2017

Build : SUCCESS

Build number : 355
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5346/

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 27, 2017

13:31:31 | NRF51_DK-IAR | NRF51_DK | tests-mbedmicro-rtos-mbed-rtostimer | FAIL | 34.99 | shell |
@scartmell-arm

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:feature-hal-spec-critical-section branch Oct 30, 2017

@scartmell-arm

This comment has been minimized.

Contributor

scartmell-arm commented Oct 30, 2017

13:31:31 | NRF51_DK-IAR | NRF51_DK | tests-mbedmicro-rtos-mbed-rtostimer | FAIL | 34.99 | shell |
@scartmell-arm

It appears to be a timeout error on Semaphore::wait. But, I'm not sure why yet. The failure only exists on the IAR compiler, usually the first test run after rebooting the board passes, all subsequent runs fail.

Failure is usually:

[1509128019.44][CONN][RXD] :115::FAIL: Expected 1 Was 0
[1509128019.44][CONN][INF] found KV pair in stream: {{__testcase_finish;Test periodic repeats continuously;0;1}}, queued...
[1509128019.52][CONN][RXD] >>> 'Test periodic repeats continuously': 0 passed, 1 failed with reason 'Assertion Failed'

or sometimes:

[1509369613.09][CONN][RXD] {{__tCMSIS-RTOS error: User Timer Callback Queue overflow (status: 0x3, task ID: 0x0, timer ID: 0x20006240) 

fails here:

mbed-os\TESTS\mbedmicro-rtos-mbed\rtostimer\main.cpp:115

slots = sem.wait(TEST_DELAY_MS + 1);
t.stop();
TEST_ASSERT_EQUAL(1, slots);

from here:

\mbed-os\rtos\Semaphore.cpp:46

int32_t Semaphore::wait(uint32_t millisec) {
    osStatus_t stat = osSemaphoreAcquire(_id, millisec);
    switch (stat) {
        ...
        case osErrorTimeout:
            return 0;
        ...
    }
}
Remove counter from nordic critical HAL implementation
hal_critical_section_enter() is safe to call multiple times, however
hal_critical_section_exit() is only called on the last exit from a
critical section. This will cause a mismatch in the counter and the
interrupt state will never be restored if the critical section is
nested. Change this to a bool so that the interrupt save state is
tracked for hal_in_critical_section() to work correctly.
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 11, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: work labels Jan 11, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 11, 2018

@cmonr cmonr added needs: work and removed needs: CI labels Jan 11, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 12, 2018

Pipe closed exception

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Jan 12, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 12, 2018

Build : SUCCESS

Build number : 856
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5346/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 12, 2018

/morph export-build

Another "Pipe closed" exception...

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 15, 2018

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added needs: review and removed needs: CI labels Jan 15, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 15, 2018

Waiting for final approval from @alzix / @orenc17A (last round of review after changes that were requested earlier).

@alzix

alzix approved these changes Jan 16, 2018

@cmonr cmonr merged commit 671c2d7 into ARMmbed:master Jan 16, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Jan 16, 2018

@scartmell-arm scartmell-arm deleted the scartmell-arm:feature-hal-spec-critical-section branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment