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

Placing #ifdefs inside or outside of source files #452

Closed
jackhumbert opened this issue Jun 27, 2016 · 7 comments
Closed

Placing #ifdefs inside or outside of source files #452

jackhumbert opened this issue Jun 27, 2016 · 7 comments

Comments

@jackhumbert
Copy link
Member

Mentioned in #451, I'd like to look at the possibility of moving a large portion of the

#ifdef FEATURE_ENABLE
  #include "feature.h"
  feature();
#endif

code blocks to having a wrapper on the entire feature.c file to disable it, and include dummy functions where necessary (they likely won't be needed that often).

Pros:

  • disable via config.h instead of Makefile (doesn't really cleaning, easier/more straight-forward to modify, all being c)
  • cleaner code, no giant #ifdefs around calls (something action.c is suffering from)
  • no #ifdefs required in keymap.c

Cons:

  • is the compiler smart enough to exclude things without affecting memory size?
  • code is less clear, since functions will always be called (unsure if they're doing anything)
  • feature.c will always be compiled (I believe) - does this have an effect on compile time?

Comments are welcome!

@algernon
Copy link
Contributor

Cons:

  • is the compiler smart enough to exclude things without affecting memory size?

I'm reasonably sure it is smart enough, or can be made smart enough with minimal instrumentation, but I'll do some testing when I have a bit of time.

  • code is less clear, since functions will always be called (unsure if they're doing anything)

I'm not convinced it's less clear, but that is likely a matter of taste. I find tons of #ifdefs much harder to follow. Having the functions there unconditionally signals intent to me better than the ifdef-forest.

  • feature.c will always be compiled (I believe) - does this have an effect on compile time?

It does affect compile time a little, but there aren't that many files, I'd think the increase is not significant. But I'll do a full make all-keyboards build with conditional compile and without, and compare the results. With quick, I'd think the difference will be below 10 seconds.

On the other hand, this all being C may increase the build speed with quick. I'll have a closer look at the makefiles, I think there's room for improvement there, and I happen to love fiddling with makefiles. In the long run, I think it would be possible to considerably speed up the build, without sacrificing any functionality.

@jackhumbert
Copy link
Member Author

There's not that many files right now, but when we have 20 similar features that all get compiled no matter what, I'd be a little more concerned with the time it takes to pull each file. I'd really like this format to be the way people can experiment and contribute new behaviors.

If we're able to remove the options from the Makefile to the config.h completely, we'll have no need for the quick command. The cleaning was added because of the risk of changing the Makefile and the compiler not realising it.

I'm certain there are ways to clean-up the Makefile a bit - there's some fancier stuff in there that might take a bit longer, especially since some of it gets called quite a few times during compilation.

@algernon
Copy link
Contributor

Yeah, with 20 files, a second each would add up indeed. I'll see what I can do to speed things up! There are ways to tell make to calculate a value only once, for example, so if you use it a thousand times, it still does not get recalculated - that's a significant speedup.

If all else fails, we can ask the Makefile to generate a header file (to a temporary name), compare it to the existing, and only overwrite if it differs. This way we have the dependencies in a header, and we preserve the timestamp if options did not change, so can default to quick.

But nevertheless, I'll have a closer look, to be sure.

@fredizzimo
Copy link
Contributor

I also think it's cleaner if the functions are called, even if they are empty. But that might be because I come from a C++ background, where it's quite common to have virtual functions that does nothing.

Regarding the optimization, I haven't been testing it, but I'm pretty sure the compiler won't optimize the empty functions away, at least unless link-time optimizations, which would considerably slow down the linking is enabled. The reason for this, is that the compiler is forced to threat each compilation unit in isolation, it can't really know if a function is empty or not, therefore it has to generate the calling code for it.

That said, there's a quite clean way of doing it, and have the compiler optimize the functions away. If the functionality is not enabled, then the empty functions should be defined as static inline functions in the header file. This way, the compiler will see that it's empty, and optimize it away. That would work like this.

#if !defined(FEATURE)
static void inline function1(void) {}
static int32_t inline funciton2(void) { return 0; }
#endif

This will clutter the public header files though, so we could also do it like this.

#if !(feature)
#include "feature_nop.inl"
#endif

And implement the empty definitions in the nop file.

For compile time optimizations, I think we could try to do things a bit differently. It's not really an issue to compile a single keyboard, at least not if you are using the quick option, but compiling multiple keyboards and keymaps takes quite long time currently. It wouldn't actually have to take this long though, considering that most of them could actually share quite a bit of readily compiled code, and let the linker resolve the differences.

We basically have two different variations for generated code. The first is the target processor, and perhaps it's clock configuration and stuff like that. Different processor's can share nothing.

Then we have the features, and their configuration. Many keyboards and keymaps share the same processor and features, and should be able to take advantage of the fact that the feature is already compiled.

One way to do this, would be to turn each feature into a static library, that is compiled once. This static library could in turn be dependent on other features, for example debug output. In that case we could to compile several variations of the library(on demand) and encode the configuration into the library name for example.

breathing_debug.a // Breathing support with debug prints
breathing_no_debug.a //Breathing support without debug prints

This of course requires each feature to be defined in the make file, and not in the config.h file like Jack proposed above. I don't think that's a major drawback though.

Since features that require other features need special handling in the make files, we should also make sure that this dependency is enforced. That means that a feature, can not include a header file from another feature, unless it's allowed and handled by the make file. This can be done by putting each feature into it's own directory, and setup the include directories correctly from the make file.

I wrote this extremely quickly, but have been thinking about it for a few days, so I hope it makes sense. I didn't want to go into too many details, unless you think it's a good idea to do it this way.

@fredizzimo
Copy link
Contributor

Empty function optimization
I just did some tests. With our current compiler and linker options, empty functions will not be optimized away. In my case a call to an empty void function added 4 bytes, and for functions taking parameters the cost is probably more.

However if we enable link time optimizations -flto, then it will optimize them. BTW, LTO, has a significant effect on the generated code size, for me the size of Ergodox EZ without LTO was 27784, and with LTO 25256, so that's 2528 bytes saved.

The drawback of LTO, is slighty slower compile times, but the other bigger drawback could be that something stops working. I didn't test it at all, but if the option was to be enabled, then it would have to be carefully tested. It's also possible to enable it just for some files.

So that means that we could write empty stubs either in the c file or the h file as inline functions as I explained previously. Note that we wouldn't need to write emtpy stubs for all the functions, as most of the functions would never be called from keyboards and keymaps not supporting some feature. In that case we can just let the linker generate the errors instead.

Config.h vs makefile
I don't really have a preference, especially not anymore since things are properly re-built when we change makefile options. The makefile is a little bit more flexible though, since it can include and exclude entire features from compilation. And doesn't necessarily mean a full re-build when you change somehting. If we move over to only using the config file, we would also need to make sure it's included from everywhere, probably by adding -include compiler option.

What I don't like is to have two different places of defining things, so I think we should either migrate the options to either file.

Compile time polymorphism
Issue #895, brought up the issue about compile time polymorphism. The new API functionality can be implemented using different backends, but with the same interface. The current solution by @jackhumbert is to define macros like this #define SEND_BYTES(mt, dt, b, l) send_bytes_sysex(mt, dt, b, l) with a define controlling what gets defined.

I think the best way of doing this would be to have a common api.h file which declares the function (not macro) send_bytes. Then we have separate implementation files for example api_sysex.c and api_hid.c, where the compilation is controlled by either an #ifdef inside the c file, or by the makefile. This way we don't need to resort to macros that much, especially since there are so many pitfalls. But of course much of it could be avoided by not using function-like macros and use simple symbolic replacement instead like this #define SEND_BYTES send_bytes Still that could mess up the IDE's autocomplete functionality.

We can of course also have an api.c file for common functionality. And it's even possible to allow overriding of single functions, while still having a common implementation for those that needs it, by using weak functions.

@drashna
Copy link
Member

drashna commented Oct 21, 2018

Was this a topic that we should leave open, or close?

@jackhumbert
Copy link
Member Author

I think this has largely been addressed - thanks!

BlueTufa pushed a commit to BlueTufa/qmk_firmware that referenced this issue Aug 6, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* Add default keymap for S7 Elephant

* Update Satan default keymap

Add commit key.

* Add default keymap for Scarlet Bandana

* Add default keymap for Scythe

* Add default keymap for Sentraq Number Pad

* Add default keymap for Sentraq S65-Plus

* Add default keymap for Shiro

* Add default keymap for Singa

* Add default keymap for Sirius Unigo66

* Add default keymap for SixKeyBoard

* Add default keymap for Skog

* Add default keymap for Snagpad

* Add default keymap for Snampad

* Add default keymap for Southpole

* Add default keymaps for Spacetime

revs. 1 and 2

* Add default keymap for Speedo

* Add default keymap for Stand Aside

* Add default keymap for Staryu

* Add default keymaps for Suihankey

alpha and rev1

* Add default keymaps for Suihankey Split

alpha and rev1
jiaxin96 pushed a commit to Oh-My-Mechanical-Keyboard/qmk_firmware that referenced this issue Oct 18, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants