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.

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

Unify behaviour of wait on AVR #14025

Merged
merged 1 commit into from
Aug 16, 2021
Merged

Unify behaviour of wait on AVR #14025

merged 1 commit into from
Aug 16, 2021

Conversation

zvecr
Copy link
Member

@zvecr zvecr commented Aug 15, 2021

Description

wait_ms/wait_us on AVR cannot handle dynamic values like the ChibiOS abstraction can.

This leads to code that bleeds implementation details outside of tmk_core. For example;

#if defined(__AVR__)
        for (uint16_t i = delay; i > 0; i--) {
            wait_ms(1);
        }
#else
        wait_ms(delay);
#endif

Hopefully the __builtin_constant_p magic should catch a lot of the existing cases, limited testing shows the same firmware sizes.

Types of Changes

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

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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).

@github-actions github-actions bot added the core label Aug 15, 2021
@zvecr zvecr requested a review from a team August 15, 2021 23:53
@zvecr zvecr marked this pull request as ready for review August 16, 2021 16:16
@zvecr zvecr merged commit 2bc8215 into qmk:develop Aug 16, 2021
Comment on lines +35 to +37
for (uint16_t i = us; i > 0; i--) { \
_delay_us(1); \
} \
Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation would probably be grossly inaccurate because of the loop overhead (note that 1µs is just 8 cycles if this code runs on a 8 MHz chip). Hope that nobody actually tries to use this wait_us() with non-constant arguments, otherwise someone might need to implement something better instead of this.

This function is also sometimes used with some rather large values which won't fit into uint16_t (does not matter while they are compile time constants though):

keyboards/handwired/dactyl/matrix.c:        wait_us(1000000);
keyboards/handwired/pterodactyl/matrix.c:        wait_us(1000000);
keyboards/hotdox/left.c:        wait_us(1000000);

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 implementation would probably be grossly inaccurate because of the loop overhead

It would have been anyway, as they would have done the gross platform check loopy loop per my example in the docs.

When there becomes a need to do a super accurate us wait, across all platforms, we would have an issue with getting that behaviour consistent on ChibiOS. A bridge to cross at some point, maybe something for docs in the short term.

on the use of wait_us(1000000);, those look like general code smell to me and could do with a tidy up.

Either way they will have to be tackled as a future iteration.

nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants