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

Add files to SIZE_OPTIMIZED #2160

Closed

Conversation

jflyper
Copy link
Contributor

@jflyper jflyper commented Jan 19, 2017

PR Status: For a reminder for discussion

Mostly initialization and/or used when tethered. Yields around 4.4Kbytes.

Configurable software serial port pin assignment #1849 would fit after this.

Mostly initialization and/or used when tethered. Yields around
4.4Kbytes.
@martinbudden martinbudden added this to the Betaflight v3.2 milestone Jan 19, 2017
@jflyper
Copy link
Contributor Author

jflyper commented Jan 19, 2017

Several more files added.

Additional yield of 12.7Kbytes.

@@ -639,7 +638,6 @@ SPEED_OPTIMISED_SRC := $(SPEED_OPTIMISED_SRC) \
drivers/rx_nrf24l01.c \
drivers/rx_spi.c \
drivers/rx_xn297.c \
drivers/pwm_output.c \
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in speed optimised. PWM output is part of the GYROPID loop. When running at 32kHz every machine cycle counts - there is less than 31 microseconds to complete the loop.

Copy link
Member

Choose a reason for hiding this comment

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

Yes correct it is part of speedloop.

Separation of init elements should be done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap.

  1. Identify "Pure GYROPID loop" functions
  2. Put them in different file and give it speed optimized (or the other way around), or
  3. Use function attribute for optimization.

Everybody seems to have their own preference...

Copy link
Contributor

Choose a reason for hiding this comment

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

well, I don't really like pragmas in .c files unless absolutely necessary. So I'd move the init functions into a pwm_init.c module. Keeping the output functions in pwm_output.c seems sensible, since they are output functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The separation would induce exposure of static objects; make pwm_impl.h (pwm_hidden.h?) to hide them from rest of the world?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is worth going to that trouble for this smallish file. That would also make the code more convoluted. So in this case, I'd say, despite my earlier comments, it's probably best just to use a pragma.

Copy link
Member

Choose a reason for hiding this comment

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

@martinbudden, @ledvinap: Do you have any insights on why gcc states that optimisation by attribute / pragma should not be used for production code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably because it is not portable, ie if you change the compiler it won't work. Doing it on a per file basis controlled by the makefile is much more portable.

Copy link
Contributor

@ledvinap ledvinap Jan 20, 2017

Choose a reason for hiding this comment

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

@mikeller : Can you link the statement?

One reason may be that mixing different optimization levels may be quite stressing on the compiler, it may uncover unexpected bugs (with LTO, optimization on file and function levels are almost identical)

@martinbudden : In case of CF/BF, the portability should not me that big problem. Other parts of (low level) code are GCC specific anyway (some of them are my fault [ or "design decision"]).
IMO it will be sufficient to use one global optimization level when building for targets other than microprocessor - other targets have no memory/speed constraints. So other compiler just needs to ignore function level optimizations

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

@martinbudden martinbudden left a comment

Choose a reason for hiding this comment

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

pwm_output.c must be speed optimised.

The intialisation functions in that module don't need to be speed optimised, but the motor output functions do.

fc/fc_init.c \
fc/config.c \
flight/servos.c \
flight/mixer.c \
Copy link
Member

@mikeller mikeller Jan 19, 2017

Choose a reason for hiding this comment

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

flight/servos.c and flight/mixer.c also contain functions that are part of the PID loop, so they probably shouldn't be size optimised in their entirety.

@martinbudden
Copy link
Contributor

Note that flight/mixers.c, flight/servos.c and drivers/pwm_output.c are part of the highly speed critical PIDLOOP and so should definitely be in SPEED_OPTIMISED_SRC. My general view is we should not change the optimisation settings for those files at all until we have exhausted all other means of ROM saving.

I would also say that, generally speaking, all device drivers should be speed optimised.

I think the telemetry modules are candidates for size optimisation.

@jflyper
Copy link
Contributor Author

jflyper commented Jan 25, 2017

Superseded by #2205

@jflyper jflyper closed this Jan 25, 2017
@jflyper jflyper deleted the bfdev-add-files-to-size_optimized branch February 22, 2017 04:02
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.

6 participants