-
Notifications
You must be signed in to change notification settings - Fork 168
Add SysTick flags #116
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 SysTick flags #116
Conversation
FIX: I have not seen that PENDSV is already added. I review PENDST code to be like PENDSV. |
bors try |
🔒 Permission denied Existing reviewers: click here to make qwerty19106 a reviewer |
src/peripheral/scb.rs
Outdated
|
||
/// Set the PENDSTCLR bit in the ICSR register which will clear a pending SysTick interrupt | ||
#[inline] | ||
pub fn clear_systick_pending(&mut self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this operation require exclusive access to SCB (&mut self
)? With clear_pendsv()
it wasn't required because setting just the PENDSTCLR
bit doesn't affect any other bits in the register, so you can't have data races.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exclusive access to SCB is not required. I think that clear_systick_pending
needs to change analogiously.
src/peripheral/scb.rs
Outdated
|
||
/// Set the PENDSTCLR bit in the ICSR register which will clear a pending SysTick interrupt | ||
#[inline] | ||
pub fn set_systick(&mut self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about calling this set_systick
when just above we have set_pendsv
. Really this one should be set_pendst
or we need to rename both (causing a major version change). Same applies to is_systick_pending
and clear_systick_pending
. I'd prefer just using pendst
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, set_pendst
looks better.
I added requested changes, needs to review. |
src/peripheral/scb.rs
Outdated
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: One blank line too many.
Do I need to run rustfmt before create commit? |
Not a hard requirement but IMHO a good thing to do. |
Are there any other changes I have to do? |
@therealprof are you happy with the result of rustfmt here, especially around L610? If anything I think it looks a bit messier now. I'm happy with everything else in the PR. |
@adamgreig I agree it looks messier now but I'm not in charge of the formatting rules. I still think adhering to the standard formatting is a good idea even if the result is not what I personally prefer. |
Okay, let's merge this and we can consider tidying up more of the code for rustfmt another time. |
bors r+ |
Thanks @qwerty19106 ! |
116: Add SysTick flags r=adamgreig a=qwerty19106 CMSIS core headers contains SCB_ICSR_PENDSVSET_*** and SCB_ICSR_PENDSTSET_*** definitions, which I need in my projects. In CMSIS it is used to check, set and clear this flags (rtx_core_cm.h and os_systick.c). I suggest adding it to scb.rs. I put initial commit, but I need help to add compiler barriers where its are needed. For details, see CMSIS: [CMSIS_5/CMSIS/Core/Include/core_cm3.h](https://github.com/ARM-software/CMSIS_5/blob/develop/CMSIS/Core/Include/core_cm3.h) (or other core_cm{X}.h) [CMSIS_5/CMSIS/RTOS2/RTX/Source/rtx_core_cm.h)](https://github.com/ARM-software/CMSIS_5/blob/develop/CMSIS/RTOS2/RTX/Source/rtx_core_cm.h) [CMSIS_5/CMSIS/RTOS2/Source/os_systick.c)](https://github.com/ARM-software/CMSIS_5/blob/develop/CMSIS/RTOS2/Source/os_systick.c) FIX: I have not seen that PENDSV is already added. I review PENDST code to be like PENDSV. Co-authored-by: qwerty19106 <[email protected]>
Build succeeded |
that contains static variables that will not be initialized by the runtime this implements only the linker section described in RFC #116 so that downstream libraries / framework can leverage it without (a) forking this crate or (b) requiring the end user to pass additional linker scripts to the linker when building their crate. This commit doesn't add the user interface described in RFC #116; instead it documents how to use this linker section in the advanced section of the crate level documentation
192: add a .uninit section r=therealprof a=japaric that contains static variables that will not be initialized by the runtime this implements only the linker section described in RFC #116 so that downstream libraries / framework can leverage it without (a) forking this crate or (b) requiring the end user to pass additional linker scripts to the linker when building their crate. This commit doesn't add the user interface described in RFC #116; instead it documents how to use this linker section in the advanced section of the crate level documentation Co-authored-by: Jorge Aparicio <[email protected]>
CMSIS core headers contains SCB_ICSR_PENDSVSET_*** and SCB_ICSR_PENDSTSET_*** definitions, which I need in my projects. In CMSIS it is used to check, set and clear this flags (rtx_core_cm.h and os_systick.c).
I suggest adding it to scb.rs. I put initial commit, but I need help to add compiler barriers where its are needed.
For details, see CMSIS:
CMSIS_5/CMSIS/Core/Include/core_cm3.h (or other core_cm{X}.h)
CMSIS_5/CMSIS/RTOS2/RTX/Source/rtx_core_cm.h)
CMSIS_5/CMSIS/RTOS2/Source/os_systick.c)
FIX: I have not seen that PENDSV is already added. I review PENDST code to be like PENDSV.