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.

Sign up for GitHub

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

Jump to bottom

cc2538: fix xtimer #6692

Merged
merged 2 commits into from
Mar 9, 2017
Merged

cc2538: fix xtimer #6692

merged 2 commits into from
Mar 9, 2017

Conversation

smlng
Copy link
Member

@smlng smlng commented Mar 3, 2017

This PR fixes #6419, at least for me.

It basically reverts xtimer configuration introduces in #5608, and resets XTIMER_SHIFT to 0 and XTIMER_HZ 1000000 (1 MHz). I have to admit, I'm not sure why this helps, but it does. In the last days I made extensive tests using the hardware timer and xtimer.

The cc2538 has 4 timers each can be configured in 16Bit mode with 2 channels and 32 Bit mode with 1 channel. For the latter only a frequency of 16 MHz is allowed, as prescaler is only available in 16 Bit mode. However, using 16Bit timer with 16Mhz or a 32Bit timer with 16 MHz the tests xtimer_drift and xtimer_periodic_wakeup always fail, and xtimer_usleep for 16Bit + 16MHz, too. All tests succeeded when using a 16 Bit timer with 1 MHz (which is the old setting).

Can someone please confirm?

@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: timers Area: timer subsystems labels Mar 3, 2017
@smlng smlng added this to the Release 2017.04 milestone Mar 3, 2017
@smlng smlng mentioned this pull request Mar 3, 2017
Closed
@kYc0o 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 3, 2017
@kYc0o
Copy link
Contributor

kYc0o commented Mar 3, 2017

I'm trying to test in a cc2538dk board applying the same modifications without success... Why it should only work for remote boards?

@smlng
Copy link
Member Author

smlng commented Mar 3, 2017

mhm, I'm unsure as well - because I was pretty sure I tested various settings yesterday, including 16Bit with 1MHz and it didn't work. However, today it worked 😕

@smlng
Copy link
Member Author

smlng commented Mar 3, 2017

I think we have cc2538dk here, too - so I'll try to test that, too

@PeterKietzmann
Copy link
Member

xtimer_hang or xtimer_usleep still not running on remote-revb

@A-Paul
Copy link
Member

A-Paul commented Mar 3, 2017

I confirm that both tests are also not running on my cc2538dk.

@kYc0o
Copy link
Contributor

kYc0o commented Mar 3, 2017

I just tested all xtimer tests and no one worked. I have a remote-reva.

@smlng
Copy link
Member Author

smlng commented Mar 3, 2017

oops, during my debugging and tests session I found that only the cc2538 was missing the periph_pm feature (other cortex cpus have that) so I added a Makefile.feature. I forgot about that, so it lay around in my tree but wasn't commited. It seems that this is the missing piece!

Please test again @PeterKietzmann, @kYc0o, and @A-Paul !

@smlng smlng changed the title cc2538: revert xtimer params from #5608 cc2538: fix xtimer Mar 3, 2017
@smlng
Copy link
Member Author

smlng commented Mar 3, 2017

I think this was caused when the old lmp code was removed in #6160 - but the cc2538 was forgotten then

@smlng
Copy link
Member Author

smlng commented Mar 4, 2017

to give a bit more rational on why periph_pm is needed: during debugging I found (with lots of debug output enabled) that everything seems to finally hang inside the idle thread. So I looked into that and since lpm removed, I guess it causes a deep sleep mode or something - in any case though the hardware timer fires initially it fails to fire again after idle thread.

haukepetersen
haukepetersen reviewed Mar 6, 2017
View reviewed changes
cpu/cc2538/Makefile.features
@@ -0,0 +1 @@
FEATURES_PROVIDED += periph_pm
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated?!

Copy link
Member Author

Choose a reason for hiding this comment

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

NO its one essential part of the fix! Read my explanation above, otherwise everything gets stuck in idle thread. I could make a separate PR for this, but its already a commit and as said its part of the fix.

@PeterKietzmann
Copy link
Member

I ran xtimer_hang, xtimer_usleep and xtimer_drift with success. The last one ran for about half an hour with no significant drift. @A-Paul please test the xtimer on your 2538dk again. @haukepetersen do you have objections against the PM stuff?

@smlng smlng force-pushed the pr/cc2538/fix_xtimer_params branch from 23010dd to e0f7a28 Compare March 7, 2017 11:15
@A-Paul
Copy link
Member

A-Paul commented Mar 7, 2017

On cc2538dk all tests/xtimer_* look good now.

@smlng smlng force-pushed the pr/cc2538/fix_xtimer_params branch from e0f7a28 to 40431fe Compare March 7, 2017 21:00
kYc0o
kYc0o approved these changes Mar 8, 2017
View reviewed changes
Copy link
Contributor

@kYc0o kYc0o left a comment

Choose a reason for hiding this comment

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

ACK. All xtimer tests passed.

@kYc0o kYc0o added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 8, 2017
@PeterKietzmann
Copy link
Member

Jenkins fails unrelated and both Murdocks succeeded. ACK and go

@PeterKietzmann PeterKietzmann merged commit 8c86aa7 into RIOT-OS:master Mar 9, 2017
@smlng smlng deleted the pr/cc2538/fix_xtimer_params branch March 9, 2017 07:17
@smlng
Copy link
Member Author

smlng commented Mar 9, 2017

finally 🎉

@kYc0o
Copy link
Contributor

kYc0o commented Mar 9, 2017

Thanks @smlng !

A-Paul
A-Paul reviewed Mar 9, 2017
View reviewed changes
boards/openmote-cc2538/include/board.h Outdated
#define XTIMER_SHIFT (4)
#define XTIMER_HZ (16000000UL)
#define XTIMER_SHIFT (0)
#define XTIMER_HZ (10000000UL)
Copy link
Member

Choose a reason for hiding this comment

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

You're sure this has to be ten times higher than on the other boards?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was already fixed before merge! Reload the page and have another look?

@smlng smlng mentioned this pull request Mar 9, 2017
Closed
@smlng smlng mentioned this pull request Jul 10, 2017
Merged
@smlng smlng mentioned this pull request Oct 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@A-Paul A-Paul A-Paul left review comments

@haukepetersen haukepetersen haukepetersen left review comments

@kYc0o kYc0o kYc0o approved these changes

Assignees

@kYc0o kYc0o

@PeterKietzmann PeterKietzmann

@A-Paul A-Paul

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: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2017.04
Development

Successfully merging this pull request may close these issues.

cpu/cc2538: timer broken?
5 participants
@smlng @kYc0o @PeterKietzmann @A-Paul @haukepetersen