You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/09/06 20:22:06 UTC
[GitHub] [incubator-nuttx] v01d opened a new pull request #1726: Nrf52 tickless
v01d opened a new pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726
## Summary
This PR extends systimer options for nRF52 arch. It is possible to use ARM SysTick either for tickless or non-tickless mode. It is also possible to now use the RTC peripheral for tickless mode. This also re-enables support for WFI/WFE sleep if RTC is used, since this counter continues to run in this mode (in contrast to SysTick).
## Impact
nRF52 architecture timing. The default continues to be SysTick in non-tickless mode so it should behave as before.
## Testing
I tested systick in both modes and tickless with RTC. I tested that time moves as expected, sleep returns correctly and also verified it continues to work once the RTC clock wraps-around.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl merged pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
raiden00pl merged pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#discussion_r485773799
##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.h
##########
@@ -0,0 +1,58 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_rtc_lowerhalf.h
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+#ifndef __ARCH_ARM_SRC_NRF52_NRF52_RTC_LOWERHALF_H
Review comment:
File removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-689690936
@raiden00pl I fixed what the problems you found, I just added these to last commit and force pushed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-687905348
> It appears that STM32 sets a zero interval timeout. So the interrupt and call to nxsched_alarm_expiration() will occurr immediately.
Ah, that makes sense. Thanks for checking.
So it is expected that the 200ms timer is always working even with one task? It would be cool if no interrupts fired unless necessary.
> NOTICE that tickless mode does not have the drift that you were concerned about when using the oneshot timer for a periodic event: The tickless mode uses both a free-running and a one-shot timer (which might be one or two times). So it gets the time that the interval timer is processed directly from the free-running timer.
I modeled this implementation for nRF52 following the idea that stm32_tickless.c uses with just one timer. I used TICKLESS_ALARM though, whereas stm32_tickless.c uses non-alarm mode.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-687916500
> > Well, that k,ind of makes sense. In Tickless mode, the OS sets the interval timer, then waits for the timer expiration which calls `nxsched_alarm_expiration()` to process the timer expiration. Since it does need an event to get started. But I don't recall having done that with other architectures.
>
> So I really misspoke here when I said: "it does need an event to get started". The event the normally causes the interval timer to be started is calling something like wd_start().
The problem is that, at least for TICKLESS_ALARM, there is a `g_stop_time` variable which starts at zero and is set on `nxsched_alarm_expiration`. And this is used to construct the absolute time for the alarm (like `g_stop_time + sleep_time`) and if the first call to `nxsched_alarm_expiration` does not occur immediately on start, the absolute time of the first alarm will be equal to `sleep_time` which is a time in the past.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-687900378
> @patacongo There is something I'm unsure about: as you can see, at the end of `up_timer_initialize()` I called `nxsched_alarm_expiration()`. If I do not do this, the 200ms scheduling alarm will not start until I for example do "sleep 1" in the console. This means that until I do so, the scheduler thinks no time has passed and thus gives a wrong value for the first alarm to up_alarm_start(). I have observed this behavior previously (and I think it is not specific to TICKLESS_ALARM) where I see the 200ms timer only starting after an initial task is launched. I'm confused how this should work. In fact, is it even necessary to keep the 200ms scheduling alarm ON even once we go back to having just one task?
Well, that k,ind of makes sense. In Tickless mode, the OS sets the interval timer, then waits for the timer expiration which calls `nxsched_alarm_expiration()` to process the timer expiration. Since it does need an event to get started. But I don't recall having done that with other architectures.
Let me look at STM32.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-687904015
> Let me look at STM32.
It appears that STM32 sets a zero interval timeout. So the interrupt and call to nxsched_alarm_expiration() will occurr immediately.
553 /* Set up to receive the callback when the counter overflow occurs */
554
555 STM32_TIM_SETISR(g_tickless.tch, stm32_tickless_handler, NULL, 0);
556
558 /* Initialize interval to zero */
559
560 STM32_TIM_SETCOMPARE(g_tickless.tch, g_tickless.channel, 0);
561
562 /* Setup compare channel for the interval timing */
563
564 stm32_tickless_setchannel(g_tickless.channel);
565
566 /* Set timer period */
567
569 STM32_TIM_SETPERIOD(g_tickless.tch, UINT32_MAX);
579
580 /* Initialize the counter */
581
582 STM32_TIM_SETMODE(g_tickless.tch, STM32_TIM_MODE_UP);
583
584 /* Start the timer */
585
586 STM32_TIM_ACKINT(g_tickless.tch, GTIM_SR_UIF);
587 STM32_TIM_ENABLEINT(g_tickless.tch, GTIM_DIER_UIE);
Am I interpreting that correctly? The match value is set to zero, the timer is started, then interrupts are enabled.
Calling nxsched_timer_expiration() has the effect of calling nxched+timer_start() to start the interval timer. So I think that the same: Setting up an initial 0-length interval or calling nxsched_tiner_expiration() directly.
NOTICE that tickless mode does not have the drift that you were concerned about when using the oneshot timer for a periodic event: The tickless mode uses both a free-running and a one-shot timer (which might be one or two times). So it gets the time that the interval timer is processed directly from the free-running timer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-687907015
>
>
> > It appears that STM32 sets a zero interval timeout. So the interrupt and call to nxsched_alarm_expiration() will occurr immediately.
>
> Ah, that makes sense. Thanks for checking.
> So it is expected that the 200ms timer is always working even with one task? It would be cool if no interrupts fired unless necessary.
Interrupts will not be used. The best alternative is to do nothing at all: Don't set a zero-interval timer; don't call nxsched_timer_expiration(). Then the interval timer will not be set until there is something that needs to be timed.
You already saw this. When you did, `nsh> sleep 1`, the interval timer started for one second then stopped. There were no interrupts before the sleep 1 and no interrupt after. That is correct behavior.
Of course, on a busy system with lots of timing, the interval timer will be running most of the time.
>
> > NOTICE that tickless mode does not have the drift that you were concerned about when using the oneshot timer for a periodic event: The tickless mode uses both a free-running and a one-shot timer (which might be one or two times). So it gets the time that the interval timer is processed directly from the free-running timer.
>
> I modeled this implementation for nRF52 following the idea that stm32_tickless.c uses with just one timer. I used TICKLESS_ALARM though, whereas stm32_tickless.c uses non-alarm mode.
Functionally, the two implementations are the same. The single timer implementation just uses fewer resources to accomplish the same thing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1726: Nrf52 tickless
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-687893971
As you can see I have implemented tickless mode on RTC directly, not via arch_* facility due to the issue discussed in #1705. The implementation exposes the RTC as an alarm. This could actually be how arch_alarm can be made to work, which requires handling the overflow interrupt (this is discussed in #1725).
@patacongo There is something I'm unsure about: as you can see, at the end of `up_timer_initialize()` I called `nxsched_alarm_expiration()`. If I do not do this, the 200ms scheduling alarm will not start until I for example do "sleep 1" in the console. This means that until I do so, the scheduler thinks no time has passed and thus gives a wrong value for the first alarm to up_alarm_start(). I have observed this behavior previously (and I think it is not specific to TICKLESS_ALARM) where I see the 200ms timer only starting after an initial task is launched. I'm confused how this should work. In fact, is it even necessary to keep the 200ms scheduling alarm ON even once we go back to having just one task?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on a change in pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on a change in pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#discussion_r485351122
##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -0,0 +1,330 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_rtc_lowerhalf.h
Review comment:
```suggestion
* arch/arm/src/nrf52/nrf52_tickless_rtc.c
```
##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.h
##########
@@ -0,0 +1,58 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_rtc_lowerhalf.h
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+#ifndef __ARCH_ARM_SRC_NRF52_NRF52_RTC_LOWERHALF_H
+#define __ARCH_ARM_SRC_NRF52_NRF52_RTC_LOWERHALF_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <nrf52_rtc.h>
+
+/****************************************************************************
+ * Public Function Prototypes
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: nrf52_rtc_initialize
+ *
+ * Description:
+ * Bind the configuration timer to a timer lower half instance and
+ * register the timer drivers at 'devpath'
+ *
+ * Input Parameters:
+ * devpath - The full path to the timer device. This should be of the
+ * form /dev/timer0
+ * timer - the timer's number.
+ * timer_drvr - Pointer where to store resulting timer_lowerhalf_s*
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; A negated errno value is returned
+ * to indicate the nature of any failure.
+ *
+ ****************************************************************************/
+
+int nrf52_rtc_initialize(FAR const char *devpath, uint8_t timer,
Review comment:
It doesn't match the nrf52_tickless_rtc.c file
##########
File path: arch/arm/src/nrf52/nrf52_systick.c
##########
@@ -123,26 +76,14 @@ static int nrf52_timerisr(int irq, uint32_t *regs, void *arg)
void up_timer_initialize(void)
{
- uint32_t regval;
-
- regval = getreg32(NVIC_SYSTICK_CTRL);
- regval &= ~NVIC_SYSTICK_CTRL_CLKSOURCE;
- putreg32(regval, NVIC_SYSTICK_CTRL);
+#if defined(CONFIG_NRF52_SYSTIMER_SYSTICK)
+ /* Use SysTick to drive system timer */
- /* Configure SysTick to interrupt at the requested rate */
+ up_timer_set_lowerhalf(systick_initialize(true, BOARD_SYSTICK_CLOCK, -1));
+#elif defined(CONFIG_NRF52_SYSTIMER_RTC)
+ /* Use RTC to drive system timer */
- putreg32(SYSTICK_RELOAD, NVIC_SYSTICK_RELOAD);
-
- /* Attach the timer interrupt vector */
-
- irq_attach(NRF52_IRQ_SYSTICK, (xcpt_t)nrf52_timerisr, NULL);
-
- /* Enable SysTick interrupts */
-
- putreg32((NVIC_SYSTICK_CTRL_CLKSOURCE | NVIC_SYSTICK_CTRL_TICKINT |
- NVIC_SYSTICK_CTRL_ENABLE), NVIC_SYSTICK_CTRL);
-
- /* And enable the timer interrupt */
-
- up_enable_irq(NRF52_IRQ_SYSTICK);
+ nrf52_rtc_initialize("/dev/systimer", CONFIG_NRF52_SYSTIMER_RTC_INSTANCE,
Review comment:
There is no such function.
##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.h
##########
@@ -0,0 +1,58 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_rtc_lowerhalf.h
Review comment:
```suggestion
* arch/arm/src/nrf52/nrf52_tickless_rtc.h
```
##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.h
##########
@@ -0,0 +1,58 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_rtc_lowerhalf.h
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+#ifndef __ARCH_ARM_SRC_NRF52_NRF52_RTC_LOWERHALF_H
Review comment:
Does not match the file name.
##########
File path: arch/arm/src/nrf52/nrf52_systick.c
##########
@@ -46,68 +46,21 @@
#include <nuttx/arch.h>
#include <arch/board/board.h>
-#include "nvic.h"
-#include "clock/clock.h"
-#include "arm_internal.h"
-#include "arm_arch.h"
+#if defined(CONFIG_NRF52_SYSTIMER_SYSTICK)
+#include <nuttx/timers/arch_timer.h>
+#include "systick.h"
+#elif defined(CONFIG_NRF52_SYSTIMER_RTC)
+#include "nrf52_rtc_lowerhalf.h"
Review comment:
There is no such file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#discussion_r485771399
##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.h
##########
@@ -0,0 +1,58 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_rtc_lowerhalf.h
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#discussion_r485772748
##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.h
##########
@@ -0,0 +1,58 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_rtc_lowerhalf.h
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+#ifndef __ARCH_ARM_SRC_NRF52_NRF52_RTC_LOWERHALF_H
+#define __ARCH_ARM_SRC_NRF52_NRF52_RTC_LOWERHALF_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <nrf52_rtc.h>
+
+/****************************************************************************
+ * Public Function Prototypes
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: nrf52_rtc_initialize
+ *
+ * Description:
+ * Bind the configuration timer to a timer lower half instance and
+ * register the timer drivers at 'devpath'
+ *
+ * Input Parameters:
+ * devpath - The full path to the timer device. This should be of the
+ * form /dev/timer0
+ * timer - the timer's number.
+ * timer_drvr - Pointer where to store resulting timer_lowerhalf_s*
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; A negated errno value is returned
+ * to indicate the nature of any failure.
+ *
+ ****************************************************************************/
+
+int nrf52_rtc_initialize(FAR const char *devpath, uint8_t timer,
Review comment:
Yes, actually this header is not necessary anymore. Will remove
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-687904015
> Let me look at STM32.
It appears that STM32 sets a zero interval timeout. So the interrupt and call to nxsched_alarm_expiration() will occurr immediately.
553 /* Set up to receive the callback when the counter overflow occurs */
554
555 STM32_TIM_SETISR(g_tickless.tch, stm32_tickless_handler, NULL, 0);
556
558 /* Initialize interval to zero */
559
560 STM32_TIM_SETCOMPARE(g_tickless.tch, g_tickless.channel, 0);
561
562 /* Setup compare channel for the interval timing */
563
564 stm32_tickless_setchannel(g_tickless.channel);
565
566 /* Set timer period */
567
569 STM32_TIM_SETPERIOD(g_tickless.tch, UINT32_MAX);
579
580 /* Initialize the counter */
581
582 STM32_TIM_SETMODE(g_tickless.tch, STM32_TIM_MODE_UP);
583
584 /* Start the timer */
585
586 STM32_TIM_ACKINT(g_tickless.tch, GTIM_SR_UIF);
587 STM32_TIM_ENABLEINT(g_tickless.tch, GTIM_DIER_UIE);
Am I interpreting that correctly? The match value is set to zero, the timer is started, then interrupts are enabled.
Calling nxsched_timer_expiration() has the effect of calling nxched_timer_start() to start the interval timer. So I think that these have the same: Setting up an initial 0-length interval or calling nxsched_tiner_expiration() directly.
NOTICE that tickless mode does not have the drift that you were concerned about when using the oneshot timer for a periodic event: The tickless mode uses both a free-running and a one-shot timer (which might be one or two times). So it gets the time that the interval timer is processed directly from the free-running timer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-687974714
@patacongo ok, I see why this is now, there's a "hack" in place that keeps the timer running.
I don't really understand the explanation. I guess it refers to a possible race condition.
Should we open an issue for this so that one day we can improve it? Disabling the
scheduling interrupt when not needed can go a long way for low-power applications.
<pre>
/* In the original design, it was planned that nxsched_reassess_timer() be
* called whenever there was a change at the head of the ready-to-run
* list. That call was intended to establish a new time-slice or to
* stop an old time-slice timer. However, it turns out that that
* solution is too fragile: The system is too vulnerable at the time
* that the ready-to-run list is modified in order to muck with timers.
*
* The kludge/work-around is simple to keep the timer running all of the
* time with an interval of no more than the timeslice interval. If we
* do this, then there is really no need to do anything when on context
* switches.
*/
#define KEEP_ALIVE_HACK 1
</pre>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-688888715
> > @patacongo ok, I see why this is now, there's a "hack" in place that keeps the timer running.
> > I don't really understand the explanation. I guess it refers to a possible race condition.
> > Should we open an issue for this so that one day we can improve it? Disabling the
> > scheduling interrupt when not needed can go a long way for low-power applications.
>
> Use of almost all internal OS interfaces require that the OS data structures always be in the correct state. During a context switch that internal state for one task is torn down and the new state for the news task is set up. In between those times, the OS is in an indeterminate state and the use of any OS internal interface (such as the timer interface) may not be reliable.
>
> I don't recall the specific issue that I faced when I wrote that comment, but I do remember that there was no simple work-around. It is, however, certainly worth revisiting if someone is motivated to really dig into the guts of the OS.
Thanks. I will create an issue for this. I could make an attempt to solve it eventually.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#discussion_r485773401
##########
File path: arch/arm/src/nrf52/nrf52_systick.c
##########
@@ -123,26 +76,14 @@ static int nrf52_timerisr(int irq, uint32_t *regs, void *arg)
void up_timer_initialize(void)
{
- uint32_t regval;
-
- regval = getreg32(NVIC_SYSTICK_CTRL);
- regval &= ~NVIC_SYSTICK_CTRL_CLKSOURCE;
- putreg32(regval, NVIC_SYSTICK_CTRL);
+#if defined(CONFIG_NRF52_SYSTIMER_SYSTICK)
+ /* Use SysTick to drive system timer */
- /* Configure SysTick to interrupt at the requested rate */
+ up_timer_set_lowerhalf(systick_initialize(true, BOARD_SYSTICK_CLOCK, -1));
+#elif defined(CONFIG_NRF52_SYSTIMER_RTC)
+ /* Use RTC to drive system timer */
- putreg32(SYSTICK_RELOAD, NVIC_SYSTICK_RELOAD);
-
- /* Attach the timer interrupt vector */
-
- irq_attach(NRF52_IRQ_SYSTICK, (xcpt_t)nrf52_timerisr, NULL);
-
- /* Enable SysTick interrupts */
-
- putreg32((NVIC_SYSTICK_CTRL_CLKSOURCE | NVIC_SYSTICK_CTRL_TICKINT |
- NVIC_SYSTICK_CTRL_ENABLE), NVIC_SYSTICK_CTRL);
-
- /* And enable the timer interrupt */
-
- up_enable_irq(NRF52_IRQ_SYSTICK);
+ nrf52_rtc_initialize("/dev/systimer", CONFIG_NRF52_SYSTIMER_RTC_INSTANCE,
Review comment:
Yes, this file is only for systick now, this was leftover code which I didn't realize was still there. Will remove the call.
##########
File path: arch/arm/src/nrf52/nrf52_systick.c
##########
@@ -46,68 +46,21 @@
#include <nuttx/arch.h>
#include <arch/board/board.h>
-#include "nvic.h"
-#include "clock/clock.h"
-#include "arm_internal.h"
-#include "arm_arch.h"
+#if defined(CONFIG_NRF52_SYSTIMER_SYSTICK)
+#include <nuttx/timers/arch_timer.h>
+#include "systick.h"
+#elif defined(CONFIG_NRF52_SYSTIMER_RTC)
+#include "nrf52_rtc_lowerhalf.h"
Review comment:
Also removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-688884714
> @patacongo ok, I see why this is now, there's a "hack" in place that keeps the timer running.
> I don't really understand the explanation. I guess it refers to a possible race condition.
> Should we open an issue for this so that one day we can improve it? Disabling the
> scheduling interrupt when not needed can go a long way for low-power applications.
Use of almost all internal OS interfaces require that the OS data structures always be in the correct state. During a context switch that internal state for one task is torn down and the new state for the news task is set up. In between those times, the OS is in an indeterminate state and the use of any OS internal interface (such as the timer interface) may not be reliable.
I don't recall the specific issue that I faced when I wrote that comment, but I do remember that there was no simple work-around. It is, however, certainly worth revisiting if someone is motivated to really dig into the guts of the OS.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#discussion_r485771485
##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -0,0 +1,330 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_rtc_lowerhalf.h
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-687907424
> Well, that k,ind of makes sense. In Tickless mode, the OS sets the interval timer, then waits for the timer expiration which calls `nxsched_alarm_expiration()` to process the timer expiration. Since it does need an event to get started. But I don't recall having done that with other architectures.
So I really misspoke here when I said: "it does need an event to get started". The event the normally causes the interval timer to be started is calling something like wd_start().
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-688921942
regarding this PR: I asked for @raiden00pl for review but he may not be available. Anyone else willing to review this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1726: nRF52: extend systimer support
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1726:
URL: https://github.com/apache/incubator-nuttx/pull/1726#issuecomment-687916835
> You already saw this. When you did, `nsh> sleep 1`, the interval timer started for one second then stopped. There were no interrupts before the sleep 1 and no interrupt after. That is correct behavior.
Actually that's the other point: the 200 ms interrupt continues to fire after `sleep 1` exits. That doesn't seem necessary with just one task.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org