Skip to content

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

Merged
jnohlgard merged 1 commit into
RIOT-OS:masterfrom
jnohlgard:pr/xtimer-shift-32768hz
Oct 10, 2017
Merged

xtimer: Set XTIMER_SHIFT to 0 if underlying timer is 32768 Hz#7704
jnohlgard merged 1 commit into
RIOT-OS:masterfrom
jnohlgard:pr/xtimer-shift-32768hz

Conversation

@jnohlgard

Copy link
Copy Markdown
Member

The error message "XTIMER_SHIFT cannot be derived for given XTIMER_HZ, verify settings!" is triggered if XTIMER_HZ is not a power of two multiple of 1 MHz. If XTIMER_SHIFT is defined to 0, then the other error message "XTIMER_SHIFT is set relative to XTIMER_HZ, no manual define required!" is triggered instead. This change was introduced by #6702

@jnohlgard jnohlgard added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) quality defect Area: timers Area: timer subsystems labels Oct 10, 2017
@jnohlgard jnohlgard added this to the Release 2017.10 milestone Oct 10, 2017

@smlng smlng left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, another (side) case which was missed during testing.

Hence, we should add a test for all those xtimer frequencies ...

@smlng smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 10, 2017
@jnohlgard

Copy link
Copy Markdown
Member Author

The frdm-kw41z board in #6995 will use 32768 as the frequency, I don't think we need to spend any more effort on adding test cases for it.

@kYc0o

kYc0o commented Oct 10, 2017

Copy link
Copy Markdown
Contributor

I think that was because #6507 was not merged before... just sayin' 🙄

@jnohlgard jnohlgard merged commit ca0db13 into RIOT-OS:master Oct 10, 2017
@jnohlgard jnohlgard deleted the pr/xtimer-shift-32768hz branch October 10, 2017 12:25
@smlng

smlng commented Oct 10, 2017

Copy link
Copy Markdown
Member

I think that was because #6507 was not merged before

nope, I just missed the already present special case of 32768HZ external crystals. Or more to the point, before #6702 it wasn't necessary to specifically care about that frequency. Btw. 32768 is no multiple of 576 as required for #6507, so that wouldn't have solved the issue!

@kYc0o

kYc0o commented Oct 10, 2017

Copy link
Copy Markdown
Contributor

What I mean is that I took into account 32768 when the calculations are done, which was not taken into account in #6702 and that's why at some point that PR was waiting for #6507, but nevermind, it doesn't matter now.

@smlng

smlng commented Oct 10, 2017

Copy link
Copy Markdown
Member

I'm (still) not convinced that #6507 solves the issue addressed here or would have prevented the regression, if merged first. Because, the bug was introduced by the missing check/case using pre-processor #if (XTIMER_HZ == 32768) in xtimer.h, a file PR #6507 doesn't touch at all. The conversions in div.h and tick_conversion.h specific to a frequency of 32768 were already there.

Hence, even if #6507 would have been merged before #6702 the issue would (IMHO) have come up, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants