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 2021/03/12 17:55:34 UTC
[GitHub] [incubator-nuttx] saramonteiro opened a new pull request #3048: xtensa/esp32: timer driver refactor
saramonteiro opened a new pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048
## Summary
It applies several improvements that were done while developing the timer driver for the ESP32-C3.
It removes unused code.
It Improves some logic in the isr attach function and in the prescaler calculation.
## Impact
All esp32 timer users.
## Testing
It was tested using the timer example for the 4 timers.
----------------------------------------------------------------
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] saramonteiro commented on a change in pull request #3048: xtensa/esp32: timer driver refactor
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048#discussion_r593392043
##########
File path: arch/xtensa/src/esp32/esp32_tim.c
##########
@@ -252,13 +248,10 @@ static void esp32_tim_modifyreg32(FAR struct esp32_tim_dev_s *dev,
*
****************************************************************************/
-static int esp32_tim_start(FAR struct esp32_tim_dev_s *dev)
+static void esp32_tim_start(FAR struct esp32_tim_dev_s *dev)
Review comment:
Please, note that this start is from lower half layer not from `esp32_tim.c`. The start function from lower half is still returning int value.
----------------------------------------------------------------
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] acassis commented on a change in pull request #3048: xtensa/esp32: timer driver refactor
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048#discussion_r593358395
##########
File path: arch/xtensa/src/esp32/esp32_tim.c
##########
@@ -73,35 +71,28 @@ static void esp32_tim_modifyreg32(FAR struct esp32_tim_dev_s *dev,
/* TIM operations ***********************************************************/
-static int esp32_tim_start(FAR struct esp32_tim_dev_s *dev);
-static int esp32_tim_stop(FAR struct esp32_tim_dev_s *dev);
-static int esp32_tim_clear(FAR struct esp32_tim_dev_s *dev);
-static int esp32_tim_configure(FAR struct esp32_tim_dev_s *dev, uint16_t pre,
- uint8_t mode, uint64_t counter_value,
- uint64_t alarm_value, bool alarm,
- bool autoreload);
-static int esp32_tim_setmode(FAR struct esp32_tim_dev_s *dev, uint8_t mode);
-static int esp32_tim_setpre(FAR struct esp32_tim_dev_s *dev, uint16_t pre);
-static int esp32_tim_getconfig(FAR struct esp32_tim_dev_s *dev,
- uint32_t *value);
-static int esp32_tim_getcounter(FAR struct esp32_tim_dev_s *dev,
- uint64_t *value);
-static int esp32_tim_setcounter(FAR struct esp32_tim_dev_s *dev,
- uint64_t value);
-static int esp32_tim_reload_now(FAR struct esp32_tim_dev_s *dev);
-static int esp32_tim_getalarmvalue(FAR struct esp32_tim_dev_s *dev,
- uint64_t *value);
-static int esp32_tim_setalarmvalue(FAR struct esp32_tim_dev_s *dev,
- uint64_t value);
-static int esp32_tim_setalarm(FAR struct esp32_tim_dev_s *dev, bool enable);
-static int esp32_tim_setautoreload(FAR struct esp32_tim_dev_s *dev,
- bool enable);
-static int esp32_tim_setisr(FAR struct esp32_tim_dev_s *dev, xcpt_t handler,
- FAR void * arg);
-static int esp32_tim_enableint(FAR struct esp32_tim_dev_s *dev);
-static int esp32_tim_disableint(FAR struct esp32_tim_dev_s *dev);
-static int esp32_tim_checkint(FAR struct esp32_tim_dev_s *dev);
-static int esp32_tim_ackint(FAR struct esp32_tim_dev_s *dev);
+static void esp32_tim_start(FAR struct esp32_tim_dev_s *dev);
+static void esp32_tim_stop(FAR struct esp32_tim_dev_s *dev);
+static void esp32_tim_clear(FAR struct esp32_tim_dev_s *dev);
+static void esp32_tim_setmode(FAR struct esp32_tim_dev_s *dev, uint8_t mode);
+static void esp32_tim_setpre(FAR struct esp32_tim_dev_s *dev, uint16_t pre);
+static void esp32_tim_getcounter(FAR struct esp32_tim_dev_s *dev,
+ uint64_t *value);
+static void esp32_tim_setcounter(FAR struct esp32_tim_dev_s *dev,
+ uint64_t value);
+static void esp32_tim_reload_now(FAR struct esp32_tim_dev_s *dev);
+static void esp32_tim_getalarmvalue(FAR struct esp32_tim_dev_s *dev,
+ uint64_t *value);
+static void esp32_tim_setalarmvalue(FAR struct esp32_tim_dev_s *dev,
+ uint64_t value);
+static void esp32_tim_setalarm(FAR struct esp32_tim_dev_s *dev, bool enable);
+static void esp32_tim_setautoreload(FAR struct esp32_tim_dev_s *dev,
+ bool enable);
+static int esp32_tim_setisr(FAR struct esp32_tim_dev_s *dev, xcpt_t handler,
+ FAR void * arg);
Review comment:
please remove space "* arg"
----------------------------------------------------------------
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] acassis commented on a change in pull request #3048: xtensa/esp32: timer driver refactor
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048#discussion_r593361260
##########
File path: arch/xtensa/src/esp32/esp32_tim.c
##########
@@ -252,13 +248,10 @@ static void esp32_tim_modifyreg32(FAR struct esp32_tim_dev_s *dev,
*
****************************************************************************/
-static int esp32_tim_start(FAR struct esp32_tim_dev_s *dev)
+static void esp32_tim_start(FAR struct esp32_tim_dev_s *dev)
Review comment:
Why to change it to void? The timer API is expecting a value:
```
case TCIOC_START:
{
/* Start the timer, resetting the time to the current timeout */
if (lower->ops->start)
{
ret = lower->ops->start(lower);
}
```
----------------------------------------------------------------
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] acassis commented on pull request #3048: xtensa/esp32: timer driver refactor
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048#issuecomment-798999507
Ok, I will merge, but I think these modifications need to applied to the timer from other arch(es) too
----------------------------------------------------------------
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] acassis commented on pull request #3048: xtensa/esp32: timer driver refactor
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048#issuecomment-797663912
I think it is not a good idea to move these functions to void, you could implement some logic testings in the future that you are don't realize now.
----------------------------------------------------------------
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] acassis merged pull request #3048: xtensa/esp32: timer driver refactor
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048
----------------------------------------------------------------
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] acassis commented on a change in pull request #3048: xtensa/esp32: timer driver refactor
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048#discussion_r593393302
##########
File path: arch/xtensa/src/esp32/esp32_tim.c
##########
@@ -269,13 +262,10 @@ static int esp32_tim_start(FAR struct esp32_tim_dev_s *dev)
*
****************************************************************************/
-static int esp32_tim_stop(FAR struct esp32_tim_dev_s *dev)
+static void esp32_tim_stop(FAR struct esp32_tim_dev_s *dev)
Review comment:
Yes, I noticed it later. But I think it is a good idea to follow the same standard. Also to keep in sync with the other timer drivers.
----------------------------------------------------------------
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] saramonteiro commented on a change in pull request #3048: xtensa/esp32: timer driver refactor
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048#discussion_r593392204
##########
File path: arch/xtensa/src/esp32/esp32_tim.c
##########
@@ -269,13 +262,10 @@ static int esp32_tim_start(FAR struct esp32_tim_dev_s *dev)
*
****************************************************************************/
-static int esp32_tim_stop(FAR struct esp32_tim_dev_s *dev)
+static void esp32_tim_stop(FAR struct esp32_tim_dev_s *dev)
Review comment:
Please, note that this stop is from lower half layer not from `esp32_tim.c`. The stop function from lower half is still returning int value.
----------------------------------------------------------------
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] acassis commented on a change in pull request #3048: xtensa/esp32: timer driver refactor
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048#discussion_r593421574
##########
File path: arch/xtensa/src/esp32/esp32_tim.c
##########
@@ -252,13 +248,10 @@ static void esp32_tim_modifyreg32(FAR struct esp32_tim_dev_s *dev,
*
****************************************************************************/
-static int esp32_tim_start(FAR struct esp32_tim_dev_s *dev)
+static void esp32_tim_start(FAR struct esp32_tim_dev_s *dev)
Review comment:
> Please, note that this start is from lower half layer not from `esp32_tim.c`. The start function from lower half is still returning int value.
Yes, I noticed it later. But I think it is a good idea to follow the same standard. Also to keep in sync with the other timer drivers.
----------------------------------------------------------------
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] acassis commented on a change in pull request #3048: xtensa/esp32: timer driver refactor
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048#discussion_r593363359
##########
File path: arch/xtensa/src/esp32/esp32_tim.c
##########
@@ -286,98 +276,12 @@ static int esp32_tim_stop(FAR struct esp32_tim_dev_s *dev)
*
****************************************************************************/
-static int esp32_tim_clear(FAR struct esp32_tim_dev_s *dev)
+static void esp32_tim_clear(FAR struct esp32_tim_dev_s *dev)
Review comment:
Ditto, keep is as int
##########
File path: arch/xtensa/src/esp32/esp32_tim.c
##########
@@ -269,13 +262,10 @@ static int esp32_tim_start(FAR struct esp32_tim_dev_s *dev)
*
****************************************************************************/
-static int esp32_tim_stop(FAR struct esp32_tim_dev_s *dev)
+static void esp32_tim_stop(FAR struct esp32_tim_dev_s *dev)
Review comment:
Ditto, keep is as int
----------------------------------------------------------------
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] saramonteiro commented on pull request #3048: xtensa/esp32: timer driver refactor
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on pull request #3048:
URL: https://github.com/apache/incubator-nuttx/pull/3048#issuecomment-797697382
> I think it is not a good idea to move these functions to void, you could implement some logic testings in the future that you are don't realize now.
I don't think it's a good idea to keep unnecessary code.
When it comes the time for developing any kind of testing, we prepare the code for this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org