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/03 19:07:54 UTC
[GitHub] [incubator-nuttx] v01d opened a new pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
v01d opened a new pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705
## Summary
This commit extends support for system timer based on ARM SysTick
or RTC timer, both for tickless and non-tickless modes. To do so,
the "arch timer" facility is used. This also restores support for WFI/WFE
which is only used when RTC is used as system timer.
## Impact
System timer
## Testing
Tested both options (systick and RTC) both in tickless and non-tickless modes.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687633129
Also, looking at the first wiki entry you linked:
<pre>
Since timers are a limited resource, the use of two timers could be an issue on some systems. The job could be done with a single timer if, for example, the single timer were kept in a free-running at all times. Some timer/counters have the capability to generate a compare interrupt when the timer matches a comparison value but also to continue counting without stopping. If your hardware supports such counters, one might used the CONFIG_SCHED_TICKLESS_ALARM option and be able to simply set the comparison count at the value of the free running timer PLUS the desired delay. Then you could have both with a single timer: An alarm and a free-running counter with the same timer!
</pre>
arch_alarm.c should indeed use a free running timer, not one that requires restarting. Otherwise you are exactly in the case of non-alarm tickless 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] patacongo commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687692680
>
>
> > > So the implementer is free to just disable the interrupt but keep the counting. This behaviour is very important to implement the tickless mode on top of oneshot_lowerhalf_s without any drift.
> >
> >
> > No. The behavior of a one shot is clear from its definition. It requires no further definition. A one shot generates a single pulse. If it generates multiple pulses it is not a on shot.
>
> @patacongo before you give ANY feedback, please read other people's comment CAREFULLY. I EXPLICTLY mention that "DISABLE THE INTERRUPT BUT KEEP THE COUNTING". The INTERRUPT already DISABLE, could you telll me how to generate MULTIPLE PULSE?
A one shot, by its definition, generates a single event. That is the required behavior to be a one shot. Anything that generates multiple events is not a one shot. What you are describing is not a one shot.
In order to get another event, the one shot has to be started again. Refer to drivers/timer/oneshot.c for the definitive use of the one shot interface.
----------------------------------------------------------------
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] btashton commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686715948
> > > I don't have that, i have this:
> > > check_patch()
> >
> >
> > CI calls `checkpatch.sh -g $commits`
> > option `g` uses `check_commit`
> > I think my fix will work
>
> Mmm didn't work.
Just tested against your PR and you are right one more change. We should not be using `show`, instead we should be using `diff` otherwise we get a list of the intermediate commit changes which may include files dropped.
With that we do not even need the `--no-renames`
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687692927
What Xiang is mentioning, I believe, is that you could have the ISR fire only once (on the ISR disable the interrupt) but have the timer continue running. This works when the timer does not autoreload itself (the ISR fires when the counter matches a value only) and will continue until wrapping 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] xiaoxiang781216 commented on a change in pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#discussion_r483925810
##########
File path: arch/arm/src/nrf52/nrf52_rtc_lowerhalf.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,
+ FAR struct timer_lowerhalf_s **timer_drvr);
Review comment:
It's easy to extend the period beyond 512 seconds if we monitor the overflow interrupt, but it's impossible to compensate the timer drift if we enable tickless with timer_lowerhalf_s. From our test with systick, the drift will bigger than one second after serveral hours(clock_gettime).
----------------------------------------------------------------
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] xiaoxiang781216 commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687660829
> > So the implementer is free to just disable the interrupt but keep the counting. This behaviour is very important to implement the tickless mode on top of oneshot_lowerhalf_s without any drift.
>
> My suggestion is to not base tickless mode on this expectation of oneshot continuing to count after expiration. But arch_alarm should indeed be implemented over an interface which guarantees the timer continues to run without any manual restart. timer_lowerhalf_s could be used for this, but current implementations do not guarantee this property either (stm32/smt32l4 will autoreload the timer, but for "advanced timers" this would be possible) and overflow should be handled by exposing the overflow ISR. We really need to document the semantics of each interface so that we agree on how things should work.
timer_lowerhalf_s is modeled for the auto reload timer, oneshot_lowerhalf_s is modeled for the no auto reload timer. That's why I select oneshot_lowerhalf_s and assume the timer keep counting.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#discussion_r483768914
##########
File path: arch/arm/src/nrf52/Make.defs
##########
@@ -89,7 +89,7 @@ ifeq ($(CONFIG_ARCH_CHIP_NRF52832),y)
CHIP_CSRCS += nrf52832_errdata.c
endif
-CHIP_CSRCS += nrf52_timerisr.c
+CHIP_CSRCS += nrf52_systimer.c
Review comment:
OK
----------------------------------------------------------------
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] btashton commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686818061
> @btashton is ccache being used for builds? I looked at the scripts and I understand it should be working when "-c" is passed but it doesn't seem to be the case for the build workflow. Unless I'm missing something.
It will not work with the Docker container. I suppose I could enable that, but the MacOS build is by far the slowest build (by at least 2x).
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687158259
> I'm just wondering if we shouldn't change the terminology related to RTC in the NRF52 family.
Lots of MCUs are in the situation and all use the RTC naming. An exception is arch/arm/src/stm32. The older STM32 F1's used a real time counter and the others in the family use a date time RTC. So a distinction was necessary there: stm32_rtcounter.c vs. stm32_rtcc.c
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686815324
@btashton is ccache being used for builds? I looked at the scripts and I understand it should be working when "-c" is passed but it doesn't seem to be the case for the build workflow. Unless I'm missing something.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687632464
>
>
> Also, if you look at stm32_oneshot.c, you will find that when the timer expires it is stopped. So I don't really understand how you can use the counter to provide up_timer_gettime after it the "alarm" expires on arch_alarm.c. I feel there's something wrong here.
Of course, it if reset and fired again it wouldn't be a one shot. The one shot timer was created to support tickless mode operation for the two timer case: https://cwiki.apache.org/confluence/display/NUTTX/Tickless+OS
The one shot is also used internally to support CPU load measurements with entropy: https://cwiki.apache.org/confluence/display/NUTTX/Oneshot+Timers+and+CPU+Load+Measurement
----------------------------------------------------------------
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] btashton commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686711752
> I don't have that, i have this:
>
> check_patch()
CI calls `checkpatch.sh -g $commits`
option `g` uses `check_commit`
I think my fix will work
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686711163
ah, sorry, was in another line
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687632464
>
>
> Also, if you look at stm32_oneshot.c, you will find that when the timer expires it is stopped. So I don't really understand how you can use the counter to provide up_timer_gettime after it the "alarm" expires on arch_alarm.c. I feel there's something wrong here.
Of course, it if reset and fired again it wouldn't be a one shot. The one shot timer was created to support tickless mode operation for the two timer case: https://cwiki.apache.org/confluence/display/NUTTX/Tickless+OS
The one shot is also used internally to support CPU load measurements with entropy: https://cwiki.apache.org/confluence/display/NUTTX/Oneshot+Timers+and+CPU+Load+Measurement
I don't know all implementations, but normally the oneshot interface is built on top of the timer interface.
----------------------------------------------------------------
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 closed pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d closed pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687632771
> Of course, it if reset and fired again it wouldn't be a one shot. The one shot timer was created to support tickless mode operation for the two timer case: https://cwiki.apache.org/confluence/display/NUTTX/Tickless+OS
That's exactly my thought. So, arch_alarm.c using oneshot does not really sound a good combination, is it?
I mean, there will be drift while the timer is stopped between the ISR and when the next up_alarm_start is called.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686702247
This is failing the check due to #1637
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687650849
Is the oneshot supposed to keep counting after it fires? I understand that oneshot fires once but at least stm32_oneshot.c stops counting. So implementing up_timer_gettime() as arch_alarm.c does:
<pre>
int up_timer_gettime(FAR struct timespec *ts)
{
int ret = -EAGAIN;
if (g_oneshot_lower != NULL)
{
ret = ONESHOT_CURRENT(g_oneshot_lower, ts);
}
return ret;
}
</pre>
would only work if the timer continues to run AND it handles wrap-around by adding one period on overflow and returns this monotonic value on ONESHOT_CURRENT(). Neither of those two conditions are met in stm32_oneshot.c. So is stm32_oneshot.c wrong or is that not how oneshot should work?
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687653103
>
>
> Is the oneshot supposed to keep counting after it fires? I understand that oneshot fires once but at least stm32_oneshot.c stops counting. So implementing up_timer_gettime() as arch_alarm.c does:
>
> int up_timer_gettime(FAR struct timespec *ts)
> {
> int ret = -EAGAIN;
>
> if (g_oneshot_lower != NULL)
> {
> ret = ONESHOT_CURRENT(g_oneshot_lower, ts);
> }
>
> return ret;
> }
>
> would only work if the timer continues to run AND it handles wrap-around by adding one period on overflow and returns this monotonic value on ONESHOT_CURRENT(). Neither of those two conditions are met in stm32_oneshot.c. So is stm32_oneshot.c wrong or is that not how oneshot should work?
Yes, resetting and stopping the timer is the correct behavior for a oneshot timer. I expect that that all of the implementations do that.
My suspicion from what you say is that the those arch drivers are not compatible with the existing MCU architectures. They are not used by any of the existing architectures in the repository; they are used only by Xiaomi hardware. They have probably implemented an incompatible version of the one shot driver for their purposes and those arch drivers may not be usable by other architectures.
I am completely unfamiliar with those drivers. I have never used them or even looked at them. You would have to discuss this with Xiaomi to see if they are using some non-standard oneshot driver. I don't know what we should do if they are. Certainly we do not want to corrupt the semantics of the oneshot driver.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#discussion_r483233171
##########
File path: tools/checkpatch.sh
##########
@@ -84,14 +84,14 @@ check_patch() {
fail=1
else
git apply $1
- diffs=`cat $1`
+ diffs=``
check_ranges <<< "$diffs"
git apply -R $1
fi
}
check_commit() {
- diffs=`git show $1`
+ diffs=`git show --no-renames $1`
Review comment:
The check failed, for some reason it seems to be getting a strange set of commits.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686710472
I don't have that, i have this:
<pre>
check_patch() {
if ! git apply --check $1; then
fail=1
else
git apply $1
diffs=`cat $1`
check_ranges <<< "$diffs"
git apply -R $1
fi
}check_patch() {
if ! git apply --check $1; then
fail=1
else
git apply $1
diffs=`cat $1`
check_ranges <<< "$diffs"
git apply -R $1
fi
}
</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] patacongo commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687694164
> The timer usually generates a separate overflow event. I understand Xiang assumption was that could could use ONESHOT_CURRENT() to retrieve the free running count even after timeout.
Okay, but none of the implementations in the repository work that way.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687663262
> > > So the implementer is free to just disable the interrupt but keep the counting. This behaviour is very important to implement the tickless mode on top of oneshot_lowerhalf_s without any drift.
> >
> >
> > My suggestion is to not base tickless mode on this expectation of oneshot continuing to count after expiration. But arch_alarm should indeed be implemented over an interface which guarantees the timer continues to run without any manual restart. timer_lowerhalf_s could be used for this, but current implementations do not guarantee this property either (stm32/smt32l4 will autoreload the timer, but for "advanced timers" this would be possible) and overflow should be handled by exposing the overflow ISR. We really need to document the semantics of each interface so that we agree on how things should work.
>
> timer_lowerhalf_s is modeled for the auto reload timer, oneshot_lowerhalf_s is modeled for the no auto reload timer. That's why I select oneshot_lowerhalf_s and assume the timer keep counting.
But this isn't documented. And at this point making oneshot conform to this expectation means to rewrite many implementations and break a lot of stuff. On the contrary, adapting arch_alarm to a different interface that gives satisfies this expectation is much easier and will not break anything currently on NuttX mainline.
The question is which interface to use to provide this semantic. timer_lowerhalf_s could be extended or a different one could be added. Either will require some work.
----------------------------------------------------------------
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] xiaoxiang781216 commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687576075
Why we provide two driver interfaces(oneshot_lowerhalf_s v.s. timer_lowerhalf_s)? The only reason is that whether the hardware will auto clear the timing count, so it's a very important difference. If you read the arch_timer.c carefully, you will find that the up_timer_gettime will drift a little bit in every interrupt and is unavoidable. Actually, I don't suggest to combine arch_timer with tickless mode since the drift is so large to make this combination useless.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687647565
>
>
> > Of course, it if reset and fired again it wouldn't be a one shot. The one shot timer was created to support tickless mode operation for the two timer case: https://cwiki.apache.org/confluence/display/NUTTX/Tickless+OS
>
> That's exactly my thought. So, arch_alarm.c using oneshot does not really sound a good combination, is it?
> I mean, there will be drift while the timer is stopped between the ISR and when the next up_alarm_start is called.
It would would not be the tidiest solution since it is just a wrapper around the timer interface and since the timer interface can be configured to support a periodic timer.
If you are clever in how you set up the next oneshot delay, then you could avoid drift.
----------------------------------------------------------------
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] xiaoxiang781216 commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687692089
> > So the implementer is free to just disable the interrupt but keep the counting. This behaviour is very important to implement the tickless mode on top of oneshot_lowerhalf_s without any drift.
>
> No. The behavior of a one shot is clear from its definition. It requires no further definition. A one shot generates a single pulse. If it generates multiple pulses it is not a on shot.
@patacongo before you give ANY feedback, please read other people's comment CAREFULLY. I EXPLICTLY mention that "DISABLE THE INTERRUPT BUT KEEP THE COUNTING". The INTERRUPT already DISABLE, could you telll me how to generate MULTIPLE PULSE?
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687298575
> The keypoint isn't whether the hardware save the time as a counter or as y/m/d/h/m/s/us, because it is very easy to convert between these two representation(mktime and gmtime_r). The major difference is whether the timer is in an always on domain, and never stop counting. If it's true, it's better to implement as rtc_lowerhalf_s, otherwise it should implement as oneshot_lowerhalf_s or timer_lowerhalf_s.
See my response to your other comment. I don't think it is appropriate to consider this timer anything different than a nRF52 TIMER or STM32 TIM, with the difference that indeed it continues to work during WFI/WFE. Exposing the timer as oneshot/rtc means that you have to give the illusion of a timer which does not wrap around. the timer_lowerhalf_s reflects better the functionality of the timer. The only complication is with the auto-clear but that can be addressed with PPI as I mentioned. Note that nrf52_tim_lowerhalf.c is implemented in the same way but in this case the TIMER peripheral indeed supports the auto-clear by hardware on its own.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687674162
> So the implementer is free to just disable the interrupt but keep the counting. This behaviour is very important to implement the tickless mode on top of oneshot_lowerhalf_s without any drift.
No. The behavior of a one shot is clear from its definition. It requires no further definition. A one shot generates a single pulse. If it generates multiple pulses it is not a on shot.
----------------------------------------------------------------
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] xiaoxiang781216 edited a comment on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687655040
> > Is the oneshot supposed to keep counting after it fires? I understand that oneshot fires once but at least stm32_oneshot.c stops counting. So implementing up_timer_gettime() as arch_alarm.c does:
> > int up_timer_gettime(FAR struct timespec *ts)
> > {
> > int ret = -EAGAIN;
> > if (g_oneshot_lower != NULL)
> > {
> > ret = ONESHOT_CURRENT(g_oneshot_lower, ts);
> > }
> > return ret;
> > }
> > would only work if the timer continues to run AND it handles wrap-around by adding one period on overflow and returns this monotonic value on ONESHOT_CURRENT(). Neither of those two conditions are met in stm32_oneshot.c. So is stm32_oneshot.c wrong or is that not how oneshot should work?
>
> Yes, resetting and stopping the timer is the correct behavior for a oneshot timer. I expect that that all of the implementations do that.
>
But the driver document never mention wether the counting should stop or not:
```
/****************************************************************************
* Name: ONESHOT_CANCEL
*
* Description:
* Cancel the oneshot timer and return the time remaining on the timer.
*
* NOTE: This function may execute at a high rate with no timer running (as
* when pre-emption is enabled and disabled).
*
* Input Parameters:
* lower Caller allocated instance of the oneshot state structure. This
* structure must have been previously initialized via a call to
* oneshot_initialize();
* ts The location in which to return the time remaining on the
* oneshot timer. A time of zero is returned if the timer is
* not running.
*
* Returned Value:
* Zero (OK) is returned on success. A call to up_timer_cancel() when
* the timer is not active should also return success; a negated errno
* value is returned on any failure.
*
****************************************************************************/
#define ONESHOT_CANCEL(l,t) ((l)->ops->cancel(l,t))
```
So the implementer is free to just disable the interrupt but keep the counting. This behaviour is very important to implement the tickless mode on top of oneshot_lowerhalf_s without any drift.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687630050
The difference between oneshot/timer was not apparent to me. At least it is not documented anywhere. It would appear to be two different possible interfaces with no apparent change in semantics. Actually, the biggest difference is the fact that oneshot receives a timespec value instead of a microsecond value.
Moreover, I realized this timespec is *relative* (I thought it was absolute), so indeed it is a matter of representation. The difference you mention about auto-clear is documented anywhere?
Anyway, it is confusing to me that arch_timer/arch_alarm both use a timer based on a relative timeout, where arch_alarm gives the illusion to NuttX that it is working in absolute values (by using the alarm time to get a relative timeout). The point of arch_alarm is that it is supposed to be used with a timer which allows to set an absolute time in the future, see:
<pre>
config SCHED_TICKLESS_ALARM
bool "Tickless alarm"
default n
---help---
The tickless option can be supported either via a simple interval
timer (plus elapsed time) or via an alarm. The interval timer allows
programming events to occur after an interval. With the alarm,
you can set a time in the future and get an event when that alarm
goes off. This option selects the use of an alarm.
The advantage of an alarm is that it avoids some small timing
errors; the advantage of the use of the interval timer is that
the hardware requirement may be less
</pre>
I always understood from this that it should be based on an RTC peripheral which allows you to set an absolute RTC alarm.
So, it seems to me that arch_timer should actually be implemented as arch_alarm is implemented (no drift in up_timer_gettime()) instead of having one combination which does not make sense for tickless (arch_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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686839950
> > > > @btashton is ccache being used for builds? I looked at the scripts and I understand it should be working when "-c" is passed but it doesn't seem to be the case for the build workflow. Unless I'm missing something.
> > >
> > >
> > > It will not work with the Docker container. I suppose I could enable that, but the MacOS build is by far the slowest build (by at least 2x).
> >
> >
> > Isn't it possible to cache it? I used to do that with GitLab
>
> It is fully doable, I am just saying that flag will not work in the docker container because it is not configured that way. The MacOS build is more of an issue in terms of time. I am working on that now over in the testing repo.
Ok, the complexity of the build system eludes me so I'm mostly thinking out loud.
>
> I am a little hesitant to preserve the cache across builds though.
I never had a problem with it. ccache uses a hash of the preprocessed file and returns the .o (also considering flags).
at most I think it would get a false cache miss than a false cache hit.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687167345
Yes, let's stick to vendor naming.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#discussion_r483231687
##########
File path: tools/checkpatch.sh
##########
@@ -84,14 +84,14 @@ check_patch() {
fail=1
else
git apply $1
- diffs=`cat $1`
+ diffs=``
Review comment:
yea, sorry
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687678040
If needed, we should correct the version of arch_alarm.c and arch_timer.c in the repository so that it works with all repositories. Currently this logic is only usable by Xiaomi code. If it is not change, then it should be removed from the repository altogether. We do not carry vendor specific implementations that are not usable to the entire community.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686837808
> > @btashton is ccache being used for builds? I looked at the scripts and I understand it should be working when "-c" is passed but it doesn't seem to be the case for the build workflow. Unless I'm missing something.
>
> It will not work with the Docker container. I suppose I could enable that, but the MacOS build is by far the slowest build (by at least 2x).
Isn't it possible to cache it? I used to do that with GitLab
> > @btashton is ccache being used for builds? I looked at the scripts and I understand it should be working when "-c" is passed but it doesn't seem to be the case for the build workflow. Unless I'm missing something.
>
> It will not work with the Docker container. I suppose I could enable that, but the MacOS build is by far the slowest build (by at least 2x).
Not sure how GitHub does it but you can bind mount a host directory when running docker. I mapped $HOME/.ccache when did similar thing on GitLab (private installation though). If you can do that and then cache the directory between builds, it should be usable even from within container.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687632464
>
>
> Also, if you look at stm32_oneshot.c, you will find that when the timer expires it is stopped. So I don't really understand how you can use the counter to provide up_timer_gettime after it the "alarm" expires on arch_alarm.c. I feel there's something wrong here.
Of course, it if reset and fired again it wouldn't be a one shot. The one shot timer was created to support tickless mode operation for the two timer case: https://cwiki.apache.org/confluence/display/NUTTX/Tickless+OS
The one shot is also used internally to support CPU load measurements with entropy: https://cwiki.apache.org/confluence/display/NUTTX/Oneshot+Timers+and+CPU+Load+Measurement
I don't know all implementations, but normally the oneshot interface is built on top of the timer interface. The implements the required oneshot behavior as originally needed for the two-timer tickless 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] xiaoxiang781216 commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687296934
> I'm just wondering if we shouldn't change the terminology related to RTC in the NRF52 family.
>
> Nordic uses the term `RTC` to refer to a `Real Time Counter`, but industry standard refers to the RTC as `Real-Time Clock`.
> They are different peripherals with different uses.
> Names like `nrf52_rtc.c` or `nrf52_rtc_lowerhalf.c` are a bit confusing if we have some experience with other architectures.
>
> We probably should change all RTC-related names to RTCTIM or something similar, so that it is immediately known that we're referring to a timer instance not a clock instance.
>
> Of course, this is a topic for a separate PR, but I'm curious what do you think about it?
The keypoint isn't whether the hardware save the time as a counter or as y/m/d/h/m/s/us, because it is very easy to convert between these two representation(mktime and gmtime_r). The major difference is whether the timer is in an always on domain, and never stop counting. If it's true, it's better to implement rtc_lowerhalf_s, otherwise it should implement as oneshot_lowerhalf_s or timer_lowerhalf_s.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686713327
> > I don't have that, i have this:
> > check_patch()
>
> CI calls `checkpatch.sh -g $commits`
>
> option `g` uses `check_commit`
> I think my fix will work
Mmm didn't work.
----------------------------------------------------------------
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 edited a comment on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
raiden00pl edited a comment on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687013933
I'm just wondering if we shouldn't change the terminology related to RTC in the NRF52 family.
Nordic uses the term `RTC` to refer to a `Real Time Counter`, but industry standard refers to the RTC as `Real-Time Clock`.
They are different peripherals with different uses.
Names like `nrf52_rtc.c` or `nrf52_rtc_lowerhalf.c` are a bit confusing if we have some experience with other architectures.
We probably should change all RTC-related names to RTCTIM or something similar, so that it is immediately known that we're referring to a timer instance not a clock instance.
Of course, this is a topic for a separate PR, but I'm curious what do you think about it?
----------------------------------------------------------------
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] btashton commented on a change in pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#discussion_r483228442
##########
File path: tools/checkpatch.sh
##########
@@ -84,14 +84,14 @@ check_patch() {
fail=1
else
git apply $1
- diffs=`cat $1`
+ diffs=``
check_ranges <<< "$diffs"
git apply -R $1
fi
}
check_commit() {
- diffs=`git show $1`
+ diffs=`git show --no-renames $1`
Review comment:
This can just be `git diff $1` which will resolve the issue with renames. `show` was the wrong command to use here.
##########
File path: tools/checkpatch.sh
##########
@@ -84,14 +84,14 @@ check_patch() {
fail=1
else
git apply $1
- diffs=`cat $1`
+ diffs=``
Review comment:
This should not have changed
----------------------------------------------------------------
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] btashton commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686780269
@v01d check issue should be resolved by this https://github.com/apache/incubator-nuttx/pull/1706
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687670561
After looking more closely into timer_lowerhalf_s it does not appear to be a good interface to provide the periodic + freerun interface either: it currently returns the "status" (which indicates the time remaining to the timeout) but not a free running counter. It would also be necessary to handle overflow. But the problem is actually that it would require distinguishing between a timer that is capable of freerunning from one that it is not (such as an STM32 basic timer, which is the current implementation).
I think this semantic should be provided by a different interface (maybe monotonic_timer_lowerhalf_s?). This would then be a better replacement for oneshot on arch_alarm.c.
An alternative path is to follow one of Xiang suggestions and expose the timer as an rtc_lowerhalf_s (using CONFIG_RTC_HIRES in this case). This is not ideal either because the timer continues to run on low power mode but **not** on system OFF (there's not battery backup domain on nRF52). Considering this lengthy discussion I think I will take this approach for nRF52. We can revisit this in the future if a new interface is added as previously mentioned.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687655700
> But the driver document never mention wether the counting should stop or not:
> So the implementer is free to just disable the interrupt but keep the counting.
OK, so this confirms my suspicion: since this is not documented we have different interpretations on how oneshot works and this leads to problems when attempting to use arch_alarm as a general solution. I think that we should document what Greg mentioned about how oneshot should work. I've only seen stm32 implementation so far, not sure if other implementations of oneshot follow this or not. Anyone familiar with other architectures?
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#discussion_r483982469
##########
File path: arch/arm/src/nrf52/nrf52_rtc_lowerhalf.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,
+ FAR struct timer_lowerhalf_s **timer_drvr);
Review comment:
> Yes, resetting and stopping the timer is the correct behavior for a oneshot timer. I expect that that all of the implementations do that.
Ok, I will try to document this since at the moment it needs to be inferred from the implementation.
What about timer_lowerhalf_s interface? On stm32 for example, the implementation is based on the autoreload set to the timeout value. So it keeps running and the period is variable. On "advanced" STM32 timers there is a compare channel which allow to have a long and fixed autoreload (maximum value), so in this case it could be implemented differently. What is the requirement of the timer_lowerhalf_s in this regard?
> My suspicion from what you say is that the those arch drivers are not compatible with the existing MCU architectures. They are not used by any of the existing architectures in the repository; they are used only by Xiaomi hardware. They have probably implemented an incompatible version of the one shot driver for their purposes and those arch drivers may not be usable by other architectures.
>
> I am completely unfamiliar with those drivers. I have never used them or even looked at them. You would have to discuss this with Xiaomi to see if they are using some non-standard oneshot driver. I don't know what we should do if they are. Certainly we do not want to corrupt the semantics of the oneshot driver.
Well arch_alarm.c is not assuming something different (it manually restarts the oneshot on the handler), so it works, but since this is to be used with TICKLESS_ALARM, a timer capable of freerun + compare event should be used.
----------------------------------------------------------------
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 edited a comment on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d edited a comment on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687693636
> > This works when the timer does not autoreload itself (the ISR fires when the counter matches a value only) and will continue until wrapping around.
>
> And, after wrapping generates an erroneous event?
The timer usually generates a separate overflow event. I understand Xiang assumption was that could could use ONESHOT_CURRENT() to retrieve the free running count even after timeout.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#discussion_r483773149
##########
File path: arch/arm/src/nrf52/nrf52_rtc_lowerhalf.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,
+ FAR struct timer_lowerhalf_s **timer_drvr);
Review comment:
No, RTC does not support auto-clearing. I'm doing this by manually clearing it on interrupt (see `nrf52_rtc_handler`). nRF52 has a peripheral which allows you to tie events to tasks, so you could connect the COMPARE event to the CLEAR task and then it will be taken care of by hardware. However, this uses up one PPI channel which may not be desireable. This is stated in the comment here (expand the _lowerhalf file diff): https://github.com/apache/incubator-nuttx/pull/1705/files#diff-57c967820db5f66391fba3ad66318487R156
Anyway, I also wondered what was the proper way to expose the RTC (via timer or arch_alarm + oneshot). The point is that oneshot interface is designed to work with absolute times, presumably with a real RTC (which does not wrap around in a long time). On up_timer_gettime from arch_alarm.c, the counter is obtained as an absolute value. Since nRF52 RTC is simply a low power timer (with a period of 512 seconds) it would be a lot more complex to offer that interface.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687693636
> > This works when the timer does not autoreload itself (the ISR fires when the counter matches a value only) and will continue until wrapping around.
>
> And, after wrapping generates an erroneous event?
The timer usually generates a separate overflow event. Xiang assumption was that could could use ONESHOT_CURRENT() to retrieve the free running count even after timeout.
----------------------------------------------------------------
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] xiaoxiang781216 commented on a change in pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#discussion_r483765306
##########
File path: arch/arm/src/nrf52/Make.defs
##########
@@ -89,7 +89,7 @@ ifeq ($(CONFIG_ARCH_CHIP_NRF52832),y)
CHIP_CSRCS += nrf52832_errdata.c
endif
-CHIP_CSRCS += nrf52_timerisr.c
+CHIP_CSRCS += nrf52_systimer.c
Review comment:
need rename the comment "arch/arm/src/nrf52/nrf52_timerisr.c" to "arch/arm/src/nrf52/nrf52_systimer.c"
##########
File path: arch/arm/src/nrf52/nrf52_rtc_lowerhalf.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,
+ FAR struct timer_lowerhalf_s **timer_drvr);
Review comment:
Does NFR52 RTC will clear the count when the count value equal comparison value? If the anwser isn't, it's better to export RTC through oneshot_lowerhalf_s interface, because the timestamp implemented through arch_timer(timer_lowerhalf_s) will be drift in the long run.
----------------------------------------------------------------
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 edited a comment on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d edited a comment on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687633129
Also, looking at the first wiki entry you linked:
> Since timers are a limited resource, the use of two timers could be an issue on some systems. The job could be done with a single timer if, for example, the single timer were kept in a free-running at all times. Some timer/counters have the capability to generate a compare interrupt when the timer matches a comparison value but also to continue counting without stopping. If your hardware supports such counters, one might used the CONFIG_SCHED_TICKLESS_ALARM option and be able to simply set the comparison count at the value of the free running timer PLUS the desired delay. Then you could have both with a single timer: An alarm and a free-running counter with the same timer!
arch_alarm.c should indeed use a free running timer, not one that requires restarting. Otherwise you are exactly in the case of non-alarm tickless 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] xiaoxiang781216 edited a comment on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687296934
> I'm just wondering if we shouldn't change the terminology related to RTC in the NRF52 family.
>
> Nordic uses the term `RTC` to refer to a `Real Time Counter`, but industry standard refers to the RTC as `Real-Time Clock`.
> They are different peripherals with different uses.
> Names like `nrf52_rtc.c` or `nrf52_rtc_lowerhalf.c` are a bit confusing if we have some experience with other architectures.
>
> We probably should change all RTC-related names to RTCTIM or something similar, so that it is immediately known that we're referring to a timer instance not a clock instance.
>
> Of course, this is a topic for a separate PR, but I'm curious what do you think about it?
The keypoint isn't whether the hardware save the time as a counter or as y/m/d/h/m/s/us, because it is very easy to convert between these two representation(mktime and gmtime_r). The major difference is whether the timer is in an always on domain, and never stop counting. If it's true, it's better to implement as rtc_lowerhalf_s, otherwise it should implement as oneshot_lowerhalf_s or timer_lowerhalf_s.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687631582
Also, if you look at stm32_oneshot.c, you will find that when the timer expires it is stopped. So I don't really understand how you can use the counter to provide up_timer_gettime after it the "alarm" expires on arch_alarm.c. I feel there's something wrong here.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687647565
>
>
> > Of course, it if reset and fired again it wouldn't be a one shot. The one shot timer was created to support tickless mode operation for the two timer case: https://cwiki.apache.org/confluence/display/NUTTX/Tickless+OS
>
> That's exactly my thought. So, arch_alarm.c using oneshot does not really sound a good combination, is it?
> I mean, there will be drift while the timer is stopped between the ISR and when the next up_alarm_start is called.
It would would not be the tidiest solution since it is just a wrapper around the timer interface and since the timer interface can be configured to support a periodic timer.
If you are clever in how you set up the next oneshot delay, then you could avoid drift. But there would be drift if you naively set up a fixed delay from the current time without accounting for the processing delay from the timer expiration.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686727548
This works:
<pre>
git diff master..HEAD | ./tools/checkpatch.sh -
</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] patacongo commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687693315
> This works when the timer does not autoreload itself (the ISR fires when the counter matches a value only) and will continue until wrapping around.
And, after wrapping generates an erroneous event?
----------------------------------------------------------------
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] btashton commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686839195
> > > @btashton is ccache being used for builds? I looked at the scripts and I understand it should be working when "-c" is passed but it doesn't seem to be the case for the build workflow. Unless I'm missing something.
> >
> >
> > It will not work with the Docker container. I suppose I could enable that, but the MacOS build is by far the slowest build (by at least 2x).
>
> Isn't it possible to cache it? I used to do that with GitLab
It is fully doable, I am just saying that flag will not work in the docker container because it is not configured that way. The MacOS build is more of an issue in terms of time. I am working on that now over in the testing repo.
I am a little hesitant to preserve the cache across builds though.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687658496
> So the implementer is free to just disable the interrupt but keep the counting. This behaviour is very important to implement the tickless mode on top of oneshot_lowerhalf_s without any drift.
My suggestion is to not base tickless mode on this expectation of oneshot continuing to count after expiration. But arch_alarm should indeed be implemented over an interface which guarantees the timer continues to run without any manual restart. timer_lowerhalf_s could be used for this, but current implementations do not guarantee this property either (stm32/smt32l4 will autoreload the timer, but for "advanced timers" this would be possible) and overflow should be handled by exposing the overflow ISR. We really need to document the semantics of each interface so that we agree on how things should work.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687681106
I don't think the current logic is wrong per se, it is simply not the best implementation. If we added another timer interface for freerun + interval then arch_alarm can be improved and would then be a good foundation for supporting tickless and non-tickless modes easily. Let's keep it as is for now, I can try to come up with something later on. I will open an issue for this and update this PR to use rtc_lowerhalf_s.
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686808109
> @v01d check issue should be resolved by this #1706
Thanks, just rebased and 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] xiaoxiang781216 commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687693750
> > > > So the implementer is free to just disable the interrupt but keep the counting. This behaviour is very important to implement the tickless mode on top of oneshot_lowerhalf_s without any drift.
> > >
> > >
> > > No. The behavior of a one shot is clear from its definition. It requires no further definition. A one shot generates a single pulse. If it generates multiple pulses it is not a on shot.
> >
> >
> > @patacongo before you give ANY feedback, please read other people's comment CAREFULLY. I EXPLICTLY mention that "DISABLE THE INTERRUPT BUT KEEP THE COUNTING". The INTERRUPT already DISABLE, could you telll me how to generate MULTIPLE PULSE?
>
> A one shot, by its definition, generates a single event. That is the required behavior to be a one shot. Anything that generates multiple events is not a one shot. What you are describing is not a one shot.
>
> In order to get another event, the one shot has to be started again. Refer to drivers/timer/oneshot.c for the definitive use of the one shot interface.
Of course, if you want to get another notification, you must call start again. Nobody change the behaviour: there is no furture event anymore once the event fired. The only difference is that the timer is keep counting.
----------------------------------------------------------------
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] xiaoxiang781216 commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687655040
> > Is the oneshot supposed to keep counting after it fires? I understand that oneshot fires once but at least stm32_oneshot.c stops counting. So implementing up_timer_gettime() as arch_alarm.c does:
> > int up_timer_gettime(FAR struct timespec *ts)
> > {
> > int ret = -EAGAIN;
> > if (g_oneshot_lower != NULL)
> > {
> > ret = ONESHOT_CURRENT(g_oneshot_lower, ts);
> > }
> > return ret;
> > }
> > would only work if the timer continues to run AND it handles wrap-around by adding one period on overflow and returns this monotonic value on ONESHOT_CURRENT(). Neither of those two conditions are met in stm32_oneshot.c. So is stm32_oneshot.c wrong or is that not how oneshot should work?
>
> Yes, resetting and stopping the timer is the correct behavior for a oneshot timer. I expect that that all of the implementations do that.
>
But the driver document never mention wether the counting should stop or not:
```
/****************************************************************************
* Name: ONESHOT_CANCEL
*
* Description:
* Cancel the oneshot timer and return the time remaining on the timer.
*
* NOTE: This function may execute at a high rate with no timer running (as
* when pre-emption is enabled and disabled).
*
* Input Parameters:
* lower Caller allocated instance of the oneshot state structure. This
* structure must have been previously initialized via a call to
* oneshot_initialize();
* ts The location in which to return the time remaining on the
* oneshot timer. A time of zero is returned if the timer is
* not running.
*
* Returned Value:
* Zero (OK) is returned on success. A call to up_timer_cancel() when
* the timer is not active should also return success; a negated errno
* value is returned on any failure.
*
****************************************************************************/
#define ONESHOT_CANCEL(l,t) ((l)->ops->cancel(l,t))
```
So the implementer is free to just disable the interrupt but keep the counting.
> My suspicion from what you say is that the those arch drivers are not compatible with the existing MCU architectures. They are not used by any of the existing architectures in the repository; they are used only by Xiaomi hardware. They have probably implemented an incompatible version of the one shot driver for their purposes and those arch drivers may not be usable by other architectures.
>
> I am completely unfamiliar with those drivers. I have never used them or even looked at them. You would have to discuss this with Xiaomi to see if they are using some non-standard oneshot driver. I don't know what we should do if they are. Certainly we do not want to corrupt the semantics of the oneshot driver.
----------------------------------------------------------------
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] btashton commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686709594
> This is failing the check due to #1637
Can you try applying this patch to work around the rename issue for now?
```
diff --git a/tools/checkpatch.sh b/tools/checkpatch.sh
index 0171d37d19..752c44eb69 100755
--- a/tools/checkpatch.sh
+++ b/tools/checkpatch.sh
@@ -91,7 +91,7 @@ check_patch() {
}
check_commit() {
- diffs=`git show $1`
+ diffs=`git show --no-renames $1`
check_ranges <<< "$diffs"
}
```
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687883361
I decided to implement tickless support using RTC timer directly as it is easier for me at this point than addressing a more general solution to the problem discussed here so far. I will close this in order to start a new PR and make it easier to review.
----------------------------------------------------------------
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 edited a comment on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d edited a comment on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-686710472
I don't have that, i have this:
<pre>
check_patch() {
if ! git apply --check $1; then
fail=1
else
git apply $1
diffs=`cat $1`
check_ranges <<< "$diffs"
git apply -R $1
fi
}
</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] raiden00pl commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687013933
I'm just wondering if we shouldn't change the terminology related to RTC in the NRF52 family.
Nordic uses the term `RTC` to refer to a `Real Time Counter`, but industry standard refers to the RTC as `Real-Time Clock`.
They are different peripherals with different uses.
Names like `nrf52_rtc.c` or `nrf52_rtc_lowerhalf.c` are a bit confusing if we have some experience with other architectures.
We probably should change all RTC-related names to RTCTIM or something similar, so that it is immediately known that we're referring to a timer instance not a clock instance.
Of course, this is a topic for a separate PR, but I'm curious what do you thing about it?
----------------------------------------------------------------
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 #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#discussion_r483232061
##########
File path: tools/checkpatch.sh
##########
@@ -84,14 +84,14 @@ check_patch() {
fail=1
else
git apply $1
- diffs=`cat $1`
+ diffs=``
check_ranges <<< "$diffs"
git apply -R $1
fi
}
check_commit() {
- diffs=`git show $1`
+ diffs=`git show --no-renames $1`
Review comment:
Ok, I confirm it works. Although I have to be standing on nuttx/. From nuttx/tools it does not work.
----------------------------------------------------------------
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] davids5 commented on pull request #1705: nRF52: refactor system timer handling; support tickless and nontickless using RTC/systick
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1705:
URL: https://github.com/apache/incubator-nuttx/pull/1705#issuecomment-687018631
> Of course, this is a topic for a separate PR, but I'm curious what do you think about it?
It should match the datasheet/reference manual for the SoC to the letter.
At the top of the `nrf52_rtc.c` source file place at least one line that says
`The is the driver for the Nordic xxx Real Time Counter`
If we do it this way, it is fully documented and self describing.
----------------------------------------------------------------
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