Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MRAA timer functions #950

Merged
merged 2 commits into from
Mar 2, 2024
Merged

Update MRAA timer functions #950

merged 2 commits into from
Mar 2, 2024

Conversation

TMRh20
Copy link
Member

@TMRh20 TMRh20 commented Mar 2, 2024

  • Found MRAA timer functions not working properly
  • Copy timing functions from SPIDEV

- Found MRAA timer functions not working properly
- Copy timing functions from SPIDEV
github-actions[bot]

This comment was marked as outdated.

@github-actions github-actions bot dismissed their stale review March 2, 2024 11:22

outdated suggestion

Copy link
Member

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified working on my RPi4.

Its surprising that this is the solution to this. I noticed it last year, but wasn't sure what the problem actually was.

Should we also revert the timing code for SPIDEV driver? I'm curious if the char-dev API would still need the delayMicrosecond(1) in ce().

@TMRh20
Copy link
Member Author

TMRh20 commented Mar 2, 2024

Should we also revert the timing code for SPIDEV driver?

Not sure what you mean. This is a straight copy of the SPIDEV timing code.

@TMRh20 TMRh20 merged commit 6d0f1b1 into master Mar 2, 2024
2 checks passed
@TMRh20 TMRh20 deleted the MRAA-Timer-Changes branch March 2, 2024 12:26
@2bndy5
Copy link
Member

2bndy5 commented Mar 2, 2024

Oops. I thought SPIDEV was using only chrono lib.

@2bndy5
Copy link
Member

2bndy5 commented Mar 2, 2024

I just tried only chrono approach, and the scanner example doesn't need the the delayMicroSeonds(1) in ce().

patch that I tested with char-dev-irq branch

diff --git a/RF24.cpp b/RF24.cpp
index 13db347..fd41e17 100644
--- a/RF24.cpp
+++ b/RF24.cpp
@@ -108,7 +108,7 @@ void RF24::ce(bool level)
 #endif
         digitalWrite(ce_pin, level);
 #ifdef RF24_LINUX
-        delayMicroseconds(1); // SPIDEV driver's GPIO needs a delay here (for some reason)
+        // delayMicroseconds(1); // SPIDEV driver's GPIO needs a delay here (for some reason)
 #endif
 #ifndef RF24_LINUX
     }
diff --git a/utility/SPIDEV/compatibility.cpp b/utility/SPIDEV/compatibility.cpp
index 6b96f65..8593fbd 100644
--- a/utility/SPIDEV/compatibility.cpp
+++ b/utility/SPIDEV/compatibility.cpp
@@ -1,10 +1,8 @@
 #include "compatibility.h"
-
-long long mtime, seconds, useconds;
-//static struct timeval start, end;
-//struct timespec start, end;
-#include <time.h>
 #include <chrono>
+//static struct timeval start, end;
+//static long mtime, seconds, useconds;
+
 /**********************************************************************/
 /**
  * This function is added in order to simulate arduino delay() function
@@ -12,35 +10,33 @@ long long mtime, seconds, useconds;
  */
 void __msleep(int milisec)
 {
-    struct timespec req; // = {0};
-    req.tv_sec = (time_t)milisec / 1000;
-    req.tv_nsec = (milisec % 1000) * 1000000L;
-    //nanosleep(&req, (struct timespec *)NULL);
-    clock_nanosleep(CLOCK_REALTIME, 0, &req, NULL);
+    struct timespec req = {0};
+    req.tv_sec = 0;
+    req.tv_nsec = milisec * 1000000L;
+    nanosleep(&req, (struct timespec*)NULL);
+    //usleep(milisec*1000);
 }
 
-void __usleep(int microsec)
+void __usleep(int milisec)
 {
-    struct timespec req; // = {0};
-    req.tv_sec = (time_t)microsec / 1000000;
-    req.tv_nsec = (microsec / 1000000) * 1000;
-    //nanosleep(&req, (struct timespec *)NULL);
-    clock_nanosleep(CLOCK_REALTIME, 0, &req, NULL);
+    struct timespec req = {0};
+    req.tv_sec = 0;
+    req.tv_nsec = milisec * 1000L;
+    nanosleep(&req, (struct timespec*)NULL);
+    //usleep(milisec);
 }
 
+auto start = std::chrono::steady_clock::now();
+
 /**
  * This function is added in order to simulate arduino millis() function
  */
-
-bool timerStarted = false;
-
 void __start_timer()
 {
+    //gettimeofday(&start, NULL);
 }
 
-auto start = std::chrono::steady_clock::now();
-
-uint32_t __millis()
+long __millis()
 {
     auto end = std::chrono::steady_clock::now();
 
diff --git a/utility/SPIDEV/compatibility.h b/utility/SPIDEV/compatibility.h
index 7015323..ffc6953 100644
--- a/utility/SPIDEV/compatibility.h
+++ b/utility/SPIDEV/compatibility.h
@@ -3,17 +3,15 @@
  * @author purinda
  *
  * Created on 24 June 2012, 3:08 PM
- * patch for safer monotonic clock & millis() correction for 64bit LDV 2018
  */
 
-#ifndef RF24_UTILITY_SPIDEV_COMPATIBLITY_H_
-#define RF24_UTILITY_SPIDEV_COMPATIBLITY_H_
+#ifndef RF24_UTILITY_MRAA_COMPATIBLITY_H_
+#define RF24_UTILITY_MRAA_COMPATIBLITY_H_
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
-#include <stdint.h> // for uintXX_t types
 #include <stddef.h>
 #include <time.h>
 #include <sys/time.h>
@@ -24,10 +22,10 @@ void __usleep(int milisec);
 
 void __start_timer();
 
-uint32_t __millis();
+long __millis();
 
 #ifdef __cplusplus
 }
 #endif
 
-#endif // RF24_UTILITY_SPIDEV_COMPATIBLITY_H_
+#endif // RF24_UTILITY_MRAA_COMPATIBLITY_H_

@TMRh20
Copy link
Member Author

TMRh20 commented Mar 2, 2024

Did you test it fully, because that code didn't work properly before? That's why it was changed, the delays don't work properly.

@2bndy5
Copy link
Member

2bndy5 commented Mar 2, 2024

I just ran the scanner example, so no I didn't fully vet it. I'm currently looking into the difference between clock_nanosleep() vs nanosleep(). Changing clock_nanosleep() to use CLOCK_MONOTONIC is not the same as using just nanosleep() for some reason.

@2bndy5
Copy link
Member

2bndy5 commented Mar 2, 2024

from hosted manpage:

Calling clock_nanosleep() with the value TIMER_ABSTIME not set in the flags argument and with a clock_id of CLOCK_REALTIME is equivalent to calling nanosleep() with the same rqtp and rmtp arguments.

@2bndy5
Copy link
Member

2bndy5 commented Mar 2, 2024

Holy crap! It was a math error. This works without extra delay in ce():

@@ -23,7 +23,7 @@ void __usleep(int microsec)
 {
     struct timespec req; // = {0};
     req.tv_sec = (time_t)microsec / 1000000;
-    req.tv_nsec = (microsec / 1000000) * 1000;
+    req.tv_nsec = (microsec % 1000000) * 1000;
     //nanosleep(&req, (struct timespec *)NULL);
     clock_nanosleep(CLOCK_REALTIME, 0, &req, NULL);
 }

microsec / 1000000 results in a decimal value when microsec < 1000000, but since microsec and 1000000 are not floats, this becomes 0. And obviously 0 * 1000 isn't a what we want.

But I might need to sleep on it and check my math when I'm more refreshed.

@TMRh20 TMRh20 restored the MRAA-Timer-Changes branch March 2, 2024 15:02
TMRh20 added a commit that referenced this pull request Mar 3, 2024
TMRh20 added a commit that referenced this pull request Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants