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/05/15 21:40:55 UTC
[GitHub] [incubator-nuttx] antmerlino opened a new pull request #3729: stm32 timers: Prefer GTIM macros and make this use more uniform.
antmerlino opened a new pull request #3729:
URL: https://github.com/apache/incubator-nuttx/pull/3729
## Summary
stm32 timers: Prefer GTIM macros and make this use more uniform. Also, make a few register operations more readable.
## Impact
This change should have no code effect. It just makes things more readable and consistent.
## Testing
CI
--
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 #3729: stm32 timers: Prefer GTIM macros and make this use more uniform.
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #3729:
URL: https://github.com/apache/incubator-nuttx/pull/3729#discussion_r633058432
##########
File path: arch/arm/src/stm32/stm32_tickless.c
##########
@@ -267,7 +268,7 @@ static int stm32_tickless_setchannel(uint8_t channel)
/* Set polarity */
- ccer_val |= ATIM_CCER_CC1P << (channel << 2);
+ ccer_val |= ATIM_CCER_CC1P << GTIM_CCER_CCXBASE(channel);
Review comment:
I get that the macro is the same, but why not use `ATIM_CCER_CCXBASE` here, seems wrong to use that in the context of `ATIM`?
--
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] antmerlino commented on a change in pull request #3729: stm32 timers: Prefer GTIM macros and make this use more uniform.
Posted by GitBox <gi...@apache.org>.
antmerlino commented on a change in pull request #3729:
URL: https://github.com/apache/incubator-nuttx/pull/3729#discussion_r633103142
##########
File path: arch/arm/src/stm32/stm32_tickless.c
##########
@@ -267,7 +268,7 @@ static int stm32_tickless_setchannel(uint8_t channel)
/* Set polarity */
- ccer_val |= ATIM_CCER_CC1P << (channel << 2);
+ ccer_val |= ATIM_CCER_CC1P << GTIM_CCER_CCXBASE(channel);
Review comment:
@btashton I just pushed an update to replace even more ATIM_/BTIM_ macros with GTIM_ macros for consistency. There are now no uses of BTIM_ macros in the codebase - which makes sense since all our drivers focus on General timer implementation with some extended features for Advanced timers. I've left the use of ATIM_ anywhere we were actually meaning to use advanced features.
I'm sure I missed some things.
--
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 merged pull request #3729: stm32 timers: Prefer GTIM macros and make this use more uniform.
Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #3729:
URL: https://github.com/apache/incubator-nuttx/pull/3729
--
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] antmerlino commented on pull request #3729: stm32 timers: Prefer GTIM macros and make this use more uniform.
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #3729:
URL: https://github.com/apache/incubator-nuttx/pull/3729#issuecomment-844290009
> @antmerlino did you verity cross check all the #defines for equality (the Bin file is the same)?
> Was there any ATIM not conditioned on TIM{1,8} ?
@davids5 I did not literally compare binaries, but I did my best to check values. I changed a lot of places that were using mix and match between ATIM and GTIM with what seemed like no good reason. Any where advanced timer functionality was either explicitly guarded for or was assumed by the driver, I left ATIM.
The place I tried to get to was:
1. No use of BTIM, since none of our drivers were designed that way
2. ATIM is used anywhere our drivers are actually intending to use advanced functionality.
3. GTIM - everything else.
--
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 #3729: stm32 timers: Prefer GTIM macros and make this use more uniform.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3729:
URL: https://github.com/apache/incubator-nuttx/pull/3729#issuecomment-842320318
@antmerlino did you verity cross check all the #defines for equality (the Bin file is the same)?
Was there any ATIM not conditioned on TIM{1,8} ?
--
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 #3729: stm32 timers: Prefer GTIM macros and make this use more uniform.
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #3729:
URL: https://github.com/apache/incubator-nuttx/pull/3729#discussion_r633058432
##########
File path: arch/arm/src/stm32/stm32_tickless.c
##########
@@ -267,7 +268,7 @@ static int stm32_tickless_setchannel(uint8_t channel)
/* Set polarity */
- ccer_val |= ATIM_CCER_CC1P << (channel << 2);
+ ccer_val |= ATIM_CCER_CC1P << GTIM_CCER_CCXBASE(channel);
Review comment:
I get that the macro is the same, but why not use `ATIM_CCER_CCXBASE` here?
##########
File path: arch/arm/src/stm32/stm32_tickless.c
##########
@@ -267,7 +268,7 @@ static int stm32_tickless_setchannel(uint8_t channel)
/* Set polarity */
- ccer_val |= ATIM_CCER_CC1P << (channel << 2);
+ ccer_val |= ATIM_CCER_CC1P << GTIM_CCER_CCXBASE(channel);
Review comment:
I get that the macro is the same, but why not use `ATIM_CCER_CCXBASE` here, seems wrong to use that in the context of `ATIM`?
--
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 #3729: stm32 timers: Prefer GTIM macros and make this use more uniform.
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #3729:
URL: https://github.com/apache/incubator-nuttx/pull/3729#discussion_r633058432
##########
File path: arch/arm/src/stm32/stm32_tickless.c
##########
@@ -267,7 +268,7 @@ static int stm32_tickless_setchannel(uint8_t channel)
/* Set polarity */
- ccer_val |= ATIM_CCER_CC1P << (channel << 2);
+ ccer_val |= ATIM_CCER_CC1P << GTIM_CCER_CCXBASE(channel);
Review comment:
I get that the macro is the same, but why not use `ATIM_CCER_CCXBASE` 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] antmerlino commented on a change in pull request #3729: stm32 timers: Prefer GTIM macros and make this use more uniform.
Posted by GitBox <gi...@apache.org>.
antmerlino commented on a change in pull request #3729:
URL: https://github.com/apache/incubator-nuttx/pull/3729#discussion_r633103142
##########
File path: arch/arm/src/stm32/stm32_tickless.c
##########
@@ -267,7 +268,7 @@ static int stm32_tickless_setchannel(uint8_t channel)
/* Set polarity */
- ccer_val |= ATIM_CCER_CC1P << (channel << 2);
+ ccer_val |= ATIM_CCER_CC1P << GTIM_CCER_CCXBASE(channel);
Review comment:
@btashton I just pushed an update to replace even more ATIM_/BTIM_ macros with GTIM_ macros for consistency. There are now no uses of BTIM_ macros in the codebase - which makes sense since all our drivers focus on General timer implementation with some extended features for Advanced timers. I've left the use of ATIM_ anywhere we were actually meaning to use advanced features.
I'm sure I missed some things.
--
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 #3729: stm32 timers: Prefer GTIM macros and make this use more uniform.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3729:
URL: https://github.com/apache/incubator-nuttx/pull/3729#issuecomment-842320318
@antmerlino did you verity cross check all the #defines for equality (the Bin file is the same)?
Was there any ATIM not conditioned on TIM{1,8} ?
--
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