sys/xtimer: tick conversion #9026

Josar · 2018-04-25T23:05:08Z

This PR adds a debug print to the xtimer conversion, this enables a better debuging of time conversion related problems.

The messages are as follows.

32bit 125000 < 1MHz : 239307 ticks = 1914456 us
32bit 125000 < 1MHz : 10000 us = 1250 ticks

jnohlgard

In general, I have no issue with adding more debug outputs as long as they are controlled by a compile time switch. See my comments below regarding the implementation though

jnohlgard · 2018-04-26T05:06:50Z

sys/include/xtimer/tick_conversion.h

+buf_1[size] = '\0';                                                     \
+size = fmt_u32_dec(buf_2, VALUE2);                                      \
+buf_2[size] = '\0';                                                     \
+printf("32bit %" PRIu32 " "EQUAL" 1MHz : %s "#VALUE1" = %s "BASE2"\n",  \


pick either fmt or stdio as output, there is no reason to use PRIu32 for one argument and preformatting with fmt_u32_dec for the other argument.

there are print_xxx functions in fmt.h which you can use as a convenience instead of managing temporary buffers manually.

But there is no real benefit in print_xxx as the code then just splits the print in multiple lines.

But there is no real benefit in print_xxx as the code then just splits the print in multiple lines.

Compare the stack usage and you will see the benefit ;-).

jnohlgard · 2018-04-26T05:06:51Z

sys/include/xtimer/tick_conversion.h

+#include <stdio.h>
+#include <string.h>
+#include "fmt.h"
+#define DEBUG_PRINT_TICKS_32( CLOCK_HZ, EQUAL, VALUE1, VALUE2, BASE2) ({\


change this to a function, it is too long to be readable as a macro.

jnohlgard · 2018-04-26T05:09:13Z

sys/include/xtimer/tick_conversion.h

+})
+#else
+#define DEBUG_PRINT_TICKS_32(CLOCK_HZ, EQUAL, VALUE1, VALUE2, BASE2) {}
+#define DEBUG_PRINT_TICKS_64(CLOCK_HZ, EQUAL, VALUE1, VALUE2, BASE2) {}


We have reached a consensus regarding ENABLE_DEBUG to use normal if statements and rely on the optimizer to discard the dead code instead of discarding the code in the preprocessor. The benefit is that the debug prints are checked for syntax errors and mistakes by the CI system even without enabling debug.
The same approach should be applicable here too.

jnohlgard · 2018-04-26T05:11:55Z

sys/include/xtimer/tick_conversion.h

@@ -37,97 +85,116 @@ extern "C" {
 #if (XTIMER_HZ != (1000000ul << XTIMER_SHIFT))
 #error XTIMER_HZ != (1000000ul << XTIMER_SHIFT)
 #endif
-/* XTIMER_HZ is a power-of-two multiple of 1 MHz */
+/* XTIMER_HZ is a power-of-two multiple of 1 MHz and is bigger than 1000MHz */


1000MHz -> 1 MHz?

jnohlgard · 2018-04-26T05:12:12Z

sys/include/xtimer/tick_conversion.h

    return (ticks >> XTIMER_SHIFT); /* divide by power of two */
 }

 #else /* !(XTIMER_HZ > 1000000ul) */
 #if ((XTIMER_HZ << XTIMER_SHIFT) != 1000000ul)
 #error (XTIMER_HZ << XTIMER_SHIFT) != 1000000ul
 #endif
-/* 1 MHz is a power-of-two multiple of XTIMER_HZ */
+/* 1 MHz is a power-of-two multiple of XTIMER_HZ and is smaller than 1000MHz */


1000MHz -> 1 MHz

Josar · 2018-04-26T08:06:55Z

@gebart not sure if i understand you correctly. Could you have a look at the changes!

jnohlgard

I can't do a proper review now, I'm just looking at it on my phone, but you seem to have the right idea on how to use the fmt print functions.

jnohlgard · 2018-04-26T09:19:15Z

sys/include/xtimer/tick_conversion.h

+
+#if ENABLE_DEBUG_PRINT_TICK_CONVERSION
+#ifndef MODULE_FMT
+#error "Module fmt is necessary to ENABLE_DEBUG_TICKS set 'USEMODULE += fmt' \


Don't wrap this line, github syntax highlighting gets confused it seems

Also, there's no need to quote the argument to #error , everything on the line is printed verbatim to stderr by the preprocessor when it encounters the error directive

Qouting is needed to wrap, if there is the no wrap, no need to qoute.

But i just try to understand this, we go ahead with code convention violations as github has a buggy code highlighting?

Or just shorten the message, add more information in a code comment near this line. The error message will have the file name and line number so it's easy to find

textmate/c.tmbundle#56

Josar · 2018-04-26T10:53:29Z

sys/include/xtimer/tick_conversion.h

 #ifndef XTIMER_TICK_CONVERSION_H
 #define XTIMER_TICK_CONVERSION_H

 #ifndef XTIMER_H
 #error "Do not include this file directly! Use xtimer.h instead"
 #endif

+#include <stdio.h>
+#include <string.h>
+#include "fmt.h"
 #include "div.h"

 #ifdef __cplusplus
 extern "C" {
 #endif


Here is also a bad github code highlighting.

smlng

general question, do we really need separate debug function for 32 and 64 bit instead of just using the latter only?

smlng · 2018-05-04T21:06:41Z

sys/include/xtimer/tick_conversion.h

 #ifndef XTIMER_TICK_CONVERSION_H
 #define XTIMER_TICK_CONVERSION_H

 #ifndef XTIMER_H
 #error "Do not include this file directly! Use xtimer.h instead"
 #endif

+#include <stdio.h>
+#include <string.h>
+#include "fmt.h"


nit picky: please add empty line between system and riot includes

smlng · 2018-05-04T21:07:07Z

sys/include/xtimer/tick_conversion.h

+ *
+ * USEMODULE += fmt
+ * CFLAGS += -DENABLE_DEBUG_PRINT_TICK_CONVERSION=1
+ * */


remove dup *

smlng · 2018-05-04T21:08:03Z

sys/include/xtimer/tick_conversion.h

+#endif
+
+inline void debug_print_ticks32(uint32_t clock_hz, char *sign, uint32_t from,
+                         char *base_from, uint32_t to,char *base_to) {


indention looks of, and please apply RIOT coding style see https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions

smlng · 2018-05-04T21:08:18Z

sys/include/xtimer/tick_conversion.h

+}
+
+inline void debug_print_ticks64(uint32_t clock_hz, char *sign, uint64_t from,
+                         char *base_from, uint64_t to,char *base_to) {


dito, see above

though I see below, that there are some diversion from the coding style here anyway, i.e. parentheses not on a new line.

i added a seperate uncrustify commit, which also adresses this.

smlng · 2018-05-04T21:13:08Z

sys/include/xtimer/tick_conversion.h

-static inline uint32_t _xtimer_ticks_from_usec(uint32_t usec) {
-    return usec; /* no-op */
+static inline uint32_t _xtimer_usec_from_ticks(uint32_t ticks) {
+    DEBUG_PRINT_TICKS_32(XTIMER_HZ, "=", ticks, ticks, "us");


this looks like a dup here?

Josar · 2018-05-08T12:56:26Z

@smlng changed it to use one print function as the overhead from fmt64 does not matter if this debug prints are used.

Josar · 2018-05-08T13:21:22Z

@gebart @smlng squash and rebase?

Josar · 2018-05-14T20:49:15Z

@gebart @smlng still waiting for requests or ACK.

smlng

still need to test this, too

smlng · 2018-05-15T09:04:07Z

sys/include/xtimer/tick_conversion.h

 }

 #else
 /* No matching implementation found, try to give meaningful error messages */
 #if ((XTIMER_HZ % 15625) == 0)
-#error Unsupported hardware timer frequency (XTIMER_HZ), missing XTIMER_SHIFT in board.h? See xtimer.h documentation for more info
+#error Unsupported hardware timer frequency (XTIMER_HZ).


why did you changed this error message (here and below). Now the important info is only in the comments instead of showing it to the developer while compiling.

see #9026 (comment)

Josar · 2018-05-23T20:36:16Z

@gebart @smlng can you find some time for this?

Josar · 2018-05-29T11:39:39Z

@gebart @smlng any further objectives?

smlng · 2018-05-29T12:32:15Z

I get the following error when building RIOT native on macOS:

Undefined symbols for architecture i386:
  "_debug_print_ticks", referenced from:
      __xtimer_ticks_from_usec in application_tests_xtimer_drift.a(main.o)
      __xtimer_usec_from_ticks in application_tests_xtimer_drift.a(main.o)
      __xtimer_usec_from_ticks64 in xtimer.a(xtimer.o)
ld: symbol(s) not found for architecture i386
collect2: error: ld returned 1 exit status
make: *** [link] Error 1

smlng · 2018-05-29T12:32:32Z

btw. tried to use it with tests/xtimer_drift

…g messages

Josar · 2018-05-29T13:48:16Z

force inlining solved that for me.

smlng · 2018-05-29T14:35:40Z

works now

Josar · 2018-06-04T13:48:51Z

@gebart any further remarks?

Josar · 2018-07-03T15:56:38Z

@smlng @gebart squash and rebase?

smlng · 2018-07-04T06:25:23Z

I've nothing to add, @gebart anything?

Josar · 2018-09-11T14:00:15Z

any news here?

PeterKietzmann · 2018-09-25T13:29:43Z

@gebart?!

stale · 2019-08-10T05:08:19Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

xtimer tick_conversion.h: uncrustify

9f4eea5

Josar force-pushed the pr/tick_conversion branch from 84cda38 to 7635781 Compare April 26, 2018 00:25

xtimer tick_conversion.h: add debug messages

b8dc65f

Josar force-pushed the pr/tick_conversion branch from 7635781 to b8dc65f Compare April 26, 2018 00:29

jnohlgard requested changes Apr 26, 2018

View reviewed changes

fixup! xtimer tick_conversion.h: add debug messages

a54e103

fixup! fixup! xtimer tick_conversion.h: add debug messages

1bacc15

jnohlgard reviewed Apr 26, 2018

View reviewed changes

Josar added 2 commits April 26, 2018 12:50

fixup! xtimer tick_conversion.h: add debug messages

0989d64

fixup! xtimer tick_conversion.h: uncrustify

2c0a48b

Josar force-pushed the pr/tick_conversion branch from 66e5634 to 2c0a48b Compare April 26, 2018 10:52

Josar commented Apr 26, 2018

View reviewed changes

smlng suggested changes May 4, 2018

View reviewed changes

Josar added 3 commits May 7, 2018 12:36

fixup! fixup! xtimer tick_conversion.h: add debug messages

deea93b

fixup! fixup! xtimer tick_conversion.h: uncrustify

9d0cb97

fixup! fixup! fixup! xtimer tick_conversion.h: add debug messages

9ca7ed6

fixup! fixup! fixup! fixup! xtimer tick_conversion.h: add debug messages

66e5975

smlng suggested changes May 15, 2018

View reviewed changes

aabadie changed the title ~~Pr/tick conversion~~ sys/xtimer: tick conversion May 15, 2018

aabadie added the Area: timers Area: timer subsystems label May 15, 2018

smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 24, 2018

fixup! fixup! fixup! fixup! fixup! xtimer tick_conversion.h: add debu…

ae8c7ab

…g messages

Josar force-pushed the pr/tick_conversion branch from 5e26b0e to ae8c7ab Compare May 29, 2018 13:47

smlng approved these changes May 29, 2018

View reviewed changes

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

stale bot closed this Sep 10, 2019

sys/xtimer: tick conversion #9026

sys/xtimer: tick conversion #9026

Conversation

Josar commented Apr 25, 2018

jnohlgard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josar commented Apr 26, 2018

jnohlgard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smlng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josar commented May 8, 2018

Josar commented May 8, 2018

Josar commented May 14, 2018

smlng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josar commented May 23, 2018

Josar commented May 29, 2018

smlng commented May 29, 2018

smlng commented May 29, 2018

Josar commented May 29, 2018

smlng commented May 29, 2018

Josar commented Jun 4, 2018

Josar commented Jul 3, 2018

smlng commented Jul 4, 2018

Josar commented Sep 11, 2018

PeterKietzmann commented Sep 25, 2018

stale bot commented Aug 10, 2019