You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by "Matias N." <ma...@imap.cc> on 2020/09/02 22:30:53 UTC
supporting tickless and non-tickless using arch_timer/alarm/rtc
Hi,
I'm looking into implementing tickless on nRF52 (first using systick, since it is an easy option, and then using a RTC timer, which works while CPU is asleep). My intention is to use the arch timer driver for this (drivers/timer/arch_timer.c) since it provides up_mdelay without using a busy loop and it supports both tickless and non-tickless. However, the code right now
turns up to be a bit akward since the arch timer is optional so you have to support the case for when it is off. This means that tickless mode needs to be separately supported. On STM32 you can see this in stm32_timerisr.c (where there are two implementations, depending on the arch timer being ON or not) and in stm32_tickless.c (which does not uses the arch timer, but provides tickless functionality on its own).
I understand this is due to arch_timer/rtc/alarm being introduced more recently but I'm not sure if it makes sense to have them as optional instead of the standard way to provide both tickless and non-tickless modes. IMHO it would be cleaner and simpler (to implement and to configure the build for each case) to do the latter. Or is there a downside to this?
If you agree, I think the change would consist of:
* deprecate the "old way" (maybe open an issue which lists which arch is not yet converted), only accept the "new way" in new ports
* convert each arch (by someone who is able to test it) by:
- replace *_timerisr.c and *_tickless.c for each arch by one file (*_systime.c?) where the appropriate arch_timer/rtc/alarm interface is invoked, based on supported options for the arch (systick should be the basic option).
- implement each possible system timer source as a arch_timer/rtc/alarm lowerhalf in its own file
Best,
Matias
Re: supporting tickless and non-tickless using arch_timer/alarm/rtc
Posted by "Matias N." <ma...@imap.cc>.
Ok. I will open a ticket for this since stm32_tim_lowerhalf has this (unnecessary) limitation.
On Fri, Sep 4, 2020, at 06:43, Xiang Xiao wrote:
> Yes, arch_timer change the interrupt interval without stopping the timer first to reduce the timer drift as much as possible. So the
> timer driver has to remove the restriction from the code.
>
> > -----Original Message-----
> > From: Matias N. <ma...@imap.cc>
> > Sent: Friday, September 4, 2020 2:13 AM
> > To: dev@nuttx.apache.org
> > Subject: Re: supporting tickless and non-tickless using arch_timer/alarm/rtc
> >
> > Hi Xiang,
> > can you confirm that arch_timer.c assumes that the timer resets itself? (as if using STM32 autoreload?) It doesn't call
> TIMER_STOP()
> > on the underlying timer on timer_cancel(). In fact, it simply sets a maximum timeout.
> > I understand this is because you also need the timer to be running to provide up_timer_gettime().
> > If this is so, it would be incorrect to do this in stm32/nrf52_tim_lowerhalf.c (I'm using it as reference):
> >
> > static int stm32_settimeout(FAR struct timer_lowerhalf_s *lower, uint32_t timeout) {
> > FAR struct stm32_lowerhalf_s *priv = (FAR struct stm32_lowerhalf_s *)lower;
> > uint64_t maxtimeout;
> >
> > if (priv->started)
> > {
> > return -EPERM;
> > }
> > ...
> >
> > (nRF52 has the same check on priv->started). This means that the settimeout does not succeed.
> > On my nrf52_rtc_lowerhalf.c I have removed this check and I get tickless working but this would mean the same fix should be
> applied
> > on other similar places. In principle this is OK because changing the timeout while timer is running should not be a problem.
> >
>
> > Best,
> > Matias
> >
> > On Thu, Sep 3, 2020, at 12:29, Matias N. wrote:
> > > Hi,
> > > thanks, I will select TIMER_ARCH from Kconfig for this arch for now. I can create an issue later.
> > > I will also try to document these three drivers and link them from the discussion about tickless support.
> > >
> > > Best,
> > > Matias
>
>
RE: supporting tickless and non-tickless using arch_timer/alarm/rtc
Posted by Xiang Xiao <xi...@gmail.com>.
Yes, arch_timer change the interrupt interval without stopping the timer first to reduce the timer drift as much as possible. So the
timer driver has to remove the restriction from the code.
> -----Original Message-----
> From: Matias N. <ma...@imap.cc>
> Sent: Friday, September 4, 2020 2:13 AM
> To: dev@nuttx.apache.org
> Subject: Re: supporting tickless and non-tickless using arch_timer/alarm/rtc
>
> Hi Xiang,
> can you confirm that arch_timer.c assumes that the timer resets itself? (as if using STM32 autoreload?) It doesn't call
TIMER_STOP()
> on the underlying timer on timer_cancel(). In fact, it simply sets a maximum timeout.
> I understand this is because you also need the timer to be running to provide up_timer_gettime().
> If this is so, it would be incorrect to do this in stm32/nrf52_tim_lowerhalf.c (I'm using it as reference):
>
> static int stm32_settimeout(FAR struct timer_lowerhalf_s *lower, uint32_t timeout) {
> FAR struct stm32_lowerhalf_s *priv = (FAR struct stm32_lowerhalf_s *)lower;
> uint64_t maxtimeout;
>
> if (priv->started)
> {
> return -EPERM;
> }
> ...
>
> (nRF52 has the same check on priv->started). This means that the settimeout does not succeed.
> On my nrf52_rtc_lowerhalf.c I have removed this check and I get tickless working but this would mean the same fix should be
applied
> on other similar places. In principle this is OK because changing the timeout while timer is running should not be a problem.
>
> Best,
> Matias
>
> On Thu, Sep 3, 2020, at 12:29, Matias N. wrote:
> > Hi,
> > thanks, I will select TIMER_ARCH from Kconfig for this arch for now. I can create an issue later.
> > I will also try to document these three drivers and link them from the discussion about tickless support.
> >
> > Best,
> > Matias
Re: supporting tickless and non-tickless using arch_timer/alarm/rtc
Posted by "Matias N." <ma...@imap.cc>.
Hi Xiang,
can you confirm that arch_timer.c assumes that the timer resets itself? (as if using STM32 autoreload?)
It doesn't call TIMER_STOP() on the underlying timer on timer_cancel(). In fact, it simply sets a maximum timeout.
I understand this is because you also need the timer to be running to provide up_timer_gettime().
If this is so, it would be incorrect to do this in stm32/nrf52_tim_lowerhalf.c (I'm using it as reference):
static int stm32_settimeout(FAR struct timer_lowerhalf_s *lower, uint32_t timeout)
{
FAR struct stm32_lowerhalf_s *priv = (FAR struct stm32_lowerhalf_s *)lower;
uint64_t maxtimeout;
if (priv->started)
{
return -EPERM;
}
...
(nRF52 has the same check on priv->started). This means that the settimeout does not succeed.
On my nrf52_rtc_lowerhalf.c I have removed this check and I get tickless working but this would mean
the same fix should be applied on other similar places. In principle this is OK because changing the
timeout while timer is running should not be a problem.
Best,
Matias
On Thu, Sep 3, 2020, at 12:29, Matias N. wrote:
> Hi,
> thanks, I will select TIMER_ARCH from Kconfig for this arch for now. I can create an issue later.
> I will also try to document these three drivers and link them from the discussion about tickless support.
>
> Best,
> Matias
Re: supporting tickless and non-tickless using arch_timer/alarm/rtc
Posted by "Matias N." <ma...@imap.cc>.
Hi,
thanks, I will select TIMER_ARCH from Kconfig for this arch for now. I can create an issue later.
I will also try to document these three drivers and link them from the discussion about tickless support.
Best,
Matias
RE: supporting tickless and non-tickless using arch_timer/alarm/rtc
Posted by Xiang Xiao <xi...@gmail.com>.
> -----Original Message-----
> From: Matias N. <ma...@imap.cc>
> Sent: Thursday, September 3, 2020 6:31 AM
> To: dev@nuttx.apache.org
> Subject: supporting tickless and non-tickless using arch_timer/alarm/rtc
>
> Hi,
> I'm looking into implementing tickless on nRF52 (first using systick, since it is an easy option, and then using a RTC timer,
which works
> while CPU is asleep). My intention is to use the arch timer driver for this (drivers/timer/arch_timer.c) since it provides
up_mdelay
> without using a busy loop and it supports both tickless and non-tickless. However, the code right now turns up to be a bit akward
since
> the arch timer is optional so you have to support the case for when it is off. This means that tickless mode needs to be
separately
> supported.
We can auto select TIMER_ARCH under config ARCH_CHIP_NRF52, so we don't need provide two implementation.
> On STM32 you can see this in stm32_timerisr.c (where there are two implementations, depending on the arch timer being
> ON or not) and in stm32_tickless.c (which does not uses the arch timer, but provides tickless functionality on its own).
>
> I understand this is due to arch_timer/rtc/alarm being introduced more recently but I'm not sure if it makes sense to have them as
> optional instead of the standard way to provide both tickless and non-tickless modes. IMHO it would be cleaner and simpler (to
> implement and to configure the build for each case) to do the latter. Or is there a downside to this?
>
Yes, it doesn't make sense to provide two implementation with the same functionality. The chip owner should select one and stick
with that approach. Selecting by Kconfig just make the code hard to maintain.
> If you agree, I think the change would consist of:
> * deprecate the "old way" (maybe open an issue which lists which arch is not yet converted), only accept the "new way" in new
ports
> * convert each arch (by someone who is able to test it) by:
> - replace *_timerisr.c and *_tickless.c for each arch by one file (*_systime.c?) where the appropriate arch_timer/rtc/alarm
interface
> is invoked, based on supported options for the arch (systick should be the basic option).
> - implement each possible system timer source as a arch_timer/rtc/alarm lowerhalf in its own file
>
Yes, we can do the conversion step by step. Once the work is done, arch_timer.c can be compiled by default and remove TIMER_ARCH
Kconfig.
> Best,
> Matias