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

navigator: Change IF statement to SWITCH statement #23534

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

muramura
Copy link
Contributor

@muramura muramura commented Aug 12, 2024

Solved Problem

Clarify each decision value.

re pr #23065

Solution

Change the IF statement to a SWITCH statement.

Changelog Entry

None

Alternatives

None

Test coverage

None

Context

None

@muramura muramura force-pushed the PX4_Change_IF_statement_to_SWITCH_statement branch from ed4c4d8 to e89874d Compare August 12, 2024 04:15
@muramura muramura force-pushed the PX4_Change_IF_statement_to_SWITCH_statement branch from e89874d to 74897d5 Compare August 12, 2024 04:51
@dayjaby
Copy link
Contributor

dayjaby commented Aug 12, 2024

With these kinds of changes, it's always nice to show or prove what exactly is getting better (or worse). Like did you consider comparing the resulting firmware sizes?

Checking the assembly results with some similar code (https://gcc.godbolt.org/z/EeW8Tzcb3) against a similar compiler (arm 9.3.0), I'd argue that the if-solution is actually better. It's not doing multiple comparisons in a row, but some highly efficient bit calculations whereas the switch-solution creates a large lookup table.

@muramura
Copy link
Contributor Author

muramura commented Aug 13, 2024

@dayjaby san.
In both the SWITCH and IF statements, the memory size was the same.
The SWITCH statement uses the same variable for multiple comparison values.
The IF statement allows for different conditions.
Before the change, the same variable was used repeatedly in the conditions.
This is not considered good coding style.

IPA, an organization in Japan, has published a business-oriented C++ embedded coding style guide.
Screenshot from 2024-08-14 00-05-35

AFTER:
[1294/1296] Linking CXX executable px4_fmu-v6xrt_default.elf
Memory region Used Size Region Size %age Used
flash: 2114556 B 3968 KB 52.04%
sram: 91756 B 1280 KB 7.00%
itcm: 209256 B 256 KB 79.82%
dtcm: 936 B 256 KB 0.36%

BEFORE:
[1294/1296] Linking CXX executable px4_fmu-v6xrt_default.elf
Memory region Used Size Region Size %age Used
flash: 2114556 B 3968 KB 52.04%
sram: 91756 B 1280 KB 7.00%
itcm: 209256 B 256 KB 79.82%
dtcm: 936 B 256 KB 0.36%

dakejahl
dakejahl previously approved these changes Aug 13, 2024
@bresch bresch merged commit dec550d into PX4:main Aug 14, 2024
1 of 90 checks passed
vertiq-jordan pushed a commit to iq-motion-control/PX4-Autopilot that referenced this pull request Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants