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

Fix timer_elapsed() overflow issue for STM32F103 and other ChibiOS boards #7595

Merged
merged 2 commits into from
Feb 1, 2020
Merged

Fix timer_elapsed() overflow issue for STM32F103 and other ChibiOS boards #7595

merged 2 commits into from
Feb 1, 2020

Conversation

dworki
Copy link
Contributor

@dworki dworki commented Dec 10, 2019

Description

Space Cadet does not work on STM32F103 (BluePill). Specifically it works a couple of times, then it stops. There may be a deeper problem in timeout computation that I'm not able to fix (being a Java programmer). Basically after a couple of any key presses the timer_elapsed() starts returning large increments (the overflow variable in chibios/timer.c) gets stuck at 65535.

Adding TIMER_DIFF_16() to the comparison in perform_space_cadet() fixed the problem. I'm aware that it is probably not the "right" solution but it works for this case.

It is a simple change but I'm a little worried that I was not able to test the fix on any other architecture.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@noroadsleft noroadsleft requested a review from a team December 10, 2019 05:53
drashna
drashna requested changes Dec 10, 2019
View reviewed changes
Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

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

Honestly, while I do like the idea of fixing this sort of weird issue, my guess is that this affects a bunch of other stuff too.

It may be best to improve the core functionality, rather than just this call.

Eg, update the function in:
https://github.com/qmk/qmk_firmware/blob/b624f32f944acdc59dcb130674c09090c5c404cb/tmk_core/common/chibios/timer.c

@fauxpark
Copy link
Member

The ATSAM and AVR timer.c's both use TIMER_DIFF_16() in timer_elapsed(). ChibiOS is the odd one out here just doing a simple subtraction.

@dworki
Copy link
Contributor Author

dworki commented Dec 10, 2019

Makes sense! I didn't occur to me to look at other implementations. I'll move the TIMER_DIFF_16() from process_space_cadet() to elapsed() in timer.c
I'm quite new to github, should i open new PR or somehow amend this one?

@fauxpark
Copy link
Member

Just commit and push to the same branch, and it will show up here :)

@dworki dworki requested a review from drashna December 10, 2019 11:05
Duckle29
Duckle29 approved these changes Dec 10, 2019
View reviewed changes
Copy link
Contributor

@Duckle29 Duckle29 left a comment

Choose a reason for hiding this comment

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

LGTM

@drashna drashna changed the title Fixed strange space cadet timer owerflow on STM32F103 Fix timer_elapsed() overflow issue for STM32F103 and other ChibiOS boards Dec 11, 2019
@drashna
Copy link
Member

drashna commented Dec 18, 2019

Been testing on my planck rev6, and on my collide 39 with a proto-proton C, and seems to not cause any issues

@tzarc tzarc mentioned this pull request Dec 26, 2019
Merged
21 tasks
@stale
Copy link

stale bot commented Feb 1, 2020

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@stale stale bot removed the awaiting changes label Feb 1, 2020
@tzarc tzarc merged commit 4e6d1ae into qmk:master Feb 1, 2020
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 12, 2020
@dworki @drashna
Fix timer_elapsed() overflow issue for STM32F103 and other ChibiOS bo…
af1e3ff 
…ards (qmk#7595)

* fixed strange space cadet timer owerflow on STM32F103

* Moved elapsed time fix to timer.c
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 14, 2020
@dworki @fdidron
Fix timer_elapsed() overflow issue for STM32F103 and other ChibiOS bo…
2de3ecc 
…ards (qmk#7595)

* fixed strange space cadet timer owerflow on STM32F103

* Moved elapsed time fix to timer.c
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Feb 21, 2020
@dworki @HokieGeek
Fix timer_elapsed() overflow issue for STM32F103 and other ChibiOS bo…
e1407fc 
…ards (qmk#7595)

* fixed strange space cadet timer owerflow on STM32F103

* Moved elapsed time fix to timer.c
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 26, 2020
@dworki @fdidron
Fix timer_elapsed() overflow issue for STM32F103 and other ChibiOS bo…
7ed8cd4 
…ards (qmk#7595)

* fixed strange space cadet timer owerflow on STM32F103

* Moved elapsed time fix to timer.c
@yulei yulei mentioned this pull request Mar 2, 2020
Closed
3 tasks
c0psrul3 pushed a commit to c0psrul3/qmk_firmware that referenced this pull request Mar 23, 2020
@dworki @c0psrul3
Fix timer_elapsed() overflow issue for STM32F103 and other ChibiOS bo…
4618a1e 
…ards (qmk#7595)

* fixed strange space cadet timer owerflow on STM32F103

* Moved elapsed time fix to timer.c
kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
@dworki @kujawakk
Fix timer_elapsed() overflow issue for STM32F103 and other ChibiOS bo…
50923fb 
…ards (qmk#7595)

* fixed strange space cadet timer owerflow on STM32F103

* Moved elapsed time fix to timer.c
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
@dworki
Fix timer_elapsed() overflow issue for STM32F103 and other ChibiOS bo…
14dbeaf 
…ards (qmk#7595)

* fixed strange space cadet timer owerflow on STM32F103

* Moved elapsed time fix to timer.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fauxpark fauxpark fauxpark approved these changes

@Duckle29 Duckle29 Duckle29 approved these changes

@drashna drashna drashna approved these changes

@yanfali yanfali yanfali approved these changes

Assignees
No one assigned
Labels
bug core needs testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dworki @fauxpark @drashna @yanfali @Duckle29 @tzarc @noroadsleft