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/01/30 20:47:41 UTC

[GitHub] [incubator-nuttx] v01d opened a new pull request #2782: nRF52 fixes

v01d opened a new pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782


   ## Summary
   
   - Improve SPI use of PPI peripheral (for workaround) by using PPI API, which internally checks we're not using an already used channel. Also, this works better when on PPI API header the number of available PPI channels is reduced because they are reserved (such as when using Nordic SDC).
   - When !SPI_EXCHANGE, the RX transfer had the parameter swapped which resulted in no reception of data
   - Tickless RTC code was dealing with the edge case (minimum settable timeout) with code which was not fast enough to properly work. This now uses more direct interaction with registers. This has solved a never-ending usleep() I had (with a low timeout period, under intensive interrupts scenario)
   
   ## Impact
   
   Fixes some bugs, improves code to avoid future problems
   
   ## Testing
   
   nrf52832-mdk with custom config (derived from unmerged nrf52832-mdk:sdc).


----------------------------------------------------------------
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 merged pull request #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
btashton merged pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782


   


----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r567341383



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       The point is that I'm looking for maximum speed, as close as possible to `*cc = *counter + 2`. I'm not sure it is worth the risk on adding extra computation if it will end up locking up a sleep() under stress execution.




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r567473659



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       My issue is that the rtc driver is abstracted for different base addresses for the RTC, but then in this driver we hard code it to a single one.   You could move the struct definition into the header (`nuttx/arch/arm/src/nrf52/nrf52_rtc.h`) if that is needed to share between the two driver sources.




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#issuecomment-772035579






----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r567487476



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       But this is not true:
   `g_tickless_dev.rtc = nrf52_rtc_init(CONFIG_NRF52_SYSTIMER_RTC_INSTANCE);`
   If `CONFIG_NRF52_SYSTIMER_RTC_INSTANCE` is not 0 then your code will be wrong.
   
   I don't see why it would be wrong to leak the private struct via the header.  You are already changing the device out from under what is in `nrf52_rtc.c` so its not really providing isolation.   If you really don't want to do that, then I would just provide another public interface to expose the base address.
   
   ```
   #define RTC_GETCOUNTER(base) getreg32(base + NRF52_RTC_COUNTER_OFFSET)
   #define RTC_SETCC(base, cc)         putreg32(cc, base + NRF52_RTC_CC_OFFSET(0))
   
   
   static void rtc_prepare_alarm(void)
   {
     ...
     rtc_base = NRF52_RTC_GETBASE(g_tickless_dev.rtc);
     counter = RTC_GETCOUNTER(rtc_base);
     ...
     counter = RTC_GETCOUNTER(rtc_base);
     ...
     RTC_SETCC(rtc_base, target_counter);
   }
   ```




----------------------------------------------------------------
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 merged pull request #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
btashton merged pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782


   


----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r568097222



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       > Why do you still have the inlines functions, they can be dropped and use the macro instead so you know it will be inlined.
   
   I missed removing them. 
   
   > Also you are still hard coding the base. We need to fetch the base from the supplied rtc struct. I had recommended adding an API to the nrf_rtc.c to expose the base address which you can then pass to the macros.
   
   Missed that as well. I will need to add a callback to the ops struct to add that as a function.
   
   




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#issuecomment-772060401


   Sorry I was waiting for the builds to finish and forgot to check back. 


----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r567484927



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       The hardcoding is a result of this code using a Kconfig macro to define which RTC instance to use, so I'm not sure there's a real benefit of reading the base address at runtime. Also, not sure if exposing the private struct is really a good practice. 
   
   Maybe the memorymap file could be extended to have something like NRF52_RTC_BASE(n) (and same for other peripherals). This way we do have the mapping of n to RTCn only in one place. What do you think of that solution?




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#issuecomment-772060534


   Sure, thanks


----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r567501991



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       Seems like nrf52_rtc.h would be a better place as that's where the function for getting the base would live.




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r567338218



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       Can we at least change this to still take the rtc pointer?  I know there is only one, but I don't think that should impact performance of this function (maybe paying for one add operation)  but keeps it more consistent with the other drivers (same with the `rtc_setcc`).
   ```
   return getreg32(((struct nrf52_rtc_priv_s *)dev)->base + NRF52_RTC_COUNTER_OFFSET);
   ```




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r567342720



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       If you are that worried, then you should not have this be a inline function and instead make these a macros or just put them inline.  This is just used as a hint and may be ignored by the compiler.
   
   I would also make sure that you have a comment saying that the code block you are concerned about must be run in a critical section.  I know that it already is, but it would be good to make that very clear.




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r567342720



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       If you are that worried, then you should not have this be a inline function and instead make these a macros or just put them inline.  This is just used as a hint and may be ignored by the compiler.  You could then actually compute the address off the base `(struct nrf52_rtc_priv_s *)dev)->base + NRF52_RTC_COUNTER_OFFSET` ahead of the computation loop which would still give you the performance you are looking for without loosing the abstraction.
   
   I would also make sure that you have a comment saying that the code block you are concerned about must be run in a critical section.  I know that it already is, but it would be good to make that very clear.




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r567501422



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       Yes, I think that's better. Should I add this to nrf52_rtc.h or leave it as part of the tickless file?




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r567450356



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       The `nrf52_rtc_priv_s` is actually a private type inside `nrf52_rtc.c`.
   
   I could maybe precompute offsets inside the function like:
   
   ```
     const uint32_t counter_offset = NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET;
     const uint32_t cc_offset = NRF52_RTC_BASE + NRF52_RTC_CC_OFFSET(0);
   ```
   
   and then simply to getreg32/putreg32 (could do a macro function to wrap around that but it would only be a change in name). 




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r568103461



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       Just pushed, please check.




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r567994926



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       Ok, I've added these macros to nrf52_rtc.h. I also reworked the alarm setup code since it was prone to still failing on the edge case. I'm now using a different strategy which is more robust and confirmed it works without lockups.




----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#issuecomment-772060401


   Sorry I was waiting for the builds to finish and forgot to check back. 


----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#issuecomment-772035579


   anyone would like to merge?


----------------------------------------------------------------
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 #2782: nRF52 fixes

Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2782:
URL: https://github.com/apache/incubator-nuttx/pull/2782#discussion_r568090321



##########
File path: arch/arm/src/nrf52/nrf52_tickless_rtc.c
##########
@@ -111,7 +111,17 @@ struct nrf52_tickless_dev_s g_tickless_dev;
  * Private Functions
  ****************************************************************************/
 
-static void rtc_counter_to_ts(uint32_t counter, struct timespec *now)
+static inline uint32_t rtc_getcounter(void)
+{
+  return getreg32(NRF52_RTC_BASE + NRF52_RTC_COUNTER_OFFSET);

Review comment:
       Why do you still have the inlines functions, they can be dropped and use the macro instead so you know it will be inlined.
   
   Also you are still hard coding the base. We need to fetch the base from the supplied rtc struct. I had recommended adding an API to the nrf_rtc.c to expose the base address which you can then pass to the macros. 




----------------------------------------------------------------
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