fix the representation of at_the_end_of_time #145

2021-02-17T14:16:26-06:00

kilograham commented

2021-02-17 14:16:26 -06:00

(Migrated from github.com)

at_the_end_of_time was erroneously ULONG_MAX which is only 32 bits; changed to INT64_MAX which is sufficiently far in the future, but allows for signed 64 bit arithmetic.

Wren6991 (Migrated from github.com) reviewed

2021-02-17 14:16:26 -06:00

lurch (Migrated from github.com) reviewed

2021-02-18 07:30:49 -06:00

src/common/pico_base/include/pico/types.h

					
				@ -36,4 +47,4 @@

				/*! fn update_us_since_boot

				 * \brief update an absolute_time_t value to represent a given number of microseconds since boot

				 * \param t the absolute time value to update

Are the param and return descriptions the wrong way around? 🤷

Are the param and return descriptions the wrong way around? :shrug:

kilograham commented

2021-02-19 13:46:57 -06:00

(Migrated from github.com)

fixes #132

lurch commented

2021-02-19 14:41:17 -06:00

(Migrated from github.com)

Looking at types.h, I wonder if it's also worth doing a small bit of refactoring in order to reduce the duplicated doxygen comments? (also, seems assert.h was missing)

diff --git a/src/common/pico_base/include/pico/types.h b/src/common/pico_base/include/pico/types.h
index 4b4f557..89ec210 100644
--- a/src/common/pico_base/include/pico/types.h
+++ b/src/common/pico_base/include/pico/types.h
@@ -10,10 +10,10 @@
 #include <stdint.h>
 #include <stdbool.h>
 #include <stddef.h>
+#include <assert.h>
 
 typedef unsigned int uint;
 
-#ifdef NDEBUG
 /*! \typedef absolute_time_t
     \brief An opaque 64 bit timestamp in microseconds
 
@@ -23,32 +23,13 @@ typedef unsigned int uint;
     \see to_us_since_boot
     \see update_us_since_boot
 */
+#ifdef NDEBUG
 typedef uint64_t absolute_time_t;
-
-/*! fn to_us_since_boot
- * \brief convert an absolute_time_t into a number of microseconds since boot.
- * \param t the absolute time to convert
- * \return a number of microseconds since boot, equivalent to t
- */
-static inline uint64_t to_us_since_boot(absolute_time_t t) {
-    return t;
-}
-
-/*! fn update_us_since_boot
- * \brief update an absolute_time_t value to represent a given number of microseconds since boot
- * \param t the absolute time value to update
- * \param us_since_boot the number of microseconds since boot to represent. Note this should be representable
- *                      as a signed 64 bit integer
- */
-static inline void update_us_since_boot(absolute_time_t *t, uint64_t us_since_boot) {
-    *t = us_since_boot;
-}
-
-#define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = value
 #else
 typedef struct {
     uint64_t _private_us_since_boot;
 } absolute_time_t;
+#endif
 
 /*! fn to_us_since_boot
  * \brief convert an absolute_time_t into a number of microseconds since boot.
@@ -56,7 +37,11 @@ typedef struct {
  * \return a number of microseconds since boot, equivalent to t
  */
 static inline uint64_t to_us_since_boot(absolute_time_t t) {
+#ifdef NDEBUG
+    return t;
+#else
     return t._private_us_since_boot;
+#endif
 }
 
 /*! fn update_us_since_boot
@@ -67,8 +52,16 @@ static inline uint64_t to_us_since_boot(absolute_time_t t) {
  */
 static inline void update_us_since_boot(absolute_time_t *t, uint64_t us_since_boot) {
     assert(us_since_boot <= INT64_MAX);
+#ifdef NDEBUG
+    *t = us_since_boot;
+#else
     t->_private_us_since_boot = us_since_boot;
+#endif
 }
+
+#ifdef NDEBUG
+#define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = value
+#else
 #define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = {value}
 #endif

Looking at `types.h`, I wonder if it's also worth doing a small bit of refactoring in order to reduce the duplicated doxygen comments? (also, seems `assert.h` was missing) ```patch diff --git a/src/common/pico_base/include/pico/types.h b/src/common/pico_base/include/pico/types.h index 4b4f557..89ec210 100644 --- a/src/common/pico_base/include/pico/types.h +++ b/src/common/pico_base/include/pico/types.h @@ -10,10 +10,10 @@ #include <stdint.h> #include <stdbool.h> #include <stddef.h> +#include <assert.h> typedef unsigned int uint; -#ifdef NDEBUG /*! \typedef absolute_time_t \brief An opaque 64 bit timestamp in microseconds @@ -23,32 +23,13 @@ typedef unsigned int uint; \see to_us_since_boot \see update_us_since_boot */ +#ifdef NDEBUG typedef uint64_t absolute_time_t; - -/*! fn to_us_since_boot - * \brief convert an absolute_time_t into a number of microseconds since boot. - * \param t the absolute time to convert - * \return a number of microseconds since boot, equivalent to t - */ -static inline uint64_t to_us_since_boot(absolute_time_t t) { - return t; -} - -/*! fn update_us_since_boot - * \brief update an absolute_time_t value to represent a given number of microseconds since boot - * \param t the absolute time value to update - * \param us_since_boot the number of microseconds since boot to represent. Note this should be representable - * as a signed 64 bit integer - */ -static inline void update_us_since_boot(absolute_time_t *t, uint64_t us_since_boot) { - *t = us_since_boot; -} - -#define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = value #else typedef struct { uint64_t _private_us_since_boot; } absolute_time_t; +#endif /*! fn to_us_since_boot * \brief convert an absolute_time_t into a number of microseconds since boot. @@ -56,7 +37,11 @@ typedef struct { * \return a number of microseconds since boot, equivalent to t */ static inline uint64_t to_us_since_boot(absolute_time_t t) { +#ifdef NDEBUG + return t; +#else return t._private_us_since_boot; +#endif } /*! fn update_us_since_boot @@ -67,8 +52,16 @@ static inline uint64_t to_us_since_boot(absolute_time_t t) { */ static inline void update_us_since_boot(absolute_time_t *t, uint64_t us_since_boot) { assert(us_since_boot <= INT64_MAX); +#ifdef NDEBUG + *t = us_since_boot; +#else t->_private_us_since_boot = us_since_boot; +#endif } + +#ifdef NDEBUG +#define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = value +#else #define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = {value} #endif ```

kilograham commented

2021-02-19 14:54:39 -06:00

(Migrated from github.com)

if you can figure out how to make doxygen work off one set of comments then great :-) - i'm not convinced it does the right things for these anyway - not much to refactor as they are different definitions of the same thing

kilograham commented

2021-02-19 14:55:05 -06:00

(Migrated from github.com)

oh let me look sorry, i didn't realize you had a patch

kilograham commented

2021-02-19 14:55:39 -06:00

(Migrated from github.com)

good idea

philipstarkey commented

2021-02-21 01:39:43 -06:00

(Migrated from github.com)

While I'm not the original reporter of #132, I've had a similar problem with the pico crashing after 1.19 hours. I've tested this PR today and can confirm it works (well other than the missing assert.h header that @lurch pointed out) and fixes the problem I was having with getchar()

While I'm not the original reporter of #132, I've had a similar problem with the pico crashing after 1.19 hours. I've tested this PR today and can confirm it works (well other than the missing `assert.h` header that @lurch pointed out) and fixes the problem I was having with `getchar()`

👍 1

liamfraser (Migrated from github.com) reviewed

2021-02-24 04:50:15 -06:00

liamfraser (Migrated from github.com) approved these changes

2021-02-25 03:11:29 -06:00

Sign in to join this conversation.

No reviewers

Wren6991

liamfraser