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

boards/nucle-f0xx: fixed timer configuration #6494

Merged
merged 2 commits into from
Feb 3, 2017
Merged

boards/nucle-f0xx: fixed timer configuration #6494

merged 2 commits into from
Feb 3, 2017

Conversation

haukepetersen
Copy link
Contributor

Timers were broken on the nucleo-f0xx boards. The reason was a mixture of broken timer configuration and a miss-conception by the timer driver: the timer driver expects the configured timer to have exactly 4 capture compare channels. But apparently not all timers on the STM32F0s have 4 of them, some have only a single channel (e.g. TIM14), while other have 2 of them (e.g. TIM15/16/17).

This PR fixes the issue for now by choosing TIM1 as default timer for all STM32F0 based nucleo boards.

I will also open an issue for adapting the timer driver to cope with non-4-cc-channel timers...

@haukepetersen haukepetersen added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 27, 2017
@haukepetersen haukepetersen added this to the Release 2017.01 milestone Jan 27, 2017
@PeterKietzmann PeterKietzmann added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jan 27, 2017
@PeterKietzmann PeterKietzmann mentioned this pull request Jan 27, 2017
Merged
@PeterKietzmann
Copy link
Member

nucelo-l091 still doesn't work. Which platform did you use for testing?

@haukepetersen
Copy link
Contributor Author

nucleo-f072 and nucleo-f091

@haukepetersen
Copy link
Contributor Author

I did not touch anything for the stm32lxx, they are still broken I guess

@PeterKietzmann
Copy link
Member

Does tests/xtimer_uslep and tests/xtimer_drift proceed on the nucleo-f091?

@haukepetersen
Copy link
Contributor Author

nope. Still broken, same for xtimer_msg. But I think I have an intuition what might be causing this: #6457, #6457, and similar changed the main timer from 32 to 16-bit. But the xtimer configuration was not adapted...

@haukepetersen
Copy link
Contributor Author

will fix this

@haukepetersen
Copy link
Contributor Author

done

PeterKietzmann
PeterKietzmann reviewed Jan 27, 2017
View reviewed changes
boards/nucleo-f030/include/board.h
* @{
*/
#define XTIMER_WIDTH (16)
#define XTIMER_WIDTH (16)
Copy link
Member

Choose a reason for hiding this comment

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

It picky but why tab?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy paste

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this addressed?

@PeterKietzmann
Copy link
Member

test/xtimer_usleep seems ok. tests/xtimer_drift seems of for the first 105 seconds and then floods the console with outputs (didn't go into details, is it intended in this test??). tests/xtimer_longterm acts a bit weird. Look at the following ouput. The first "1min" notification comes after 2 minutes and the second "1min" comes at the same time (after 2 minutes as intended). The third "1min" notification comes in time,. After the first "3min" notification is seems as if some "1min" ticks are lost.

2017-01-27 11:51:27,176 - INFO # xtimer long-term test
2017-01-27 11:51:27,182 - INFO # Refer to the README to get information on the expected output.
2017-01-27 11:51:27,187 - INFO # sleep -- 18min -- 0 ticks since
2017-01-27 11:51:27,188 - INFO # sleep -- 5min  -- 0 ticks since
2017-01-27 11:53:27,138 - INFO # TICK  -- 1min
2017-01-27 11:53:27,138 - INFO # TICK  -- 1min
2017-01-27 11:54:27,186 - INFO # TICK  -- 1min
2017-01-27 11:54:27,198 - INFO # msg   -- 3min  -- 3 ticks since
2017-01-27 11:56:27,138 - INFO # TICK  -- 1min
2017-01-27 11:56:27,139 - INFO # TICK  -- 1min
2017-01-27 11:56:27,192 - INFO # sleep -- 5min  -- 5 ticks since
2017-01-27 11:57:27,258 - INFO # TICK  -- 1min
2017-01-27 11:57:27,258 - INFO # msg   -- 3min  -- 3 ticks since
2017-01-27 11:59:27,204 - INFO # TICK  -- 1min
2017-01-27 11:59:27,205 - INFO # TICK  -- 1min

@haukepetersen
Copy link
Contributor Author

seems like this has something to do with the xtimer configuration. But I must say, I have never understood how to anticipate the correct values for this...

@PeterKietzmann
Copy link
Member

Neither did I :-(. I already assumed xtimer config problems in #6419. Maybe @kaspar030 can have a brief look at this?

@kYc0o
Copy link
Contributor

kYc0o commented Jan 31, 2017

I didn't test on other nucleo boards but at least on nucleo-f091 it gaves me impressive results:

2017-01-31 23:14:21,261 - INFO # main(): This is RIOT! (Version: 2017.04-devel-2-g57948-snake.local-pr/stm32f0_timers)
2017-01-31 23:14:21,263 - INFO # xtimer long-term test
2017-01-31 23:14:21,268 - INFO # Refer to the README to get information on the expected output.
2017-01-31 23:14:21,271 - INFO # sleep -- 18min -- 0 ticks since
2017-01-31 23:14:21,274 - INFO # sleep -- 5min  -- 0 ticks since
2017-01-31 23:15:21,227 - INFO # TICK  -- 1min
2017-01-31 23:16:21,229 - INFO # TICK  -- 1min
2017-01-31 23:17:21,230 - INFO # TICK  -- 1min
2017-01-31 23:17:21,281 - INFO # msg   -- 3min  -- 3 ticks since
2017-01-31 23:18:21,232 - INFO # TICK  -- 1min
2017-01-31 23:19:21,233 - INFO # TICK  -- 1min
2017-01-31 23:19:21,285 - INFO # sleep -- 5min  -- 5 ticks since
2017-01-31 23:20:21,302 - INFO # TICK  -- 1min
2017-01-31 23:20:21,355 - INFO # msg   -- 3min  -- 3 ticks since
2017-01-31 23:21:21,303 - INFO # TICK  -- 1min
2017-01-31 23:22:21,305 - INFO # TICK  -- 1min
2017-01-31 23:23:21,307 - INFO # TICK  -- 1min
2017-01-31 23:23:21,364 - INFO # msg   -- 3min  -- 3 ticks since
2017-01-31 23:24:21,309 - INFO # TICK  -- 1min
2017-01-31 23:24:21,363 - INFO # sleep -- 5min  -- 5 ticks since
2017-01-31 23:25:21,311 - INFO # TICK  -- 1min
2017-01-31 23:26:21,313 - INFO # TICK  -- 1min
2017-01-31 23:26:21,373 - INFO # msg   -- 3min  -- 3 ticks since
2017-01-31 23:27:21,315 - INFO # TICK  -- 1min
2017-01-31 23:28:21,316 - INFO # TICK  -- 1min
2017-01-31 23:28:21,368 - INFO # msg   -- 14min -- 14 ticks since
2017-01-31 23:29:21,318 - INFO # TICK  -- 1min
2017-01-31 23:29:21,375 - INFO # sleep -- 5min  -- 5 ticks since
2017-01-31 23:29:21,381 - INFO # msg   -- 3min  -- 3 ticks since
2017-01-31 23:30:21,320 - INFO # TICK  -- 1min
2017-01-31 23:31:21,321 - INFO # TICK  -- 1min
2017-01-31 23:32:21,324 - INFO # TICK  -- 1min
2017-01-31 23:32:21,371 - INFO # sleep -- 18min -- 18 ticks since
2017-01-31 23:32:21,388 - INFO # msg   -- 3min  -- 3 ticks since

I'd say that this PR solves the problem and should be merged asap.

As for @PeterKietzmann results, I think it could be related to some UART issues, which revision your board has? Do you have external oscillator? Try also to upgrade the firmware of the ST-Link part, since we (@haukepetersen and me) experienced some bug fixing by applying the update.

@PeterKietzmann PeterKietzmann modified the milestones: Release 2017.01, Release 2017.04 Feb 1, 2017
@PeterKietzmann PeterKietzmann mentioned this pull request Feb 1, 2017
Closed
@PeterKietzmann
Copy link
Member

I opened #6542 to keep track of this

@PeterKietzmann PeterKietzmann removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Feb 1, 2017
@vincent-d vincent-d mentioned this pull request Feb 2, 2017
Closed
aabadie
aabadie approved these changes Feb 2, 2017
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

I tested several tests: periph_rtc, xtimer_msg, xtimer_usleep on the nucleo-f0xx I have.

  • nucleo-f070 => ok
  • nucleo-f030 => ok
  • nucleo-f091(RC) => ok
  • nucleo-f072 => ok

Alles gut for me.

@aabadie
Copy link
Contributor

aabadie commented Feb 2, 2017

But I think I have an intuition what might be causing this: #6457, #6457, and similar changed the main timer from 32 to 16-bit

My bad, sorry :)

@miri64 miri64 merged commit 59150b6 into RIOT-OS:master Feb 3, 2017
@kYc0o
Copy link
Contributor

kYc0o commented Feb 3, 2017

@miri64 why?

@miri64
Copy link
Member

miri64 commented Feb 3, 2017

Not good? Saw it was approved and the CIs were happy so why not?

@kYc0o
Copy link
Contributor

kYc0o commented Feb 3, 2017

I thought there was a comment without modification. I commented above.

@miri64
Copy link
Member

miri64 commented Feb 3, 2017

That was really nitpicky (and also please if you do want it to be changed, mark it as change request ;-))

@kYc0o
Copy link
Contributor

kYc0o commented Feb 3, 2017

It was not my comment, was @PeterKietzmann, if he's OK, I have nothing to object ;-)

@haukepetersen haukepetersen deleted the fix_nucleo_timerconfig branch February 8, 2017 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeterKietzmann PeterKietzmann PeterKietzmann left review comments

@kYc0o kYc0o kYc0o left review comments

@aabadie aabadie aabadie approved these changes

Assignees

@aabadie aabadie

@PeterKietzmann PeterKietzmann

Labels
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
Milestone
Release 2017.04
Development

Successfully merging this pull request may close these issues.
5 participants
@haukepetersen @PeterKietzmann @kYc0o @aabadie @miri64