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

no-systick targets: fix systick irq handler setup #11479

Merged
merged 1 commit into from Sep 16, 2019

Conversation

@maciejbocianski
Copy link
Member

commented Sep 13, 2019

Description

Fix for the #11426

For targets with no H/W systick (NO_SYSTICK), SysTick_Handler needs to be remapped to specific irq emulating systick.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@kjbracey-arm

Release Notes

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

@ciarmcom ciarmcom requested review from kjbracey-arm and ARMmbed/mbed-os-maintainers Sep 13, 2019
@maciejbocianski maciejbocianski force-pushed the maciejbocianski:nrf51_systick_handler_fix branch from 9424eff to 03fcf7c Sep 13, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

Not following the logic of the change. Isn't this effectively just side-stepping the assert?

If NO_SYSTICK is set, SysTimer::get_irq_number should be telling us the interrupt number that is substituting for SysTick - reported by mbed_get_m0_tick_irqn().

If there is a RAM vector table, then we try to program that interrupt number for the handler we want. If there is no RAM vector table, then we check that it has been hard-coded to call the handler we want - in an RTOS build it should be hard-coded to call the RTOS tick handler.

I think your change just stops us doing the check and makes us try to set it anyway - which won't work because there's no RAM vector table.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

Oh, I see.

There's an issue here in that I was assuming that all targets with RAM vectors had NVIC_RAM_VECTOR_ADDRESS defined in their cmsis_nvic.h.

It appears that M0 devices like this one don't all have this define public - they sometimes have it internal.

It looks like the original code called NVIC_SetVector unconditionally. From which we can deduce that any targets with ROM vectors just ignore the call, or fault if it's set to something different than they have in ROM.

So I suggest changing this code to

NVIC_SetPriority(irq, 0xFF);
NVIC_SetVector(irq, (uint32_t)handler);
MBED_ASSERT(handler == (IRQHandler_t)NVIC_GetVector(irq));

Always attempt the set, and read it back to make sure it worked.

@maciejbocianski

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2019

Exactly the old code called it unconditionally:

void SysTimer::setup_irq()
{
#if (defined(NO_SYSTICK) && !defined (TARGET_CORTEX_A))
NVIC_SetVector(mbed_get_m0_tick_irqn(), (uint32_t)SysTick_Handler);
NVIC_SetPriority(mbed_get_m0_tick_irqn(), 0xFF); /* RTOS requires lowest priority */
NVIC_EnableIRQ(mbed_get_m0_tick_irqn());
#else
// Ensure SysTick has the correct priority as it is still used
// to trigger software interrupts on each tick. The period does
// not matter since it will never start counting.
OS_Tick_Setup(osRtxConfig.tick_freq, OS_TICK_HANDLER);
#endif
}

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:nrf51_systick_handler_fix branch from 03fcf7c to 7791032 Sep 13, 2019
Remove checking that vectors were copied to RAM as not all targets
have NVIC_RAM_VECTOR_ADDRESS defined as public. Instead always
call NVIC_SetVector unconditionally as old implementation does.
@maciejbocianski maciejbocianski force-pushed the maciejbocianski:nrf51_systick_handler_fix branch from 7791032 to 94a08e9 Sep 13, 2019
@maciejbocianski

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2019

@kjbracey-arm updated

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

This PR fix the regression #11426, so I marked this as 5.14 rc3, @0xc0170

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 13, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Sep 13, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
@maciejbocianski

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2019

Looks not related.
Strange errors while example compilation:

#### STDERR ####
[mbed] WARNING: Missing Python modules were not auto-installed.
       The Mbed OS tools in this program require the following Python modules: hidapi, pywin32, wmi
       You can install all missing modules by running "pip install -r requirements.txt" in "/builds/ws/mbed-os-ci_build-IAR/examples/mbed-os-example-ble-HeartRate/mbed-os"
       On Posix systems (Linux, etc) you might have to switch to superuser account or use "sudo"
---
[mbed] ERROR: "/usr/bin/python" returned error.
       Code: -11
       Path: "/builds/ws/mbed-os-ci_build-IAR/examples/mbed-os-example-ble-HeartRate"
       Command: "/usr/bin/python -u /builds/ws/mbed-os-ci_build-IAR/examples/mbed-os-example-ble-HeartRate/mbed-os/tools/make.py -t IAR -m NRF52_DK --source . --build ./BUILD/NRF52_DK/IAR"
       Tip: You could retry the last command with "-v" flag for verbose output
Compiling [mbed-os-example-blinky] for [CY8CPROTO_064_SB] with toolchain [GCC_ARM]

>  mbed-cli compile -t GCC_ARM -m CY8CPROTO_064_SB

#### STDOUT ####
[mbed] Working path "/builds/ws/mbed-os-ci_build-GCC_ARM@2/examples/mbed-os-example-blinky" (program)

#### STDERR ####
[mbed] WARNING: Could not find mbed program in current path "/builds/ws/mbed-os-ci_build-GCC_ARM@2/examples/mbed-os-example-blinky".
       You can fix this by calling "mbed new ." in the root of your program.
---
[mbed] ERROR: "/usr/bin/python" returned error.
       Code: -11
       Path: "/builds/ws/mbed-os-ci_build-GCC_ARM@2/examples/mbed-os-example-blinky"
       Command: "/usr/bin/python -m pip list -l"
       Tip: You could retry the last command with "-v" flag for verbose output
---
@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

👍

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Sep 16, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Ci restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Sep 16, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 16, 2019
@0xc0170 0xc0170 merged commit 171fe5a into ARMmbed:master Sep 16, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8643 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.