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

libraries/SPI: abs -> std::abs and cast fixes #7362

Merged
merged 6 commits into from
Jun 13, 2020

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Jun 8, 2020

Not to complicate comment chain in #6294 further

SPI library code erroneously assumed that abs() is a C function, so include stdlib.h is required.
what happens instead is Arduino.h shadows abs() with it's own macro

freq - calFreq or freq - bestFreq may produce negative results and require abs()
In the referenced PR, when adding using std::abs; it confuses GCC by introducing multiple functions named abs(), neither of which have uint32_t argument. As it turns out, uint32_t() - int32_t() promotes to uint32_t, where old libc abs() silently converted it back to int

Fixing casts for freq to explicitly make it a signed int when comparing with generated values, both of which are signed.

edit: noting that abs() isn't a macro and we actually want signedness

SPI library code erroneously assumed that:
- abs() is a C function, so include stdlib.h is required.
  what happens instead is Arduino.h shadows `abs()` with it's own macro
- uint32_t() - int32_t() promotes to int32_t, thus needing abs()

Fix both issues, leaving existing calculations as-is.
libraries/SPI/SPI.cpp Outdated Show resolved Hide resolved
- restore abs call, cast freq to correctly display the intent
- update magic numbers comments
- move some spiclk_t magic numbers to func consts
@mcspr mcspr changed the title libraries/SPI: remove pointless abs(...) call libraries/SPI: abs -> std::abs and cast fixes Jun 9, 2020
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

I'm a little on the fence about converting 0x1fff->(1<<13)-1 on asthetic grounds (1fff is easier to grok for me when I think of HW register bitmasks), but generally LGTM. Thx.

@earlephilhower earlephilhower merged commit 599492e into esp8266:master Jun 13, 2020
@mcspr mcspr deleted the lib-spi/man-abs branch January 7, 2023 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants