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

Unify behaviour of wait on AVR #14025

Merged
merged 1 commit into from
Aug 16, 2021
Merged

Unify behaviour of wait on AVR #14025

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions tmk_core/common/avr/_wait.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,26 @@

#include <util/delay.h>

#define wait_ms(ms) _delay_ms(ms)
#define wait_us(us) _delay_us(us)
#define wait_ms(ms) \
do { \
if (__builtin_constant_p(ms)) { \
_delay_ms(ms); \
} else { \
for (uint16_t i = ms; i > 0; i--) { \
_delay_ms(1); \
} \
} \
} while (0)
#define wait_us(us) \
do { \
if (__builtin_constant_p(us)) { \
_delay_us(us); \
} else { \
for (uint16_t i = us; i > 0; i--) { \
_delay_us(1); \
} \
Comment on lines +35 to +37
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.

} \
} while (0)

/* The AVR series GPIOs have a one clock read delay for changes in the digital input signal.
* But here's more margin to make it two clocks. */
Expand Down