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/06/15 21:32:59 UTC
[GitHub] [incubator-nuttx] v01d opened a new pull request #1248: Do not set oneshot zero timer period
v01d opened a new pull request #1248:
URL: https://github.com/apache/incubator-nuttx/pull/1248
## Summary
The oneshot timer would sets autoreload register to zero when a period equal to zero was requested. This would never be triggered since the ARR needs to be at least 1. This commit changes this case to set a minimum period of zero.
The tone driver had an artificial delay of 1s possibly due to this problem which introduced an undersired lag when trying to play a tune. Since the oneshot is now fixed, this delay is removed and it now works as expected without lag.
There is one caveat: I found that using stop() would not allow further notes to be played. Looking at the PWM code it would seem that the tone should be stopped setting a duty of zero, which is what I did. But I'm not sure this is a bug in the PWM code. Could someone verify that?
## Impact
STM32L4 oneshot timer for period == 0. I don't think there are other cases for this scenario currently so it should not affect anything else.
## Testing
Tested the tone driver on STM32L4 and worked as expected without the delay. This was on tickless mode which uses oneshot() for the underlying timer, so this indirectly tests also that.
----------------------------------------------------------------
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 #1248: Do not set oneshot zero timer period
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1248:
URL: https://github.com/apache/incubator-nuttx/pull/1248#issuecomment-647135549
Ok, I switched to using a DEBUGASSERT and made the tone driver start playing the tune without scheduling the timer call.
----------------------------------------------------------------
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] Ouss4 merged pull request #1248: Do not set oneshot zero timer period
Posted by GitBox <gi...@apache.org>.
Ouss4 merged pull request #1248:
URL: https://github.com/apache/incubator-nuttx/pull/1248
----------------------------------------------------------------
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 #1248: Do not set oneshot zero timer period
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #1248:
URL: https://github.com/apache/incubator-nuttx/pull/1248#issuecomment-646229938
I think it is the responsibility of the caller to provide a valid input data for the called function. If the input is incorrect, we expect the called function to return an error.
We're discussing a rare situation so in most cases there will never be a problem.
But if a problem occurs, we have the following options:
1. Trigger the callback with an incorrect time. The caller thinks everything is OK. We can spend hours searching for why something is wrong.
2. Do not trigger the callback and return the error code. The caller knows that something is wrong.
sleep()/usleep() are not designed to provide precise time delays, but one-shot timers are based on more precise timers, so they can be used for more precise triggering. Looking for time-related bugs in a situation where the system gives no feedback can be a nightmare :)
----------------------------------------------------------------
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 #1248: Do not set oneshot zero timer period
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #1248:
URL: https://github.com/apache/incubator-nuttx/pull/1248#issuecomment-645180373
I'm not sure if this is the correct behavior. Shouldn't we return an error when a given period is not achievable?
This is the case where the function argument is out of the supported range, and in this case we should return an error (probably -EINVAL).
----------------------------------------------------------------
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 #1248: Do not set oneshot zero timer period
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #1248:
URL: https://github.com/apache/incubator-nuttx/pull/1248#issuecomment-645774933
We currently have ONESHOT_MAX_DELAY() macro, so the additional ONESHOT_MIN_DELAY() macro seems like a good idea.
It seems to me that not only stm32l4 is affected by this problem, so it would be good to make the right decision here.
In the case of start_tune(), why not just call oneshot_callback() directly?
----------------------------------------------------------------
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 #1248: Do not set oneshot zero timer period
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1248:
URL: https://github.com/apache/incubator-nuttx/pull/1248#issuecomment-645430572
My 2 cents is: The change, as is, solves the problem of mapping freg to ARR and the onshot will be fired, albeit immediately.
----------------------------------------------------------------
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 #1248: Do not set oneshot zero timer period
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1248:
URL: https://github.com/apache/incubator-nuttx/pull/1248#issuecomment-646875326
Ok, I can make it simply return an error if period == 0. Adding a min_delay seems redundant when the oneshot timer already receives a "resolution" parameter. It would be expected that this is in particular the minimum number allowed.
Should this be a recoverable error? Or a DEBUGASSERT() just like the `DEBUGASSERT(period <= UINT32_MAX);`?
----------------------------------------------------------------
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 #1248: Do not set oneshot zero timer period
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1248:
URL: https://github.com/apache/incubator-nuttx/pull/1248#issuecomment-645424929
> I'm not sure if this is the correct behavior. Shouldn't we return an error when a given period is not achievable?
> This is the case where the function argument is out of the supported range, and in this case we should return an error (probably -EINVAL).
This may be another option. And in fact, you could argue that requesting zero timeout is like sleep(0), it should "hang" (in the context of the oneshot timer, not get called). However, when you call e.g. usleep(1) you're not guaranteed to sleep exactly 1us but may be more than that. If one requests a oneshot() timer with a small period but > 0 there's also the chance that the ARR ends up zero due to the timer frequency. In other words, the caller of the oneshot setup call has no way of knowing what is the minimum valid time for the timer. If you return EINVAL there should be a mechanism of requesting the smallest possible valid oneshot() time.
----------------------------------------------------------------
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 #1248: Do not set oneshot zero timer period
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #1248:
URL: https://github.com/apache/incubator-nuttx/pull/1248#issuecomment-647096020
`DEBUGASSERT(period > 0);` is 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] v01d commented on pull request #1248: Do not set oneshot zero timer period
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1248:
URL: https://github.com/apache/incubator-nuttx/pull/1248#issuecomment-646051733
> We currently have ONESHOT_MAX_DELAY() macro, so the additional ONESHOT_MIN_DELAY() macro seems like a good idea.
Adding that macro can be useful but not modifying the code for when the timer period counter is rounded to zero means that one would be always be forced to ensure it is not requesting a delay less than the minimum settable delay.
Since it is expected that if you're setting up the oneshot timer you want it to trigger, I would consider this special case and round up, not down, so that any requested time below min_delay fires after min_delay. Then, if it is important to the caller to know if the requested delay will be achievable, min_delay can be used to check.
> It seems to me that not only stm32l4 is affected by this problem, so it would be good to make the right decision here.
>
You're right, this should probably be extended to other platforms. At least for STM32 I think this is always the case (ARR needs to be at least 1) but I'm not sure about which others.
> In the case of start_tune(), why not just call oneshot_callback() directly?
Maybe @acassis can answer that. Yes, that's probably simpler. I assumed it was required that the callback be from an interrupt context for some reason.
----------------------------------------------------------------
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