sys/xtimer: allow frequencies multiple of 921600Hz by kYc0o · Pull Request #6507 · RIOT-OS/RIOT

kYc0o · 2017-01-27T17:26:39Z

This PR will add xtimer compatibility for boards running at frequencies multiple of 576 (e.g. waspmote-pro running at 14.7456 MHz with timers prescaled to this frequency).

Partially (re) fixes #5597. An upcoming PR will fix the xtimer configuration on the waspmote.

kaspar030 · 2017-01-27T18:03:15Z

sys/include/div.h

+/**
+ * @brief Operand for 576/625
+ */
+#define MUL_576                576ul


pls drop these defines, they don't make sense.

kaspar030 · 2017-01-27T18:03:24Z

sys/include/div.h

+/**
+ * @brief Operand for 625/576
+ */
+#define MUL_625                625ul


It was to avoid the "ul" everywhere (8 and 16 bit requirement), but I can remove it.

for 625 you don't need UL. That only matters on the left-hand side of an expression (left-hand side defines type being used for calculation), or if the value is >16bit.

OK, got it.

Addressed. Thanks for the explanation.

kaspar030 · 2017-01-27T18:04:09Z

sys/include/div.h

+ */
+static inline uint64_t div_u64_by_625div576(uint64_t val)
+{
+    /*


did you measure this? or C&P?

Oops, copy-paste, I'll check if it does but I think it makes exactly the same as before. Moreover, it makes sense to take care of the overflow, I can just remove the comment but the code seems ok for me.

kaspar030 · 2017-01-27T18:04:16Z

sys/include/div.h

+ */
+static inline uint32_t div_u64_by_576div625(uint64_t val)
+{
+    /*


kYc0o · 2017-01-30T12:23:42Z

Ready?

lebrush

I think the #ifs and #else are not matching and there is some dead code. Besides it would be great if you add some empty lines to improve readability.

lebrush · 2017-01-31T08:34:34Z

sys/include/xtimer/tick_conversion.h

+ */
+#if (XTIMER_HZ % 576 == 0)
 #if (XTIMER_SHIFT != 0)
+#if (XTIMER_HZ % 576 != 0)


You will never run into this, since you first checked for the opposite, right?

#if (XTIMER_HZ % 576 == 0) #if (XTIMER_HZ % 576 != 0)

Yes, I can remove it but I need a way to throw the error if shifting is enabled.

lebrush · 2017-01-31T08:36:46Z

sys/include/xtimer/tick_conversion.h

+inline static uint64_t _xtimer_usec_from_ticks64(uint64_t ticks) {
+    return div_u64_by_576div625(ticks) >> XTIMER_SHIFT;
+}
+#else /* (XTIMER_HZ < 921600ul) */


/* (XTIMER_HZ <= 921600ul) */

lebrush · 2017-01-31T08:42:00Z

sys/include/xtimer/tick_conversion.h

+inline static uint64_t _xtimer_usec_from_ticks64(uint64_t ticks) {
+    return div_u64_by_576div625(ticks) << XTIMER_SHIFT;
+}
+#endif /* defined(XTIMER_SHIFT) && (XTIMER_SHIFT != 0) */


This is actually /* (XTIMER_HZ > 921600ul) */ or /* (XTIMER_HZ <= 921600ul) */ (depends on the notation you are using)

Rather the first, and also XTIMER_SHIFT. Will fix.

lebrush · 2017-01-31T08:43:34Z

sys/include/xtimer/tick_conversion.h

+    return div_u64_by_576div625(ticks) << XTIMER_SHIFT;
+}
+#endif /* defined(XTIMER_SHIFT) && (XTIMER_SHIFT != 0) */
+#elif (XTIMER_HZ == 921600ul)


Is here expected to be: (XTIMER_SHIFT == 0) && (XTIMER_HZ == 921600ul)? If that's the case then is correct

Yes, that's what is expected. I tried to reproduce a similar behaviour than for the 1MHz case.

lebrush · 2017-01-31T08:45:02Z

sys/include/xtimer/tick_conversion.h

+    return div_u64_by_576div625(ticks);
+}
+#endif /* (XTIMER_HZ == 921600ul) */
+#elif (XTIMER_SHIFT != 0) /* (XTIMER_HZ % 576 == 0) */


I think this branch belongs to the #if (XTIMER_HZ % 576 == 0)... then the code will enter the first time you test for #if (XTIMER_SHIFT != 0) and not here.

Yes I was trying to solve the issue. Actually I need a way on which we can detect, first, if we're using either 1MHz or 921600Hz clock based timer, then we need to know if the shifting is used, to finally choose the right functions.

The current code will check first if there's shifting activated, then if it's a 1MHz or 921600Hz based clock. The process is repeated to ensure that we can choose 1MHz clocks.

The code will enter here if the clock is not a multiple of 576, for which I need to check again if shifting is used.

kYc0o · 2017-01-31T15:41:08Z

@lebrush I added more comments to clarify all those definitions. I still have no idea on how I can simplify them, so if you have suggestions they're welcomed (maybe I'm doing redundant things and I cannot see them by myself).

kYc0o · 2017-01-31T21:41:43Z

Some thoughts @kaspar030 ?

kYc0o · 2017-02-03T13:05:14Z

ping @kaspar030 !

smlng · 2017-02-03T13:42:04Z

sys/include/xtimer/tick_conversion.h

+ * If it uses shifting, just check if it's not multiple of 576, if so,
+ * throw an error
+ */
+#if (XTIMER_HZ % 576 != 0)


This will never be true, as you check above the opposite, or do I miss something?

Yes I know, but then how to check this? I need to know if XTIMER_HZ is valid when using shifting, but I'll just remove this line...

kYc0o · 2017-02-07T13:39:52Z

@lebrush comments addressed.

lebrush

@kYc0o thanks for the heads up! I have some quick comments regarding the comments :-)

I've thought how to factorize the code and it's hard if we want to keep it readable. One possibility is to use some macro magic, despite of readability:

Example:

...
#define PASTER(len,low,high) div_u ## len ## _by ## low ## div ## high
#define EVALUATOR(len,low,high)  PASTER(len,low,high)
...

#if (XTIMER_HZ % 576 == 0)
    #if (XTIMER_SHIFT != 0)
        #define LOW 625
        #define HIGH 576

        #if (XTIMER_HZ > 921600ul)
            #if (XTIMER_HZ != (921600ul << XTIMER_SHIFT))
            #error XTIMER_HZ != (921600ul << XTIMER_SHIFT)
            #endif

            #define USEC_SHIFT(len) DIVIDER(len) << XTIMER_SHIFT
            #define TICKS_SHIFT(len) DIVIDER(len) >> XTIMER_SHIFT
        #else
            #if ((XTIMER_HZ << XTIMER_SHIFT) != 921600ul)
            #error (XTIMER_HZ << XTIMER_SHIFT) != 921600ul
            #endif

            #define USEC_SHIFT(len) DIVIDER(len) >> XTIMER_SHIFT
            #define TICKS_SHIFT(len) DIVIDER(len) << XTIMER_SHIFT
        #endif
...
#define DIVIDER(len) EVALUATOR(len, LOW, HIGH)
...
inline static uint32_t _xtimer_ticks_from_usec(uint32_t usec) {
    return USEC_SHIFT(32); /* i.e. div_u32_by_625div576(usec) << XTIMER_SHIFT; */
}

inline static uint64_t _xtimer_ticks_from_usec64(uint64_t usec) {
    return USEC_SHIFT(64);
}

inline static uint32_t _xtimer_usec_from_ticks(uint32_t ticks) {
    return TICKS_SHIFT(32);
}

inline static uint64_t _xtimer_usec_from_ticks64(uint64_t ticks) {
    return TICKS_SHIFT(64);
}

lebrush · 2017-02-08T15:38:05Z

sys/include/xtimer/tick_conversion.h

+/*
+ * If it's not greater, check if it's lower (XTIMER_HZ < 921600ul).
+ */
+#else


I guess here /* (XTIMER_HZ <= 921600ul) */ should be enough. Mind the equality.

Ok I'll fix.

lebrush · 2017-02-08T15:40:52Z

sys/include/xtimer/tick_conversion.h

+ * No more shifted multiple of 921600Hz frequencies to check
+ * (XTIMER_SHIFT != 0)
+ */
+#endif


Nitpick: /* (XTIMER_SHIFT != 0) */. I like documentation but I think it's clear enough with a one-liner. In my previous review I didn't mean that you include more comments regarding the code flow, simply that sometimes the one-liner where in the wrong place... :-)

Ok no problem, I just wanted to be as clear as possible, even for me. I find this code a bit complicated to understand in itself, so a bit more of documentation won't hurt IMHO.

lebrush · 2017-02-08T16:15:45Z

sys/include/xtimer/tick_conversion.h

+ * Check for the actual frequency without any shifting (921600Hz)
+ * (XTIMER_SHIFT == 0)
+ */
+#elif (XTIMER_HZ == 921600ul)


I don't think you need a special case for that, shifting to any side by 0 (XTIMER_SHIFT should be 0 here). Would give the same result.

Yes you're right, I think I can do the same for the 1MHz frequency.

kYc0o · 2017-02-10T14:24:56Z

I somehow like your suggestion with the macros magic. However, we have an "unofficial" preference to keep macro usage as low as possible, thus very often we use static inline functions instead of macros.

I can try to adapt your proposition with static inline functions to make it "standard", but I'd like to have @gebart opinion on this, since he was the contributor of this code.

kaspar030 · 2017-02-10T14:54:23Z

One possibility is to use some macro magic

I'd strongly oppose this. Please rather try to find a solution without any macros. Most of the checks could be done in C-Code, being syntax checked, and optimized out by the compiler.

kYc0o · 2017-02-10T14:56:33Z

One possibility is to use some macro magic
I'd strongly oppose this. Please rather try to find a solution without any macros. Most of the checks could be done in C-Code, being syntax checked, and optimized out by the compiler.

That's what I meant when I said I could try to adapt those macros to static in line functions.

jnohlgard · 2017-02-10T14:57:09Z

@kYc0o feel free to go ahead with the macro magic -> static inline conversion.

lebrush · 2017-02-13T09:05:28Z

I agree that macro magic makes sometimes code difficult to follow :-)
Another possibility is to create a separate tick_conversion.h for each frequency block (i.e. tick_conversion_921600.h) and include them in the corresponding #if #else blocks.

kYc0o · 2017-02-13T12:35:04Z

I was trying to convert the ifdefs to inline functions but at the end I was adding more complexity, when why that code is there is to optimise the conversions.

I removed some unnecessary checks, I find now it's a bit more readable and works correctly.

I tested with samr21-xpro and arduino-mega2560 with success. What is missing is a test for cc2538 but xtimer is broken for that platform due to different reasons.

kYc0o · 2017-02-14T12:59:33Z

For the separate file, IMHO, maybe it would be required if the cases of different frequencies increase. For now I think it's manageable, even if it was hard to get it in a less confusing way.

kYc0o · 2017-02-28T16:45:59Z

Ping! Can we maybe get this in for the Hack'n'ACK?

kYc0o · 2017-03-01T13:10:34Z

Can I squash to get the CI working?

kYc0o · 2017-03-06T16:01:30Z

ping @lebrush @kaspar030

lebrush

Codewise looks good.

kYc0o · 2017-03-17T13:15:14Z

Well, everything seems good here, just a review from @kaspar030 is missing...

kYc0o · 2017-03-21T17:17:05Z

ping @kaspar030

kYc0o · 2017-04-03T12:00:43Z

re-ping @kaspar030

kYc0o · 2017-04-18T08:57:04Z

@kaspar030 any objections to get this into the release? I have 2 more PRs which are based on this one, so I think they won't make it for the release if we don't merge this first soon.

smlng · 2017-05-12T09:41:38Z

@kaspar030 could you please spare some time to ACK (or/and comment on) this and approve changes done?! I'd rather not dismiss your review, but have your trusted ACK here 😉

kYc0o · 2017-05-22T13:37:29Z

@kaspar030 will you leave this PR open until the xtimer rework is done? in such case we can close it since it won't certainly be compatible with the new xtimer code.

kaspar030 · 2017-05-22T13:43:44Z

@kaspar030 will you leave this PR open until the xtimer rework is done? in such case we can close it since it won't certainly be compatible with the new xtimer code.

Hm, good question. ;)

By using a 32khz as backend for xtimer (not new in this PR), all code throughout the codebase would need to be checked if it's still working. E.g., are usec values <1/2**15 rounded down to 0?

We need multiple timers edit in the API edit...

kYc0o · 2017-05-22T13:52:50Z

E.g., are usec values <1/2**15 rounded down to 0?

Well, it will depend how much usec you need, since here we're certainly losing accuracy by shifting.

kYc0o · 2017-06-30T13:46:24Z

What do we do with this one? It's needed to support xtimer on waspmote-pro...

jnohlgard

I don't mind adding more specializations for the xtimer frequencies, but some changes are necessary:

Needs rebase
Minor comments above
Add test cases to unittests/tests-div for the new div.h functions, along with smart choices for the test vector (try to catch rounding errors, errors with large numbers close to the overflow limit, test for numbers near the magic number 10485759996).
It may be possible to reduce the very long preprocessor if block in xtimer.h for all different kinds of XTIMER_SHIFT. The special case of XTIMER_SHIFT == 0 is not necessary, a shift of constant literal 0 will be optimized away by the compiler, and worst case it would be a no-op machine instruction by shifting with 0.

jnohlgard · 2017-10-11T08:09:52Z

sys/include/div.h

 * @brief Approximation of (2**l)/d for d=15625, l=12, 32 bits
 */
-#define DIV_H_INV_15625_32    0x431bde83ul
+#define DIV_H_INV_15625_32     0x431bde83ul


It doesn't matter if you have parentheses or not here, these are just magic numbers and never any complex expressions. As for the letter case, I don't care either way, I tend to prefer lowercase for the suffixes, but I don't see any point in forcing it.

jnohlgard · 2017-10-11T08:16:49Z

sys/include/xtimer/tick_conversion.h

+ */
+#endif
+/*
+ * Now, check if XTIMER_HZ is a frequency multiple of 1MHz (or 15675, which is


typo: 15675 -> 15625

jnohlgard · 2017-10-11T08:24:04Z

sys/include/xtimer/tick_conversion.h

 #error Unknown hardware timer frequency (XTIMER_HZ), check board.h and/or add an implementation in sys/include/xtimer/tick_conversion.h
 #endif
+/*
+ * End of optimisations


I would prefer to have this comment on the line with the #endif on L275, and related to the if condition. Suggestion:

#endif /* XTIMER_HZ == xxx */

(multiples of 921600/1000000)

kYc0o · 2018-05-22T15:59:21Z

Sorry for forgetting this PR since looong time, I was kind of waiting for big changes on xtimer but apparently that won't come soon.

I want to update and if possible merge this PR to support another frequency for the mega-xplained board.

ZetaR60 · 2018-06-06T00:00:11Z

Some new relevant PRs: #9280, #9283

kYc0o · 2018-06-06T09:43:13Z

Yes! I think that will be the way to go for supporting other clock sources in xtimer.

stale · 2019-08-10T06:08:15Z

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.

kYc0o added the Area: timers Area: timer subsystems label Jan 27, 2017

kYc0o added this to the Release 2017.01 milestone Jan 27, 2017

kYc0o assigned jnohlgard Jan 27, 2017

kaspar030 requested changes Jan 27, 2017

View reviewed changes

kYc0o mentioned this pull request Jan 27, 2017

xtimer with any value clock frequency #5237

Closed

kYc0o added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jan 30, 2017

lebrush suggested changes Jan 31, 2017

View reviewed changes

PeterKietzmann modified the milestones: Release 2017.01, Release 2017.04 Feb 1, 2017

smlng reviewed Feb 3, 2017

View reviewed changes

kYc0o force-pushed the xtimer_diff_freq branch from 55827a7 to 2e87e54 Compare February 7, 2017 13:38

lebrush reviewed Feb 8, 2017

View reviewed changes

lebrush approved these changes Mar 7, 2017

View reviewed changes

kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 17, 2017

kaspar030 modified the milestones: Release 2017.07, Release 2017.04 Apr 21, 2017

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 30, 2017

kYc0o mentioned this pull request Oct 10, 2017

xtimer: Set XTIMER_SHIFT to 0 if underlying timer is 32768 Hz #7704

Merged

jnohlgard requested changes Oct 11, 2017

View reviewed changes

kYc0o removed this from the Release 2017.10 milestone Oct 16, 2017

kYc0o added 3 commits May 18, 2018 16:40

sys/include/xtimer: add 921600Hz compatibility

bc4e6c2

sys/include/div: allow conversions for 576/625

93d0e81

(multiples of 921600/1000000)

fixup! sys/include/xtimer: add 921600Hz compatibility

bf63a79

kYc0o force-pushed the xtimer_diff_freq branch from da37a2f to bf63a79 Compare May 22, 2018 15:53

fixup! sys/include/xtimer: add 921600Hz compatibility

9cafae1

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

Conversation

kYc0o commented Jan 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Jan 30, 2017

Uh oh!

lebrush left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Jan 31, 2017

Uh oh!

kYc0o commented Jan 31, 2017

Uh oh!

kYc0o commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Feb 7, 2017

Uh oh!

lebrush left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Feb 10, 2017

Uh oh!

kaspar030 commented Feb 10, 2017

Uh oh!

kYc0o commented Feb 10, 2017

Uh oh!

jnohlgard commented Feb 10, 2017

kaspar030 commented May 22, 2017 •

edited

Loading