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

Fixed HAL priorities #219

Merged
merged 2 commits into from
Nov 23, 2021
Merged

Fixed HAL priorities #219

merged 2 commits into from
Nov 23, 2021

Conversation

marcoaccame
Copy link
Contributor

@marcoaccame marcoaccame commented Nov 23, 2021

This PR adjusts the way the HAL sets the priorities in the NVIC so that it is compliant to its use w/ an RTOS.
This PR is linked w/ this issue.

So why this PR?
In an attempt to solve the fatal error happening in the above issue I found out that the priorities of some IRQ handlers used in some devices of the DC motor control of the mc4plus violates the original design of the HAL. In particular, they change the priority grouping of the NVIC and use an invalid priority which conflicts w/ what is use by the RTOS.

In here is a clarification comment I have added to the code of HAL:

// marco.accame on 23 nov 2021: _HAL_TAG_USE_OF_NVIC_PRIORITIES 
// the HAL system uses NVIC_PriorityGroup_4 so that we have 16 pre-emption priorities and 0 subpriorities
// this is done by design: 
// - we set NVIC_PriorityGroup_4 in here (and it is forbidden to change it afterwards!!)
// - we defined hal_interrupt_priority_t to contain values only from 0 to 15.
// the RTOS uses the lowest two priorities which it finds on the system, so in here priorities
// 14 and 15 are reserved to the RTOS which assigns 15 to SysTick_Handler() and PendSV_Handler() 
// and 14 to SVC_Handler().
// it is very important never and ever change priority group expecially after the start of the RTOS because
// that may cause crashes as described in https://developer.arm.com/documentation/ka003146/latest
// why is that? because a change in priority grouping can change the correct rules of preemption of the
// SVC, PendSV and SysTick among themselves and vs other IRQs and there can be a wrong assignment of the 
// thread stack in the context switch done by PendSV and/or SysTick. See for that a note by the guru  Joseph Yiu:
// https://community.arm.com/support-forums/f/architectures-and-processors-forum/9485/cortex-m-rtos-related-exceptions-and-concepts?pifragment-22341=104        
// 
// so in HAL we must use only priorities in range [0, 13].

Now, I believe that this correction is required.

I also hope that it can solve the sporadic fatal error for reason which I wrote in here.

There is an associated PR for the binaries in here.

The tests done on a dedicated setup have shown that the mc4plus can effectively move a motor w/ yarprobotinterface, so we can safely merge both PRs for use in the robots.

…_4 and never changed

- NVIC_PriorityGroupConfig() execuets only in hal_sys_init()
- fixed some priorities to avoid using 14 and 15 which are reserved to the RTOS
ems 3.46, mc4plus 3.43, mc2plus 3.29
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.

1 participant