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

Enable and fix additional warnings, switch to -O3 and LTO. #363

Conversation

abcminiuser
Copy link
Contributor

This is more of a RFC than an actual request to pull as-is. I'm just getting acquainted with the project and code.

This is a roll-up of several minor changes:

  1. Switching from -O3 to -Os. Most ARM embedded platforms don't really gain a whole lot from -O3 other than code bloat, and generally any hot loops can be marked as such with attributes, or compiled with alternative settings.

  2. Enabling Link Time Optimization. As long as we're OK targeting modern GCC installations, i.e. GCC 8+, LTO is quite stable for ARM Cortex M. This greatly reduces the code size and helps inlining across translation units, leading to smaller and faster code.

Right V2 firmware with -O3:

Memory region         Used Size  Region Size  %age Used
    m_interrupts:          1 KB         1 KB    100.00%
          m_text:         52 KB       463 KB     11.23%
          m_data:       53496 B        64 KB     81.63%
        m_data_2:       41920 B      65280 B     64.22%
        m_noinit:          16 B        255 B      6.27%

Right V2 firmware with LTO and -Os:

Memory region         Used Size  Region Size  %age Used
    m_interrupts:          1 KB         1 KB    100.00%
          m_text:       39384 B       463 KB      8.31%
          m_data:       52288 B        64 KB     79.79%
        m_data_2:       41920 B      65280 B     64.22%
        m_noinit:          16 B        255 B      6.27%
  1. Enabling additional warnings. Fairly self-explanatory. I've disabled some of the nosier ones that are enabled with -Wextra and patched the code to fix the new warnings that were generated.

  2. Add missing const to pointer params. Just a few instances I noticed where functions that were not expected to alter their pointer arguments pass into them, which I've now altered to be passed as const T`.

I've built and tested the repo on my local Drone CI, and built/flashed my new V2 unit with it without any apparent issues.

@cla-sign-bot
Copy link

cla-sign-bot bot commented May 2, 2021

Thank you for your contribution!
Please sign the CLA.

@mondalaci
Copy link
Member

Thanks for your contribution!

Unfortunately, after flashing your branch, my UHK doesn't enumerate anymore over USB. Any ideas why?

I'm running gcc 9.3.0

@abcminiuser
Copy link
Contributor Author

Interesting.

That's almost certainly the LTO -- it works for me on GCC 10 and in my experience has been OK for embedded ARM since GCC 8 or so, but it does depend on well defined behaviors of the code.

It's possible that there's some latent undefined behaviour in the Kinetis SDK or just a bad interaction on the version you have installed.

In that case, given the large amount of flash available, it's not worth trying to troubleshoot and can be re-disabled. The other changes in this branch should still be valid.

@mondalaci
Copy link
Member

LTO has indeed caused the issue and removing it made the firmware work with gcc 9.3.0. We can enable LTO later when newer GCC versions are more widely available.

@mondalaci mondalaci merged commit 55fa6bd into UltimateHackingKeyboard:master May 3, 2021
@abcminiuser abcminiuser deleted the feature/enable_lto_and_warnings branch May 9, 2021 11:11
ert78gb pushed a commit that referenced this pull request Dec 4, 2024
…tings_reloads

Fix address settings reloading, and consequently dongle leds updates.
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.

2 participants