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.

Sign up for GitHub

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

Jump to bottom

cpu/samd21: set optimization level to -Os #2848

Merged
merged 1 commit into from
May 29, 2015
Merged

cpu/samd21: set optimization level to -Os #2848

merged 1 commit into from
May 29, 2015

Conversation

haukepetersen
Copy link
Contributor

I was wondering about the huge code-size for the samr21-xpro - turned out, the optimization for the board is turned off... Looks better now.

@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Apr 21, 2015
@PeterKietzmann
Copy link
Member

Nice one! ACK, waiting for Travis..

@OlegHahm
Copy link
Member

Have you tested with some applications to check if this has no unexpected side effect?

@kaspar030
Copy link
Contributor

We did, it has some problems with the GPIO implementation AFAIR.

@PeterKietzmann
Copy link
Member

Tested with some easy applications but not with peripheral tests. In this case I revert my ACK and need to do some further testings :(

@haukepetersen
Copy link
Contributor Author

I did some more tests, the GPIO drivers are not cleanly implemented. I was not really going to look into this in detail... But I will try to do so.

Just a remark: the GPIO issues are actually unrelated to this PR, it just seems that enabling the compiler optimization triggers the compiler to detect them...

@OlegHahm
Copy link
Member

Just a remark: the GPIO issues are actually unrelated to this PR, it just seems that enabling the compiler optimization triggers the compiler to detect them...

Well, while I think it's okay for this particular test application, that was exactly the idea behind my question: even if the underlying problem is unrelated to this change, it may introduce failures that won't happen without the optimization. rpl_udp is typically a good candidate, but I have only one SAMR21 here.

@haukepetersen
Copy link
Contributor Author

I will look into this once I find some time...

@kaspar030 kaspar030 mentioned this pull request Apr 22, 2015
Merged
21 tasks
@PeterKietzmann
Copy link
Member

I think we can either (i) merge this PR and open an issue, close this and open an issue (iii) close this PR as it is also part of #2852 and move the discussion there

@haukepetersen
Copy link
Contributor Author

I say lets leave this PR open for now and and see if after #2865 is merged either this PR or #2852 is faster...

@OlegHahm OlegHahm modified the milestone: Release 2015.06 Apr 29, 2015
@haukepetersen
Copy link
Contributor Author

Just saw that the GPIO warnings are not the only issue for the samr21-xpro board. When enabling -Os, the tests/driver_at86rf2xx application does also not work -> it returns an error in the radio initialization function. I would suspect something the low level drivers to cause that...

@biboc
Copy link
Member

biboc commented Apr 30, 2015

@haukepetersen, might be the cause of the GPIO as well? or SPI?

@haukepetersen
Copy link
Contributor Author

I had no time to look further into this, yet. But I hope to get to it in the end of the week.

@biboc biboc added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label May 26, 2015
@haukepetersen haukepetersen force-pushed the fix_samr21_optimization branch from fb42b72 to 32410bb Compare May 29, 2015 10:15
@haukepetersen
Copy link
Contributor Author

Good news: the samr21 is now doing fine with -Os. So please verify this and let's merge this soon to save some time flashing the nodes!

@haukepetersen haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 29, 2015
@haukepetersen haukepetersen changed the title boards/samr21-xpro: set optimization level to -Os cpu/samd21: set optimization level to -Os May 29, 2015
biboc
biboc reviewed May 29, 2015
View reviewed changes
cpu/samd21/Makefile.include
@@ -6,7 +6,4 @@ export CFLAGS += -DDONT_USE_CMSIS_INIT
# use the hwtimer compatibility layer
USEMODULE += hwtimer_compat

# TODO: remove once the CPU implementation is stable with -Os
CFLAGS_OPT = -O0

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we change it by -Os then?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the common makefile defines CFLAGS_OPT ?= -Os.

Copy link
Member

Choose a reason for hiding this comment

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

ok

@haukepetersen
Copy link
Contributor Author

somebody up to acking this?

@kaspar030
Copy link
Contributor

Just connected the samr21, sec

@kaspar030
Copy link
Contributor

works. ACK&go.

kaspar030 added a commit that referenced this pull request May 29, 2015
@kaspar030
Merge pull request #2848 from haukepetersen/fix_samr21_optimization
9ecaea4 
cpu/samd21: remove -O0 override
@kaspar030 kaspar030 merged commit 9ecaea4 into RIOT-OS:master May 29, 2015
@haukepetersen haukepetersen deleted the fix_samr21_optimization branch May 29, 2015 15:01
@haukepetersen
Copy link
Contributor Author

awesome, thx.

@haukepetersen
Copy link
Contributor Author

seems we are moving in the right direction with this board :-)

@kaspar030
Copy link
Contributor

yeah ;)

@PeterKietzmann PeterKietzmann mentioned this pull request May 24, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@PeterKietzmann PeterKietzmann

Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2015.09
Development

Successfully merging this pull request may close these issues.
5 participants
@haukepetersen @PeterKietzmann @OlegHahm @kaspar030 @biboc